search

aiortc / aioquic

QUIC and HTTP/3 implementation in Python
BSD 3-Clause "New" or "Revised" License
1.66k stars 236 forks source link

Comply with RFC 9000 section 14.1 #481

Closed rthalley closed 6 months ago

rthalley commented 6 months ago

We were accepting some initial packets that were too small [#401].

We were also emitting datagrams that were too small for client initial acks and server initial ack-eliciting packets.

codecov[bot] commented 6 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 100.00%. Comparing base (ae282aa) to head (ed8da26).

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #481 +/- ## ========================================= Coverage 100.00% 100.00% ========================================= Files 25 25 Lines 4966 4970 +4 ========================================= + Hits 4966 4970 +4 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

rthalley commented 6 months ago

We don't currently have super complicated coalescing, and when we detect a need for padding we just do it. This may result in some lost coalescing opportunities, but I don't want to attempt anything that complicated at the moment.

A number of tests had to be updated due to datagram counts changing. I recomputed some of the numbers by hand to make sure they were correct, but after that I just updated the remaining tests only checking for sanity. I.e. if the new values I got looked reasonable, I updated the test to accept them. In cases where additional datagrams were emitted, I transmitted those values to the other peer in the test.

I wrote an additional test to purposely send too-short data to the server, and verify that the server follows 14.1 and drops it. I check this by recording a qlog and making sure the drop event is in it.

I haven't seen any problems interoperating with other servers after making this fix, and we continue to interoperate with old versions of ourselves as the invalid short packets we now drop are acks, and things seem to catch up as the handshake succeeds.

jlaine commented 6 months ago

Wow, this is interesting. I was aware we were not rejecting some INITIAL which we should have rejected but not that we were emitting insufficiently padded packets. I think this should be two different PRs or at the very least two different commits.

For the part where a server rejects insufficiently padded INITIAL packets : I see the RFC allows us to close the connection with a protocol violation, any reason for not doing this?

rthalley commented 6 months ago

I can split this into two PRs. I agree the RFC allows us to close immediately, and I didn't do it just because I was trying to have a light touch on the tests. I can try to do the immediate close when I split it. That will probably happen on Monday too.

jlaine commented 6 months ago

I can split this into two PRs. I agree the RFC allows us to close immediately, and I didn't do it just because I was trying to have a light touch on the tests. I can try to do the immediate close when I split it. That will probably happen on Monday too.

I think I would rather close the connection, because I don't see how we can sanely recover from the situation. If the remote peer isn't padding their packets sufficiently: