Proposal for adding ECN support to QUIC. #1372
Conversation
…ound connection migration, ensure more straightforward checks.
…ound connection migration, ensure more straightforward checks.
…hake. Clarified what the first packets that can be marked as ECT are in a connection.
|
As regards to the pseudo code. if (ack is of type ACK_ECN): guess that is perhaps not correct syntax or ? |
… on continous verification.
|
@gloinul, thanks for putting this together. I have opened a pull request on your repo that you should check and merge. I've fixed typos and the build-related problems. Once we're done with that, we can talk about the other editorial comments I have for you. |
draft-ietf-quic-transport.md
Outdated
|
|
||
| : Initial value = 0, incremented when a packet marked CE is received | ||
|
|
||
| Reception of duplicate packets SHOULD NOT increment the counters. |
There was a problem hiding this comment.
Can we count the packet (for all three counters) before detecting duplicates instead?
IMO, theoretically a receiver should send back all the information it has obtained. Ignoring the CE flag in duplicate packets goes against that principle.
Counting packets after detecting duplicates makes sense only when we can assume that duplication happens near to the receiver than where ECN is applied. I do not think that we have such an assumption.
In practice, the recovery draft with the proposed changes only uses the CE counter to detect if any congestion was reported by the ACK_ECN frame (it's essentially a binary flag). Considering that, I do not think that changing the moment of counting the packets causes any harm.
And lastly but importantly, by changing the rule as such, we can get rid of the requirement to detect duplicates.
See also: #1405 (comment)
There was a problem hiding this comment.
Disagree, see my answer on issue #1405
There was a problem hiding this comment.
#1439 proposes a different scheme that removes the requirement on the receiver-side to detect duplicates.
… avoid unclarity in regards to CE marks and receovery period.
… some sent packets. Removed editorial note that are covered by issue quicwg#1402.
|
Note, I have just added a paragraph for an issue related to persistent ACK loss for some packets. Such events causes an insensitiveness in the continuous verification equal to the number of packets that the receiver seen with ECN markings and never acknowledged in the sender. Please review this late addition. |
|
Based on the discussion in #1405 and the text that is in master branch towards next version requiring frame types to be idempotent I will add text so that the ACK_ECN frame will have this property even if packet duplication occurs. I will do so in what I consider a straight forward and simple way. I will require that only the first received ECN field for an packet number is added to the counters. This should be straightforward as the receiver's ACK data structure should allow one to determine if a packet is older than anything retained, new or duplicate. |
|
Actually if possible, CE should never be ignored because a network node setting CE does not know if a packet is a duplicate or not... |
|
@mirjak In general, QUIC doesn't process duplicate packets and QUIC doesn't ACK anything it can't process, so ignoring duplicate marked packets matches with this model. I suspect a lot more marked packets will be dropped because they were undecryptable(ie: 0RTT) than because of duplication, since duplication is very rare in my experience, and when it does occur, it'd commonly be better to ignore the second mark anyway, since the marking would occur before the duplication. |
draft-ietf-quic-transport.md
Outdated
| application, the codepoint 00 (Not-ECT) MUST be assumed. If any packet | ||
| are duplicated by the network then only the value of the ECN field of | ||
| the packet copy first received SHALL be included in the counters, the ECN | ||
| field value for a duplicate SHALL be ignored. This to prevent the |
There was a problem hiding this comment.
Clarification question: considering the use of "SHALL," does this imply that an endpoint MUST detect all duplicates?
I understand that it is possible to implement such a scheme; an endpoint will keep track of the cut-off PN (the biggest PN that it would ignore) as well as all the PNs that it have seen above the cut-off PN. But I wonder if the intent here is to require all implementations to have such defense.
There was a problem hiding this comment.
Yes, that is the intention, to avoid getting ECN field values for duplicates added to the counters for packets that are included in any ACK. So if a receiver has included it in an ACK, it need to remember that this PN already have an ECN field valued added to the counters. But it doesn't need to detect duplicates for packet that are discarded directly after decryption, i.e. older than the cut-off PN.
This is straightforward and fulfills the requirements that appears to currently exist. If #1405 concludes it would be easier to determine if there are other approaches including other ECN field value encodings as discussed in #1439.
There was a problem hiding this comment.
Thank you for the clarification. My two cents go to that this is an unnecessary burden, because IMO while idempotency is required for frame payloads, it is not required for ECN signal that is injected by an on-path device (even though we need some defense against injection attacks).
There was a problem hiding this comment.
This is incorrect. The requirements that currently exists in tsv is that ALL CEs must be counted because the network does not know if a packet is a duplicate or not!
There was a problem hiding this comment.
I believe the reason TCP counts all CEs is because TCP retransmits packets, not because of an effort to count packets the network inadvertently duplicated. QUIC does not duplicate packets and instead uses new packet numbes for retransmissions, so I believe this embodies the intent of the requirement.
There was a problem hiding this comment.
I agree that this is an edge case and should hopefully not be a problem but saying in the spec that duplicated CEs shall be ignored is not the right thing to do.
There was a problem hiding this comment.
Let's back up just a bit.
First, QUIC receivers are far less likely to receive duplicates because as @ianswett pointed out, retransmissions are not duplicates. As @mirjak notes, this doesn't mean that the network won't do stupid things, but that really is the exception, not the rule. Duplicates in QUIC are necessarily network duplication. This is a significant difference in terms of how much this problem matters. I think suppression is fine, since the tradeoff is between enabling an on-the-side performance attack vs. an infinitesimally more aggressive response to a CE marked packet.
Second, I agree with @kazuho that requiring duplicate detection should not be required, if only because I can cook up conditions under which an endpoint won't be able to do this without infinite state. Consider an endpoint that nacks a packet (sends an ack with this packet in a gap), receives an ack for this nack, and moves its "left edge" past that PN. Now, it receives a packet with that PN. It cannot verify that this packet is a duplicate, since it may never have received the original.
I'd like to point out that this is ultimately a performance attack, and as with any other such attacks that use unauthenticated bits (ICMP?), we can provide some guidance to the receiver of a duplicate. I think the text here can changed a bit, with little to no real difference in impact.
I'll propose the following text: "The ECN counters that an endpoint reports include values of the ECN field only from packets that are subsequently newly acknowledged. Note that an endpoint may newly acknowledge a duplicate packet if it no longer has state about the first receipt of the same packet. An endpoint may decrypt and drop a duplicate packet if it has state about the first receipt of the same packet, and in this case, the endpoint will not increase the ECN counters based on the duplicate packet."
There was a problem hiding this comment.
The problem with that text is that it assumes something quite dangerous: that a receiver might accept a packet when it isn't sure that it has received it before. And content might be different.
What @ianswett suggested before is better: if you move the trailing edge forward, then packets that you believe to be behind that edge are dropped unconditionally. Or, more concisely stated: you only accept packets that you know you haven't accepted before. That errs on the side of dropping rather than accepting, which I believe to be safe.
There was a problem hiding this comment.
The problem with that text is that it assumes something quite dangerous: that a receiver might accept a packet when it isn't sure that it has received it before. And content might be different.
I am not sure if the content could be different. I'd assume that the only case in which it could be different is when the sender reuses a packet number. I do not think that we need to detect that (and if we do, it should be a PROTOCOL_VIOLATION rather than a drop).
There was a problem hiding this comment.
@martinthomson it was not my intention to require infinite state. I will see if I can reformulate to say that for any packet within the window of PNs that you will accept to process at all you need to maintain duplication detection before adding the ECN field values to the counters.
@mirjak TSVWG has asked if there was anything from the QUIC ECN work that was of general nature and should be discussed in TSVWG and I think this is one of those things. However, my opinion is that due to ECN's unauthenticated nature, it is reasonable to suppress network duplicates. Otherwise use of ECN makes one more open to attacks to drive down the congestion window for a connection than when not using it. Yes, the connection will be slightly less responsive to the congestion signal, how much is depending on the amount of network duplication that occur.
There are a few sections here that seem to be quite verbose. Nothing technically wrong, but I think that fewer words would be OK.
draft-ietf-quic-recovery.md
Outdated
| previous_ecn_ce_ctr. | ||
|
|
||
| ~~~ | ||
| CongestionEvent(packet_number): |
There was a problem hiding this comment.
This looks like a generic function (it's invoked from OnPacketsLost). It should probably be moved to a common section..
draft-ietf-quic-transport.md
Outdated
|
|
||
| The first client packet of the cryptographic handshake protocol MUST fit | ||
| within a 1232 octet QUIC packet payload. This includes overheads that | ||
| reduce the space available to the cryptographic handshake protocol. |
There was a problem hiding this comment.
Not sure what is going on here. Looks like gratuitous reflow.
draft-ietf-quic-transport.md
Outdated
| Explicit Congestion Notification (ECN) {{!RFC3168}} is feature that allows for | ||
| non-destructive congestion notification by a network node. That is, packets are | ||
| marked instead of being discarded by routers and other devices along a network | ||
| path. QUIC endpoints perform capability verification on connection establishment |
There was a problem hiding this comment.
non-sequitur: this moves from saying what ECN is to capability verification. Suggest: "QUIC endpoints determine whether a path correctly supports ECN marking by verifying paths when connections are established and when migrating the connection to a new path."
draft-ietf-quic-transport.md
Outdated
| marked instead of being discarded by routers and other devices along a network | ||
| path. QUIC endpoints perform capability verification on connection establishment | ||
| as well as connection migration. ECN can be used unidirectionally, | ||
| bidirectionally, or disabled depending on endpoint and path capabilities. |
There was a problem hiding this comment.
This is odd, but you might instead say "Each peer independently validates network paths, which leads to ECN being enabled separately for each direction on a path."
draft-ietf-quic-transport.md
Outdated
| The capability check makes use of the ACK_ECN frame in {{frame-ack-ecn}}. Each | ||
| endpoint individually performs an ECN capability check for its send path. The | ||
| check is performed by attempting to set the ECN bits in the IP header to either | ||
| ECT(0) or ECT(1) for packets generated by the endpoint, clients starting with |
There was a problem hiding this comment.
I'm struggling a little with the length of this sentence, and I think that we could be a little more concise with the remainder of this section:
"""
ECN is validated by setting the ECT(0) or ECT(1) bit in the IP header, following the guidelines of {{!RFC8311}}. Clients and servers both mark the IP packet containing an Initial packet ({{packet-initial}}). If ECN is supported, the recipient of a marked packet will increase the corresponding counter and send an ACK_ECN frame.
An endpoint uses the ACK_ECN frame to verify that the ECT markings were received by its peer. The endpoint counts the number of acknowledged QUIC packets that were sent in marked IP packets. If the counters in the ACK_ECN frame match that number, the path and peer do not remove ECN markings. The endpoint records this path as being ECN capable and continues to mark IP packets that it sends.
If an endpoint receives an ACK frame, indicating that no markings were received, or the counter in the ACK_ECN frame do not match the number of QUIC packets that were sent in marked IP packets, then the path or peer remove ECN markings. The endpoint records this path as not being ECN capable and it ceases marking of packets.
IP packets sent on a new path SHOULD be marked with ECT(0) or ECT(1) to verify that the new path supports ECN, see {{ecn-connection-migration}}.
"""
Note that I've chosen to assume the stream 0 changes here. It simplifies things.
I've also made clear the distinction between QUIC packets and IP packets. With coalescing, that distinction is critical. I've opted for counting of QUIC packets in marked IP packets rather than marked IP packets. More on this below...
There was a problem hiding this comment.
I think this proposal is very good, and agree about the IP packet thing and Stream 0 change. The only thing I 100% happy is the explicit mention of ECT(0) being the default.
draft-ietf-quic-transport.md
Outdated
| again. The ECN capability as indicated in section {{ecn-capability-check}} | ||
| should be repeated when a connection is migrated. This verifies that the | ||
| endpoints are ECN-capable and that the ECN field is not bleached along the new | ||
| path. |
There was a problem hiding this comment.
This section could probably be more concise.
"""
Each new path is probed to determine whether it supports ECN. Packets sent on the new path are sent in IP packets with an ECT marking as described in {{previous-section-on-ecn}}.
Markings, or absence of markings, on packets sent on multiple paths can make it difficult to correctly attribute counters with markings on specific packets. Recording the packet number when connection migration occurred might help in correlating increases in counters with packets sent on the new path.
If an acknowledgment indicates that the markings were retained, the path is marked as ECN capable and subsequent IP packets sent on that path continue to be ECT marked. If an acknowledgment indicates that ECN markings are removed, subsequent packets are sent with Non-ECT.
"""
draft-ietf-quic-transport.md
Outdated
| The ECN block is described below. The size (i) indicates variable-length | ||
| encoding, explained in {{integer-encoding}}. The encoding length for each | ||
| counter (1, 2, 4 or 8 bytes) should be selected such that all the | ||
| significant bits are represented. |
There was a problem hiding this comment.
This second sentence is unnecessary and potentially misleading.
draft-ietf-quic-transport.md
Outdated
| congestion avoidance behavior of the sender. Removing any ECN-CE marking causes | ||
| senders to maintain or increase their sending rate beyond that the path can | ||
| sustain, which will eventually result in loss. Adding an ECN-CE marking causes | ||
| senders to reduce their sending rate. The later could equally be accomplished by |
draft-ietf-quic-transport.md
Outdated
| sustain, which will eventually result in loss. Adding an ECN-CE marking causes | ||
| senders to reduce their sending rate. The later could equally be accomplished by | ||
| dropping packets for the connection. Section 18 and 19 of {{!RFC3168}} discusses | ||
| the effects of undesired manipulation of the ECN field in more details. |
draft-ietf-quic-transport.md
Outdated
|
|
||
| If a receiver does not discard duplicate packets, an off-path attacker can | ||
| retransmit packets with ECN bits set and manipulate the senders congestion | ||
| avoidance state. If duplicate packets are dropped, the off-path attacker will |
…ions. Changed capability check and connection migration text significantly.
Editorial fixes
draft-ietf-quic-transport.md
Outdated
| +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+ | ||
| | ACK Blocks (*) ... | ||
| +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+ | ||
| | ECN Block ... |
There was a problem hiding this comment.
As discussed on the list, I think it makes sense to put the ACK Block Count and ACK Blocks at the end of this frame, to make it simpler to truncate the ack blocks if necessary.
There was a problem hiding this comment.
Agree, if no ones protests, I will implement that change tomorrow.
draft-ietf-quic-transport.md
Outdated
| @@ -2999,10 +2999,10 @@ appended at the end. | |||
| +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+ | |||
| | ACK Block Count (i) ... | |||
| +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+ | |||
There was a problem hiding this comment.
Can you move the ACK Block Count down as well. It's odd to have that up here.
There was a problem hiding this comment.
Also, how about putting the ECN info in this diagram, rather than having it in a separate diagram?
There was a problem hiding this comment.
Yes, I merged the ECN counters and text into a single section for the ACK_ECN frame. I think that is very reasonable. So please check this latest change.
…e the ACK Block Count.
| A QUIC connection MUST keep counters for each ECN codepoint, recording the | ||
| number of packets that were received with the corresponding ECN codepoint in the | ||
| IP header. If the header is not readable from the application, the codepoint 00 | ||
| (Not-ECT) MUST be assumed. If any packet are duplicated by the network then only |
There was a problem hiding this comment.
s/by the network/by the network,/
| (Not-ECT) MUST be assumed. If any packet are duplicated by the network then only | ||
| the value of the ECN field of the packet copy first received SHALL be included | ||
| in the counters. This to prevent the on-the-side attack ({{security-ecn}}) and | ||
| ensure that ACK_ECN frames becomes idempotent in the event of packet |
| ensure that ACK_ECN frames becomes idempotent in the event of packet | ||
| duplication. Note, a receiver is not required to maintain indefinite state for | ||
| which packet numbers have been received far into the history. Packets discarded | ||
| for this reason, their ECN values are also not counted. |
There was a problem hiding this comment.
These sentences read awkwardly. Maybe "when information about old packets is discarded at a receiver, any ECN information associated with them is also discarded" or something like that?
| which packet numbers have been received far into the history. Packets discarded | ||
| for this reason, their ECN values are also not counted. | ||
|
|
||
| ACK_ECN Frame MUST be used when when an endpoint is acknowledging a packet were |
There was a problem hiding this comment.
s/ACK_ECN Frame/An ACK_ECN frame/
There are other instances where "Frame" is capitalized; they should also be lowercased IMO for consistency with the rest of the docs.
| @@ -1427,6 +1431,63 @@ a single packet. | |||
|
|
|||
| In TLS, the Retry packet type is used to carry the HelloRetryRequest message. | |||
|
|
|||
| ## ECN Capability Check {#ecn-capability-check} | |||
|
|
|||
| Explicit Congestion Notification (ECN) {{!RFC3168}} is feature that allows for | |||
| If the acknowledgements from the receiver are lost such that one or more packet | ||
| are received by the receiver, but never acknowledged to the sender an | ||
| insensitivity to bleaching will be created. In this situation the ECN counters | ||
| reported will have increase, but the sender side total for acknowledged packets |
| the current number of totally acknowledged packets and latest ECN counters. Then | ||
| comparison are done by subtracting these stored values from the respective | ||
| counters prior to the comparison. Note that any out-of-order ACK_ECN frames | ||
| can't be used for determining any loss of acknowledgements. |
There was a problem hiding this comment.
This whole paragraph is really difficult to understand. "Comparison points" are not mentioned anywhere else, for starters.
| Markings, or absence of markings, on packets sent on multiple paths can make it | ||
| difficult to correctly attribute counters with markings on specific packets. | ||
| Recording the packet number when connection migration occurred might help in | ||
| correlating increases in counters with packets sent on the new path. |
| ~~~ | ||
| {: #ACN_ECN_FRAME_FORMAT title="ACK_ECN Frame Format"} | ||
|
|
||
| The receiver side report three ECN counters in the ACK_ECN frame. These counters |
| {: #ACN_ECN_FRAME_FORMAT title="ACK_ECN Frame Format"} | ||
|
|
||
| The receiver side report three ECN counters in the ACK_ECN frame. These counters | ||
| counts the number of packets marked with this codepoint since the start of the |
|
@martinthomson : I assume you're planning to merge my PR on top of this? |
|
Your PR went into master at the same time. This closed when I merged your changes in. |
This pull request contains an initial adaptation of what was in the design teams Wiki, but with more explanatory glue, and modifications to places where ACK or ACK_ECN frames can be used for example.
Recovery draft changes are only pseudo code level so far and need a bit more work.