Skip to content

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

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.

Sign up for GitHub

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

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

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

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
29 commits
Select commit Hold shift + click to select a range
e859096
doc: log level constatns add nginx phase
detailyang Nov 2, 2016
91a6d7d
Merge remote-tracking branch 'openresty/master'
detailyang Jan 13, 2017
635a569
Merge remote-tracking branch 'openresty/master'
detailyang Feb 2, 2017
1c10853
Merge remote-tracking branch 'openresty/master'
detailyang Feb 4, 2017
2ec655d
feature: ssl.create_ctx and tcp:setsslctx
detailyang Feb 19, 2017
df4387b
tests: remove unused openssl version judge
detailyang Feb 21, 2017
7086ff1
refactor: use protocols as the arg to create_ctx
detailyang Feb 26, 2017
ca8c3cb
refactor: remove unsed C macro
detailyang Feb 27, 2017
6ce2ea5
tests: remove duplicated tests
detailyang Feb 27, 2017
1b2eaac
refactor: remove superfluous variable
detailyang Feb 28, 2017
697f0eb
style: combine arguments to one line
detailyang Feb 28, 2017
0409076
refactor: expose tcp object metatable to REGISTRY
detailyang Feb 28, 2017
17c3141
refactor: caller should allocate error message buf
detailyang Mar 8, 2017
8b7e0f5
style: do not exceed 80 columns in source code
detailyang Mar 8, 2017
a30bbf3
tests: use lua-resty-core to test FFI
detailyang Mar 8, 2017
73c5aa8
refactor: copy literal string to caller err buffer
detailyang Mar 11, 2017
0b04b54
travis: use personal lua-resty-core to pass test
detailyang Mar 11, 2017
902ec8d
refactor: use ngx_min to decide the size of msg
detailyang Mar 11, 2017
c94df6d
style: align function argments
detailyang May 5, 2017
6e38b47
style: align function arguemnts (ditto)
detailyang May 5, 2017
8e06dc5
tests: remove unused module
detailyang May 7, 2017
c105935
style: align for aesthetic considerations
detailyang May 7, 2017
69f841f
refactor: replace ngx_copy to ngx_memcpy
detailyang May 7, 2017
3bcc9f7
Merge remote-tracking branch 'openresty/master' into lua-ffi-api-sslctx
detailyang May 7, 2017
f52e7a2
style: variable name tweaks
detailyang May 17, 2017
b57b9db
Merge remote-tracking branch 'openresty/master' into lua-ffi-api-sslctx
detailyang May 21, 2017
f13fd50
Merge remote-tracking branch 'openresty/master' into lua-ffi-api-sslctx
detailyang May 25, 2017
fb52c37
style: variable declaration in #if block
detailyang May 26, 2017
1f1b090
feature: support ciphers, CRL, ca, cert_store
detailyang Jun 1, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
56 changes: 55 additions & 1 deletion src/ngx_http_lua_socket_tcp.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -183,6 +183,12 @@ enum {
}


#if (NGX_HTTP_SSL)

#define ngx_http_lua_ngx_socket_tcp_mt_key "__ngx_socket_tcp_mt"

#endif

static char ngx_http_lua_req_socket_metatable_key;
static char ngx_http_lua_raw_req_socket_metatable_key;
static char ngx_http_lua_tcp_socket_metatable_key;
Expand Down Expand Up @@ -316,6 +322,19 @@ ngx_http_lua_inject_socket_tcp_api(ngx_log_t *log, lua_State *L)

lua_pushvalue(L, -1);
lua_setfield(L, -2, "__index");

#if (NGX_HTTP_SSL)

#ifndef NGX_LUA_NO_FFI_API

/* expose tcp object metatable to global for FFI */
lua_pushvalue(L, -1);
lua_setglobal(L, ngx_http_lua_ngx_socket_tcp_mt_key);
Copy link
Member

Choose a reason for hiding this comment

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

maybe we'd better set it into REGISTRY table? And we can get it by debug.getregistry.
like: https://github.com/openresty/lua-resty-core/blob/master/lib/resty/core/ctx.lua#L12

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agree. Put it into REGISTRY table is better than global table.


