feature: implement `ssl.verify_client()` #289

ArchangelSDY · 2020-03-20T05:48:19Z

This PR adds an API 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.

See openresty/lua-nginx-module#1666 for FFI changes.

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

doujiang24 · 2020-03-25T15:12:55Z

@ArchangelSDY this feature looks cool to me, will you please add some test case to cover it? thanks!

ArchangelSDY · 2020-03-29T05:08:47Z

@doujiang24 I've added some tests in this and other repos. Could you take a look?

doujiang24

@ArchangelSDY Cool, just found a style issue now, will take a deep look soon.

t/ssl.t

doujiang24 · 2020-04-06T15:52:37Z

lib/ngx/ssl.lua

@@ -78,6 +79,9 @@ if subsystem == 'http' then
    void ngx_http_lua_ffi_free_cert(void *cdata);

    void ngx_http_lua_ffi_free_priv_key(void *cdata);
+
+    int ngx_http_lua_ffi_ssl_verify_client(void *r,
+        int depth, void *cdata, char **err);


style: we can move more arguments to the previous line, like:

int ngx_http_lua_ffi_ssl_verify_client(void *r, int depth, void *cdata, char **err);

doujiang24 · 2020-04-06T16:03:47Z

lib/ngx/ssl.md

@@ -475,6 +475,28 @@ This function was first added in version `0.1.7`.

 [Back to TOC](#table-of-contents)

+verify_client
+------------
+**syntax:** *ok, err = ssl.verify_client(depth, ca_certs)*


may be this style can be better? we use the depth in ssl_verify_depth by default.ssl.verify_client(ca_certs, depth?).
thoughts? @thibaultcha @agentzh @spacewander

ping @doujiang24 @thibaultcha @agentzh @spacewander
any comment?

so, what's your opinion for my above suggestion? @ArchangelSDY

Actually ca_certs can also be optional so i think the default form should be just ssl.verify_client(). I have no preference for the order of parameters. If you prefer ca_certs first, let me know. @doujiang24

what the behavior when the ca_certs is nil? just rewrite the depth at the request level?

I prefer ca_certs first personally, I think it's more often to set ca_certs.

we'd better add a ? suffix after the argument name if it's optional, clarify the behavior when it's omitted or nil.

When ca_certs is nil, it will use system CA list to verify client cert.

Then I will update the order and work on a new patch.

Updated. @doujiang24

doujiang24

@ArchangelSDY good job. we will merge it after the new release it out in the near feature days if no other ones request changes, and it will be bundled in the next release that will be out in about June.

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

@ArchangelSDY
Hello, the default cert is same with the cert loaded by ssl.verify_client, so I concert that we can not cover the case if load cert failed?

rainingmaster

LGTM

pommi · 2020-06-08T11:15:03Z

Hi! First of all, thank you for developing this patch! I've been testing it and I run into an issue.

When the contents of ca_certs changes in verify_client(ca_certs, depth), the old ca_certs value still seems to be valid until you execute nginx reload. Is this expected behavior? Let's for example say you want to allow access to client certificates signed by a different CA and disallow access to client certificates signed by the CA that was there before. Having "dynamic" client certificate verification, I would expect the old ca_certs value not to be valid anymore when the contents of ca_certs changes.

Test Case

I've got a Client 1 signed by Root CA 1 and a Client 2 signed by Root CA 2.

This is my nginx config:

server {
    [..]

    ssl_certificate_by_lua_block {
        local ssl = require "ngx.ssl"

        local f = assert(io.open("/path/to/my-ca.pem"))
        local cert_data = f:read("*a")
        f:close()

        local cert, err = ssl.parse_pem_cert(cert_data)
        if not cert then
            ngx.log(ngx.ERR, "failed to parse pem cert: ", err)
            return
        end

        local ok, err = ssl.verify_client(cert, 0)
        if not ok then
            ngx.log(ngx.ERR, "failed to verify client: ", err)
            return
        end
    }

    location / {
        default_type 'text/plain';
        content_by_lua_block {
            ngx.say('SSL-Client-S-DN: ', ngx.var.ssl_client_s_dn)
            ngx.say('SSL-Client-I-DN: ', ngx.var.ssl_client_i_dn)
            ngx.say('SSL Client Verify: ', ngx.var.ssl_client_verify)
        }
    }
}

/path/to/my-ca.pem contains Root CA 1. Testing this setup shows this:

$ curl --key client-1.key --cert client-1.crt https://domain.tld
SSL-Client-S-DN: CN=Client 1,[..]
SSL-Client-I-DN: CN=Root CA 1,[..]
SSL Client Verify: SUCCESS

$ curl --key client-2.key --cert client-2.crt https://domain.tld
SSL-Client-S-DN: CN=Client 2,[..]
SSL-Client-I-DN: CN=Root CA 2,[..]
SSL Client Verify: FAILED:unable to verify the first certificate

So Client 2 is not allowed to have access, which is good.

I change the content of /path/to/my-ca.pem to Root CA 2 and I get this:

$ curl --key client-1.key --cert client-1.crt https://domain.tld
SSL-Client-S-DN: CN=Client 1,[..]
SSL-Client-I-DN: CN=Root CA 1,[..]
SSL Client Verify: SUCCESS

$ curl --key client-2.key --cert client-2.crt https://domain.tld
SSL-Client-S-DN: CN=Client 2,[..]
SSL-Client-I-DN: CN=Root CA 2,[..]
SSL Client Verify: SUCCESS

While I expect only Client 2 to get a successful verification, both Client 1 and Client 2 get SUCCESS.

Now I execute nginx reload and I get this:

$ curl --key client-1.key --cert client-1.crt https://domain.tld
SSL-Client-S-DN: CN=Client 1,[..]
SSL-Client-I-DN: CN=Root CA 1,[..]
SSL Client Verify: FAILED:unable to verify the first certificate

$ curl --key client-2.key --cert client-2.crt https://domain.tld
SSL-Client-S-DN: CN=Client 2,[..]
SSL-Client-I-DN: CN=Root CA 2,[..]
SSL Client Verify: SUCCESS

Is this expected behavior?

ArchangelSDY · 2020-06-08T16:59:56Z

@pommi Nice catch! Looks like I call a wrong API which sets the global store in ctx. I've submitted a fix. Could you try again?

pommi · 2020-06-09T06:59:01Z

Could you try again?

Yes, this works as expected. No nginx reload required anymore.

rainingmaster · 2020-06-10T14:05:37Z

@pommi Nice catch! Looks like I call a wrong API which sets the global store in ctx. I've submitted a fix. Could you try again?

Should we add a test case to cover it?

lib/ngx/ssl.md

agentzh · 2020-07-20T19:02:38Z

t/ssl.t

+
+
+
+=== TEST 23: verify client with CA certificates


This test case fails with the following leak error in the valgrind test mode on my side using OpenSSL 1.1.1g:

t/ssl.t .. # I found ONLY: maybe you're debugging? TEST 23: verify client with CA certificates t/ssl.t .. 1/14 ==29716== 5,491 (648 direct, 4,843 indirect) bytes in 1 blocks are definitely lost in loss record 43 of 48 ==29716== at 0x4C2EE3B: malloc (vg_replace_malloc.c:309) ==29716== by 0x6338608: CRYPTO_zalloc (mem.c:230) ==29716== by 0x5F62C98: SSL_SESSION_new (ssl_sess.c:74) ==29716== by 0x5F63462: ssl_get_new_session (ssl_sess.c:370) ==29716== by 0x5F71319: tls_construct_client_hello (statem_clnt.c:1121) ==29716== by 0x5F7064E: write_state_machine (statem.c:843) ==29716== by 0x5F7064E: state_machine.part.5 (statem.c:443) ==29716== by 0x5F5BEE3: SSL_do_handshake (ssl_lib.c:3663) ==29716== by 0x454067: ngx_ssl_handshake (ngx_event_openssl.c:1606) ==29716== by 0x454067: ngx_ssl_handshake (ngx_event_openssl.c:1593) ==29716== by 0x47C30C: ngx_http_upstream_ssl_init_connection (ngx_http_upstream.c:1721) ==29716== by 0x47C30C: ngx_http_upstream_ssl_init_connection (ngx_http_upstream.c:1668) ==29716== by 0x47D2CB: ngx_http_upstream_init_request (ngx_http_upstream.c:813) ==29716== by 0x470959: ngx_http_read_client_request_body (ngx_http_request_body.c:77) ==29716== by 0x470959: ngx_http_read_client_request_body (ngx_http_request_body.c:30) ==29716== by 0x4B2400: ngx_http_proxy_handler (ngx_http_proxy_module.c:937) ==29716== by 0x4B2400: ngx_http_proxy_handler (ngx_http_proxy_module.c:849) ==29716== by 0x4624DE: ngx_http_core_content_phase (ngx_http_core_module.c:1179) ==29716== by 0x45D354: ngx_http_core_run_phases (ngx_http_core_module.c:868) ==29716== by 0x468C5F: ngx_http_process_request_headers (ngx_http_request.c:1480) ==29716== by 0x469095: ngx_http_process_request_line (ngx_http_request.c:1151) ==29716== by 0x44F067: ngx_epoll_process_events (ngx_epoll_module.c:901) ==29716== by 0x444591: ngx_process_events_and_timers (ngx_event.c:257) ==29716== by 0x44E6A2: ngx_single_process_cycle (ngx_process_cycle.c:333) ==29716== by 0x422CDE: main (nginx.c:382) ==29716== { <insert_a_suppression_name_here> Memcheck:Leak match-leak-kinds: definite fun:malloc fun:CRYPTO_zalloc fun:SSL_SESSION_new fun:ssl_get_new_session fun:tls_construct_client_hello fun:write_state_machine fun:state_machine.part.5 fun:SSL_do_handshake fun:ngx_ssl_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 }

Fixed by adding proxy_ssl_session_reuse: off.

t/ssl.t

feature: implement ssl.verify_client()

978e012

ArchangelSDY force-pushed the ssl-verify-client branch from 6a51aa4 to 978e012 Compare March 20, 2020 05:59

ArchangelSDY mentioned this pull request Mar 20, 2020

feature: add FFI interface to verify SSL client certificate openresty/lua-nginx-module#1666

Merged

doujiang24 reviewed Mar 29, 2020

View reviewed changes

t/ssl.t Outdated Show resolved Hide resolved

tests: add unit tests for client certificate verification API

8d975a8

ArchangelSDY force-pushed the ssl-verify-client branch from 82f2253 to 8d975a8 Compare March 29, 2020 14:18

doujiang24 reviewed Apr 7, 2020

View reviewed changes

feature: update arguments order of ssl.verify_client and update doc

029bcf0

doujiang24 approved these changes Apr 18, 2020

View reviewed changes

ArchangelSDY added 2 commits April 20, 2020 19:18

tests: use a different cert at server side

39d2ded

tests: refine and clean up

2a2c419

rainingmaster approved these changes Apr 22, 2020

View reviewed changes

agentzh requested changes Jul 20, 2020

View reviewed changes

bugfix: fix memory leak in test cases

1218b5e

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

feature: implement ssl.verify_client() #289

feature: implement ssl.verify_client() #289

Uh oh!

Conversation

ArchangelSDY commented Mar 20, 2020

Uh oh!

doujiang24 commented Mar 25, 2020

Uh oh!

ArchangelSDY commented Mar 29, 2020

Uh oh!

doujiang24 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

doujiang24 Apr 17, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

doujiang24 left a comment

Choose a reason for hiding this comment

Uh oh!

rainingmaster commented Apr 20, 2020

Uh oh!

rainingmaster left a comment

Choose a reason for hiding this comment

Uh oh!

pommi commented Jun 8, 2020

Test Case

Uh oh!

ArchangelSDY commented Jun 8, 2020

Uh oh!

pommi commented Jun 9, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rainingmaster commented Jun 10, 2020

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

feature: implement `ssl.verify_client()` #289

feature: implement `ssl.verify_client()` #289

doujiang24 Apr 17, 2020 •

edited

Loading

pommi commented Jun 9, 2020 •

edited

Loading