New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add minimal handling of NEW_CONNECTION_ID frames #20892
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me. But I think this PR is small enough we can add the tests in this PR (IMO).
Also, can you confirm if you've tested this code against a server which issues a Retry? I'd be interested to know. |
Do you have any hints on how to write the test? I can think of only somehow modifying the test server to issue NEW_CONNECTION_ID frame. |
As far as I know the Cloudflare Quiche issues a retry. But I'll verify. |
Looks good, needs second reapproval. @paulidale |
@hlandau I've added also a negative testcase, please reconfirm |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice
This pull request is ready to merge |
Sorry but this PR is currently incompatible with master branch. Needs rebasing and fixing for test/quic_newcid_test.c because of missing parameters. |
We actively use only the latest DCID received. And retire only DCIDs requested by the peer to be retired. Also changed the active_conn_id_limit to 2 as the minimum value allowed.
seq_id must be >= retire_prior_to. Add negative testcase.
Rebased to resolve merge conflicts please reconfirm @paulidale @hlandau |
This pull request is ready to merge |
Merged to master. Thank you. |
Reviewed-by: Hugo Landau <hlandau@openssl.org> Reviewed-by: Paul Dale <pauli@openssl.org> (Merged from #20892)
We actively use only the latest DCID received. And retire only DCIDs requested by the peer to be retired. Also changed the active_conn_id_limit to 2 as the minimum value allowed. Reviewed-by: Hugo Landau <hlandau@openssl.org> Reviewed-by: Paul Dale <pauli@openssl.org> (Merged from #20892)
seq_id must be >= retire_prior_to. Add negative testcase. Reviewed-by: Hugo Landau <hlandau@openssl.org> Reviewed-by: Paul Dale <pauli@openssl.org> (Merged from #20892)
Reviewed-by: Hugo Landau <hlandau@openssl.org> Reviewed-by: Paul Dale <pauli@openssl.org> (Merged from #20892)
Reviewed-by: Hugo Landau <hlandau@openssl.org> Reviewed-by: Paul Dale <pauli@openssl.org> (Merged from #20892)
Reviewed-by: Hugo Landau <hlandau@openssl.org> Reviewed-by: Paul Dale <pauli@openssl.org> (Merged from #20892)
We actively use only the latest DCID received. And retire only
DCIDs requested by the peer to be retired.
Also changed the active_conn_id_limit to 2 as the minimum value allowed.
Adding test for this will be non-trivial but we will need it so setting
tests: deferred
.I assume we can do something with the fault injector.