feature: cosocket client certificate support (client mutual TLS handshake) #1599
Conversation
dndx
changed the title
dndx
force-pushed
the
feat/cosocket_client_certificate
branch
from
8d1bf50
to
51c3b1f
Compare
certificate support if it is present (for luaL_checkcdataptr).
|
@agentzh @thibaultcha would you please take a look at this PR and let me know what you thinks? Thanks very much! |
|
|
||
| ngx.say("connected: ", ok) | ||
|
|
||
| local session, err = sock:sslhandshake(1, 2, 3, 4, 5, 6) |
There was a problem hiding this comment.
This test case assumes that the number of arg check is performed before arg type check, which assumes too much about the underlying implementation.
| local chain = assert(ssl.parse_pem_cert(cert_data)) | ||
| local priv = assert(ssl.parse_pem_priv_key(key_data)) | ||
|
|
||
| assert(sock:sslhandshake(nil, nil, nil, nil, { client_cert = chain, client_priv_key = priv, })) |
There was a problem hiding this comment.
I think we should introduce a separate method instead to avoid the requirement of an extra table here. Also it makes connection pool reuse easier.
There was a problem hiding this comment.
Do you mean to implement something that is the superset of sock:sslhandshake just like settimeout and settimeouts? For example:
sock:tlshandshake({
client_cert = chain,
client_priv_key = priv,
server_name = "example.com",
ssl_verify = true,
})
Personally I like this design, but wasn't sure at first so went for the more conservative approach.
There was a problem hiding this comment.
Second thought. I think we can get rid of the sock:sslhandshake C function altogether and instead implement it (for backward compatibility) in lua-resty-core and simply calls sock:tlshandshake. This way we do not need to maintain two sets of logic for the same thing. Because lua-resty-core is already required for current version of ngx_lua to work properly, this approach should be acceptable.
@agentzh @thibaultcha thoughts?
|
|
||
| if (!lua_isnil(L, -1)) { | ||
| #ifdef OPENRESTY_LUAJIT | ||
| chain = luaL_checkcdataptr(L, -1); |
There was a problem hiding this comment.
I think maybe it's the time to rewrite this sslhandshake method with FFI.
There was a problem hiding this comment.
Oh, if we introduce a separate method to set the client certificates and keys, then we don't need to touch this method at all and just need to introduce a new FFI-based method instead. This will be easier.
There was a problem hiding this comment.
Yes if we are decided on adding a brand new API then I'm all in for implementing it using FFI.
|
Superseded by #1602. |
I hereby granted the copyright of the changes in this pull request
to the authors of this lua-nginx-module project.
This PR aims to provide a method to support client TLS certificate for cosocket API.
An existing PR #997 tried to solve the same problem but has not been accepted by the maintainers, primarily due to runtime safety concern and easiness of use issues.
This PR's approach is similar to the existing
ssl_certificate_by*APIs by reusing the parser providedngx.sslmodule. Onlycdatapointer is passed which is harder to mess up (compared to fullSSL_CTXswap). In addition, this design meanscdatacan still be cached and reused like we do inssl_certificate_by*today.The introduction of options table means we can add more options like those (such as cipher suite) into the API without having to add more positional arguments, which is already getting long.
Depends on: openresty/luajit2#74
If
OPENRESTY_LUAJITis not detected, attempting to set client certificate using the option table throws an Lua error.