Skip to content

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

Merged
merged 13 commits into from
Jul 30, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
136 changes: 136 additions & 0 deletions src/ngx_http_lua_ssl_certby.c
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Copy link
Contributor

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 ?

Copy link
Contributor Author

@ArchangelSDY ArchangelSDY Apr 20, 2020

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.

Copy link
Member

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.

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;
}

Copy link
Member

Choose a reason for hiding this comment

The 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 content_by_lua_block.

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Check added.

Copy link
Member

Choose a reason for hiding this comment

The 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?

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) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we'd better set the depth to 1 when sscf is NULL, and a comment: same as the default value of ssl_verify_depth.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sure. updated.

depth = sscf->verify_depth;

} else {
/* same as the default value of ssl_verify_depth */
depth = 1;
}
}

SSL_set_verify_depth(ssl_conn, depth);

/* set CA chain */

if (chain != NULL) {
ca_store = X509_STORE_new();
if (ca_store == NULL) {
*err = "X509_STORE_new() failed";
return NGX_ERROR;
}

/* 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);

X509_STORE_free(ca_store);

return NGX_ERROR;
}


#endif /* NGX_HTTP_SSL */
214 changes: 214 additions & 0 deletions t/140-ssl-c-api.t
Original file line number Diff line number Diff line change
Expand Up @@ -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_
}
Expand Down Expand Up @@ -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)
Copy link
Member

Choose a reason for hiding this comment

The 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):

