feature: add client SSL certificiate support #957

detailyang · 2017-01-16T07:21:18Z

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

@agentzh This feature is talked at openresty/issues/222 and be listed in TODO list

Now I just add the api and tests but lack document. When we think it can be merged to master, i will supplement the api usage later.

Please help me review this PR, many thanks 😁

agentzh · 2017-01-18T19:06:42Z

t/129-ssl-socket.t

+
+
+
+=== TEST 33: setsslcert, too many arguments


These new tests should go to a separate test file. This 129-ssl-socket.t file is already quite large.

well, how about 151-ssl-client-setcert.t?

Should be fine.

agentzh · 2017-01-18T19:09:44Z

t/129-ssl-socket.t

+            local cert = read_file("$TEST_NGINX_HTML_DIR/client.crt")
+            local key = read_file("$TEST_NGINX_HTML_DIR/client.unsecure.key")
+
+            local ok, err = sock:setsslcert(cert, key)


I'm thinking about a different Lua API than this. Loading the raw certificates upon every SSL cosocket is quite expensive and the client-side certificates tend to be reused over and over again. I think we can introduce an opaque SSL ctx object that loads the certificates only once and for each new SSL cosocket, we just feed the SSL ctx object to it directly. Other custom SSL configurations added in the future can be configured in a similar way. What do you think?

