openssl icon indicating copy to clipboard operation
openssl copied to clipboard

(minor) syntactic sugar to use lighttpd.conf include_shell to set var.rootroot

Open gstrauss opened this issue 4 years ago • 35 comments

# Be nice to not have to use an absolute path here.
var.rootroot="/home/stephen/code/openssl/esnistuff"

https://github.com/sftcd/openssl/blob/512c18169eb668db2b4b4b8c43e8d8ed91151bb2/esnistuff/lighttpdmin.conf#L4

This might do what you want for testing (current working directory). include_shell "printf var.rootroot=\\"$PWD\\""

gstrauss avatar Oct 14 '20 21:10 gstrauss

On 14/10/2020 22:49, Glenn Strauss wrote:

# Be nice to not have to use an absolute path here.
var.rootroot="/home/stephen/code/openssl/esnistuff"

https://github.com/sftcd/openssl/blob/512c18169eb668db2b4b4b8c43e8d8ed91151bb2/esnistuff/lighttpdmin.conf#L4

This might do what you want for testing (current working directory). include_shell "printf var.rootroot=\\"$PWD\\""

Fair point, thanks. Will try it out.

S.

sftcd avatar Oct 14 '20 22:10 sftcd

FYI: I am a lighttpd developer and have started reading up on ECH (was ESNI). I am glad I found your work here, though I have not yet delved into it.

If you'd like to discuss improvements and integration, please open a topic in the lighttpd development forum: https://redmine.lighttpd.net/projects/lighttpd/boards/3

For example, the development version of lighttpd now supports OCSP stapling, and provides a convenient way to periodically check for updated external files, such as an updated staple.der, or updated ESNI keys.

gstrauss avatar Oct 14 '20 23:10 gstrauss

On 15/10/2020 00:30, Glenn Strauss wrote:

FYI: I am a lighttpd developer and have started reading up on ECH (was ESNI). I am glad I found your work here, though I have not yet delved into it.

If you'd like to discuss improvements and integration, please open a topic in the lighttpd development forum: https://redmine.lighttpd.net/projects/lighttpd/boards/3

For example, the development version of lighttpd now supports OCSP stapling, and provides a convenient way to periodically check for updated external files, such as an updated staple.der, or updated ESNI keys.

Great, thanks. Probably be a while before I get back to that. I'm currently waiting 'till all the ECH tweaking settles down some more, but will be sure to get in touch when I get back to application integrations.

S.

sftcd avatar Oct 14 '20 23:10 sftcd

[Posting my notes here to checkpoint my work. Please forgive the noise. I am happy to continue development and discussion here or elsewhere if you are interested and have the time.]

Here is a cleaner way to inherit paths for the test environment. I also added the ability to override LIGHTY so that I could test with an alternate lighttpd development location.

diff --git a/esnistuff/lighttpdmin.conf b/esnistuff/lighttpdmin.conf
index 9b3dcc220f..d3fc0f6d24 100644
--- a/esnistuff/lighttpdmin.conf
+++ b/esnistuff/lighttpdmin.conf
@@ -1,7 +1,7 @@
 #debug.log-request-handling = "enable"

 # Be nice to not have to use an absolute path here.
-var.rootroot="/home/stephen/code/openssl/esnistuff"
+var.rootroot= env.TOP + "/esnistuff"

 # standard stuff - testlighttpd.sh makes these dirs if needed
 server.document-root = rootroot + "/lighttpd/www"
diff --git a/esnistuff/testlighttpd.sh b/esnistuff/testlighttpd.sh
index f09f1d8fff..940da3b7cf 100755
--- a/esnistuff/testlighttpd.sh
+++ b/esnistuff/testlighttpd.sh
@@ -2,8 +2,11 @@

 set -x

-OSSL="$HOME/code/openssl"
-LIGHTY="$HOME/code/lighttpd1.4"
+: ${TOP=$HOME/code/openssl}
+export TOP
+
+OSSL="$TOP"
+: ${LIGHTY="$HOME/code/lighttpd1.4"}

 export LD_LIBRARY_PATH=$OSSL

gstrauss avatar Oct 18 '20 22:10 gstrauss

