feature: ssl.create_ctx and tcp:setsslctx by detailyang · Pull Request #89 · openresty/lua-resty-core

detailyang · 2017-02-21T04:04:00Z

This is a sister PR at lua-nginx-module/pull/997.

I refer to Node.js JS Design as a guide. Now the API is the fowllong:

local ssl = require "ngx.ssl"
local ctx, err = ssl.create_ctx{
    method = "SSLv23_method", #default SSLv23_method
    cert = "pem-data", #parsed by ssl.parse_pem_cert
    priv_key = "pem-data", #parsed by ssl.parse_pem_priv_key
}

if ctx ~= nil then
    return error("create ssl ctx error" .. err)
end

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

local sock = ngx.socket.tcp()
local ok, err = sock:setsslctx(ctx) #the setsslctx is a Lua function atop FFI whcich been implement on lua-resty-core
sock:sslhandshake(...)

Now we proivde 3 arguments as the follwoing to feed SSL_CTX object.

method
Openssl protocols are listed as OPENSSL PROTOCOL METHODS
cert
Optional certificate in PEM format which can be parsed by ssl.parse_pem_cert
priv_key
Optional private keys in PEM format which can be parsed by ssl.parse_pem_priv_key

It 's convenient that if we want to expose more arguments to user in future if we want.

Now it is not ready to provide the README document. when we agree on the API Design, I will provide.

@agentzh @thibaultcha @doujiang24

Can have a look at this PR ? Many thanks :D

detailyang · 2017-02-26T13:24:59Z

After refactor the codebase, these is a little different things. We remove the method argument, and provide the protocols argument. The protocols argument is the SSL.PROTOCOL_SSLv2|PROTOCOL_SSLv3|PROTOCOL_TLSv1|PROTOCOL_TLSv1_1|PROTOCOL_TLSv1_2

doujiang24 · 2017-02-28T03:34:25Z

lib/ngx/ssl.lua

