feature: add FFI interface to verify SSL client certificate #1666

ArchangelSDY · 2020-03-13T11:51:18Z

This PR adds a FFI interface that allows users to configure SSL client certificate verification dynamically. For example, Nginx can now read SNI first and then determine whether to request a client certificate during handshake. Optionally, caller can pass in a CA list used for verification.

I hereby granted the copyright of the changes in this pull request
to the authors of this lua-nginx-module project.

doujiang24 · 2020-03-20T01:18:39Z

@ArchangelSDY will you create another PR in lua-resty-core to expose Lua API?

ArchangelSDY · 2020-03-20T06:00:30Z

@doujiang24 Sure if you are fine with the API change in this PR. Here they are:

openresty/lua-resty-core#289
openresty/meta-lua-nginx-module#75
openresty/stream-lua-nginx-module#190

src/ngx_http_lua_ssl_certby.c

t/140-ssl-c-api.t

doujiang24 · 2020-03-30T03:18:45Z

@ArchangelSDY we'd better use new commit for the new changes, force push is not friendly for the reviewer to check the new changes only, thanks :)

ArchangelSDY · 2020-03-30T15:39:34Z

I see. Will follow that for new changes.

doujiang24

otherwise, it looks good to me.

src/ngx_http_lua_ssl_certby.c

doujiang24 · 2020-04-18T16:05:09Z

src/ngx_http_lua_ssl_certby.c

