Add mbedtls_ssl_get_cipher_info() accessor

[gstrauss]

Description

Add mbedtls_ssl_get_cipher_info() accessor for mbedtls 3.0

This is a follow-up to an item in #5331

Status

READY

Requires Backporting

NO

Migrations

Yes: adds accessor to retrieve (const mbedtls_cipher_info_t *) from session

Todos

• Changelog updated

[gstrauss]

Follow-up from #5331 where @mpg wrote:

By the way:

I can't think of a meaningful difference between SSL_CIPHER_ALGKEYSIZE and SSL_CIPHER_USEKEYSIZE with any ciphersuite currently in use (or at least, implemented in Mbed TLS / that I know of). Perhaps this was meaningful for export ciphersuites which used artificially short keys (for example 40-bit RC4), but I think nowadays those two are always equal.

That is my thought, too, but I am not an expert in this area.

I've tried looking around a bit, and it's not clear to me what SSL_CIPHER is actually supposed to hold: namely, if it's the name of the cipher proper, or of the TLS ciphersuite. In all examples I've found, it was the name of the ciphersuite, and and actually the non-standard name used by the SSL engine (so, OpenSSL in most cases).

Ah, the joys of magic numbers vs (non-standard) human-readable strings. Thanks for pointing it out.

Generally speaking, is there any kind of standard about what variables are to be defined in a CGI environment and what they should mean? Or has it evolved organically, with everyone trying to provide compatibility with previous/popular implementations?

Yes for CGI variables RFC3875 The Common Gateway Interface (CGI) Version 1.1

No for SSL variables. Variables like SSL_CIPHER_ALGKEYSIZE and SSL_CIPHER_USEKEYSIZE likely did have significance at some point in the past. The same may be said for some other SSL_ variables. Still other SSL_ variables can be quite useful for backends implement policy checks on the same machine servicing requests where TLS is terminated in the web server.

---

const char *cs_name = mbedtls_ssl_get_ciphersuite( &ssl );
int cs_id = mbedtls_ssl_get_ciphersuite_id( cs_name );
size_t key_bits = mbedtls_ssl_get_ciphersuite_key_bits( cs_id );

Would that work for you?

That is a bit wasteful to look up a name, and then take the name to look up an id in a different table, when the first func operated with the id that is the result. (ssl->session->ciphersuite)

const char *mbedtls_ssl_get_ciphersuite( const mbedtls_ssl_context *ssl )
{
    if( ssl == NULL || ssl->session == NULL )
        return( NULL );

    return mbedtls_ssl_get_ciphersuite_name( ssl->session->ciphersuite );
}

Aside: given the naming convention for accessors, I might expect mbedtls_ssl_get_ciphersuite_id() to take &ssl as its param instead of (const char *) (or might expect mbedtls_ssl_get_ciphersuite() to return the ciphersuite member, which is the ciphersuite id, but there would need to be an alt routine for the name, e.g. mbedtls_ssl_get_ciphersuite_name()). Also, mbedtls_ssl_get_ciphersuite_key_bits does not exist (yet?).

I'd prefer a single convenience routine (this PR) which gets the (const mbedtls_cipher_info_t *) for the session, and I can use the accessor size_t algkeysize = mbedtls_cipher_info_get_key_bitlen(cipher_info); Based on your observation above, I may choose mbedtls_ssl_get_ciphersuite( &ssl ) instead of mbedtls_cipher_info_get_name(cipher_info); for SSL_CIPHER.

I am ok with updating the lighttpd code to note that SSL_CIPHER_ALGKEYSIZE and SSL_CIPHER_USEKEYSIZE are equivalent with the current set of modern ciphers.

[mpg]

That is a bit wasteful to look up a name, and then take the name to look up an id in a different table, when the first func operated with the id that is the result.

