-
Notifications
You must be signed in to change notification settings - Fork 2k
feature: add FFI interface to verify SSL client certificate #1666
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
abfee94
6c25967
f4803ff
8eb9ab9
017a1aa
76e8b97
e9577ed
889b9a7
6b58760
1206d9c
72f3124
ca0ae92
1691004
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -1294,4 +1294,140 @@ ngx_http_lua_ffi_set_priv_key(ngx_http_request_t *r, | |
} | ||
|
||
|
||
static int | ||
ngx_http_lua_ssl_verify_callback(int ok, X509_STORE_CTX *x509_store) | ||
{ | ||
/* | ||
* we never terminate handshake here and user can later use | ||
* $ssl_client_verify to check verification result. | ||
* | ||
* this is consistent with Nginx behavior. | ||
*/ | ||
return 1; | ||
} | ||
|
||
|
||
int | ||
ngx_http_lua_ffi_ssl_verify_client(ngx_http_request_t *r, void *ca_certs, | ||
int depth, char **err) | ||
{ | ||
ngx_http_lua_ctx_t *ctx; | ||
ngx_ssl_conn_t *ssl_conn; | ||
ngx_http_ssl_srv_conf_t *sscf; | ||
STACK_OF(X509) *chain = ca_certs; | ||
STACK_OF(X509_NAME) *name_chain = NULL; | ||
X509 *x509 = NULL; | ||
X509_NAME *subject = NULL; | ||
X509_STORE *ca_store = NULL; | ||
#ifdef OPENSSL_IS_BORINGSSL | ||
size_t i; | ||
#else | ||
int i; | ||
#endif | ||
|
||
ctx = ngx_http_get_module_ctx(r, ngx_http_lua_module); | ||
if (ctx == NULL) { | ||
*err = "no request ctx found"; | ||
return NGX_ERROR; | ||
} | ||
|
||
if (!(ctx->context & NGX_HTTP_LUA_CONTEXT_SSL_CERT)) { | ||
*err = "API disabled in the current context"; | ||
return NGX_ERROR; | ||
} | ||
|
||
if (r->connection == NULL || r->connection->ssl == NULL) { | ||
*err = "bad request"; | ||
return NGX_ERROR; | ||
} | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As the document describe, here should check the phase like ngx.exit. And now the function can be run other phase such as There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I see other SSL APIs don't have this check either. Is this simply because of some historical reasons? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Check added. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Yes, I agree, but I think we should fix these inconsistencies(such as set_der_cert) with the documentation and add some test cases, right? @doujiang24 |
||
ssl_conn = r->connection->ssl->connection; | ||
if (ssl_conn == NULL) { | ||
*err = "bad ssl conn"; | ||
return NGX_ERROR; | ||
} | ||
|
||
/* enable verify */ | ||
|
||
SSL_set_verify(ssl_conn, SSL_VERIFY_PEER, ngx_http_lua_ssl_verify_callback); | ||
|
||
/* set depth */ | ||
|
||
if (depth < 0) { | ||
sscf = ngx_http_get_module_srv_conf(r, ngx_http_ssl_module); | ||
if (sscf != NULL) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. we'd better set the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. sure. updated. |
||
depth = sscf->verify_depth; | ||
|
||
} else { | ||
ArchangelSDY marked this conversation as resolved.
Show resolved
Hide resolved
|
||
/* same as the default value of ssl_verify_depth */ | ||
depth = 1; | ||
} | ||
} | ||
|
||
SSL_set_verify_depth(ssl_conn, depth); | ||
ArchangelSDY marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
/* set CA chain */ | ||
|
||
if (chain != NULL) { | ||
ca_store = X509_STORE_new(); | ||
if (ca_store == NULL) { | ||
*err = "X509_STORE_new() failed"; | ||
return NGX_ERROR; | ||
ArchangelSDY marked this conversation as resolved.
Show resolved
Hide resolved
|
||
} | ||
|
||
/* construct name chain */ | ||
|
||
name_chain = sk_X509_NAME_new_null(); | ||
if (name_chain == NULL) { | ||
*err = "sk_X509_NAME_new_null() failed"; | ||
goto failed; | ||
} | ||
|
||
for (i = 0; i < sk_X509_num(chain); i++) { | ||
x509 = sk_X509_value(chain, i); | ||
if (x509 == NULL) { | ||
*err = "sk_X509_value() failed"; | ||
goto failed; | ||
} | ||
|
||
/* add subject to name chain, which will be sent to client */ | ||
subject = X509_NAME_dup(X509_get_subject_name(x509)); | ||
if (subject == NULL) { | ||
*err = "X509_get_subject_name() failed"; | ||
goto failed; | ||
} | ||
|
||
if (!sk_X509_NAME_push(name_chain, subject)) { | ||
*err = "sk_X509_NAME_push() failed"; | ||
X509_NAME_free(subject); | ||
goto failed; | ||
} | ||
|
||
/* add to trusted CA store */ | ||
if (X509_STORE_add_cert(ca_store, x509) == 0) { | ||
*err = "X509_STORE_add_cert() failed"; | ||
goto failed; | ||
} | ||
} | ||
|
||
if (SSL_set0_verify_cert_store(ssl_conn, ca_store) == 0) { | ||
*err = "SSL_set0_verify_cert_store() failed"; | ||
goto failed; | ||
} | ||
|
||
SSL_set_client_CA_list(ssl_conn, name_chain); | ||
} | ||
|
||
return NGX_OK; | ||
|
||
failed: | ||
|
||
sk_X509_NAME_free(name_chain); | ||
ArchangelSDY marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
X509_STORE_free(ca_store); | ||
|
||
return NGX_ERROR; | ||
} | ||
|
||
|
||
#endif /* NGX_HTTP_SSL */ |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -63,6 +63,10 @@ ffi.cdef[[ | |
void ngx_http_lua_ffi_free_priv_key(void *cdata); | ||
|
||
int ngx_http_lua_ffi_ssl_clear_certs(void *r, char **err); | ||
|
||
int ngx_http_lua_ffi_ssl_verify_client(void *r, void *cdata, | ||
int depth, char **err); | ||
|
||
]] | ||
_EOC_ | ||
} | ||
|
@@ -812,3 +816,213 @@ lua ssl server name: "test.com" | |
--- no_error_log | ||
[error] | ||
[alert] | ||
|
||
|
||
|
||
=== TEST 6: verify client with CA certificates | ||
--- http_config | ||
server { | ||
listen unix:$TEST_NGINX_HTML_DIR/nginx.sock ssl; | ||
server_name test.com; | ||
|
||
ssl_certificate_by_lua_block { | ||
collectgarbage() | ||
|
||
require "defines" | ||
local ffi = require "ffi" | ||
|
||
local errmsg = ffi.new("char *[1]") | ||
|
||
local r = require "resty.core.base" .get_request() | ||
if r == nil then | ||
ngx.log(ngx.ERR, "no request found") | ||
return | ||
end | ||
|
||
local f = assert(io.open("t/cert/test.crt", "rb")) | ||
local cert_data = f:read("*all") | ||
f:close() | ||
|
||
local cert = ffi.C.ngx_http_lua_ffi_parse_pem_cert(cert_data, #cert_data, errmsg) | ||
if not cert then | ||
ngx.log(ngx.ERR, "failed to parse PEM cert: ", | ||
ffi.string(errmsg[0])) | ||
return | ||
end | ||
|
||
local rc = ffi.C.ngx_http_lua_ffi_ssl_verify_client(r, cert, 1, errmsg) | ||
if rc ~= 0 then | ||
ngx.log(ngx.ERR, "failed to verify client: ", | ||
ffi.string(errmsg[0])) | ||
return | ||
end | ||
|
||
ffi.C.ngx_http_lua_ffi_free_cert(cert) | ||
} | ||
|
||
ssl_certificate ../../cert/test2.crt; | ||
ssl_certificate_key ../../cert/test2.key; | ||
|
||
location / { | ||
default_type 'text/plain'; | ||
content_by_lua_block { | ||
print('client certificate subject: ', ngx.var.ssl_client_s_dn) | ||
ngx.say(ngx.var.ssl_client_verify) | ||
} | ||
more_clear_headers Date; | ||
} | ||
} | ||
--- config | ||
location /t { | ||
proxy_pass https://unix:$TEST_NGINX_HTML_DIR/nginx.sock; | ||
proxy_ssl_certificate ../../cert/test.crt; | ||
proxy_ssl_certificate_key ../../cert/test.key; | ||
proxy_ssl_session_reuse off; | ||
} | ||
|
||
--- request | ||
GET /t | ||
--- response_body | ||
SUCCESS | ||
|
||
--- error_log | ||
client certificate subject: [email protected],CN=test.com | ||
|
||
--- no_error_log | ||
[error] | ||
[alert] | ||
|
||
|
||
|
||
=== TEST 7: verify client without CA certificates | ||
--- http_config | ||
server { | ||
listen unix:$TEST_NGINX_HTML_DIR/nginx.sock ssl; | ||
server_name test.com; | ||
|
||
ssl_certificate_by_lua_block { | ||
collectgarbage() | ||
|
||
require "defines" | ||
local ffi = require "ffi" | ||
|
||
local errmsg = ffi.new("char *[1]") | ||
|
||
local r = require "resty.core.base" .get_request() | ||
if r == nil then | ||
ngx.log(ngx.ERR, "no request found") | ||
return | ||
end | ||
|
||
local rc = ffi.C.ngx_http_lua_ffi_ssl_verify_client(r, nil, -1, errmsg) | ||
if rc ~= 0 then | ||
ngx.log(ngx.ERR, "failed to verify client: ", | ||
ffi.string(errmsg[0])) | ||
return | ||
end | ||
} | ||
|
||
ssl_certificate ../../cert/test2.crt; | ||
ssl_certificate_key ../../cert/test2.key; | ||
|
||
location / { | ||
default_type 'text/plain'; | ||
content_by_lua_block { | ||
print('client certificate subject: ', ngx.var.ssl_client_s_dn) | ||
ngx.say(ngx.var.ssl_client_verify) | ||
} | ||
more_clear_headers Date; | ||
} | ||
} | ||
--- config | ||
location /t { | ||
proxy_pass https://unix:$TEST_NGINX_HTML_DIR/nginx.sock; | ||
proxy_ssl_certificate ../../cert/test.crt; | ||
proxy_ssl_certificate_key ../../cert/test.key; | ||
proxy_ssl_session_reuse off; | ||
} | ||
|
||
--- request | ||
GET /t | ||
--- response_body | ||
FAILED:self signed certificate | ||
|
||
--- error_log | ||
client certificate subject: [email protected],CN=test.com | ||
|
||
--- no_error_log | ||
[error] | ||
[alert] | ||
|
||
|
||
|
||
=== TEST 8: verify client but client provides no certificate | ||
--- http_config | ||
server { | ||
listen unix:$TEST_NGINX_HTML_DIR/nginx.sock ssl; | ||
server_name test.com; | ||
|
||
ssl_certificate_by_lua_block { | ||
collectgarbage() | ||
|
||
require "defines" | ||
local ffi = require "ffi" | ||
|
||
local errmsg = ffi.new("char *[1]") | ||
|
||
local r = require "resty.core.base" .get_request() | ||
if r == nil then | ||
ngx.log(ngx.ERR, "no request found") | ||
return | ||
end | ||
|
||
local f = assert(io.open("t/cert/test.crt", "rb")) | ||
local cert_data = f:read("*all") | ||
f:close() | ||
|
||
local cert = ffi.C.ngx_http_lua_ffi_parse_pem_cert(cert_data, #cert_data, errmsg) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This call allocates memory which is never released. You should free it yourself here. Otherwise this test fails in the valgrind test mode (and there is another leak below which I don't understand immediately):
See this ebook on how to run the tests in the valgrind mode: https://openresty.gitbooks.io/programming-openresty/content/testing/test-modes.html I tried both OpenSSL 1.1.0k and 1.1.1g when running this test. Same errors. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I see. Just fixed them. For the second leak I think it should be caused by SSL session reuse. After I set |
||
if not cert then | ||
ngx.log(ngx.ERR, "failed to parse PEM cert: ", | ||
ffi.string(errmsg[0])) | ||
return | ||
end | ||
|
||
local rc = ffi.C.ngx_http_lua_ffi_ssl_verify_client(r, cert, 1, errmsg) | ||
if rc ~= 0 then | ||
ngx.log(ngx.ERR, "failed to verify client: ", | ||
ffi.string(errmsg[0])) | ||
return | ||
end | ||
|
||
ffi.C.ngx_http_lua_ffi_free_cert(cert) | ||
} | ||
|
||
ssl_certificate ../../cert/test2.crt; | ||
ssl_certificate_key ../../cert/test2.key; | ||
|
||
location / { | ||
default_type 'text/plain'; | ||
content_by_lua_block { | ||
print('client certificate subject: ', ngx.var.ssl_client_s_dn) | ||
ngx.say(ngx.var.ssl_client_verify) | ||
} | ||
more_clear_headers Date; | ||
} | ||
} | ||
--- config | ||
location /t { | ||
proxy_pass https://unix:$TEST_NGINX_HTML_DIR/nginx.sock; | ||
proxy_ssl_session_reuse off; | ||
} | ||
|
||
--- request | ||
GET /t | ||
--- response_body | ||
NONE | ||
|
||
--- error_log | ||
client certificate subject: nil | ||
|
||
--- no_error_log | ||
[error] | ||
[alert] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if arg ca_certs is path of the cert, then the code would be must easizer. just like ngx_ssl_client_certificate
why not use path ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since
ssl.set_cert
accepts a pointer rather than a path, I think it should be better to keep them consistent. Besides, it provides some flexibility that these certificates do not have to be on disk. For example, in Kubernetes ingress controller use case, certificates are kept in Nginx memory and are update via a REST API.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with @ArchangelSDY here. Also we don't want to parse the x509 certificates on a per connection basis. We can cache the x509 C data structure directly with modules like lua-resty-lrucache.