Should SSL_get_servername() depend on SNI callback (no-)ACK?

[Yann Ylavic]

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(&hostname, 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.