Yes,I was just trying to re-use the existing API. If I was designing that API from scratch today, it would probably look different. However, while it's indeed inefficient and feels wrong to first look up a string from a numerical identifier and then immediately loop up again the other way, in the grand scheme of things, this only happens after the handshake, which was way more resource consuming, so I'm not sure it's worth redesigning that part of the API for that now, especially as the natural name for a function that get the ciphersuite id from the context is already taken by a function which, as you note, does something different.

(Perhaps in 4.0 we can clean that up. But for now existing functions have to stay as they are and we can only add new functions.)

Also, mbedtls_ssl_get_ciphersuite_key_bits does not exist (yet?).

Yes, I meant to suggest we add that function. Reading my message again, I did not make that clear at all, sorry.

I'd prefer a single convenience routine (this PR) which gets the (const mbedtls_cipher_info_t *)

At any other time that would indeed be the obvious right thing to do. But this is a special time: as I explained, we are phasing out the Cipher module (or at least as a first step, its usage from the TLS layer). We'd like, in some 3.x version of Mbed TLS, to be able to compile the TLS part with MBEDTLS_CIPHER_C disabled, and in a future major version, completely remove cipher.c. So, we absolutely don't want to expose mbedtls_cipherstructures in the public API, or design APIs so that people need to call mbedtls_cipher functions. Again, other than that this solution would very reasonable, but given our plans, it's just not acceptable, sorry.

So, I would suggest:

1. Adding a function that takes a ciphersuite ID and returns the corresponding key length.
2. If you need to extract some other info, add functions taking a ciphersuite ID and returning the info you need.
3. If you can find a good name for it given the constraints of the existing functions, adding a function that takes an SSL context and returns the corresponding ciphersuite ID (numerical), because I agree the current double look-up is irritating.

Would that work for you?

[mpg]

Unfortunately at this point exposing mbedtls_cipher as part of the public SSL/TLS API is not compatible with our plans for migrating to PSA.

[mpg]

(I had labeled "enhancement" by mistake: this is a regression, because some useful information that was accessible before 3.0 is no longer conveniently accessible.)

[gstrauss]

At any other time that would indeed be the obvious right thing to do. But this is a special time: as I explained, we are phasing out the Cipher module (or at least as a first step, its usage from the TLS layer). We'd like, in some 3.x version of Mbed TLS, to be able to compile the TLS part with MBEDTLS_CIPHER_C disabled

Thanks for repeating that. I had not grok'ed that I should also avoid mbedtls_cipher_info_get_*() functions to keep up with the future direction of the library.

Should I close this PR and create a new PR to discuss mbedtls_ssl_get_ciphersuite_key_bits and similar?

[gstrauss]

Prior art in ssl_ciphersuites.h suggests
mbedtls_ssl_ciphersuite_get_name()
and extending that to the underlying cipher (which you want to abstract from the TLS layer)
mbedtls_ssl_ciphersuite_get_cipher_key_bitlen() (instead of mbedtls_ssl_get_ciphersuite_key_bits())
These would take (mbedtls_ssl_ciphersuite_t *) and could (at least for mbedtls 3.0) be inline functions in ssl_ciphersuites.h

To obtain ciphersuite id from the ssl_context -- and with mbedtls_ssl_get_ciphersuite() and mbedtls_ssl_get_ciphersuite_id() already used incompatibly -- I would propose following prior art in ssl_ciphersuites.h, where mbedtls_ssl_ciphersuite_from_string() and mbedtls_ssl_ciphersuite_from_id() already exist. Would a new mbedtls_ssl_get_ciphersuite_id_from_ssl() be acceptable in ssl.h?

[gstrauss]

As an alternative name to mbedtls_ssl_get_ciphersuite_id_from_ssl(), how about mbedtls_ssl_get_ssl_ciphersuite_id()? This naming uses "ssl_ciphersuite" to match the convention used in ssl_ciphersuites.h. I think I prefer the former only because the latter might be confused with the existing routines.

[gstrauss]

Alternate proposal filed as PR #5428 and should replace this PR.

[mpg]

Closing, replaced with #5428