Skip to content
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.

Sign up for GitHub

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

Jump to bottom

QUIC: Do not discard the INITIAL el too early #21713

Closed
wants to merge 6 commits into from
Closed

QUIC: Do not discard the INITIAL el too early #21713

wants to merge 6 commits into from

Conversation

t8m
Copy link
Member

@t8m t8m commented Aug 10, 2023 •
edited by tom-cosgrove-arm

RFC says that successful decryption of HANDSHAKE el packet triggers the discard on server side only.

On client we discard INITIAL el when we successfully send a HANDSHAKE packet.

Fixes #21607

@t8m
QUIC: Do not discard the INITIAL el too early
c32bb07 
RFC says that successful decryption of HANDSHAKE el packet
triggers the discard on server side only.

On client we discard INITIAL el when we successfully send
a HANDSHAKE packet.
@t8m t8m added branch: master Merge to master branch approval: review pending This pull request needs review by a committer approval: otc review pending This pull request needs review by an OTC member triaged: bug The issue/pr is/fixes a bug tests: exempted The PR is exempt from requirements for testing labels Aug 10, 2023
@t8m t8m mentioned this pull request Aug 10, 2023
Closed
@t8m
Copy link
Member Author

t8m commented Aug 10, 2023

This fixes interop with quic.tech:4433

@t8m
Copy link
Member Author

t8m commented Aug 10, 2023

Not sure how to test it so setting test: exempted but if you have any hints/ideas for the test, they are welcome.

@t8m t8m requested a review from hlandau August 10, 2023 16:11
@jfclere
Copy link

jfclere commented Aug 10, 2023

75-test_quicapi.t needs to be adjusted.

@t8m
Update the ssltraceref.txt
d004878 
Also adds saving the new trace to ssltraceref-new.txt in
test-runs which can be handy when the trace changes and
needs to be updated.
@t8m
quic_trace.c: Fix typo in traces
67d7122
@t8m
Copy link
Member Author

t8m commented Aug 10, 2023

75-test_quicapi.t needs to be adjusted.

Yep, and that is actually the test case. 😁

@t8m t8m added tests: present The PR has suitable tests present and removed tests: exempted The PR is exempt from requirements for testing labels Aug 10, 2023
@t8m t8m self-assigned this Aug 10, 2023
@t8m
Copy link
Member Author

t8m commented Aug 10, 2023

Aargh... this makes the ssltraceref non-deterministic.

@hlandau
Copy link
Member

hlandau commented Aug 10, 2023

Good catch. LGTM once this passes.

@t8m
Copy link
Member Author

t8m commented Aug 10, 2023

Good catch. LGTM once this passes.

It seems we're sending PINGs very early. I'll need to investigate that.

jfclere added a commit to jfclere/openssl-h3-examples that referenced this pull request Aug 11, 2023
@jfclere
Arrange the logic...
003361d 
For some reason openssl/openssl#21713
causes SSL_accept_stream() to return NULL now.
note that looks OK according to:
https://www.openssl.org/docs/manmaster/man3/SSL_accept_stream.html
@jfclere
Copy link

jfclere commented Aug 11, 2023

All my interop tests are passing now.

@jfclere
Copy link

jfclere commented Aug 15, 2023

I did a bunch of tests on a "slow" win64 I am not able to reproduce the failing test.

@t8m t8m closed this Aug 15, 2023
@t8m t8m reopened this Aug 15, 2023
@t8m
Copy link
Member Author

t8m commented Aug 15, 2023

Hmm... this looks much better. @hlandau what do you think about d878df2

@t8m t8m closed this Aug 16, 2023
@t8m t8m reopened this Aug 16, 2023
@t8m
Copy link
Member Author

t8m commented Aug 16, 2023

Changing MAX_NAT_INTERVAL (in ssl/quic/quic_channel.c) makes the test pass... May be we can detect a slow box and adjust or force the ping in the test.

Does it really? The MAX_NAT_INTERVAL is 25s where the the half of max_idle_timeout is 15s

@jfclere
Copy link

jfclere commented Aug 16, 2023

@t8m After more tests in fact it doesn't change anything.

@jfclere
Copy link

jfclere commented Aug 17, 2023

I am getting crazy: I have put a trace in ssl/quic/quic_channel.c near:
/* If a ping is due, inform TXP. */
but I don't see it.. I also traced ch_update_ping_deadline() and the ch->ping_deadline is too big to be triggered.
Any ideas?

@t8m
Copy link
Member Author

