> On 23 Mar 2023, at 18:10, Maxim Dounin <mdounin@mdounin.ru> wrote:
>
> Hello!
>
> On Wed, Mar 22, 2023 at 12:57:56PM +0400, Sergey Kandaurov wrote:
>
>>> On 18 Mar 2023, at 18:14, Maxim Dounin <mdounin@mdounin.ru> wrote:
>>>
>>> # HG changeset patch
>>> # User Maxim Dounin <mdounin@mdounin.ru>
>>> # Date 1679105686 -10800
>>> # Sat Mar 18 05:14:46 2023 +0300
>>> # Node ID 86c394a226d2a7d463da7a1b7e88375c71c0c69b
>>> # Parent 3c9aa6c23fc836725b96cf056d218217a5a81603
>>> Tests: separate SSL session reuse tests.
>>>
>>> Instead of being mixed with generic SSL tests, session reuse variants
>>> are now tested in a separate file.
>>>
>>> In the generic SSL tests only basic session reuse is now tested,
>>> notably with session tickets enabled and a shared SSL session cache.
>>> This should make it possible to reuse sessions in all cases (except
>>> when it's not supported, such as with LibreSSL with TLSv1.3).
>>>
>>> Note that session reuse with tickets implies that $ssl_session_id
>>> is selected by the client and therefore is not available on the
>>> initial connection. Relevant test is modified to handle this.
>>>
>>> Further, BoringSSL does not use legacy session ID with TLSv1.3 even
>>> if it is sent by the client. In contrast, OpenSSL always generates
>>> an unique legacy session id, so it is available with TLSv1.3 even if
>>> session resumption does not work (such as with old Net::SSLeay and
>>> IO::Socket::SSL modules).
>>
>> Note that TLSv1.3 has only ticket based session resumption.
>> BoringSSL has a different notion on using legacy session IDs
>> in TLSv1.3, see tls13_create_session_with_ticket() in sources:
>>
>> // Historically, OpenSSL filled in fake session IDs for ticket-based sessions.
>> // Envoy's tests depend on this, although perhaps they shouldn't.
>> SHA256(CBS_data(&ticket), CBS_len(&ticket), session->session_id);
>>
>> Later, SSL_SESSION_get_id() was additionally annotated in ssl.h:
>>
>> // As a workaround for some broken applications, BoringSSL sometimes synthesizes
>> // arbitrary session IDs for non-ID-based sessions. This behavior may be
>> // removed in the future.
>>
>> As for TLSv1.3 server context, BoringSSL doesn't seem to use "session ID"
>> besides echoing the client's legacy_session_id field content in the
>> legacy_session_id_echo field of ServerHello/HRR during handshake,
>> as mandated in RFC 8446, 4.1.3. So it doesn't settle in the session.
>
> Yes, that's basically what "BoringSSL does not use legacy session
> ID with TLSv1.3..." sentence describes.
>
>>>
>>> diff --git a/ssl.t b/ssl.t
>>> --- a/ssl.t
>>> +++ b/ssl.t
>>> @@ -31,7 +31,7 @@ eval { IO::Socket::SSL::SSL_VERIFY_NONE(
>>> plan(skip_all => 'IO::Socket::SSL too old') if $@;
>>>
>>> my $t = Test::Nginx->new()->has(qw/http http_ssl rewrite proxy/)
>>> - ->has_daemon('openssl')->plan(28);
>>> + ->has_daemon('openssl')->plan(21);
>>>
>>> $t->write_file_expand('nginx.conf', <<'EOF');
>>>
>>> @@ -47,7 +47,6 @@ http {
>>>
>>> ssl_certificate_key localhost.key;
>>> ssl_certificate localhost.crt;
>>> - ssl_session_tickets off;
>>>
>>> log_format ssl $ssl_protocol;
>>>
>>> @@ -59,6 +58,7 @@ http {
>>> ssl_certificate_key inner.key;
>>> ssl_certificate inner.crt;
>>> ssl_session_cache shared:SSL:1m;
>>> + ssl_session_tickets on;
>>> ssl_verify_client optional_no_ca;
>>>
>>> keepalive_requests 1000;
>>> @@ -100,57 +100,11 @@ http {
>>> }
>>>
>>> server {
>>> - listen 127.0.0.1:8081;
>>> - server_name localhost;
>>> -
>>> - # Special case for enabled "ssl" directive.
>>> -
>>> - ssl on;
>>
>> Removing tests for the "ssl" legacy directive doesn't feel right
>> now and is out of scope of this change. Please put this back.
>> Not being able to test it is fragile and can be left silently
>> broken, especially with the upcoming quic merge.
>
> It used to be tested as a side effect of session reuse tests, and
> hence it is now gone.

You are correct, yet it's useful to have it in some sort.

> It is also tested at least in
> ssl_reject_handshake.t, so I tend to think it is enough.
>

It is, though not very useful, at least now.
Currently the tested configuration in ssl_reject_handshake.t is
supplemented with "listen .. ssl" in adjacent virtual servers.
If the "ssl" directive gets broken and e.g. will stop doing its work,
the SSL mode will remain enabled across servers of the listening socket,
so this may be left unnoticed.
To make it more useful, we can remove "listen .. ssl" on virtual servers.

# HG changeset patch
# User Sergey Kandaurov <pluknet@nginx.com>
# Date 1679586784 -14400
# Thu Mar 23 19:53:04 2023 +0400
# Node ID 938f2657257c24ac2ceb8df39536d330bcc03930
# Parent 5abbc8e2d39dfda3913939a6ae293744f5f80933
Tests: removed ssl from virtual servers in ssl_reject_handshake.t.

In particular, this allows to test that the "ssl" directive actually works.

diff --git a/ssl_reject_handshake.t b/ssl_reject_handshake.t
--- a/ssl_reject_handshake.t
+++ b/ssl_reject_handshake.t
@@ -59,8 +59,8 @@ http {
}

server {
- listen 127.0.0.1:8080 ssl;
- listen 127.0.0.1:8081 ssl;
+ listen 127.0.0.1:8080;
+ listen 127.0.0.1:8081;
server_name virtual;

ssl_certificate localhost.crt;
@@ -76,12 +76,12 @@ http {
}

server {
- listen 127.0.0.1:8082 ssl;
+ listen 127.0.0.1:8082;
server_name virtual1;
}

server {
- listen 127.0.0.1:8082 ssl;
+ listen 127.0.0.1:8082;
server_name virtual2;

ssl_reject_handshake on;

BTW, the old documentation on the "ssl" directive stated that it
"Enables the HTTPS protocol for the given *virtual* server", though
it appears to be rather opposite (i.e. for the default one).

>> On the other hand, I'm fine with finally removing this directive.
>> It was made obsolete in version 1.15.0, released on 05 Jun 2018,
>> with the "listen .. ssl" upgrade path available back to 0.7.14,
>> released on 01 Sep 2008.
>
> I think we'll consider this in 1.25.x branch.
>
> [..]
>>> server_name localhost;
>>>
>>> ssl_session_cache shared:SSL:1m;
>>> + ssl_session_tickets on;
>>> ssl_session_timeout 1;
>>>
>>> location / {
>>> @@ -216,59 +170,34 @@ foreach my $name ('localhost', 'inner')
>>> or die "Can't create certificate for $name: $!\n";
>>> }
>>>
>>> -# suppress deprecation warning
>>> -
>>> -open OLDERR, ">&", \*STDERR; close STDERR;
>>> $t->run();
>>> -open STDERR, ">&", \*OLDERR;
>>>
>>> ###############################################################################
>>>
>>> -my $ctx;
>>> +# ssl session reuse
>>>
>>> -SKIP: {
>>> -skip 'no TLS 1.3 sessions', 6 if get('/protocol', 8085) =~ /TLSv1.3/
>>> - && ($Net::SSLeay::VERSION < 1.88 || $IO::Socket::SSL::VERSION < 2.061);
>>> +my $ctx = get_ssl_context();
>>>
>>> -$ctx = get_ssl_context();
>>> +like(get('/', 8085, $ctx), qr/^body \.$/m, 'session');
>>>
>>> -like(get('/', 8085, $ctx), qr/^body \.$/m, 'cache shared');
>>> -like(get('/', 8085, $ctx), qr/^body r$/m, 'cache shared reused');
>>> -
>>> -$ctx = get_ssl_context();
>>> +TODO: {
>>> +local $TODO = 'no TLSv1.3 sessions, old Net::SSLeay'
>>> + if $Net::SSLeay::VERSION < 1.88 && test_tls13();
>>> +local $TODO = 'no TLSv1.3 sessions, old IO::Socket::SSL'
>>> + if $IO::Socket::SSL::VERSION < 2.061 && test_tls13();
>>
>> Not sure why do you convert this to TODO.
>>
>> TODO blocks are used for something we control or able to fix.
>> SSL libraries are not something like that, so using SKIP there
>> is more appropriate and follows other platform-specific tests.
>> Besides that, TODOs plagues diagnostic output for no purpose.
>>
>> See also Test::More documentation:
>>
>> When do I use SKIP vs. TODO?
>> If it's something the user might not be able to do, use SKIP. This
>> includes optional modules that aren't installed, running under an OS
>> that doesn't have some feature (like "fork()" or symlinks), or maybe
>> you need an Internet connection and one isn't available.
>>
>> If it's something the programmer hasn't done yet, use TODO. This is
>> for any code you haven't written yet, or bugs you have yet to fix,
>> but want to put tests in your testing script (always a good idea).
>
> As you can see from the Test::More documentation, the main
> distinction is: SKIP is to be used when something cannot be
> tested, and TODO is to use when something doesn't work (but we can
> test it).
>
> In this particular case, things can be tested - but tests will
> fail due to lack of support in IO::Socket::SSL / Net::SSLeay.
>
> Immediate benefits of using TODO here is that a) test output
> provides good indication of what actually happens with these
> module versions (that is, it does not unexpectedly die or
> segfault, but session reuse does not work), b) tests are actually
> run and it can be easily spotted if they unexpectedly succeed, and
> c) it combines nicely with LibreSSL / BoringSSL TODOs (which are
> TODO for similar reasons, see the following patches).
>
> As the code in question is not under our control, it might be good
> enough to use SKIP instead of TODO (and the patch series does this
> at least once). But I tend to think that at least in case of
> LibreSSL it would be much better to use TODOs, so we'll be able to
> see when things are finally fixed in LibreSSL - I would expect
> this to happen in a foreseeable future.
>

Ok, I am partially agree with you (item "a" is the most prominent).
Let's leave TODOs.

--
Sergey Kandaurov
_______________________________________________
nginx-devel mailing list
nginx-devel@nginx.org
https://mailman.nginx.org/mailman/listinfo/nginx-devel