feature: ssl: add FFI function to set SSL ciphers #961

ghedo · 2017-01-19T22:11:44Z

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

ghedo · 2017-02-02T00:48:25Z

@agentzh @doujiang24 ping? This seems pretty straightforward.

agentzh · 2017-02-02T01:35:42Z

@ghedo Sorry for the delay on our side. This PR looks good to me :) @doujiang24 ?

agentzh · 2017-02-02T01:36:07Z

@ghedo Have you created a PR for lua-resty-core yet?

ghedo · 2017-02-02T04:59:10Z

Made openresty/lua-resty-core#86 now.

doujiang24 · 2017-02-06T02:43:08Z

src/ngx_http_lua_ssl_certby.c

+
+    if (!SSL_set_cipher_list(ssl_conn, ciphers)) {
+        *err = "SSL_set_cipher_list() failed";
+        goto failed;


Seems we can omit the goto here？Just move the failed code here?

ghedo · 2017-03-18T19:32:13Z

@agentzh @doujiang24 ping? Is there anything else needed here?

ghedo · 2017-07-13T20:17:02Z

@agentzh @doujiang24 it's now been almost 6 months since this and openresty/lua-resty-core#86 were opened, any news?

agentzh · 2017-07-13T23:43:51Z

@ghedo Sorry for the long delay on our side. We'll look into this as soon as we can manage. Thanks for your understanding!

agentzh · 2017-11-18T01:47:57Z

src/ngx_http_lua_ssl_certby.c

+        return NGX_ERROR;
+    }
+
+    if (!SSL_set_cipher_list(ssl_conn, ciphers)) {


@ghedo I wonder if you really need per-connection ciphers. Maybe you just need a per-domain cipher suite? If it's the latter, then we can just reuse a domain-level SSL ctx to avoid per-request cipher name parsing and setting.

Or maybe we can simply set a parsed SSL cipher suite C data structure to avoid the string parsing here on a hot code path?

Yes I agree that calling SSL_set_cipher_list on a per request basis is a terrible idea as it both has lots of malloc along the way and the cipher search algorithm is linear.

Better way of doing this is to store the cipher stack created by OpenSSL once and reuse them later in new connections with some kind of Lua handle (e.g. cdata).

There may be other way that's better but I haven't looked too much into other OpenSSL cipher related APIs.

Ooops, missed these comments...

Anyway, the function is intended to be used in ssl_cert_by_lua, so per-connection, not per-request, and even then not all connections (otherwise you might as well use ssl_protocols in the config).

AFAICT there's no OpenSSL API that would allow you to parse a cipher string into an array or stack of SSL_CIPHER, without actually changing the cipher list on an SSL or SSL_CTX struct. And even then, there's no OpenSSL API to set a cipher list from an array or stack of SSL_CIPHER. Did you have anything specific in mind?

In any case the overhead of SSL_set_cipher_list() is really not that bad, and even if it was too slow/heavy for whatever use case (which would likely prevent other OpenResty APIs from being used as well), nobody would be forced to use this.

@ghedo Well, I think we are just trying to do it right :) BTW, have you actually measured the overhead of parsing and loading long cipher strings in this context?

dndx · 2017-11-18T02:04:21Z

src/ngx_http_lua_ssl_certby.c

+        return NGX_ERROR;
+    }
+
+    if (!SSL_set_cipher_list(ssl_conn, ciphers)) {


Yes I agree that calling SSL_set_cipher_list on a per request basis is a terrible idea as it both has lots of malloc along the way and the cipher search algorithm is linear.

Better way of doing this is to store the cipher stack created by OpenSSL once and reuse them later in new connections with some kind of Lua handle (e.g. cdata).

There may be other way that's better but I haven't looked too much into other OpenSSL cipher related APIs.

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