#endif /* NGX_LUA_NO_FFI_API */

#endif /* NGX_HTTP_SSL */

lua_rawset(L, LUA_REGISTRYINDEX);
/* }}} */

Expand Down Expand Up @@ -587,6 +606,12 @@ ngx_http_lua_socket_tcp_connect(lua_State *L)

u->conf = llcf;

#if (NGX_HTTP_SSL)

u->ssl = llcf->ssl;

#endif

pc = &u->peer;

pc->log = r->connection->log;
Expand Down Expand Up @@ -1200,6 +1225,35 @@ ngx_http_lua_socket_conn_error_retval_handler(ngx_http_request_t *r,

#if (NGX_HTTP_SSL)


#ifndef NGX_LUA_NO_FFI_API

int
ngx_http_lua_ffi_socket_tcp_setsslctx(ngx_http_request_t *r,
ngx_http_lua_socket_tcp_upstream_t *u, void *cdata_ctx, char **err)
{
SSL_CTX *ssl_ctx = cdata_ctx;

ngx_ssl_t *ssl;

ssl = ngx_pcalloc(r->pool, sizeof(ngx_ssl_t));
if (ssl == NULL) {
*err = "no memory";
return NGX_ERROR;
}

ssl->ctx = ssl_ctx;
Copy link

Choose a reason for hiding this comment

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

Should increment ref count of SSL_CTX*?

ssl->log = u->ssl->log;
ssl->buffer_size = u->ssl->buffer_size;

u->ssl = ssl;

return NGX_OK;
}

#endif /* NGX_LUA_NO_FFI_API */


static int
ngx_http_lua_socket_tcp_sslhandshake(lua_State *L)
{
Expand Down Expand Up @@ -1286,7 +1340,7 @@ ngx_http_lua_socket_tcp_sslhandshake(lua_State *L)
return 1;
}

if (ngx_ssl_create_connection(u->conf->ssl, c,
if (ngx_ssl_create_connection(u->ssl, c,
NGX_SSL_BUFFER|NGX_SSL_CLIENT)
!= NGX_OK)
{
Expand Down
1 change: 1 addition & 0 deletions src/ngx_http_lua_socket_tcp.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -92,6 +92,7 @@ struct ngx_http_lua_socket_tcp_upstream_s {

#if (NGX_HTTP_SSL)
ngx_str_t ssl_name;
ngx_ssl_t *ssl;
Copy link
Member

Choose a reason for hiding this comment

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

Style: alignment issue. The ssl identifier should be aligned, excluding the * prefix.

#endif

unsigned ft_type:16;
Expand Down
136 changes: 136 additions & 0 deletions src/ngx_http_lua_ssl.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,11 @@
#if (NGX_HTTP_SSL)


#define ngx_http_lua_ssl_check_method(method, method_len, s) \
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 Lua inline functions instead of macros for this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This macro is remaining after refactoring and should be removed.

(method_len == sizeof(s) - 1 \
&& ngx_strncmp((method), (s), method_len) == 0)


int ngx_http_lua_ssl_ctx_index = -1;


Expand All @@ -34,4 +39,135 @@ ngx_http_lua_ssl_init(ngx_log_t *log)
}


#ifndef NGX_LUA_NO_FFI_API


void *
ngx_http_lua_ffi_ssl_ctx_init(ngx_uint_t protocols, char **err)
{
SSL_CTX *ssl_ctx;
ngx_ssl_t ssl;

ssl.log = ngx_cycle->log;
if (ngx_ssl_create(&ssl, protocols, NULL) != NGX_OK) {
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 if we should inherit existing configurations from ngx_http_lua_loc_conf_t's ssl field? Please see ngx_http_lua_set_ssl for more details. And here in the create_ctx API, we can simply allow the user to override the default settings. This would be better.

Copy link
Contributor Author

@detailyang detailyang Jun 1, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Well. inheriting existing configurations from ngx_http_lua_loc_conf_ts ssl field will required this API need ngx_http_request_t. we should allow it run any phase. Especially, it can be allowed to run in init_by_lua_* to easily cache it. May we expose these FFI API (like ciphers, crl, ca and so on) to Lua Land . It will be more flexible and effective to feed arguments to SSL_CTX object.

Copy link
Member

@agentzh agentzh Jun 1, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

@detailyang We'll add cosocket support to init_by_lua* soon. By then it will also have a (fake) request object, just like the timer handler. So I don't think this is a showstopper. Any other things I'm overlooking here?

*err = "failed to create ssl ctx";
Copy link
Member

Choose a reason for hiding this comment

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

Also, I think it would be better to return OpenSSL's own error messages if there's any. Otherwise fixed error messages made up in the caller does not give much helpful info.

Copy link
Member

Choose a reason for hiding this comment

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

Well, this is not a priority though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here we reuse the ngx_ssl_create API. However we cannot get more OpenSSL's error info from this API. Surely, we can rewrite this API to get more detail if we want :)

ngx_log_error(NGX_LOG_ERR, ssl.log, 0, *err);
return NULL;
}

ssl_ctx = ssl.ctx;

ngx_log_debug2(NGX_LOG_DEBUG_HTTP, ngx_cycle->log, 0,
"lua ssl ctx init: %p:%d", ssl_ctx, ssl_ctx->references);

return ssl_ctx;
}


int
ngx_http_lua_ffi_ssl_ctx_set_cert(void *cdata_ctx, void *cdata_cert, char **err)
{
#ifdef LIBRESSL_VERSION_NUMBER

*err = "LibreSSL not supported";
return NGX_ERROR;

#else

# if OPENSSL_VERSION_NUMBER < 0x1000205fL

*err = "at least OpenSSL 1.0.2e required but found " OPENSSL_VERSION_TEXT;
Copy link
Member

Choose a reason for hiding this comment

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

Is this really requiring OpenSSL 1.0.2e+?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The API SSL_CTX_add1_chain_cert says

These functions were first added to OpenSSL 1.0.2.

So I choose to requiring 1.0.2e in order to keep consistent with other function requires

Copy link
Member

Choose a reason for hiding this comment

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

@detailyang Okay. Good to know :)

return NGX_ERROR;

# else

X509 *x509 = NULL;
SSL_CTX *ssl_ctx = cdata_ctx;
STACK_OF(X509) *cert = cdata_cert;

#ifdef OPENSSL_IS_BORINGSSL
Copy link
Member

Choose a reason for hiding this comment

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

Here it should use 2 levels of indentation after #.

size_t i;
#else
int i;
#endif

if (sk_X509_num(cert) < 1) {
*err = "sk_X509_num() failed";
ngx_ssl_error(NGX_LOG_ERR, ngx_cycle->log, 0, *err);
return NGX_ERROR;
}

x509 = sk_X509_value(cert, 0);
if (x509 == NULL) {
*err = "sk_X509_value() failed";
ngx_ssl_error(NGX_LOG_ERR, ngx_cycle->log, 0, *err);
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 instead of logging the real error message into nginx's error log file, we should propagate it to the Lua land via the err output parameter (of course, the type of err needs adjustment). Let's avoid direct logging in these FFI C functions. We should let the Lua land caller to handle these error and inspect these error messages, which is much more flexible.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed. propagating the OpenSSL error message to Lua land is more flexible. So I'm thinking expose the error code by unsigned long ERR_get_error(void);, let the Lua land caller to get the error string with error code by these functions as the following:

 #include <openssl/err.h>

 char *ERR_error_string(unsigned long e, char *buf);
 void ERR_error_string_n(unsigned long e, char *buf, size_t len);

 const char *ERR_lib_error_string(unsigned long e);
 const char *ERR_func_error_string(unsigned long e);
 const char *ERR_reason_error_string(unsigned long e);

Also we will implement the FFI API upon these functions. @agentzh How about this?

Copy link
Member

Choose a reason for hiding this comment

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

@agentzh I think agentzh just means remove the ngx_ss_error here :)
Seems we don't need ERR_get_error here?

Copy link
Contributor Author

@detailyang detailyang Mar 4, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

@doujiang24 uhh, It looks like I'm worrying. But how to actively get the specific error message in Lua land without call ERR_get_error ?

Copy link
Member

Choose a reason for hiding this comment

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

@detailyang Okay, I see, yes we need ERR_get_error.
But I think we should do it in C land, something like this:

int ffi_c_func(char *err, int *err_len) {
    /* get message by ERR_get_error or something else */
    err_len = sprintf(err, message) - err;
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@doujiang24 Maybe the err_len is no need. I check the API char *ERR_error_string(unsigned long e, char *buf); It says:

ERR_error_string() generates a human-readable string representing the error code e, and places it at buf. buf must be at least 120 bytes long. If buf is NULL , the error string is placed in a static buffer.

So pointing the static buffer is not bad IMO as the following:

   if (SSL_CTX_use_PrivateKey(ssl_ctx, key) == 0) {
        e = ERR_get_error();
        if (e == 0) {
            *err = "SSL_CTX_use_PrivateKey() failed";

        } else {
            *err = ERR_error_string(e, NULL);
        }

        return NGX_ERROR;
    }

@doujiang24 @agentzh How about this?

return NGX_ERROR;
}

if (SSL_CTX_use_certificate(ssl_ctx, x509) == 0) {
*err = "SSL_CTX_use_certificate() failed";
ngx_ssl_error(NGX_LOG_ERR, ngx_cycle->log, 0, *err);
return NGX_ERROR;
}

/* read rest of the chain */

for (i = 1; i < sk_X509_num(cert); i++) {
Copy link
Member

Choose a reason for hiding this comment

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

Why call sk_X059_num for more than once?

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 we can simply save the result of the first sk_X509_num above to a local C variable.


x509 = sk_X509_value(cert, i);
if (x509 == NULL) {
*err = "sk_X509_value() failed";
ngx_ssl_error(NGX_LOG_ERR, ngx_cycle->log, 0, *err);
return NGX_ERROR;
}

if (SSL_CTX_add1_chain_cert(ssl_ctx, x509) == 0) {
*err = "SSL_add1_chain_cert() failed";
ngx_ssl_error(NGX_LOG_ERR, ngx_cycle->log, 0, *err);
return NGX_ERROR;
}
}

return NGX_OK;

# endif /* OPENSSL_VERSION_NUMBER < 0x1000205fL */
#endif
}


int
ngx_http_lua_ffi_ssl_ctx_set_priv_key(void *cdata_ctx, void *cdata_key,
char **err)
{
SSL_CTX *ssl_ctx = cdata_ctx;
EVP_PKEY *key = cdata_key;

if (SSL_CTX_use_PrivateKey(ssl_ctx, key) == 0) {
*err = "SSL_CTX_use_PrivateKey() failed";
ngx_ssl_error(NGX_LOG_ERR, ngx_cycle->log, 0, *err);
return NGX_ERROR;
}

return NGX_OK;
}


void
ngx_http_lua_ffi_ssl_ctx_free(void *cdata)
{
SSL_CTX *ssl_ctx = cdata;

ngx_log_debug2(NGX_LOG_DEBUG_HTTP, ngx_cycle->log,
0,
"lua ssl ctx free: %p:%d",
ssl_ctx,
ssl_ctx->references);
Copy link
Member

Choose a reason for hiding this comment

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

style: maybe bellow can be better?

0, "lua ssl ctx free: %p:%d",
ssl_ctx, ssl_ctx->references);

I mean we don't need one argument one line.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

gotacha :D

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe it's better as the following ?

    ngx_log_debug2(NGX_LOG_DEBUG_HTTP, ngx_cycle->log,
                   0, "lua ssl ctx free: %p:%d", ssl_ctx, ssl_ctx->references);

It will do not exceed 80 columns.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, one line is better :)


SSL_CTX_free(ssl_ctx);
}


#endif /* NGX_LUA_NO_FFI_API */


#endif /* NGX_HTTP_SSL */
Loading