BTW, regarding lighttpd use of SSL_CTX_set_client_hello_cb() to inspect the CLIENT_HELLO for (cleartext) SNI, this change was made because I seem to recall having read that SSL_CTX_set_client_hello_cb() obsoleted the older SSL_CTX_set_tlsext_servername_callback(), SSL_CTX_set_tlsext_servername_arg() interfaces. Additionally, discovering the target host at the very beginning of the handshake allows lighttpd to set configuration options specific to that hostname, in case such settings might affect other steps in the handshake. In practice, this is not an issue since SSL_CTX_set_tlsext_servername_callback() occurs prior to the cert_cb (SSL_CTX_set_cert_cb()). The extra complexity of having both callbacks selectively enabled in the code is due to lighttpd supporting older version of openssl (and other openssl-compatible TLS libraries), prior to the creation of the cert_cb in openssl 1.0.2.

gstrauss avatar Oct 18 '20 22:10 gstrauss

Question: Why does esni_check_if_only() check 'cover' (clear_sni) if SSL_ESNI_STATUS_SUCCESS?

" * If ESNI worked and we had a cover, we'll check on that."

Why? 'hidden' should contain the host after successful ESNI decryption, so why bother checking 'cover'?

Question: Instead of ssl.esnikeydir, would it be more user-friendly to associate ESNI keys with a certificate pemfile and private key, similar to an OCSP stapling file? A given cert might be associated with more than one host (via SAN or wildcard cert), so perhaps a naming convention hostname.pem and hostname.esni.identifier.{priv,pub} and I can modify the code to glob() for "hostname.esni.*" to load associated ESNI keys.

Question: Regarding refreshing ESNI keys, do your patches to openssl support reloading the same key? Does it support multiple keys for the same host (so that valid keys overlap while older keys are rotated out)? Should lighttpd keep track of stat-info for each key file and reload the file only if changed? (This would also imply changes to how SSL_CTX_esni_server_flush_keys() is called.)

Question: The interfaces you have created for reading ESNI keys require filenames. May I make a request that the core interfaces take in-memory buffers (not BIOs!), and, optionally, that wrapper routines take file names? Doing so would allow more flexibility in how servers obtain and refresh ESNI keys, e.g. via JSON objects, or a binary format which might include validity lifetimes. The latter is how the next version of lighttpd allows admins to manage session ticket encryption keys (STEKs) across server farms.

gstrauss avatar Oct 18 '20 22:10 gstrauss

I am so sorry that you had to fight with the lighttpd internal configuration structures. The next version of lighttpd includes major changes to config management, and parses user configuration into structured data at startup. Also, mod_openssl has been updated to use the openssl 1.0.2 (and later) APIs including SSL_CTX_set_cert_cb(). Error logging is now printf-like, rather than the historic custom implementation in lighttpd.

I have rewritten your patches and tried to address questions and shortcomings that you noted in code comments. I have also added more paranoid handling of Host (or :authority) request headers to avoid exposing ECH-only hosts.

https://github.com/gstrauss/lighttpd1.4/tree/ESNI-experimental

