TLS 1.3: SRV: Implement mbedtls_ssl_read_early_data()

[ronald-cron-arm]

Description

Fix #6341

Note for reviewers:
Following the work done in #6621, the implementation of mbedtls_ssl_read_early_data() started in #6541 with the function following a logic similar as the mbedtls_ssl_read() one with the function triggering an handshake sequence until some early data are found and returned to the caller. Working with that guideline I realized that the resulting function could also be used easily as a companion of the mbedtls_ssl_handshake/read/write() functions. These functions all trigger an handshake sequence and if as part of this sequence early data are detected, the functions return with an appropriate return value and then the caller can call mbedtls_ssl_read_early_data() to read the early data. That's convenient to adapt current mbedtls_ssl_handshake/read/write() workflows to read early data as well as I experienced when updating ssl_server2 to support early data. Thus now I am wondering if we should just keep the companion behavior of mbedtls_ssl_read_early_data() and thus practically remove from it the call to mbedtls_ssl_handshake(). This would narrow its scope and thus make it easier to apprehend and use I'd say. I would appreciate opinions, thoughts about that.

Opted for the simplest: mbedtls_ssl_read_early_data() does not trigger or resume the handshake (no call to mbedtls_ssl_handshake()). It just read early data that have been detected by a previous call to mbedtls_ssl_handshake(), mbedtls_ssl_handshake_step(), mbedtls_ssl_handshake_read() or mbedtls_ssl_handshake_write().

PR checklist

• changelog not required yet
• backport not required, new feature
• tests provided

[mpg]

(Partial review so far.)

[ronald-cron-arm]

@mpg I believe I have addressed your comments. Following our discussions, I have also added documentation to highlight that mbedtls_ssl_handshake(), mbedtls_ssl_write() may now return a new error code. I have also simplified the read API.

[ronald-cron-arm]

I have some troubles with generate_ssl_debug_helpers.py (only on CI, not locally ...), trying to figure out the commit causing the problem. Sorry for the noise.

[ronald-cron-arm]

CI all green but Windows-2013 not finished, I think this is ready for review.

[mpg]

Thanks for addressing my feedback! Looks almost all good to me, just one question and a few suggestions.

The question (about not checking ssl->early_data_state.srv) is the only reason I'm not approving now. The suggestions are minor and could be left to a follow-up in the interest of avoiding another round-trip.

Note about my review: at this point I'm not familiar enough with the spec and our code to be confident we're only accepting early data when we should (for example, I had completely missed the thing about HRR). So, I'm mostly assuming that negotiation of EarlyData is correct as it has been reviewed before, and also we'll have an opportunity to revisit that when adding negative testing.

[ronald-cron-arm]

The question (about not checking ssl->early_data_state.srv) is the only reason I'm not approving now.

I have removed it.

Note about my review: at this point I'm not familiar enough with the spec and our code to be confident we're only accepting early data when we should (for example, I had completely missed the thing about HRR). So, I'm mostly assuming that negotiation of EarlyData is correct as it has been reviewed before, and also we'll have an opportunity to revisit that when adding negative testing.

I think that's kind of normal you did not spot the need for the change relative to the reception of an HRR while reviewing this PR. The change does not fit well in this PR (I probably should have done it in another PR, sorry about that). It is indeed something we missed when working on and reviewing ssl_tls13_update_early_data_status() (now ssl_tls13_is_early_data_accepted()). PR 8727 adds a test where an HRR is received and thus early data rejected.

Otherwise I had to rebase following the merge of #8711. There was only one conflict, that was easy to resolve.

[mpg]

Thanks for addressing my feedback - and more, I really like the simplification of state handling on the server.

I still have mostly the same question though, which I've tried to clarify. To be clear: I'm not saying there's something wrong, just that I can't convince myself I know what the code is doing, at this point.

[mpg]

Thanks for answering my question. Looks all good to me now.

[tom-cosgrove-arm]

LGTM