Re: Should SSL_get_servername() depend on SNI callback (no-)ACK?
Hiya, On 22/10/2019 17:09, Yann Ylavic wrote: > Sorry for the shortcut, by "tlsext_hostname" I meant the name of the > field in SSL_SESSION_ASN1. > My observation is that when browsers resume a session, s->hit is set > but s->session->ext.hostname is NULL, which I interpret as no SNI > found in the SSL_SESSION (and thus no SNI encoded in the session > ticket, presumably). > On the other hand, the SNI is always in ClientHello (though there is > no way to match it against the session's). FWIW, I also had to play about a bit with that to get ESNI working with tickets. I can chase down the bits of code for that in my fork [1] if it's useful. Cheers, S. [1] https://github.com/sftcd/openssl/ 0x5AB2FAF17B172BEA.asc Description: application/pgp-keys signature.asc Description: OpenPGP digital signature
Re: Should SSL_get_servername() depend on SNI callback (no-)ACK?
On Tue, Oct 22, 2019 at 5:09 PM Benjamin Kaduk wrote: > > There's some (additional?) discussion on this topic in > https://github.com/openssl/openssl/pull/10018 . A couple comments inline, > though... Thanks, will look at it. More comment below too... > On Tue, Oct 22, 2019 at 02:30:37PM +0200, Yann Ylavic wrote: > > > > And it _seems_ that browsers (or Chrome only?) don't set a > > tlsext_hostname in their session (ASN1) on resumption, so > > I don't understand what you mean by "set a tlsext_hostname in their session > (ASN1) > on resumption". Are you saying that browsers do not send the > server_name_indication > extension in the ClientHello for resumption handshakes? Sorry for the shortcut, by "tlsext_hostname" I meant the name of the field in SSL_SESSION_ASN1. My observation is that when browsers resume a session, s->hit is set but s->session->ext.hostname is NULL, which I interpret as no SNI found in the SSL_SESSION (and thus no SNI encoded in the session ticket, presumably). On the other hand, the SNI is always in ClientHello (though there is no way to match it against the session's). > > Note that the documentation for SSL_get_servername() is in > the page for SSL_CTX_set_tlsext_servername_callback() and claims to > return the value from the Client Hello (if present). Yet, historically, > prior to TLS 1.3 on resumption we did not even look at the ClientHello > to see whether the extension was present; we just resumed and thus the > "Client Hello" in the above would be the *initial* ClientHello. This requires that the initial SNI be encoded in the session (ticket), does this changed with TLS 1.3 somehow? > This > was perhaps tolerable if the servername callback was never called, if > you assume that the API would only be called from that callback, but > that assumption is no longer anywhere close to true. Yes, I found this "issue" by trying to modify Apache httpd, which was previously using the SNI callback to select the right TLS configuration (per virtual host), to now do that in the new ClientHello callback (which allows for setting the configured TLS protocol version too, whereas the SNI callback is too late for that). When doing this, I naively first removed the SNI callback completely, and discovered that further call to SSL_get_hostname() was returning NULL, for some cases (resumption)... Regards, Yann.
Re: Should SSL_get_servername() depend on SNI callback (no-)ACK?
There's some (additional?) discussion on this topic in
https://github.com/openssl/openssl/pull/10018 . A couple comments inline,
though...
On Tue, Oct 22, 2019 at 02:30:37PM +0200, Yann Ylavic wrote:
> Hi,
>
> in master (and 1.1.1), SSL_get_servername() returns either
> s->session->ext.hostname (when s->hit == 1), or s->ext.hostname
> (otherwise).
>
> It seems, according to final_server_name(), that
> s->session->ext.hostname is set only:
> if (sent && ret == SSL_TLSEXT_ERR_OK && (!s->hit || SSL_IS_TLS13(s)))
> {
> ...
> s->session->ext.hostname = OPENSSL_strdup(s->ext.hostname);
> ...
> }
> that is if a SNI callback exists and returns OK (no callback
> defaulting to NOACK).
>
> And it _seems_ that browsers (or Chrome only?) don't set a
> tlsext_hostname in their session (ASN1) on resumption, so
I don't understand what you mean by "set a tlsext_hostname in their session
(ASN1)
on resumption". Are you saying that browsers do not send the
server_name_indication
extension in the ClientHello for resumption handshakes?
> s->session->ext.hostname isn't set by that either. Note that this
> leaves s->servername_done = 0 in the below code from
> tls_parse_ctos_server_name():
> if (s->hit) {
> /*
> * TODO(openssl-team): if the SNI doesn't match, we MUST
> * fall back to a full handshake.
> */
> s->servername_done = (s->session->ext.hostname != NULL)
> && PACKET_equal(, s->session->ext.hostname,
> strlen(s->session->ext.hostname));
>
> if (!s->servername_done && s->session->ext.hostname != NULL)
> s->ext.early_data_ok = 0;
> }
>
> So it's possible that, on session resumption (s->hit == 1),
> SSL_get_servername() returns NULL (unset s->session->ext.hostname)
> even though an SNI is provided by ClientHello. Shouldn't it return the
> ClientHello's SNI regardless of the presence/return of the callback?
> Something like the below (where s->servername_done is also tested)?
There's something of a philosophical debate; some additional (heated)
discussion can be found in https://github.com/openssl/openssl/pull/7115
but I think the real cause of problems in this space is that
the SSL_get_servername() API contract has been underspecified from the
start. Note that the documentation for SSL_get_servername() is in
the page for SSL_CTX_set_tlsext_servername_callback() and claims to
return the value from the Client Hello (if present). Yet, historically,
prior to TLS 1.3 on resumption we did not even look at the ClientHello
to see whether the extension was present; we just resumed and thus the
"Client Hello" in the above would be the *initial* ClientHello. This
was perhaps tolerable if the servername callback was never called, if
you assume that the API would only be called from that callback, but
that assumption is no longer anywhere close to true.
-Ben
> const char *SSL_get_servername(const SSL *s, const int type)
> {
> if (type != TLSEXT_NAMETYPE_host_name)
> return NULL;
>
> /*
> * SNI is not negotiated in pre-TLS-1.3 resumption flows, so fake up an
> * SNI value to return if we are resuming/resumed. N.B. that we still
> * call the relevant callbacks for such resumption flows, and callbacks
> * might error out if there is not a SNI value available.
> */
> if (s->hit && s->servername_done)
> return s->session->ext.hostname;
> return s->ext.hostname;
> }
>
>
> Regards,
> Yann.
Should SSL_get_servername() depend on SNI callback (no-)ACK?
Hi,
in master (and 1.1.1), SSL_get_servername() returns either
s->session->ext.hostname (when s->hit == 1), or s->ext.hostname
(otherwise).
It seems, according to final_server_name(), that
s->session->ext.hostname is set only:
if (sent && ret == SSL_TLSEXT_ERR_OK && (!s->hit || SSL_IS_TLS13(s))) {
...
s->session->ext.hostname = OPENSSL_strdup(s->ext.hostname);
...
}
that is if a SNI callback exists and returns OK (no callback
defaulting to NOACK).
And it _seems_ that browsers (or Chrome only?) don't set a
tlsext_hostname in their session (ASN1) on resumption, so
s->session->ext.hostname isn't set by that either. Note that this
leaves s->servername_done = 0 in the below code from
tls_parse_ctos_server_name():
if (s->hit) {
/*
* TODO(openssl-team): if the SNI doesn't match, we MUST
* fall back to a full handshake.
*/
s->servername_done = (s->session->ext.hostname != NULL)
&& PACKET_equal(, s->session->ext.hostname,
strlen(s->session->ext.hostname));
if (!s->servername_done && s->session->ext.hostname != NULL)
s->ext.early_data_ok = 0;
}
So it's possible that, on session resumption (s->hit == 1),
SSL_get_servername() returns NULL (unset s->session->ext.hostname)
even though an SNI is provided by ClientHello. Shouldn't it return the
ClientHello's SNI regardless of the presence/return of the callback?
Something like the below (where s->servername_done is also tested)?
const char *SSL_get_servername(const SSL *s, const int type)
{
if (type != TLSEXT_NAMETYPE_host_name)
return NULL;
/*
* SNI is not negotiated in pre-TLS-1.3 resumption flows, so fake up an
* SNI value to return if we are resuming/resumed. N.B. that we still
* call the relevant callbacks for such resumption flows, and callbacks
* might error out if there is not a SNI value available.
*/
if (s->hit && s->servername_done)
return s->session->ext.hostname;
return s->ext.hostname;
}
Regards,
Yann.