+_M.PROTOCOL_TLSv1 = 0x0008
+_M.PROTOCOL_TLSv1_1 = 0x0010
+_M.PROTOCOL_TLSv1_2 = 0x0020
+local default_protocols = bor(bor(bor(_M.PROTOCOL_SSLv3,_M.PROTOCOL_TLSv1),


bor can accept multiple arguments, like:

resty -e " ngx.say(bit.bor(1, 2, 4)); " 7

doujiang24 · 2017-02-28T03:35:54Z

lib/ngx/ssl.lua

+    local protocols = default_protocols
+
+    if options.protocols ~= nil then
+        protocols = options.protocols


I think local protocols = options.protocols or options.protocols can be simpler :)

gotcha local protocols = options.protocols or default_protocols

doujiang24 · 2017-02-28T06:26:15Z

lib/resty/core/socket/tcp.lua

+
+    local rc = C.ngx_http_lua_ffi_socket_tcp_setsslctx(r, tcp, ssl_ctx, errmsg)
+    if rc ~= FFI_OK then
+        return false, ffi_str(errmsg[0])


better use nil instead false here?

Agree. I found other statements is also using nil rather than false.

detailyang · 2017-03-11T09:24:53Z

Now the CI failed since the newest luacheck ignore the custom API of coroutine.

I create a PR to supplement these API or we can specify the older version of luacheck in travis.yml

detailyang · 2017-03-12T07:19:40Z

CI passed after merge newest commit on master branch :D

agentzh · 2017-05-05T18:46:44Z

lib/ngx/ssl.lua

+_M.PROTOCOL_TLSv1 = 0x0008
+_M.PROTOCOL_TLSv1_1 = 0x0010
+_M.PROTOCOL_TLSv1_2 = 0x0020
+local default_protocols = bor(_M.PROTOCOL_SSLv3, _M.PROTOCOL_TLSv1,


Better put a blank line before this line for aesthetic considerations.

agentzh · 2017-05-05T18:47:33Z

lib/resty/core/socket/tcp.lua

+
+    int
+    ngx_http_lua_ffi_socket_tcp_setsslctx(ngx_http_request_t *r,
+        void *u, void *cdata_ctx, char **err);


Please be consistent with the style in lib/ngx/ssl.lua.

agentzh · 2017-05-05T18:50:26Z

t/ssl-ctx.t

+    local cert =  ssl.parse_pem_cert(read_file("$TEST_NGINX_HTML_DIR/client.crt"))
+    local priv_key = ssl.parse_pem_priv_key(read_file("$TEST_NGINX_HTML_DIR/client.unsecure.key"))
+
+    local ssl_ctx, err = ssl.create_ctx({


You know that we can safely omit the parentheses here?

agentzh · 2017-05-05T18:53:02Z

lib/ngx/ssl.lua

+        return nil, ffi_str(errmsg[0])
+    end
+
+    ctx = ffi_gc(ctx, C.ngx_http_lua_ffi_ssl_ctx_free)


We do not need to assign to ctx here.

agentzh · 2017-05-05T18:56:24Z

lib/ngx/ssl.lua

+_M.PROTOCOL_SSLv3 = 0x0004
+_M.PROTOCOL_TLSv1 = 0x0008
+_M.PROTOCOL_TLSv1_1 = 0x0010
+_M.PROTOCOL_TLSv1_2 = 0x0020


I thikn we can just omit the PROTOCOL_ prefix from these keys. They are not really needed IMHO.

Signed-off-by: detailyang <detailyang@gmail.com>

…a-resty-core into lua-ffi-api-sslctx

detailyang · 2017-05-07T17:01:58Z

Fix the issue as the following and rebase the newest codebase:

omit prefix PROTOCOL
alignment
no need to assign ctx again after ffi_gc
omit parentheses only one argument

Sorry for duplicated commits because of wrong usage of git rebase

detailyang mentioned this pull request Feb 21, 2017

feature: support ssl.create_ctx and tcp:setsslctx openresty/lua-nginx-module#997

Open

doujiang24 reviewed Feb 28, 2017

View reviewed changes

agentzh requested changes May 5, 2017

View reviewed changes

detailyang added 13 commits May 7, 2017 22:43

feature: support ssl.create_ctx and tcp:setsslctx

15c95e7

Signed-off-by: detailyang <detailyang@gmail.com>

tests: test sslctx with lrucache

9407f3c

Signed-off-by: detailyang <detailyang@gmail.com>

refactor: use protocols as arg to create_ctx

818327e

Signed-off-by: detailyang <detailyang@gmail.com>

refactor: get tcp metatable from REGISTRY

a81d912

refactor: use nil to replace false as return value

1099992

refactor: caller allocate error message buffer

896f36e

refactor: copy literal to caller error buffer

aa58488

travis: use personal lua-nginx-module to pass test

f4fa4bc

style: remove unused variable

f1be855

refactor: omit PROTOCOL prefix in ngx.ssl

ea2e634

style: ffi.cdef should not be indented

2052108

refactor: no need to assign ctx again after ffi_gc

4b26b17

style: omit parentheses only one argument

9ebd5b6

detailyang force-pushed the lua-ffi-api-sslctx branch from bb43bbb to 9ebd5b6 Compare May 7, 2017 15:09

detailyang added 5 commits May 8, 2017 00:38

tests: fix create_ctx must pass options

23b1d04

cert: regenerate ca client server key and crt

38fb434

cert: regenerate ca client server key and crt

a2d7c28

travis: empty commit to trigger travis-ci

91a0065

Merge branch 'lua-ffi-api-sslctx' of https://github.com/detailyang/lu…

e300597

…a-resty-core into lua-ffi-api-sslctx

detailyang added 4 commits May 17, 2017 13:16

style: rename variable err_buf to err

6fbc8a2

Merge remote-tracking branch 'openresty/master' into lua-ffi-api-sslctx

cd4f37a

Merge remote-tracking branch 'openresty/master' into lua-ffi-api-sslctx

ea18436

feature: add ciphers, CRL, ca, cert_store

930aa47

detailyang added 4 commits June 1, 2017 15:32

tests: ssl-ctx throw error when open file failed

bfca881

tests: support ubuntu system certs

91c58e1

tests: add travis certificate path

b795d4e

tests: ssl-ctx.t style tweaks

3f5ce42

mikz mentioned this pull request Oct 3, 2017

Ability to set client certificate per API 3scale/APIcast#440

Closed

agentzh mentioned this pull request Jan 2, 2018

ngx.process.master_pid #158

Closed

Conversation

detailyang commented Feb 21, 2017

Uh oh!

detailyang commented Feb 26, 2017

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!

detailyang Feb 28, 2017 • 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!

detailyang commented Mar 11, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

detailyang commented Mar 12, 2017

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!

detailyang commented May 7, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

detailyang Feb 28, 2017 •

edited

Loading

detailyang commented Mar 11, 2017 •

edited

Loading