@agentzh I agree Loading the raw certificates upon every SSL cosocket is quite expensive and the client-side certificates tend to be reused over and over again.So how about the new directivelua_ssl_client_keyandlua_ssl_client_certificate` to feed SSL ctx object to it directly? Also can we provide this Lua API to allow user to set different certificates on the fly?

Just now i misunderstand what you said, now i do understand. I agree to add new Lua API to be easily configure custom SSL configurations, And do you have a good suggestion about the new Lua API?

I understand the loading part that @agentzh as actually parsing the certificates (which would be solved by re-using the existing API as I already commented).

However I also think that adding the certificate to the "global" SSL_CTX is wrong, since that would affect any other SSL objects created from it (past and present), so having an API to create a new SSL_CTX and apply configurations to it would be best.

detailyang · 2017-01-28T12:31:53Z

@agentzh so sorry long time to reply.

I know you are thinking a different Lua API. Can you explain to me what's the different Lua API so that to change this codebase? Now I have enough time to follow your minds.

By the way, happy new year 2017:)

agentzh · 2017-01-30T05:41:20Z

@detailyang I'm not exactly sure at this point. Maybe something like this?

local ssl = require "ngx.ssl"
local ctx = ssl.create_ctx{
    client_certificate = "pem-data",
}

-- we can cache the ctx object in a cache like lua-resty-lrucache

local sock = ngx.socket.tcp()
local ok, err = sock:setsslctx(ctx)
sock:sslhandshake(...)

@lziest @ghedo @doujiang24 What do you think?

detailyang · 2017-01-30T10:46:47Z

@agentzh well, it looks like not bad. I will attempt to implement it in localhost

ghedo · 2017-02-02T00:52:12Z

src/ngx_http_lua_ssl.c

+    * Reading the PEM-formatted certificate from memory into an X509
+    */
+
+    x509 = PEM_read_bio_X509(cbio, NULL, 0, NULL);


Given that we already have PEM (and DER) parsing functions, should users maybe use those and pass the already parsed chain to this function?

@ghedo Yesterday I have try to reuse the already parsed chain. Finally I have found these data is type of Luajit's cdata and we cannot reference these data in Lua CFunction.:(

ghedo · 2017-02-02T01:01:35Z

@agentzh yes, that looks better, since it would avoid setting the certificate for the global SSL_CTX (so other SSLs wouldn't get the same configuration). An alternative API would be having a ngx_http_lua_socket_tcp_sslhandshake_with_ctx() which would be passed an ctx explicitly, and then reimplement ngx_http_lua_socket_tcp_sslhandshake() so that it would only call that. Would still need something to create the SSL_CTX in the first place.

ghedo · 2017-02-02T01:05:58Z

src/ngx_http_lua_ssl.c

+        goto done;
+    }
+
+    pkey = PEM_read_bio_PrivateKey(pbio, NULL,


As above. We don't yet support parsing keys with passwords, but that should be added to the existing API instead IMO, so it would be more generally useful. Or better yet, add an additional function (e.g. ngx_http_lua_ffi_parse_pem_priv_key_with_password()) to do just that.

Also, I think this should be a separate PR, so this one would get smaller and easier to review.

agentzh · 2017-02-02T01:39:55Z

@ghedo Agreed :)

detailyang · 2017-02-02T09:36:08Z

@ghedo @agentzh Thanks for yours eager suggestion. I guess I need more time to finish this feature (so sorry for my level). So just want to finish these easy parts pointed by @ghedo (parsing the private key with password), then do other parts.

I wil create another PR to add the existing API (ngx_http_lua_ffi_parse_pem_priv_key) or creating additional api (ngx_http_lua_ffi_parse_pem_priv_key_with_password) for parsing the private key with password.

What do you thinks of this?

detailyang · 2017-02-10T05:39:33Z

@agentzh @lziest @ghedo

Hello, guys. I have successfully run a demo for Lua API to feed SSL Secure Context Object. For reduce waste yours time to review, I wish we can agree on the Lua API design.

Recently I I have try to implement two versions Lua API to feed SSL_CTX* object as the following:

userdata version

local ctx = ngx.ssl.create_ctx{
    client_certificate = "pem-data",
}

-- we can cache the ctx object in a cache like lua-resty-lrucache

local sock = ngx.socket.tcp()
local ok, err = sock:setsslctx(ctx)
sock:sslhandshake(...)

This version ssl.create_ctx will return Userdata from Lua stack, it is enough. But if we want to feed more arguments to SSL_CTX * in future, we will have to add more option for options table and the create_ctx CFunction will become more and more large.

table version like ngx.socket.tcp

local ctx = ngx.ssl.ctx()
local ok, err = ctx:init({
    method = 'xxx', -- the openssl protocol methods
    cert = 'xxx', -- cert chains in PEM format 
    key = 'xxx', --  private keys in PEM format
    cacert = 'xxx'  -- the trusted CA certificates
})

if not ok then
    return error("init ssl ctx error: " .. err)
end

-- we can cache the ctx table in a cache like lua-resty-lrucache

local sock = ngx.socket.tcp()
local ok, err = sock:setsslctx(ctx)
sock:sslhandshake(...)

This version ssl.create_ctx will return Table from Lua stack, it wraps SSL_CTX * to table. This is more extended. If we want to feed more arguments to SSL_CTX *, we just add new CFunction to ctx table metatable like the following:

local ctx = ngx.ssl.ctx()
local ok, err = ctx:init({
    method = 'xxx', -- the openssl protocol methods default SSLv23_method
    cert = 'xxx', -- cert chains in PEM format 
    key = 'xxx', --  private keys in PEM format
    cacert = 'xxx' -- the trusted CA certificates in PEM format
})

if not ok then
    return error("init ssl ctx error: " .. err)
end

--we can add more api in future as the following

ctx:set_key()
ctx:set_cert()
ctx:add_cacert()
ctx:add_crl()
ctx:set_ciphres()
ctx:set_ecdhcurve()
ctx:set_dhparam()

Also I refer to the Node.js JS API Design and Node.js C++ API Design as a guide to our Lua API.

Which one is better? Need yours feedback and let us make a choice :D

detailyang · 2017-02-14T01:58:31Z

Ping @agentzh @doujiang24 @thibaultcha @lziest @ghedo. Can you have a look at the above, If you are free now. Thanks :D

agentzh · 2017-02-14T02:09:18Z

@detailyang Sorry for the delay on my side. Been massively distracted. Regarding the API design, I don't quite like the ctx.init() syntax. It should be in the ngx.ssl module of lua-resty-core. As I've demonstrated before:

local ssl = require "ngx.ssl"
local ctx = ssl.create_ctx{
    client_certificate = "pem-data",
}

-- we can cache the ctx object in a cache like lua-resty-lrucache

local sock = ngx.socket.tcp()
local ok, err = sock:setsslctx(ctx)
sock:sslhandshake(...)

But yeah, we'll have to implement the setsslctx function atop FFI instead of as a CFunction.

The ctx data returned by ssl.create_ctx() is an opaque pointer as an FFI cdata object.

detailyang · 2017-02-14T02:29:07Z

@agentzh Thanks for reply me. I have try to implement the setsslctx function atop FFI. But I'm failed because the tcp:sslhandshake is the CFunction. I'm confused that how can we pass the ctx cdata to tcp:sslhandshake CFunction. Can you give me a little hint ? After carefully rereading what you have said, I have understand. Thanks you :D

detailyang added 5 commits January 16, 2017 15:04

feature: tcpsock:setsslcert to send client cert

e72094d

style: add empty line for test

adfcb87

typo: ngx_http_lua_ssl_password_callback

34bfc06

style: add empty line before else

e233ba3

refactor: remove SSL_CTX_set_default_passwd_cb

af5f8e0

agentzh requested changes Jan 18, 2017

View reviewed changes

ghedo reviewed Feb 2, 2017

View reviewed changes

detailyang mentioned this pull request Feb 2, 2017

feature: support load private key with password #973

Closed

detailyang closed this Feb 8, 2017

detailyang reopened this Feb 8, 2017

detailyang mentioned this pull request Feb 21, 2017

feature: support ssl.create_ctx and tcp:setsslctx #997

Open

detailyang closed this Feb 22, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feature: add client SSL certificiate support #957

feature: add client SSL certificiate support #957

detailyang commented Jan 16, 2017 •

edited

agentzh Jan 18, 2017

detailyang Jan 19, 2017

ghedo Feb 2, 2017

agentzh Jan 18, 2017

detailyang Jan 19, 2017 •

edited

ghedo Feb 2, 2017

detailyang commented Jan 28, 2017 •

edited

agentzh commented Jan 30, 2017

detailyang commented Jan 30, 2017

ghedo Feb 2, 2017

detailyang Feb 6, 2017 •

edited

ghedo commented Feb 2, 2017 •

edited

ghedo Feb 2, 2017 •

edited

ghedo Feb 2, 2017

agentzh commented Feb 2, 2017

detailyang commented Feb 2, 2017

detailyang commented Feb 10, 2017

detailyang commented Feb 14, 2017 •

edited

agentzh commented Feb 14, 2017

detailyang commented Feb 14, 2017 •

edited

feature: add client SSL certificiate support #957

feature: add client SSL certificiate support #957

Conversation

detailyang commented Jan 16, 2017 • edited

agentzh Jan 18, 2017

Choose a reason for hiding this comment

detailyang Jan 19, 2017

Choose a reason for hiding this comment

ghedo Feb 2, 2017

Choose a reason for hiding this comment

agentzh Jan 18, 2017

Choose a reason for hiding this comment

detailyang Jan 19, 2017 • edited

Choose a reason for hiding this comment

ghedo Feb 2, 2017

Choose a reason for hiding this comment

detailyang commented Jan 28, 2017 • edited

agentzh commented Jan 30, 2017

detailyang commented Jan 30, 2017

ghedo Feb 2, 2017

Choose a reason for hiding this comment

detailyang Feb 6, 2017 • edited

Choose a reason for hiding this comment

ghedo commented Feb 2, 2017 • edited

ghedo Feb 2, 2017 • edited

Choose a reason for hiding this comment

ghedo Feb 2, 2017

Choose a reason for hiding this comment

agentzh commented Feb 2, 2017

detailyang commented Feb 2, 2017

detailyang commented Feb 10, 2017

detailyang commented Feb 14, 2017 • edited

agentzh commented Feb 14, 2017

detailyang commented Feb 14, 2017 • edited

detailyang commented Jan 16, 2017 •

edited

detailyang Jan 19, 2017 •

edited

detailyang commented Jan 28, 2017 •

edited

detailyang Feb 6, 2017 •

edited

ghedo commented Feb 2, 2017 •

edited

ghedo Feb 2, 2017 •

edited

detailyang commented Feb 14, 2017 •

edited

detailyang commented Feb 14, 2017 •

edited