+
+    if (depth < 0) {
+        sscf = ngx_http_get_module_srv_conf(r, ngx_http_ssl_module);
+        if (sscf != NULL) {


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

sure. updated.

rainingmaster · 2020-04-20T03:46:44Z

@ArchangelSDY
Hello, I see the default cert is same with the cert loaded in ffi api, maybe we should change another cert as new CA cert? I also comment here

zhuizhuhaomeng · 2020-04-20T07:40:08Z

src/ngx_http_lua_ssl_certby.c

+
+
+int
+ngx_http_lua_ffi_ssl_verify_client(ngx_http_request_t *r, void *ca_certs,


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 ?

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.

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.

ArchangelSDY · 2020-04-20T11:10:00Z

@rainingmaster I see your concern. Then I will use a different cert at server side. Now both certs are self signed so the CA lists should not interfere with each other.

rainingmaster · 2020-04-20T13:43:12Z

@rainingmaster I see your concern. Then I will use a different cert at server side. Now both certs are self signed so the CA lists should not interfere with each other.

@ArchangelSDY Good job, and I think Test 8 compare to Test 6 should only difference in client provides no certificate, but should provide CA cert here, like Test 7 should only different with Test 6 in CA certificates?

Beside, server_tokens and lua_ssl_trusted_certificate should be useless in new test cases;

rainingmaster · 2020-04-21T12:58:36Z

src/ngx_http_lua_ssl_certby.c

+        *err = "bad request";
+        return NGX_ERROR;
+    }
+


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.

I see other SSL APIs don't have this check either. Is this simply because of some historical reasons?

Check added.

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

ArchangelSDY · 2020-04-21T14:23:10Z

@rainingmaster I see your concern. Then I will use a different cert at server side. Now both certs are self signed so the CA lists should not interfere with each other.

@ArchangelSDY Good job, and I think Test 8 compare to Test 6 should only difference in client provides no certificate, but should provide CA cert here, like Test 7 should only different with Test 6 in CA certificates?

Beside, server_tokens and lua_ssl_trusted_certificate should be useless in new test cases;

Make sense. Updated Test 8 and removed those useless parts.

rainingmaster

LGTM

src/ngx_http_lua_ssl_certby.c

rainingmaster · 2020-07-09T10:55:03Z

@agentzh @doujiang24 hihi, since 1.17.8 is released, could we merge this PR now?

src/ngx_http_lua_ssl_certby.c

agentzh · 2020-07-20T18:44:52Z

src/ngx_http_lua_ssl_certby.c

+
+
+int
+ngx_http_lua_ffi_ssl_verify_client(ngx_http_request_t *r, void *ca_certs,


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.

agentzh · 2020-07-20T18:50:04Z

t/140-ssl-c-api.t

+            local cert_data = f:read("*all")
+            f:close()
+
+            local cert = ffi.C.ngx_http_lua_ffi_parse_pem_cert(cert_data, #cert_data, errmsg)


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.

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.

doujiang24 · 2020-10-12T14:29:48Z

@ArchangelSDY Thanks for your contribution again.
The new OpenResty 1.19.3.1 release is coming, and we are preparing an online open meeting to introduce the new release.
Do you have time to have a talk about this new feature?
My email is doujiang24#gmail.com and my WeChat ID is doujiang24, welcome to contact me if you are interested in it.

ArchangelSDY force-pushed the ssl-verify-client branch from 101fe3c to 57af78c Compare March 13, 2020 11:58

ArchangelSDY mentioned this pull request Mar 20, 2020

feature: implement ssl.verify_client() openresty/lua-resty-core#289

Merged

ArchangelSDY force-pushed the ssl-verify-client branch 4 times, most recently from e1be8ce to 09cb269 Compare March 28, 2020 16:13

doujiang24 reviewed Mar 29, 2020

View reviewed changes

src/ngx_http_lua_ssl_certby.c Outdated Show resolved Hide resolved

t/140-ssl-c-api.t Outdated Show resolved Hide resolved

ArchangelSDY added 2 commits March 29, 2020 21:58

feature: add FFI interface to verify SSL client certificate

abfee94

tests: add unit tests for client certificate verification API

6c25967

ArchangelSDY force-pushed the ssl-verify-client branch from 09cb269 to 6c25967 Compare March 29, 2020 13:59

doujiang24 reviewed Apr 6, 2020

View reviewed changes

src/ngx_http_lua_ssl_certby.c Outdated Show resolved Hide resolved

src/ngx_http_lua_ssl_certby.c Outdated Show resolved Hide resolved

doujiang24 requested review from spacewander and rainingmaster April 6, 2020 15:50

spacewander approved these changes Apr 7, 2020

View reviewed changes

ArchangelSDY added 2 commits April 7, 2020 23:53

style: address review issues

f4803ff

feature: make client cert verify depth optional

8eb9ab9

doujiang24 reviewed Apr 18, 2020

View reviewed changes

feature: set default client cert verify depth to 1

017a1aa

ArchangelSDY force-pushed the ssl-verify-client branch from 90309db to 017a1aa Compare April 19, 2020 04:05

zhuizhuhaomeng reviewed Apr 20, 2020

View reviewed changes

tests: use a different cert for server side cert

76e8b97

rainingmaster reviewed Apr 21, 2020

View reviewed changes

tests: refine and clean up

e9577ed

feature: check context when calling ssl.verify_client

889b9a7

rainingmaster approved these changes Apr 22, 2020

View reviewed changes

bugfix: set client cert verify store per connection

6b58760

spacewander requested changes Jun 9, 2020

View reviewed changes

src/ngx_http_lua_ssl_certby.c Show resolved Hide resolved

ArchangelSDY added 2 commits June 9, 2020 11:16

bugfix: goto failed when failed to create name_chain

1206d9c

bugfix: fix error message

72f3124

spacewander approved these changes Jun 9, 2020

View reviewed changes

doujiang24 mentioned this pull request Jul 14, 2020

PRs that ready to be merged #1747

Closed

agentzh requested changes Jul 21, 2020

View reviewed changes

ArchangelSDY added 2 commits July 22, 2020 15:39

style: address review comments

ca0ae92

bugfix: fix memory leak in test cases

1691004

doujiang24 merged commit c0e460a into openresty:master Jul 30, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feature: add FFI interface to verify SSL client certificate #1666

feature: add FFI interface to verify SSL client certificate #1666

ArchangelSDY commented Mar 13, 2020

doujiang24 commented Mar 20, 2020

ArchangelSDY commented Mar 20, 2020

doujiang24 commented Mar 30, 2020

ArchangelSDY commented Mar 30, 2020

doujiang24 left a comment

doujiang24 Apr 18, 2020

ArchangelSDY Apr 19, 2020

rainingmaster commented Apr 20, 2020

zhuizhuhaomeng Apr 20, 2020

ArchangelSDY Apr 20, 2020 •

edited

Loading

agentzh Jul 20, 2020

ArchangelSDY commented Apr 20, 2020

rainingmaster commented Apr 20, 2020

rainingmaster Apr 21, 2020

ArchangelSDY Apr 21, 2020

ArchangelSDY Apr 21, 2020

rainingmaster Apr 22, 2020

ArchangelSDY commented Apr 21, 2020

rainingmaster left a comment

rainingmaster commented Jul 9, 2020

agentzh Jul 20, 2020

agentzh Jul 20, 2020

ArchangelSDY Jul 22, 2020

doujiang24 commented Oct 12, 2020



		int
		ngx_http_lua_ffi_ssl_verify_client(ngx_http_request_t r, void ca_certs,

feature: add FFI interface to verify SSL client certificate #1666

feature: add FFI interface to verify SSL client certificate #1666

Conversation

ArchangelSDY commented Mar 13, 2020

doujiang24 commented Mar 20, 2020

ArchangelSDY commented Mar 20, 2020

doujiang24 commented Mar 30, 2020

ArchangelSDY commented Mar 30, 2020

doujiang24 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rainingmaster commented Apr 20, 2020

Choose a reason for hiding this comment

ArchangelSDY Apr 20, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ArchangelSDY commented Apr 20, 2020

rainingmaster commented Apr 20, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ArchangelSDY commented Apr 21, 2020

rainingmaster left a comment

Choose a reason for hiding this comment

rainingmaster commented Jul 9, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

doujiang24 commented Oct 12, 2020

ArchangelSDY Apr 20, 2020 •

edited

Loading