> On 23 Mar 2023, at 21:24, Maxim Dounin <mdounin@mdounin.ru> wrote:
>
> Hello!
>
> On Thu, Mar 23, 2023 at 07:59:41PM +0400, Sergey Kandaurov wrote:
>
>>> On 23 Mar 2023, at 18:10, Maxim Dounin <mdounin@mdounin.ru> wrote:
>>>
>>> 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;
>
> Looks good (except the "virtual servers" term, see below).
>
>> 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).
>
> The term "virtual server" includes everything virtual being run on
> a given physical server, including IP-based, port-based, and
> name-based virtual servers. The default server (for a listening
> socket) is still a virtual server. See, for example, description
> of the "server" directive:
>
> http://nginx.org/en/docs/http/ngx_http_core_module.html#server
>
> So, basically, the description of the "ssl" directive you are
> referring to is perfectly correct. Except the fact that it might
> additionally state that the directive only works if the server is
> the default virtual server for the listening socket in question,
> and does nothing in non-default virtual servers.
>
> So a better commit log for the above patch should say that it
> removes the "ssl" option from listening sockets in non-default
> [virtual] servers. Something like this should be good enough:
>
> : Tests: improved "ssl" directive test in ssl_reject_handshake.t.
> :
> : The "ssl" option was removed from listening sockets in non-default
> : servers. In particular, this allows to test that the "ssl" directive
> : actually works.
>

Thanks, applied and pushed, together with the whole series.

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