Skip to content
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

feature: ssl.create_ctx and tcp:setsslctx #89

Open
wants to merge 26 commits into
base: master
Choose a base branch
from

Conversation

detailyang
Copy link
Contributor

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
Copy link
Contributor Author

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

lib/ngx/ssl.lua Outdated
_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),
Copy link
Member

Choose a reason for hiding this comment

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

bor can accept multiple arguments, like:

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

gotcha

lib/ngx/ssl.lua Outdated
local protocols = default_protocols

if options.protocols ~= nil then
protocols = options.protocols
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

gotcha local protocols = options.protocols or default_protocols


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

Choose a reason for hiding this comment

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

better use nil instead false here?

Copy link
Contributor Author

@detailyang detailyang Feb 28, 2017

Choose a reason for hiding this comment

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

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

@detailyang
Copy link
Contributor Author

detailyang commented Mar 11, 2017

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
Copy link
Contributor Author

CI passed after merge newest commit on master branch :D

lib/ngx/ssl.lua Outdated
_M.PROTOCOL_TLSv1 = 0x0008
_M.PROTOCOL_TLSv1_1 = 0x0010
_M.PROTOCOL_TLSv1_2 = 0x0020
local default_protocols = bor(_M.PROTOCOL_SSLv3, _M.PROTOCOL_TLSv1,
Copy link
Member

Choose a reason for hiding this comment

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

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


int
ngx_http_lua_ffi_socket_tcp_setsslctx(ngx_http_request_t *r,
void *u, void *cdata_ctx, char **err);
Copy link
Member

Choose a reason for hiding this comment

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

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

t/ssl-ctx.t Outdated
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({
Copy link
Member

Choose a reason for hiding this comment

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

You know that we can safely omit the parentheses here?

lib/ngx/ssl.lua Outdated
return nil, ffi_str(errmsg[0])
end

ctx = ffi_gc(ctx, C.ngx_http_lua_ffi_ssl_ctx_free)
Copy link
Member

Choose a reason for hiding this comment

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

We do not need to assign to ctx here.

lib/ngx/ssl.lua Outdated
_M.PROTOCOL_SSLv3 = 0x0004
_M.PROTOCOL_TLSv1 = 0x0008
_M.PROTOCOL_TLSv1_1 = 0x0010
_M.PROTOCOL_TLSv1_2 = 0x0020
Copy link
Member

Choose a reason for hiding this comment

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

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

@detailyang
Copy link
Contributor Author

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants