Simplify SSL write API for Mbed TLS 4

[hanno-becker]

Context: The SSL API mbedtls_ssl_write() is for writing application data through an established TLS connection. In Mbed TLS 2.X, the semantics of this API is that it always attempts to deliver the application data to the underlying transport immediately, and returning a dedicated non-fatal error MBEDTLS_ERR_SSL_WANT_WRITE if that's not possible.

Problem: There are two issues arising from this design:

1. It is not possible to batch multiple calls to mbedtls_ssl_write() prior to delivering them to the underlying transport. This is arguably not an issue for TLS because the user could concatenate the data itself before calling mbedtls_ssl_write() once, but it is a limitation for DTLS, where every invocation of mbedtls_ssl_write() shall correspond to exactly one DTLS datagram, multiple of which may be delivered in the same datagram of the underlying transport (such as UDP).

2. More importantly, when calling mbedtls_ssl_write(), we have to distinguish the following situations:

   • Data prepared in an earlier call to mbedtls_ssl_write() is still to be delivered but the underlying transport is not available, returning MBEDTLS_ERR_SSL_WANT_WRITE. The new data was not prepared queued for sending.
   • The new data was prepared and queued for sending, but couldn't be delivered because the underlying transport is not available, again returning MBEDTLS_ERR_SSL_WANT_WRITE.

   With the current design, it is impossible to tell those cases apart. The resolution employed by Mbed TLS 2.X is to mandate that if mbedtls_ssl_write() fails with MBEDTLS_ERR_SSL_WANT_WRITE, then the user must retry the call with the exact same parameters until mbedtls_ssl_write() succeeds, and only then it may be called again with different data. In fact, the implementation entirely ignores the arguments to mbedtls_ssl_write() if an earlier write is still unfinished, and mbedtls_ssl_write() is demoted to a plain flush in this case -- see here

In addition to not allowing batching of datagrams, the above design seems confusing, and we've had issues with it in the past.

Task: Agree on and implement a more suitable writing API.

Concrete proposal:
Split the writing API in two parts:

1. mbedtls_ssl_write() which prepares outgoing application data for sending and queues it internally, but does not yet send it.
2. mbedtls_ssl_flush(), which attempts to deliver all data previously prepared via mbedtls_ssl_write() to the underlying transport.
   Both calls may fail with MBEDTLS_ERR_SSL_WANT_WRITE, but for different reasons:

• If mbedtls_ssl_write() fails with MBEDTLS_ERR_SSL_WANT_WRITE, it's because new outgoing data can only be prepared after previously prepared data has been completely or partially flushed, freeing up internal space. In this case, the user knows that its data was not prepared and queued, but there is no obligation to call mbedtls_ssl_write() again with the same or different parameters - it's up to the user.
• If mbedtls_ssl_flush() fails with MBEDTLS_ERR_SSL_WANT_WRITE, then not all data previously prepared via mbedtls_ssl_write() could be delivered to the underlying transport.

This approach fixes both issues above: At no point is the user obliged to call mbedtls_ssl_write() again with specific parameters, and it's always unambiguous which data has successfully been passed to the TLS stack and only requires flushing, and for which one the user should call mbedtls_ssl_write() again. Moreover, the design allows for multiple DTLS datagrams to be packed into a single UDP datagram by the messaging implementation, because mbedtls_ssl_write() is no longer forced to attempt immediate delivery.

[hanno-becker]

A similar change should likely be made to mbedtls_ssl_close_notify().

[gilles-peskine-arm]

Split the writing API in two parts:

What is the migration path for existing code? Is the old mbedtls_ssl_write(ssl, buf, size) equivalent to this?

write_ret = mbedtls_ssl_write(ssl, buf, size);
if (write_ret < 0) return write_ret;
flush_ret = mbedtls_ssl_flush(ssl);
if (flush_ret < 0) return flush_ret;
return write_ret;