t/140-ssl-c-api.t .. # I found ONLY: maybe you're debugging?
TEST 6: verify client with CA certificates
t/140-ssl-c-api.t .. 1/18 ==20252== 3,737 (40 direct, 3,697 indirect) bytes in 1 blocks are definitely lost in loss record 75 of 81
==20252==    at 0x4C2EE3B: malloc (vg_replace_malloc.c:309)
==20252==    by 0x62EA829: CRYPTO_malloc (mem.c:99)
==20252==    by 0x62EA85C: CRYPTO_zalloc (mem.c:107)
==20252==    by 0x63339E4: OPENSSL_sk_new (stack.c:108)
==20252==    by 0x63339C0: OPENSSL_sk_new_null (stack.c:101)
==20252==    by 0x51B14C: sk_X509_new_null (x509.h:97)
==20252==    by 0x51B14C: ngx_http_lua_ffi_parse_pem_cert (ngx_http_lua_ssl_certby.c:1067)
==20252==    by 0x5470E7D: lj_vm_ffi_call (in /opt/luajit21sysm/lib/libluajit-5.1.so.2.1.0)
==20252==    by 0x54C46CE: lj_ccall_func (lj_ccall.c:1377)
==20252==    by 0x54DBDD2: lj_cf_ffi_meta___call (lib_ffi.c:230)
==20252==    by 0x546E8A2: lj_BC_FUNCC (in /opt/luajit21sysm/lib/libluajit-5.1.so.2.1.0)
==20252==    by 0x4FD6A1: ngx_http_lua_run_thread (ngx_http_lua_util.c:1067)
==20252==    by 0x519F08: ngx_http_lua_ssl_cert_by_chunk (ngx_http_lua_ssl_certby.c:508)
==20252==    by 0x51A7BA: ngx_http_lua_ssl_cert_handler (ngx_http_lua_ssl_certby.c:294)
==20252==    by 0x5F72918: tls_post_process_client_hello (statem_srvr.c:1406)
==20252==    by 0x5F71373: ossl_statem_server_post_process_message (statem_srvr.c:799)
==20252==    by 0x5F63F71: read_state_machine (statem.c:630)
==20252==    by 0x5F6398D: state_machine (statem.c:396)
==20252==    by 0x5F63440: ossl_statem_accept (statem.c:176)
==20252==    by 0x5F5B30F: SSL_do_handshake (ssl_lib.c:3233)
==20252==    by 0x453C29: ngx_ssl_handshake (ngx_event_openssl.c:1606)
==20252==    by 0x468C8D: ngx_http_ssl_handshake (ngx_http_request.c:751)
==20252==    by 0x44EE27: ngx_epoll_process_events (ngx_epoll_module.c:901)
==20252==    by 0x444351: ngx_process_events_and_timers (ngx_event.c:257)
==20252==    by 0x44E462: ngx_single_process_cycle (ngx_process_cycle.c:333)
==20252==    by 0x422A9E: main (nginx.c:382)
==20252==
{
   <insert_a_suppression_name_here>
   Memcheck:Leak
   match-leak-kinds: definite
   fun:malloc
   fun:CRYPTO_malloc
   fun:CRYPTO_zalloc
   fun:OPENSSL_sk_new
   fun:OPENSSL_sk_new_null
   fun:sk_X509_new_null
   fun:ngx_http_lua_ffi_parse_pem_cert
   fun:lj_vm_ffi_call
   fun:lj_ccall_func
   fun:lj_cf_ffi_meta___call
   fun:lj_BC_FUNCC
   fun:ngx_http_lua_run_thread
   fun:ngx_http_lua_ssl_cert_by_chunk
   fun:ngx_http_lua_ssl_cert_handler
   fun:tls_post_process_client_hello
   fun:ossl_statem_server_post_process_message
   fun:read_state_machine
   fun:state_machine
   fun:ossl_statem_accept
   fun:SSL_do_handshake
   fun:ngx_ssl_handshake
   fun:ngx_http_ssl_handshake
   fun:ngx_epoll_process_events
   fun:ngx_process_events_and_timers
   fun:ngx_single_process_cycle
   fun:main
}
==20252== 5,230 (352 direct, 4,878 indirect) bytes in 1 blocks are definitely lost in loss record 76 of 81
==20252==    at 0x4C2EE3B: malloc (vg_replace_malloc.c:309)
==20252==    by 0x62EA829: CRYPTO_malloc (mem.c:99)
==20252==    by 0x62EA85C: CRYPTO_zalloc (mem.c:107)
==20252==    by 0x5F5FDD0: SSL_SESSION_new (ssl_sess.c:86)
==20252==    by 0x5F6054D: ssl_get_new_session (ssl_sess.c:308)
==20252==    by 0x5F65B0F: tls_construct_client_hello (statem_clnt.c:720)
==20252==    by 0x5F65768: ossl_statem_client_construct_message (statem_clnt.c:534)
==20252==    by 0x5F64272: write_state_machine (statem.c:777)
==20252==    by 0x5F639C9: state_machine (statem.c:405)
==20252==    by 0x5F63421: ossl_statem_connect (statem.c:171)
==20252==    by 0x5F5B30F: SSL_do_handshake (ssl_lib.c:3233)
==20252==    by 0x453C29: ngx_ssl_handshake (ngx_event_openssl.c:1606)
==20252==    by 0x47B7CC: ngx_http_upstream_ssl_init_connection (ngx_http_upstream.c:1721)
==20252==    by 0x47B7CC: ngx_http_upstream_ssl_init_connection (ngx_http_upstream.c:1668)
==20252==    by 0x47C78B: ngx_http_upstream_init_request (ngx_http_upstream.c:813)
==20252==    by 0x46FE19: ngx_http_read_client_request_body (ngx_http_request_body.c:77)
==20252==    by 0x46FE19: ngx_http_read_client_request_body (ngx_http_request_body.c:30)
==20252==    by 0x4B18C0: ngx_http_proxy_handler (ngx_http_proxy_module.c:937)
==20252==    by 0x4B18C0: ngx_http_proxy_handler (ngx_http_proxy_module.c:849)
==20252==    by 0x46199E: ngx_http_core_content_phase (ngx_http_core_module.c:1179)
==20252==    by 0x45C814: ngx_http_core_run_phases (ngx_http_core_module.c:868)
==20252==    by 0x46811F: ngx_http_process_request_headers (ngx_http_request.c:1480)
==20252==    by 0x468555: ngx_http_process_request_line (ngx_http_request.c:1151)
==20252==    by 0x44EE27: ngx_epoll_process_events (ngx_epoll_module.c:901)
==20252==    by 0x444351: ngx_process_events_and_timers (ngx_event.c:257)
==20252==    by 0x44E462: ngx_single_process_cycle (ngx_process_cycle.c:333)
==20252==    by 0x422A9E: main (nginx.c:382)
==20252==
{
   <insert_a_suppression_name_here>
   Memcheck:Leak
   match-leak-kinds: definite
   fun:malloc
   fun:CRYPTO_malloc
   fun:CRYPTO_zalloc
   fun:SSL_SESSION_new
   fun:ssl_get_new_session
   fun:tls_construct_client_hello
   fun:ossl_statem_client_construct_message
   fun:write_state_machine
   fun:state_machine
   fun:ossl_statem_connect
   fun:SSL_do_handshake
   fun:ngx_ssl_handshake
   fun:ngx_http_upstream_ssl_init_connection
   fun:ngx_http_upstream_ssl_init_connection
   fun:ngx_http_upstream_init_request
   fun:ngx_http_read_client_request_body
   fun:ngx_http_read_client_request_body
   fun:ngx_http_proxy_handler
   fun:ngx_http_proxy_handler
   fun:ngx_http_core_content_phase
   fun:ngx_http_core_run_phases
   fun:ngx_http_process_request_headers
   fun:ngx_http_process_request_line
   fun:ngx_epoll_process_events
   fun:ngx_process_events_and_timers
   fun:ngx_single_process_cycle
   fun:main
}

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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 proxy_ssl_session_reuse to off, it looks good now.

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]