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

detailyang · 2017-02-21T03:53:31Z

I hereby granted the copyright of the changes in this pull request
to the authors of this lua-nginx-module project.

This PR have been discussed at #957. These FFI API as the following:

ngx_http_lua_ffi_ssl_ctx_init(const u_char *method, size_t method_len, char **err)
ngx_http_lua_ffi_ssl_ctx_set_cert(void *cdata_ctx, void *cdata_cert, char **err)
ngx_http_lua_ffi_ssl_ctx_set_priv_key(void *cdata_ctx, void *cdata_key, char **err)
ngx_http_lua_ffi_ssl_ctx_free(void *cdata)

I refer to the Node.js C++ as a guide. Now It exposes set_cert and set_priv_key to FFI and It's convenient that to feed more argument to SSL_CTX object in future if we want.

need sure we all agree on this API Design:)

These is a sister pr about lua-resty-core. The API usage is as the following:

local ssl = require "ngx.ssl"
local ctx, err = ssl.create_ctx{
    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(...)

@agentzh @thibaultcha @doujiang24
have a look at this PR? Many thanks :D

Signed-off-by: detailyang <[email protected]>

ghedo · 2017-02-21T23:15:20Z

It seems there is quite a lot of code duplication from NGINX which is unnecessary IMO.

Why not make ngx_http_lua_ffi_ssl_ctx_init() allocate and return the ngx_ssl_t instead of the SSL_CTX? This way you can simply call ngx_ssl_create() (with llcf->ssl_protocols) in there and avoid all the duplication. The ngx_ssl_t now becomes your cdata pointer.

Then you can use that ngx_ssl_t in ngx_http_lua_ffi_socket_tcp_setsslctx() instead of allocating a new one. This would also make the method parameter superfluous, which is a good thing IMO.

Also, the currently implemented tests are probably not enough to test the functionality.

detailyang · 2017-02-22T02:49:39Z

@ghedo Thanks for you review :D

It seems there is quite a lot of code duplication from NGINX which is unnecessary IMO.

These duplicated code will appear because I cannot reuse ngx_ssl_create function. I have consider to alloc ngx_ssl_t in FFI Function on ngx_cycle->pool, I'm not sure it is the best practice to alloc memory in FFI Function on ngx_cycle->pool. So I choose the SSL_CTX.

This would also make the method parameter superfluous, which is a good thing IMO.

Since we have provide API to feed the SSL_CTX object, so I choose to provide the superfluous method to more purely control SSL negotiation protocols not default llcf->ssl_protocols.

Also, the currently implemented tests are probably not enough to test the functionality.

Just simply test in lua-nginx-module and most tests is in lua-resty-core.

@agentzh I think @ghedo' suggestion is almost good for me. How do you think of it ? By the way, I will continue to refactor until we agree on the API Design :D

ghedo · 2017-02-25T13:16:44Z

I have consider to alloc ngx_ssl_t in FFI Function on ngx_cycle->pool, I'm not sure it is the best practice to alloc memory in FFI Function on ngx_cycle->pool.

ngx_http_lua_ssl_ctx_create_method() can probably take a ngx_http_request_t as argument, like the other FFI functions do, that way you can use r->pool.

Since we have provide API to feed the SSL_CTX object, so I choose to provide the superfluous method to more purely control SSL negotiation protocols not default llcf->ssl_protocols.

IMO it would still be better to have a string parameter in ngx_http_lua_ssl_ctx_create_method() that uses the same format as lua_ssl_protocols. You wouldn't have to use llcf->ssl_protocols itself, just the string format. This way you could stilll use ngx_ssl_create(), you wouldn't expose the underlying OpenSSL API and it would be more consistent.

But really, configuring enabled protocols doesn't need to happen at SSL_CTX creation time. A somewhat better API IMO would separate the ctx creation (and use llcf->ssl_protocols by default, so the user doesn't need to think about this if they don't want to) but provide an additional function to change the enabled protocols if the user wants to do that. But again, IMO :)

Just simply test in lua-nginx-module and most tests is in lua-resty-core.

Well, yeah, but you also need to test the functionality you are adding. Something like creating a ctx, setting a user certificate on it, and using that ctx with a socket to connect to a server.

detailyang · 2017-02-26T02:45:17Z

@ghedo

ngx_http_lua_ssl_ctx_create_method() can probably take a ngx_http_request_t as argument, like the other FFI functions do, that way you can use r->pool.

Since we want to cache ssl_ctx in lrucache, we should make sure the lifecycle of ssl_ctx do not bind to the r->pool. I'm ready to refactor the code you are suggesting as the following:

void *ngx_http_lua_ffi_ssl_ctx_init(ngx_uint_t protocols, char **err);

We will alloc ngx_ssl_t on ngx_cycle->pool, because we want to cache ssl_ctx in lrucache.

IMO it would still be better to have a string parameter in ngx_http_lua_ssl_ctx_create_method() that uses the same format as lua_ssl_protocols. You wouldn't have to use llcf->ssl_protocols itself, just the string format. This way you could stilll use ngx_ssl_create(), you wouldn't expose the underlying OpenSSL API and it would be more consistent.
But really, configuring enabled protocols doesn't need to happen at SSL_CTX creation time. A somewhat better API IMO would separate the ctx creation (and use llcf->ssl_protocols by default, so the user doesn't need to think about this if they don't want to) but provide an additional function to change the enabled protocols if the user wants to do that. But again, IMO :)

Now that we are ready to reuse ngx_ssl_create, so let's remove the argument method, just use protocols to as the protocols argument. Also we can construct the protocols argument in lua-resty-core then pass the it to ngx_http_lua_ffi_ssl_ctx_init(ngx_uint_t protocols, char **err).

uses the same format as lua_ssl_protocols

lua_ssl_protocols SSLv3 TLSv1 TLSv1.1 TLSv1.2 is the usage of nginx conf rather than lua. Maybe bit.bor(ssl.PROTOCOL_SSLv3, ssl.PROTOCOL_TLSv1) is better?

Well, yeah, but you also need to test the functionality you are adding. Something like creating a ctx, setting a user certificate on it, and using that ctx with a socket to connect to a server.

That's good and it's worth to do to make sure the functionality is okay without lua-resty-code :D

Signed-off-by: detailyang <[email protected]>

detailyang · 2017-02-26T13:19:23Z

@ghedo After refactor the codebase, It reuse the ngx_ssl_create function. But about the ngx_ssl_t, it's alloced on the stack rather than heap. Since there is not a good way to manage the memory (malloc?) in the FFI function. So just alloc on the stack which is a not bad way IMO. In tcp:setsslctx, we alloc the ngx_ssl_t in r->pool to make sure the lifecycle of ngx_ssl_t is the same as request.

@agentzh How do you think of it?

agentzh · 2017-02-26T19:51:47Z

@detailyang We should not allocate SSL data structures in ngx_cycle->pool since for small memory blocks in the nginx memory pool, they can never be freed individually (they will only be freed when the whole pool is destroyed).

Also, we should not let the SSL CTX thing bound to r->pool either.

agentzh · 2017-02-26T19:56:28Z

@detailyang Your current implementation seems good. I'll need to have a closer look.

@doujiang24 What do you think of it?

@ghedo Thanks for your review! Appreciated!

agentzh · 2017-02-26T19:57:46Z

t/151-ssl-ctx.t

+lua_package_path "\$prefix/html/?.lua;$TEST_NGINX_LUA_PACKAGE_PATH/?.lua;;../lua-resty-lrucache/lib/?.lua;";
+
+init_by_lua_block {
+    local ffi = require "ffi"


The lua-nginx-module's test suite already depends on lua-resty-core. I think we'd better avoid duplicating such lua-resty-core code here in the test suite, which adds maintenance overhead. We can just test against the lua-resty-core Lua API directly.

Yup, these tests actually adds maintenance overhead for me, Since these tests is the same as lua-resty-core. According what you have said, Can I think that if we are adding the FFI C API, we should simply test it in lua-nginx-module and specially test in lua-resty-core ?

agentzh · 2017-02-26T20:00:01Z

src/ngx_http_lua_ssl.c

@@ -13,6 +13,11 @@
 #if (NGX_HTTP_SSL)


+#define ngx_http_lua_ssl_check_method(method, method_len, s)             \


Better use Lua inline functions instead of macros for this.

This macro is remaining after refactoring and should be removed.

agentzh · 2017-02-26T20:00:50Z

src/ngx_http_lua_ssl.c

+
+#   if OPENSSL_VERSION_NUMBER < 0x1000205fL
+
+    *err = "at least OpenSSL 1.0.2e required but found " OPENSSL_VERSION_TEXT;


Is this really requiring OpenSSL 1.0.2e+?

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

@detailyang Okay. Good to know :)

detailyang · 2017-02-28T02:45:17Z

@agentzh @doujiang24 @ghedo
I did some refactoring again, can you guys have a look at it?
Thanks :D

doujiang24 · 2017-02-28T07:20:32Z

src/ngx_http_lua_socket_tcp.c

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


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

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

doujiang24 · 2017-02-28T07:25:58Z

src/ngx_http_lua_ssl.c

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


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.

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.

Yeah, one line is better :)