If mbedtls_ssl_flush() fails with MBEDTLS_ERR_SSL_WANT_WRITE, then not all data previously prepared via mbedtls_ssl_write() could be delivered to the underlying transport.

I don't understand in what case mbedtls_ssl_flush would return WANT_WRITE rather than some other error, and what the corrective action is. Attempting to write to a non-blocking socket when it would block, maybe?

[hanno-becker]

@gilles-peskine-arm Yes, in essence that's the migration path. I'm currently drafting an implementation of the new API, and for the example programs, it comes down to adding an mbedtls_ssl_flush() loop at the end of the mbedtls_ssl_write() loop. The mbedtls_ssl_write() loop itself need not be changed, though.

I don't understand in what case mbedtls_ssl_flush would return WANT_WRITE rather than some other error

I'm not sure I got your question right, but mbedtls_ssl_flush() returns WANT_WRITE if not all data previously dispatched via mbedtls_ssl_write() could be delivered to the underlying transport.

[hanno-becker]

What is the migration path for existing code? Is the old mbedtls_ssl_write(ssl, buf, size) equivalent to this?

@gilles-peskine-arm I was a bit inaccurate above: While the pattern is right, the code you gave will not be equivalent to the old semantics of mbedtls_ssl_write(). Instead, it would be something like

if( previous_write_needs_flush == 0 )
{
    write_ret = mbedtls_ssl_write( ssl, buf, size );
    if( write_ret < 0 )
        return( write_ret );

    previous_write_ret = write_ret;
    previous_write_needs_flush = 1;
}

flush_ret = mbedtls_ssl_flush( ssl );
if( flush_ret != 0 )
    return( flush_ret );

previous_write_needs_flush = 0;
return( previous_write_ret );

[jethrogb]

Until the new API is in place, are you willing to commit to the following API description change? I believe it doesn't require any code changes.

mbedtls_ssl_flush_output is now considered public API. If mbedtls_ssl_write returns MBEDTLS_ERR_SSL_WANT_WRITE, then instead of calling mbedtls_ssl_write again, you may call mbedtls_ssl_flush_output. If you do so, you must call mbedtls_ssl_flush_output until it returns 0. Now you may call mbedtls_ssl_write again with different parameters.

[dragan-ponos]

I think this issue is very important for the logic I am writing. Could you please assist me for the following scenario?

1. I try to write data, but it returns MBEDTLS_ERR_SSL_WANT_WRITE.
2. The upper layer decides to close the connection.

I would like to "abort" or "give up on" the write and proceed with sending close_notify.
Is this possible in the old API? Will it be possible in the upcoming API?
I get the impression it is not, and I would need to write data fully before proceeding with close_notify.
Thanks!

[gilles-peskine-arm]

@dragan-ponos In principle you should be able to call mbedtls_ssl_close_notify at that point, possibly in a loop while it returns WANT_WRITE. I'm not sure it actually works, though: I don't think we test that case.

Unfortunately, we're not going to be able to make any changes to the TLS API in Mbed TLS 3.x. Hopefully we'll have the improved API requested here in 4.0.

[dragan-ponos]

@gilles-peskine-arm My code was doing exactly this for some time, but recently, on closer inspection, this came up as a potential issue.

What I see in code is that both mbedtls_ssl_close_notify (which calls mbedtls_ssl_send_alert_message) and mbedtls_ssl_write have a condition near start:

if (ssl->out_left != 0) {
    return mbedtls_ssl_flush_output(ssl);
}

It seems to me that the call to mbedtls_ssl_close_notify will try to flush the pending message, and if it succeeds, it will return 0. The caller would mistakenly think that "close_notify" has been successfully sent.

[gilles-peskine-arm]

@dragan-ponos Thanks for the feedback! Indeed on code inspection this looks like a bug in mbedtls_ssl_close_notify: I guess it should return WANT_WRITE if flush_output returns 0? Would you mind filing a separate issue with at least rough instructions to reproduce the problem, since this seems like a bug we could fix independently of an API change?