feature: support ssl.create_ctx and tcp:setsslctx

[detailyang]

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

[ghedo]

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]

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

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]

@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

[detailyang]

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

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

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

[detailyang]

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

[detailyang]

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

[ElvinEfendi]

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!

[detailyang]

@ElvinEfendi Yup, the ssl_trusted_certificate is in our plan. But now we need sure the API Design is okay so that we do not break the API Design if we want to add new API in future :D

[agentzh]

@detailyang I think it's cleaner to let the caller allocate the error message buffer. See existing FFI C API used by lua-resty-core for such examples (like resty.core.shdict).

[detailyang]

@agentzh @doujiang24

I peek the OpenSSL implement of ERR_error_string, when buffer is NULL , it use static buffer as the return value. But In multi thread, it's wrong. So caller should allocate the error message buffer :D .

Now the new implement is as the following (POC):

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

    u_long       e;

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

        } else {
            ERR_error_string_n(e, *err_buf, err_buf_len);
        }

        return NGX_ERROR;
    }

    return NGX_OK;
}

These are two questions to point out.

1. the type of err_buf_len is size_t rather than size_t * When Setting the error message by ERR_error_string_n, cannot get the error message length. Since the ERR_error_string_n writes at most length characters (including the terminating 0) and truncates the string if necessary. So it guarantee the error message is '\0' terminating.

2. the ERR_error_string_n was added in OpenSSL 0.9.6 Should we ask for the OpenSSL version at least 0.9.6 ? Or use ERR_error_string to replace when OpenSSL version is less than 0.9.6. It will need more works to implement.

[agentzh]

@detailyang I think we can just drop support for openssl versions older than 0.9.6 :)

[detailyang]

@agentzh How about this API now as the following ?

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

    u_long       e;

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

        } else {
            ERR_error_string_n(e, *err_buf, err_buf_len);
        }

        return NGX_ERROR;
    }

    return NGX_OK;
}

It's a little tricky that the caller should pass the error buffer address、 the length of error buffer and the error buffer address will be used to point the literal string or be placed the error message from OpenSSL ERR_error_string_n

BTW, the tests failed because I use lua-resty-core rather than FFI to simplely test.

[agentzh]

@detailyang No, that's wrong to overwrite the buffer pointer since the caller may not check the pointer value at all, like this:

const char buf[64];
if (foo(&buf, 64) != 0) {
    printf("error: %64s\n", buf);
}

So we should use ngx_copy() to copy the literal string into the user-supplied error string buffer.

BTW, you should edit .travis.yml to make the lua-resty-core repo point to your own in this PR. We can change it later when we merge your work.

[detailyang]

@agentzh gotcha :D

After rethinking about a lot of API design (refer to the other lua-resty-core FFI implementations) and I implemented different version on my localhost, now the API design is as the following:

static size_t
ngx_http_lua_ssl_get_error(u_long e, u_char *ssl_err_buf,
    size_t ssl_err_buf_len, u_char *default_errmsg,
    size_t default_errmsg_len)
{
    size_t len;

    if (e == 0) {
        len = ngx_min(ssl_err_buf_len, default_errmsg_len);
        ssl_err_buf = ngx_copy(ssl_err_buf, default_errmsg, len);
        return len;
    }

    ERR_error_string_n(e, (char *) ssl_err_buf, ssl_err_buf_len);

    return ngx_strlen(ssl_err_buf);
}


int
ngx_http_lua_ffi_ssl_ctx_set_priv_key(void *cdata_ctx, void *cdata_key,
    u_char *err_buf, size_t *err_buf_len)
{
    SSL_CTX     *ssl_ctx = cdata_ctx;
    EVP_PKEY    *key = cdata_key;

    char        *err;

    if (SSL_CTX_use_PrivateKey(ssl_ctx, key) == 0) {
        err = "SSL_CTX_use_PrivateKey() failed";
        goto failed;
    }

    return NGX_OK;

failed:

    *err_buf_len = ngx_http_lua_ssl_get_error(ERR_get_error(), err_buf,
                                                  *err_buf_len, (u_char *)err,
                                                  ngx_strlen(err));
    return NGX_ERROR;
}

You can review the latest changes on above commits when you are free :D

[detailyang]

@agentzh Is it okay ? And what something can I change for this PR

[detailyang]

At first I'm super sorry It's too long not to update and I'm stuck in personal work recently.Now It's time to continue completing this feature.

@agentzh Can you help me to continue review this PR ?

[detailyang]

Fix the issues as the following and merge the newest openresty/master branch:

• alignment issue
• useless return value, use ngx_memcpy
• 2 levels of indentation after #
• use local variable to eliminate more than function call
• remove unused module in tests

@agentzh Thanks for reviewing :-)

[detailyang]

@agentzh I did some code refactor again and not sure it's good enough. Can you review this PR again if you are free. Thanks

[agentzh]

@dndx Will you please have a look at this PR as well as openresty/lua-resty-core#89 ? We also need client certificate support ourselves :) Thanks!

[detailyang]

@agentzh I'm ready to provide additional API as the following:

 ngx_http_lua_ffi_ssl_ctx_add_ca_cert；

 ngx_http_lua_ffi_ssl_ctx_set_ciphers；
 
 ngx_http_lua_ffi_ssl_ctx_set_crl；
 
 ngx_http_lua_ffi_ssl_ctx_set_cert_store；
 
 ngx_http_lua_ffi_ssl_x509_store_init；
 
 ngx_http_lua_ffi_ssl_x509_store_free；
 
 ngx_http_lua_ffi_ssl_x509_store_add_cert；

Based these API, it can provide the ability like lua_ssl_ciphers and lua_ssl_crl and lua_ssl_trusted_certificate; The x509 store API will be more useful to reduce loading trusted certificate frequently. Caller can load the system trusted certificate like the following:

local system_cert =  read_file("/etc/pki/tls/cert.pem")
local local_cert =  read_file("xx.pem")
local cert_store, err = ssl.create_x509_store(system_cert, local_cert)
if cert_store == nil then
    return error(err)
end

local ssl_ctx, err = ssl.create_ctx{
    priv_key = priv_key,
    cert = cert,
    cert_store = cert_store
}

[agentzh]

@detailyang That's awesome! We'll look into this ASAP.

[kostyay-lms]

Any updates on this? I also need this functionality :)

[detailyang]

@kostyay-lms Sorry, i'm stuck in my side. However luckily, @agentzh is helping us review this PR, it well be okay as soon as possible :)

[daurnimator]

Instead of adding the large API for managing the SSL_CTX* explicitly; could we just add :setsslctx() and rely on external libraries to implement the ssl context creation? There are already a few libraries in the lua ecosystem that support SSL_CTX* management (e.g. luasec, luaossl) that I feel like it would be a waste to reinvent.

[agentzh]

@detailyang Please rebase to the latest master. Thank you! It seems like there are some merge conflicts.

[agentzh]

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

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