The code has been lightly tested using your esnistuff/* scripts. (Thank you!)

In my branch, you can also #define LIGHTTPD_OPENSSL_ESNI_DEBUG at the top of mod_openssl.c for additional ESNI debug trace.

Design choices different from your patches:

  • ssl.ech-opts is a key,value list for each socket (each SSL_CTX) and subsumes ssl.esnikeydir, ssl.esnirefresh, ssl.esnitrialdecrypt. ssl.ech-opts "keydir" => "string" "refresh" => number # (default 1800) "trial-decrypt" => "enable" # (default) ssl.openssl.ssl-conf-cmd += ("Options" => "ESNITrialDecrypt") (or "ECHTrialDecrypt") is preferred instead of ssl.ech-opts += ("trial-decrypt" => "enable")
  • ssl.non-ech-host replaces ssl.esnionly I created ssl.non-ech-host = "hostname" to define a fallback-host for non-ECH. If this is present in an $HTTP["host"] == "..." condition, then the host in the condition is added to a list of ECH-only hosts, which can not be accessed on non-ECH TLS connections. Having the fallback value available allows for well-defined non-ECH behavior. ... I may consider modifying this to be a parameter for ssl.ech-opts, so that an entire SSL_CTX could be declared ECH-only and the fallback host defined once.
  • ESNI keys are kept around for 2x refresh intervals by adding 5 seconds to max age passed to SSL_CTX_esni_server_flush_keys(). This allows key validity to overlap for one refresh interval.

Besides the patches posted a few comments above, I modified lighttpdmin.conf:

diff --git a/esnistuff/lighttpdmin.conf b/esnistuff/lighttpdmin.conf
index 9b3dcc220f..d3fc0f6d24 100644
--- a/esnistuff/lighttpdmin.conf
+++ b/esnistuff/lighttpdmin.conf
@@ -30,40 +30,34 @@ ssl.engine                  = "enable"
 ssl.pemfile                 = cadir + "/example.com.pem"
 ssl.cipher-list             = "HIGH"   # default
 ssl.ca-file                 = cadir + "/oe.csr"
-ssl.esnikeydir              = rootroot + "/esnikeydir"
-# attempt to re-load keys every hour
-#ssl.esnirefresh             = 3600
-# want to see it happen? reload often then...
-ssl.esnirefresh             = 10
-# trial decryption allows clients to hide better by not sending real digests
-# that is turned on by default (as we're likely a small server so no harm and
-# better privacy), but you can disable it...
-# ssl.esnitrialdecrypt        = "disable"
-ssl.openssl.ssl-conf-cmd    = ("Protocol" => "-ALL, TLSv1.2, TLSv1.3")
+ssl.openssl.ssl-conf-cmd    = ("MinProtocol" => "TLSv1.2")
+ssl.ech-opts = (
+  "keydir" => rootroot + "/esnikeydir",
+
+  #"refresh" => 3600, # reload hourly
+  "refresh" => 60,    # reload every minute (testing)
+  # (minimum check interval is actually 64 seconds (2^6))
+
+  # trial decryption allows clients to hide better by not sending real digests
+  # that is turned on by default (as we're likely a small server so no harm and
+  # better privacy), but you can disable it...
+  #"trial-decrypt" => "disable",
+)
+
 server.name="example.com"
 $HTTP["host"] == "foo.example.com" {
     ssl.pemfile                 = cadir + "/foo.example.com.pem"
+    server.name                 = "foo.example.com"
 }
 $HTTP["host"] == "baz.example.com" {
-    ssl.esnionly                = "enable"
+    ssl.non-ech-host            = "example.com"
     ssl.pemfile                 = cadir + "/baz.example.com.pem"
     server.document-root        = rootroot + "/lighttpd/baz"
 }

-# If you wanted a cleartext hTTP listener on e.g. port 3000 as
-# well then you'd remove all the ssl stuff above, add a server.port
-# of 3000 and then add this below for the port 3443 listener
-#$SERVER["socket"] == "127.0.0.1:3443" {
-    #ssl.engine                  = "enable"
-    #ssl.cipher-list             = "HIGH"   # default
-    #ssl.ca-file                 = cadir + "/oe.csr"
-    #ssl.openssl.ssl-conf-cmd    = ("Protocol" => "-ALL, TLSv1.2, TLSv1.3")
-    #ssl.pemfile                 = cadir + "/example.com.pem"
-    #server.name                 = "example.com"
-    #$HTTP["host"] == "foo.example.com" {
-        #ssl.pemfile                 = cadir + "/foo.example.com.pem"
-        #server.name                 = "foo.example.com"
-    #}
+# If you wanted a cleartext HTTP listener on e.g. port 3000:
+#$SERVER["socket"] == "127.0.0.1:3000" {
+    #ssl.engine                  = "disable"
     # same as before, probaby better if not:-)
     #server.document-root        = rootroot + "/lighttpd/www"
 #}

gstrauss avatar Oct 18 '20 22:10 gstrauss

On 18/10/2020 23:53, Glenn Strauss wrote:

[Posting my notes here to checkpoint my work. Please forgive the noise. I am happy to continue development and discussion here or elsewhere if you are interested and have the time.] Please continue. I'm currently distracted by other work but will catch up to this.

Thanks, S.

sftcd avatar Oct 18 '20 22:10 sftcd

FYI: plans are to release lighttpd 1.4.56 very soon. If you have a chance to review and discuss, some of my experimental code might be included in lighttpd 1.4.56 https://github.com/gstrauss/lighttpd1.4/tree/ESNI-experimental

gstrauss avatar Nov 13 '20 08:11 gstrauss

@sftcd I am still interested in your thoughts here.

Is your forward development on your master branch? What branch should I follow? Thanks.

gstrauss avatar Mar 27 '21 17:03 gstrauss

Hiya,

On 27/03/2021 17:26, Glenn Strauss wrote:

@sftcd I am still interested in your thoughts here.

Is your forward development on your master branch? What branch should I follow? Thanks.

Yep, the master branch.

The only other one still really in-play is called "draft-09" and represents what currently interops with cloudflare and NSS. But note that both are expected to move up to draft-10 (or maybe -11) "soon." The master branch currently does what I think is draft-10.

In terms of lighttpd integration, I hope to get that working with ECH and the master branch in the next week.

Cheers, S.

sftcd avatar Mar 27 '21 18:03 sftcd

Whenever you get back to this, here's some current notes. No rush. Just hoping to save you some time.

I rebased my lighttpd ESNI-experimental branch against lighttpd master branch, and built against your openssl master branch. https://github.com/gstrauss/lighttpd1.4/tree/ESNI-experimental

apps/s_server.c and lighttpd mod_openssl.c call SSL_CTX_esni_server_enable(ctx,NULL,privname,pubname), passing NULL as the second parameter. This is now crashing with a NULL pointer dereference when ESNI keys are loaded at lighttpd startup.

Program received signal SIGSEGV, Segmentation fault.
0x00007ffff7c52182 in ssl_generate_param_group (s=0x0, id=29) at ssl/s3_lib.c:4751
4751        const TLS_GROUP_INFO *ginf = tls1_group_id_lookup(s->ctx, id);
Missing separate debuginfos, use: dnf debuginfo-install nettle-3.6-3.fc33.x86_64 pcre-8.44-2.fc33.x86_64
(gdb) where
#0  0x00007ffff7c52182 in ssl_generate_param_group (s=0x0, id=29) at ssl/s3_lib.c:4751
#1  0x00007ffff7c3feed in SSL_ESNI_RECORD_new_from_binary (ctx=0x536ec0, con=0x0, binbuf=0x519b50 "\377\003", binblen=77, leftover=0x7fffffffd904) at ssl/esni.c:632
#2  0x00007ffff7c47d47 in SSL_CTX_esni_server_enable (ctx=0x536ec0, con=0x0, esnikeyfile=0x4614d0 "/dev/shm/esnistuff/esnikeydir/ff03.priv", esnipubfile=0x45dde0 "/dev/shm/esnistuff/esnikeydir/ff03.pub") at ssl/esni.c:3265
#3  0x00007ffff7cfabbb in mod_openssl_refresh_esni_keys_ctx (srv=0x45c540, s=0x536380, cur_ts=1616958787) at mod_openssl.c:564
#4  0x00007ffff7cf7f44 in mod_openssl_set_defaults_sockets (srv=0x45c540, p=0x464d00) at mod_openssl.c:2983
#5  0x000000000041abe9 in plugins_call_init (srv=srv@entry=0x45c540) at plugin.c:451
#6  0x000000000040b999 in server_main_setup (srv=srv@entry=0x45c540, argc=argc@entry=6, argv=argv@entry=0x7fffffffdf88) at server.c:1253
#7  0x000000000040ce6b in main (argc=6, argv=0x7fffffffdf88) at server.c:1976

ssl_generate_param_group() takes (SSL *s) and internally accesses s->ctx (which is private) to get (SSL_CTX *). It does not actually need (SSL *), and it would seem like there should be an interface SSL_CTX_generate_param_group() which takes (SSL_CTX *), which can be called from ssl_generate_param_group() as SSL_CTX_generate_param_group(s->ctx, id).

Time allowing, I plan to do some reading and to look into the history to see what changed between Oct (when I last looked at this) and now.

gstrauss avatar Mar 28 '21 20:03 gstrauss

commit 9d2d857f to openssl for https://github.com/openssl/openssl/pull/11914 modified ssl_generate_param_group() to start using SSL *s argument, including dereferencing it. That was not happening (yet) last Oct when I was testing against your commit 512c1816

You may need to duplicate code from the current ssl/s3_lib.c:ssl_generate_param_group() into ssl/esni.c order to use SSL_CTX * instead of SSL *, unless openssl creates an additional API which takes SSL_CTX *.

gstrauss avatar Mar 29 '21 01:03 gstrauss

Hiya, So I've finally gotten back to this. What I've done so far is:

  • moved from ESNI->ECH (currently draft-10 of the spec)
  • rebased with lighttpd1.4 a few days ago
  • got it more or less working again:-)

I'm not clear if you had wanted to preserve the ESNI code (draft-02 of the spec) or not? I'd say there's not much value in that now TBH as FF and CF have both dropped their ESNI code in favour of ECH. (Mind you ECH still has at least one breaking change to come for HRR handling, so it's not quite "done" either.)

Cheers, S.

sftcd avatar Apr 07 '21 19:04 sftcd

I agree. There is no value in bring along legacy ESNI baggage.

gstrauss avatar Apr 07 '21 19:04 gstrauss

If you haven't taken a look at my experimental branch, please do so https://github.com/gstrauss/lighttpd1.4/tree/ESNI-experimental I'll update it later today to rebase against changes in your openssl branch from the past two weeks.

gstrauss avatar Apr 07 '21 20:04 gstrauss

Cool - looking at that now. I'd bet your approach to handlng the config settings is better than mine so it might make sense if I try model the new ECH config stuff on your ESNI code in that branch. If so, maybe wait 'till I push a version like that before you take my newer code?

sftcd avatar Apr 07 '21 20:04 sftcd

I don't mind a little bit of iteration. I had hoped to save you some time integrating with the lighttpd code.

I'm currently in the process of removing "ESNI" and replacing with "ECH". Almost ready to test against your latest openssl master.

So far, the only "ESNI" interface I am using for which there is no "ECH" interface is SSL_get_esni_status

gstrauss avatar Apr 07 '21 21:04 gstrauss

yeah, SSL_get_esni_status goes to SSL_ech_get_status (sorry - I oscillated on that a few times;-) I have code for that added locally that's not yet pushed/working - what's pushed is enough to actually do ECH but doesn't export info that could be used by e.g. PHP

sftcd avatar Apr 07 '21 21:04 sftcd

FYI: there are replacements for a few deprecated functions (also available in OpenSSL 1.1.1). These deprecated functions result in noisy warnings in OpenSSL 3.0

ssl/esni.c:412:5: warning: 'SHA256_Init' is deprecated: Since OpenSSL 3.0 [-Wdeprecated-declarations]
ssl/esni.c:416:5: warning: 'SHA256_Update' is deprecated: Since OpenSSL 3.0 [-Wdeprecated-declarations]
ssl/esni.c:420:5: warning: 'SHA256_Final' is deprecated: Since OpenSSL 3.0 [-Wdeprecated-declarations]
ssl/esni.c:3967:5: warning: 'RAND_set_rand_method' is deprecated: Since OpenSSL 3.0 [-Wdeprecated-declarations]
crypto/esnierr.c:33:5: warning: 'ERR_func_error_string' is deprecated: Since OpenSSL 3.0 [-Wdeprecated-declarations]

gstrauss avatar Apr 07 '21 21:04 gstrauss

yeah, sorry - those esni files will shortly disappear and aren't needed for ECH

sftcd avatar Apr 07 '21 21:04 sftcd

Looks like SSL_CTX_esni_server_enable() which took .pub and .priv files has a different interface than SSL_CTX_ech_server_enable() which takes a single PEM file. Was this interface change intentional?

SSL_CTX_esni_server_enable() allowed .pub and .priv to be separate files or to be combined into a single file. (I am not arguing for or against; only pointing out the interface difference.)

gstrauss avatar Apr 07 '21 21:04 gstrauss

Yep, for now I've moved to a single PEM file - figured it was simpler to just provide one way to do things

sftcd avatar Apr 07 '21 21:04 sftcd

those can be generated using an openssl command now btw see "openssl ech -h" for the few associated details

sftcd avatar Apr 07 '21 21:04 sftcd

also - there're a few things only implemented as stubs for ECH (e.g. SSL_CTX_ech_server_flush_keys()) I'll flesh 'em out as I go but just have enough for now that ECH will work - I think that should be ok for initial tests

sftcd avatar Apr 07 '21 22:04 sftcd

prbuf[] is on the stack. You must not call OPENSSL_free(prbuf);

diff --git a/ssl/ech.c b/ssl/ech.c
index ed640e1771..47fd51cb18 100644
--- a/ssl/ech.c
+++ b/ssl/ech.c
@@ -344,7 +344,7 @@ static int ech_readpemfile(SSL_CTX *ctx, const char *pemfile, SSL_ECH **sechs)
 err:
     if (priv!=NULL) EVP_PKEY_free(priv);
     if (inbuf!=NULL) OPENSSL_free(inbuf);
-    if (prbuf!=NULL) OPENSSL_free(prbuf);
+//    if (prbuf!=NULL) OPENSSL_free(prbuf);
     if (pheader!=NULL) OPENSSL_free(pheader); 
     if (pname!=NULL) OPENSSL_free(pname); 
     if (pdata!=NULL) OPENSSL_free(pdata); 

gstrauss avatar Apr 08 '21 00:04 gstrauss

thanks! fixed that and also added code for SSL_CTX_ech_server_flush_keys() & pushed PS: late here, tomorrow (also here:-) before I do more on this, great to get all this help - really appreciated

sftcd avatar Apr 08 '21 00:04 sftcd

Yes, it is late there! I didn't expect you to see most of this until tomorrow. I'm currently looking into why esnistuff/esnikeydir/*.key is not loading for me in ech_readpemfile()

gstrauss avatar Apr 08 '21 00:04 gstrauss

I am having trouble loading the esnistuff/esnikeydir/*.key files from make keys. It fails on olen > (binblen-2) at ssl/ech.c:621

ECHConfigs_from_binary (binbuf=0x512340 "\377\002*~", <incomplete sequence \336>, binblen=94, leftover=0x7fffffff3824) at ssl/ech.c:618
618	    if (!PACKET_get_net_2(&pkt,&olen)) {
(gdb) 
621	    if (olen < ECH_MIN_ECHCONFIG_LEN || olen > (binblen-2)) {
(gdb) bt full
#0  ECHConfigs_from_binary (binbuf=0x512340 "\377\002*~", <incomplete sequence \336>, binblen=94, leftover=0x7fffffff3824) at ssl/ech.c:621
        er = 0x0
        te = 0x0
        rind = 0
        remaining = 0
        pkt = {curr = 0x512342 "*~", <incomplete sequence \336>, remaining = 92}
        olen = 65282
        ooffset = 0
        not_to_consume = 0
        lleftover = 0

Is there something else I should be doing to generate ECH keys? The tags in *.key are still -----BEGIN ESNIKEY----- and -----END ESNIKEY-----, so as I switch from ESNI to ECH, perhaps I need to do something differently for key generation.

[Edit] ...Yeah. Looking at ech_readpemfile(), I need to generate keys with -----BEGIN ECHCONFIG----- and -----END ECHCONFIG-----

My ESNI-experimental branch for lighttpd has been renamed ECH-experimental for the switch from ESNI to ECH. Going forwards, please refer to https://github.com/gstrauss/lighttpd1.4/tree/ECH-experimental

gstrauss avatar Apr 08 '21 01:04 gstrauss

I used doech.sh to create echconfig.pem, and then renamed that to foo.example.com.ech. Then, I modified lighttpd mod_openssl to load *.ech. This is on https://github.com/gstrauss/lighttpd1.4/tree/ECH-experimental

Not much else has changed from my prior ESNI-experimental branch other than renaming "ESNI" to "ECH", so please review the comments in the top two commits, as I have some design questions.

Your ESNI/ECH patches use SSL_CTX_set_tlsext_servername_callback(), but since openssl 1.0.2, an alternative (and better) interface is available in SSL_CTX_set_client_hello_cb(). My current ECH-experimental code uses the old interface to work with your ECH code, but I think we'll need to get things working with the preferred SSL_CTX_set_client_hello_cb() interface.

gstrauss avatar Apr 08 '21 04:04 gstrauss