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

forbid key discarding before receiving a packet protected with new keys #4079

Merged
merged 3 commits into from Sep 10, 2020
Merged

forbid key discarding before receiving a packet protected with new keys #4079

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
3064d6b
forbid key discarding before receiving a packet protected with new keys
marten-seemann Sep 8, 2020
1bad278
Update draft-ietf-quic-tls.md
ianswett Sep 8, 2020
4898bcd
Apply suggestions from code review
marten-seemann Sep 9, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion draft-ietf-quic-tls.md
View file
Open in desktop
Expand Up @@ -1497,7 +1497,7 @@ receiving packets. These keys will be needed to process packets the peer sends
after updating.

An endpoint MUST retain old keys until it has successfully unprotected a packet
sent using the new keys. An endpoint SHOULD NOT discard old keys immediately
sent using the new keys. An endpoint SHOULD retain old keys for some time
after unprotecting a packet sent using the new keys. Discarding old keys too
early can cause delayed packets to be discarded. Discarding packets will be
interpreted as packet loss by the peer and could adversely affect performance.
Copy link
Member

@kazuho kazuho Sep 8, 2020 •
edited

Choose a reason for hiding this comment

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

Regarding the first sentence, I think SHOULD works too, but I agree that MUST is better here.

I do not think we need the second sentence, as we state that an endpoint "SHOULD retain no more than 3 PTO" down below.

I think the crux of the problem is the last two sentences that are left unmodified. As correctly pointing out by the issue, the outcome of discarding the keys too early is more than just a performance degradation. Doing so might kill the connection. We should state that clearly, and then the choice of the keyword in the first sentence becomes more of an editorial preference.

Copy link
Contributor

@MikeBishop MikeBishop Sep 8, 2020 •
edited

Choose a reason for hiding this comment

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

It is just a performance issue provided the initiator keeps sending. You lose some reordered packets and have spurious detection of some extra packet loss. Where the connection hard-fails is when the peer never updates / learns about the update because the initiator stops sending after the key update.

Copy link
Member

@kazuho kazuho Sep 8, 2020 •
edited

Choose a reason for hiding this comment

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

I'm not sure if I agree. The point is that the initiator is not required to keep sending.

As @marten-seemann points out, an endpoint that only receives data (but does not send any) might decide to do a key update. If that endpoint drops the old receive key before any of the packets it sent reaches the peer, the connection will be stuck.

That is because all the ack-eliciting packets involved will be the ones that are sent by the peer using the old key, which the endpoint has dropped that key.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So here's what happens when you initiate a key update from key phase N to N+1: You start using N+1 for encrypting packets. As it's not guaranteed that those packets are received by the peer, it's not safe to drop keys for key phase N yet. You only know that the peer received a packet protected with N+1 keys once you successfully unprotect a packet encrypted with N+1 keys (as the peer is required to update its keys immediately once it receives your N+1 packet).
From this point on, it is safe to drop key N. However, it's not recommended, as there might be reordering, so keeping the key for a bit longer allows you to process delayed packets. At this point, this is just a performance optimization.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, it's safe to drop key N when you receive packets protected with key N+1. And it's true that keeping keys for a short while helps avoid dropping reordered packets.

However, it it generally safer to only drop key N when you receive an acknowledgment for a packet sent with N+1 keys. Because the other side might have updated on its own. That means that the N+1 packet you received isn't any indication that they received your key update. This distinction doesn't matter for dropping keys, but it does matter for determining whether you can initiate an update to N+2 keys later.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

However, it it generally safer to only drop key N when you receive an acknowledgment for a packet sent with N+1 keys.

That's correct. In my mental model (and in my implementation), these are two separate conditions though. I can start the timer to drop the keys before I'm allowed to update to N+2.

Do you think we need to make any changes to this text here?

Copy link
Member

Choose a reason for hiding this comment

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

I think that it's OK to have two events, and technically the drop events are bound to use of keys. In that regard, I'm comfortable with the text you have. I don't think we need to rely exclusively on acknowledgments for driving this.

Expand Down