detailyang · 2017-03-02T12:56:32Z

Hello :D
@agentzh @doujiang24 @ghedo How about it now about the API design and implement?

agentzh · 2017-03-03T20:58:20Z

t/151-ssl-ctx.t

+
+    int ngx_http_lua_ffi_ssl_ctx_set_cert(void *cdata_ctx,
+        void *cdata_cert, char **err);
+]]


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.

Well. The only thing I'm confused is what's the 'simply test' ? if we want to simply test the ngx_http_lua_ffi_ssl_ctx_set_priv_key and ngx_http_lua_ffi_ssl_ctx_set_cert , should we set the priv_key and cert then send it to server ? Or just set the priv_key and cert then test the return value.

@agentzh Sorry for my poor English :(.
I took a few minutes to understand what's your meaning again. You want to tell me that just require lua-resty-core API rather than call FFI.C.API to simply test functionality ?

@detailyang Yeah, you got it :)

agentzh · 2017-03-03T21:01:37Z

src/ngx_http_lua_ssl.c

+    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);


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.

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?

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

@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 ?

@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; }

@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?

ElvinEfendi · 2017-03-04T19:57:36Z

Is there any plan to implement setting ssl_trusted_certificate in the same manner(i.e ngx_http_lua_ffi_ssl_ctx_set_trusted_cert)?

