-
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 12 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 |
---|---|---|
|
@@ -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 |
---|---|---|
|
@@ -34,4 +34,129 @@ 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) | ||
{ | ||
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; | ||
} | ||
|
||
ngx_log_debug2(NGX_LOG_DEBUG_HTTP, ssl.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); | ||
|
||
SSL_CTX_free(ssl_ctx); | ||
} | ||
|
||
|
||
#endif /* NGX_LUA_NO_FFI_API */ | ||
|
||
|
||
#endif /* NGX_HTTP_SSL */ |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,73 @@ | ||
# vim:set ft= ts=4 sw=4 et fdm=marker: | ||
|
||
use Test::Nginx::Socket::Lua; | ||
use Digest::MD5 qw(md5_hex); | ||
|
||
repeat_each(3); | ||
|
||
plan tests => repeat_each() * (blocks()); | ||
|
||
$ENV{TEST_NGINX_HTML_DIR} ||= html_dir(); | ||
|
||
log_level 'debug'; | ||
|
||
no_long_string(); | ||
|
||
add_block_preprocessor(sub { | ||
my $block = shift; | ||
|
||
if (!defined $block->user_files) { | ||
$block->set_value("user_files", <<'_EOC_'); | ||
>>> defines.lua | ||
local ffi = require "ffi" | ||
|
||
ffi.cdef[[ | ||
void *ngx_http_lua_ffi_ssl_ctx_init(unsigned int protocols, char **err); | ||
|
||
void ngx_http_lua_ffi_ssl_ctx_free(void *cdata); | ||
|
||
int ngx_http_lua_ffi_ssl_ctx_set_priv_key(void *cdata_ctx, | ||
void *cdata_key, char **err); | ||
|
||
int ngx_http_lua_ffi_ssl_ctx_set_cert(void *cdata_ctx, | ||
void *cdata_cert, char **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. Let's simply test the lua-resty-core's Lua API wrappers here in lua-nginx-module's test suite. Such things add to maintenance overhead. 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. @agentzh Sorry for my poor English :(. 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 Yeah, you got it :) |
||
_EOC_ | ||
} | ||
|
||
my $http_config = $block->http_config || ''; | ||
$http_config .= <<'_EOC_'; | ||
lua_package_path "$prefix/html/?.lua;;"; | ||
_EOC_ | ||
$block->set_value("http_config", $http_config); | ||
}); | ||
|
||
run_tests(); | ||
|
||
__DATA__ | ||
|
||
=== TEST 1: ssl ctx init and free | ||
--- log_level: debug | ||
--- config | ||
location /t { | ||
content_by_lua_block { | ||
require "defines" | ||
local ffi = require "ffi" | ||
local errmsg = ffi.new("char *[1]") | ||
local ctx = ffi.C.ngx_http_lua_ffi_ssl_ctx_init(0, errmsg) | ||
if ctx == nil then | ||
ngx.say(ffi.string(errmsg[0])) | ||
return | ||
end | ||
|
||
ffi.C.ngx_http_lua_ffi_ssl_ctx_free(ctx) | ||
} | ||
} | ||
--- request | ||
GET /t | ||
--- ignore_response | ||
--- grep_error_log eval: qr/lua ssl ctx (?:init|free): [0-9A-F]+:\d+/ | ||
--- grep_error_log_out eval | ||
qr/^lua ssl ctx init: ([0-9A-F]+):1 | ||
lua ssl ctx free: ([0-9A-F]+):1 | ||
$/ |
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.
Should increment ref count of
SSL_CTX*
?