-
Notifications
You must be signed in to change notification settings - Fork 2k
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: support ssl.create_ctx and tcp:setsslctx #997
base: master
Are you sure you want to change the base?
Changes from 7 commits
e859096
91a6d7d
635a569
1c10853
2ec655d
df4387b
7086ff1
ca8c3cb
6ce2ea5
1b2eaac
697f0eb
0409076
17c3141
8b7e0f5
a30bbf3
73c5aa8
0b04b54
902ec8d
c94df6d
6e38b47
8e06dc5
c105935
69f841f
3bcc9f7
f52e7a2
b57b9db
f13fd50
fb52c37
1f1b090
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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; | ||
|
@@ -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); | ||
|
||
#endif /* NGX_LUA_NO_FFI_API */ | ||
|
||
#endif /* NGX_HTTP_SSL */ | ||
|
||
lua_rawset(L, LUA_REGISTRYINDEX); | ||
/* }}} */ | ||
|
||
|
@@ -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; | ||
|
@@ -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; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should increment ref count of |
||
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) | ||
{ | ||
|
@@ -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) | ||
{ | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Style: alignment issue. The |
||
#endif | ||
|
||
unsigned ft_type:16; | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -13,6 +13,11 @@ | |
#if (NGX_HTTP_SSL) | ||
|
||
|
||
#define ngx_http_lua_ssl_check_method(method, method_len, s) \ | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Better use Lua inline functions instead of macros for this. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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; | ||
|
||
|
||
|
@@ -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) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think if we should inherit existing configurations from There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Well. inheriting existing configurations from There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @detailyang We'll add cosocket support to |
||
*err = "failed to create ssl ctx"; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Well, this is not a priority though. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Here we reuse the |
||
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; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is this really requiring OpenSSL 1.0.2e+? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The API SSL_CTX_add1_chain_cert says
So I choose to requiring 1.0.2e in order to keep consistent with other function requires There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 #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? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @agentzh I think agentzh just means remove the There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @detailyang Okay, I see, yes we need
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @doujiang24 Maybe the
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++) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why call There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think we can simply save the result of the first |
||
|
||
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); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. style: maybe bellow can be better?
I mean we don't need one argument one line. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. gotacha :D There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 */ |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.