Motivation: When you need to use SSL(with client verification) in init_worker context with timer.at you have to set trusted certificate in main config section. And if you do so, that certificate file is being loaded for every location context declared even though it's only needed in init_worker. In addition this can lead to a lot of redundant memory allocation(in Linux) by Openssl because when it needs more memory for storing the decoded certificate content it calls malloc and because sbrk fails(because of https://github.com/openresty/lua-nginx-module/blob/master/src/ngx_http_lua_module.c#L1294) malloc ends up calling mmap and allocates 1024x1024 bytes in every ngx_http_lua_merge_loc_conf call.

Thanks for working on this!

agentzh · 2018-02-26T05:50:17Z

@daurnimator I can see your point. But I think this relatively low level API is better to be self-contained and minimally implemented. interfacing with other Lua libraries create undesired dependencies and could also be slow.

daurnimator · 2018-02-26T12:34:53Z

src/ngx_http_lua_socket_tcp.c

+        return NGX_ERROR;
+    }
+
+    ssl->ctx = ssl_ctx;


Should increment ref count of SSL_CTX*?

daurnimator · 2018-02-26T12:36:11Z

@daurnimator I can see your point. But I think this relatively low level API is better to be self-contained and minimally implemented.

I think those two goals are at odds: I was suggesting a minimal implementation (of just setsslctx) without being self-contained (i.e. leave the SSL_CTX* creation to something else; keep it all in e.g. a lua-resty-sslctx project)

daurnimator · 2018-02-26T12:37:25Z

Also consider an API of :setssl that takes an openssl SSL* instead of an SSL_CTX*.
You can derive an SSL* from an SSL_CTX* but not the inverse; which makes SSL* more general.

agentzh · 2018-02-26T22:12:49Z

@daurnimator By "minimal" I mean zero external dependencies for this thing. So as a whole, the software is minimized (including any external deps). Sorry for the confusion.

xiwenc · 2018-05-03T11:30:04Z

@detailyang Would love to have this PR accepted. Are you still actively working on this?

pfremm · 2018-08-02T20:17:53Z

I am also curious. @detailyang will you be working on a merge?

spacewander · 2018-08-07T05:09:58Z

src/ngx_http_lua_ssl.c

@@ -34,4 +38,504 @@ ngx_http_lua_ssl_init(ngx_log_t *log)
 }


+static size_t
+ngx_http_lua_ssl_get_error(u_long e, u_char *ssl_err,
+    size_t ssl_err_len, const char *default_err, size_t default_err_len)


Just put some notes here:
OpenSSL stores errors in a queue. And it might generate multiple errors in a call.
Therefore,

You need to drain the error queue to get all errors.

Someone might only fetch the first error, and leave the others in the queue.

Current implementation of this function doesn't satisfy the deduction 1 and is vulnerable to the deduction 2.

helinbo2015 · 2018-08-09T02:59:56Z

@detailyang @agentzh
how is the progress of this PR? i am eager that this PR can be accepted by master branch.

moonming · 2019-07-05T10:46:04Z

@thibaultcha do you have time to take a look this PR? thanks.

mergify · 2020-06-26T00:40:40Z