Do not generate new random number while receiving HRR

[BensonLiou]

Description

According to RFC8446, random number of 2nd clientHello should be same as first one while receiving HRR. This PR is to preventing re-generating random number on 2nd clientHello Issue reported on 8669

PR checklist

Please tick as appropriate and edit the reasons (e.g.: "backport: not needed because this is a new feature")

• [x] changelog provided, or not required
• [x] backport done, or not required
• [x] tests provided, or not required

Notes for the submitter

Please refer to the contributing guidelines, especially the checklist for PR contributors.

[gilles-peskine-arm]

Thanks for contributing a fix! Please amend your commit to add a sign-off line. We need the DCO check to pass to accept external contributions.

We normally want a non-regression test for bug fixes. Would you mind adding one? I guess it would be a unit test in test/suites/test_suite_ssl that does two 1.3 handshakes, arranges for the server to send a HelloRetryRequest, records the random values sent by the client and checks that they're the same.

[BensonLiou]

I will do it but I might need time to understand how to add test cases to the framework. It will be helpful if there are documents to describe it.

[gilles-peskine-arm]

There's general guidance in https://mbed-tls.readthedocs.io/en/latest/kb/development/test_suites/ though you can probably figure most of it by example looking at tests/suites/test_suite_ssl.data and tests/suites/test_suite_ssl.function.

For how to write the test cases, I'm not very familiar with the helper functions we have for SSL, but I think a good start would be the move_handshake_to_state function, with some added code to have the server send ServerRetryRequest, and compare the random read from the handshake structure before and after the client processes the SRR.

[gilles-peskine-arm]

Please avoid including merge commits in pull requests, unless a conflict appears after the first review. It's ok if a pull request forks from an old commit as long as there's no conflict.

[BensonLiou]

The 2nd merge is required because I need the test function tls13_cli_early_data_status() I check the random number from client side instead of server side since client state is more controllable and also less code change.

[gilles-peskine-arm]

Thanks for the update! We are two days from the 3.6.0 release code freeze, we'll try to include this fix.

One thing I can see before reviewing in detail is that this is missing a changelog entry. Something like:

Bugfix
   * In TLS 1.3 clients, fix an interoperability problem due to the client
     generating a new random after a HelloRetryRequest. Fixes #8669.

For future reference, when a pull request hasn't been reviewed yet, we prefer to rebase on top of development rather than merge (but only if there are conflicts, if not a pull request forked from an older commit is fine), because merges take work to review. But at this point here, please keep the existing history with the merges.

[gilles-peskine-arm]

Unfortunately there's a merge conflict again :cry:

[BensonLiou]

I use the github CLI to resolve the conflict and it seems github do the merge for me. Sorry for that😣

[gilles-peskine-arm]

Thanks! We still need a changelog entry though.

[gilles-peskine-arm]

Thanks everyone for the quick reactions! We'll have this fixed in the 3.6.0 release.