t8m commented Aug 17, 2023

Have you ever triggered the actual failure of the test locally? Because I am unable to. It just happens in the CI.

@t8m
qtest: Use fake time on both client and server
772f5c6 
And use QTEST_FLAG_FAKE_TIME with test_ssl_trace().
@t8m
Copy link
Member Author

t8m commented Aug 17, 2023

@jfclere another possibility of ping frame is a probe whose deadline is much shorter because it is based on RTT.

The faked client time seems to resolve this. So this is now ready for review.

@t8m t8m requested a review from mattcaswell August 17, 2023 19:12
@t8m
Copy link
Member Author

t8m commented Aug 17, 2023

d878df2 and 56ea15b could be possibly dropped but I think both are beneficial anyway.

@mattcaswell mattcaswell removed the approval: otc review pending This pull request needs review by an OTC member label Aug 18, 2023
hlandau
hlandau requested changes Aug 18, 2023
View reviewed changes
Copy link
Member

@hlandau hlandau left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good other than using fake time, that doesn't make sense AFAICT.

test/quicapitest.c Show resolved Hide resolved
@t8m t8m requested a review from hlandau August 18, 2023 14:14
@hlandau hlandau added approval: done This pull request has the required number of approvals and removed approval: review pending This pull request needs review by a committer labels Aug 21, 2023
@openssl-machine openssl-machine removed the approval: done This pull request has the required number of approvals label Aug 22, 2023
@openssl-machine
Copy link
Collaborator

This pull request is ready to merge

@openssl-machine openssl-machine added the approval: ready to merge The 24 hour grace period has passed, ready to merge label Aug 22, 2023
@t8m
Copy link
Member Author

t8m commented Aug 22, 2023

Merged to master branch. Thank you for the reviews.

@t8m t8m closed this Aug 22, 2023
openssl-machine pushed a commit that referenced this pull request Aug 22, 2023
@t8m
QUIC: Do not discard the INITIAL el too early
b6125b5 
RFC says that successful decryption of HANDSHAKE el packet
triggers the discard on server side only.

On client we discard INITIAL el when we successfully send
a HANDSHAKE packet.

Reviewed-by: Matt Caswell <matt@openssl.org>
Reviewed-by: Hugo Landau <hlandau@openssl.org>
(Merged from #21713)
openssl-machine pushed a commit that referenced this pull request Aug 22, 2023
@t8m
Update the ssltraceref.txt
b7278ee 
Also adds saving the new trace to ssltraceref-new.txt in
test-runs which can be handy when the trace changes and
needs to be updated.

Reviewed-by: Matt Caswell <matt@openssl.org>
Reviewed-by: Hugo Landau <hlandau@openssl.org>
(Merged from #21713)
openssl-machine pushed a commit that referenced this pull request Aug 22, 2023
@t8m
quic_trace.c: Fix typo in traces
604a607 
Reviewed-by: Matt Caswell <matt@openssl.org>
Reviewed-by: Hugo Landau <hlandau@openssl.org>
(Merged from #21713)
openssl-machine pushed a commit that referenced this pull request Aug 22, 2023
@t8m
QUIC: Update ping deadline when we receive a packet
8fd32a0 
Reviewed-by: Matt Caswell <matt@openssl.org>
Reviewed-by: Hugo Landau <hlandau@openssl.org>
(Merged from #21713)
openssl-machine pushed a commit that referenced this pull request Aug 22, 2023
@t8m
qtest: Run both client and server during connect
608a95f 
Reviewed-by: Matt Caswell <matt@openssl.org>
Reviewed-by: Hugo Landau <hlandau@openssl.org>
(Merged from #21713)
openssl-machine pushed a commit that referenced this pull request Aug 22, 2023
@t8m
qtest: Use fake time on both client and server
617cab0 
And use QTEST_FLAG_FAKE_TIME with test_ssl_trace().

Reviewed-by: Matt Caswell <matt@openssl.org>
Reviewed-by: Hugo Landau <hlandau@openssl.org>
(Merged from #21713)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hlandau hlandau hlandau approved these changes

@mattcaswell mattcaswell mattcaswell approved these changes

Assignees

@t8m t8m

Labels
approval: ready to merge The 24 hour grace period has passed, ready to merge branch: master Merge to master branch tests: present The PR has suitable tests present triaged: bug The issue/pr is/fixes a bug
Projects
Project Board
Status: Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@t8m @jfclere @hlandau @openssl-machine @mattcaswell