Include handshake anti-deadlock logic in pseudocode #2281

Ralith · 2019-01-01T01:08:42Z

Recovery §6.2.1 instructs:

Because the server could be blocked until more packets are received, the client MUST start the crypto retransmission timer even if there is no unacknowledged CRYPTO data. If the timer expires and the client has no CRYPTO data to retransmit and does not have Handshake keys, it SHOULD send an Initial packet in a UDP datagram of at least 1200 bytes. If the client has Handshake keys, it SHOULD send a Handshake packet.

but this was not reflected in the pseudocode. I think these changes reflect the intended behavior.

martinthomson

I think that this requirement only applies when Initial packets are in flight. The text allows for Handshake, but I think that we don't need these special rules for Handshake packets. Though we might want to send more aggressively for Handshake, the standard rules are sufficient there. I wonder if we need to change the text.

draft-ietf-quic-recovery.md

Ralith · 2019-01-01T04:26:26Z

I think that this requirement only applies when Initial packets are in flight. The text allows for Handshake, but I think that we don't need these special rules for Handshake packets.

I interpreted "MUST NOT send more than three times as many bytes as the number of bytes they have received" as referring to real bytes transmitted, rather than congestion control bytes in flight. Given that, repeated complete loss of the server's handshake flights can cause a server to overrun its budget before the client's address is validated by handshake completion, requiring a padding packet to be transmitted even if the client received acknowledgement for its Initial.

martinthomson · 2019-01-01T22:49:23Z

The x3 rule does refer to simple bytes as opposed to bytes in flight as calculated for the purposes of congestion control. So PADDING is completely sufficient for the client.

My point was that this extra "code" exists only for the purposes of driving that x3 rule, and is only necessary when the client is sending Initial packets. If it is sending Handshake packets, there is no need because any Handshake packet of any size is sufficient to validate the client address and disable the x3 rule.

(BTW, this wasn't directly a comment on this PR, but on the cited text as well as the code.)

Ralith · 2019-01-01T23:11:58Z

BTW, this wasn't directly a comment on this PR, but on the cited text as well as the code.

Right, but I think the currently-required behavior is justified. Consider this case:

Client sends Initial w/ CRYPTO
Server sends Initial w/ CRYPTO, ACK, followed by Handshake flight; Handshake flight is lost in full
Client establishes Handshake keys; sends Initial w/ ACK
Server Handshake packets continue to be lost until the 3x budget is spent.

Because the client's crypto packets have all been ACKed, and it has received no further ack-eliciting packets, it has nothing to send.

If the client's second Initial was also lost, meaning the budget is only about 3600 bytes, this can happen almost immediately; a single Quinn server Handshake flight is larger than that.

martinthomson · 2019-01-01T23:44:23Z

The client can get Handshake keys in this case, but it won't be sending Handshake packets until it has all of the server Handshake packets, so I think we're still in the original state. This is still the case that there are outstanding Initial packets.

If the client were to be sending Handshake packets, this rule - as written - would still apply, but my point is that it isn't necessary.

Ralith · 2019-01-02T00:03:50Z

it won't be sending Handshake packets until it has all of the server Handshake packets

Is that accurate? Transport §17.6 has:

Once a client has received a Handshake packet from a server, it uses Handshake packets to send subsequent cryptographic handshake messages and acknowledgments to the server.

It seems that client might reasonably send Handshake-level ACKs (for example) prior to receiving the server's full flight even under ideal network conditions.

This is still the case that there are outstanding Initial packets.

Ah, it wasn't clear to me that the client's second, ack-only, Initial packet qualified as "in flight"/"outstanding" here. Even then, the client discards the Initial space after first sending a Handshake packet, which might not be ack-eliciting, and might become lost, so a padding-only packet might still be necessary.

martinthomson · 2019-01-02T00:09:10Z

Right, I had forgotten. Handshake ACKs are sent earlier. Either way, once that starts, the risk of deadlocking is gone.

Ah, it wasn't clear to me that the client's second, ack-only, Initial packet qualified as "in flight"/"outstanding" here.

Good point. The current text says "When crypto packets are sent, the sender MUST set a timer for the crypto timeout period." And crypto packets don't include ACK-only Initial packets.

That suggests a bigger problem with that text. I would suggest then that the condition is "Initial packets outstanding and no packet from a higher encryption level ever sent".

Ralith · 2019-01-02T00:21:38Z

Either way, once that starts, the risk of deadlocking is gone.

The risk of deadlocking remains so long as the client has not sent any ack-eliciting, i.e. retransmittable, Handshake packets and has not received any Handshake ACKs, because it is possible that all of the client's Handshake packets might be lost, at which point the server may reach the 3x threshold and cease transmitting while the client has nothing to send.

Ralith · 2019-01-04T00:31:50Z

On review, the client certainly doesn't need to pad Handshake level anti-deadlock packets to 1200 bytes, though it must send them so long as it hasn't seen an ACK or sent anything that it would otherwise be required to retransmit.

ianswett · 2019-01-06T21:06:32Z

draft-ietf-quic-recovery.md

-      return
-
-    if (crypto packets are in flight):
+    if (crypto packets are in flight || handshaking client):


I agree the pseudocode doesn't match the text, but I think the term "handshaking client" is not specific enough a definition to use in the pseudocode.

Yeah, I wasn't sure what to go with. Should I introduce a new defined variable?

Ralith · 2019-01-12T05:06:03Z

Updated to use an explicit variable, and to use unpadded packets at Handshake level.

ianswett

Thanks, I think this is heading in the right direction. I'd like it to be cleaner, but this isn't a clean mechanism.

ianswett · 2019-01-15T20:48:25Z

draft-ietf-quic-recovery.md

@@ -699,6 +703,9 @@ Pseudocode for OnAckReceived and UpdateRtt follow:
    largest_acked_packet = max(largest_acked_packet,
                               ack.largest_acked)

+    if (from Handshake packet):


Suggested change

if (from Handshake packet):

if (ack from Handshake packet):

Can you add a short comment like "Once the client receives an ACK in a Handshake packet, it knows the server considers its address verified."?

I don't think that we should be using acknowledgments to drive state at this level. I realize that this is less optimal, but I don't think that we need this variable.

The client can know that it is unverified if it is sending Initial packets and not Handshake (or greater). That should be enough. As for the test in SetLossDetectionTimer() or LossDetectionTimeout(), how is it possible for a crypto packet to be in flight, where this condition would be true?

The client can know that it is unverified if it is sending Initial packets and not Handshake (or greater).

That's not true, though: the client doesn't know it's been validated until it's seen an ACK using Handshake keys (or greater). The server needs to see a Handshake packet to validate the client, and until the client's seen such an ACK it can't be sure that any Handshake packets it might have sent so far weren't lost.

e: "Sending initial packets" is sufficient but not necessary information to determine that the client is unverified, but we need to determine this accurately to avoid handshake deadlock when client Handshake packets are lost.

As for the test in SetLossDetectionTimer() or LossDetectionTimeout(), how is it possible for a crypto packet to be in flight, where this condition would be true?

I'm not sure I understand the question. The tests in those functions are to ensure that a client sends packets to either earn anti-amplification credit or validate its address even when it has no information to send.

Sorry, I should proof-read these. I should have said that the client can assume that it is unverified for longer than it actually is. The cost, for a client that is sending Handshake packets is small and for Initial packets, the time is well bounded.

Are you suggesting that we ditch unverified_client in favor of checking whether the endpoint is a client that doesn't yet have 1-RTT? I think that works as a simplification, if I'm understanding correctly.

I think that works. For the most part "do I have crypto packets in flight" covers the rest of it. Though "in flight" might be wrong, and it might be better as "are any crypto packets that I have sent, but are unacknowledged".

The cost is that the client might keep this timer well past the point it is needed when acknowledgments are lost, but the simplification is pretty big.

draft-ietf-quic-recovery.md

Ralith · 2019-01-16T02:01:09Z

Added @ianswett's tweaks and relaxed the validation check slightly, since a client could conceivably receive a 1-RTT ACK before a Handshake ACK due to even a single packet's worth of reordering.

janaiyengar

Should this be happening in an issue? I think we're close here, but I would recommend moving this discussion to an issue if it continues.

FWIW, I'm in agreement with @martinthomson that it's better to drive logic here using the client's state instead of the received packets.

Ralith · 2019-03-31T01:41:05Z

Rebased and switched to the simplified logic as suggested.

martinthomson · 2019-03-31T07:00:15Z

draft-ietf-quic-recovery.md

    // Crypto retransmission timeout.
    RetransmitUnackedCryptoData()
    crypto_count++
+  else if (endpoint is client without 1-RTT keys):
+    if (handshake keys available)


Suggested change

if (handshake keys available)

if (handshake keys available):

Unfortunately, this is still incorrect. As we discussed on #2519 (comment) there is a requirement to pad any time the client needs to send an Initial packet (it's a little more complicated in practice, but this is what the resolution boils down to).

If the availability of Handshake keys isn't a strong enough condition for that, what should be used instead? I'd be inclined to just fall back on SendOnePacket and let the nontrivial padding logic be implied by the text elsewhere, but that's where we started for this fragment.

If you need to send an Initial packet, pad.

martinthomson · 2019-03-31T07:01:01Z

draft-ietf-quic-recovery.md

  // Retransmit crypto data if no packets were lost
  // and there are still crypto packets in flight.
-  else if (crypto packets are in flight):
+  if (crypto packets are in flight):
    // Crypto retransmission timeout.
    RetransmitUnackedCryptoData()


I assume that RetransmitUnackedCryptoData() is going to catch any need for padding...

ianswett

Thanks for fixing this. Because of the deadlock prevention, this is definitely a trick to get right.

It'd be nice to use the "handshake complete" concept in recovery, but defining it may be more work than it's worth.

draft-ietf-quic-recovery.md

ianswett · 2019-04-02T21:52:48Z

draft-ietf-quic-recovery.md

    // Crypto retransmission timeout.
    RetransmitUnackedCryptoData()
    crypto_count++
+  else if (endpoint is client without 1-RTT keys):
+    if (will send Initial packet):


I'd say
"if (has Handshake keys):
SendOneHandshakePacket()
else
SendOnePaddedInitialPacket()"

@martinthomson, is this consistent with your earlier suggestions?

No. The important factor is not whether you have handshake keys, but whether your peer believes your address to be validated.

To be clear, my suggestion started at line 1133. The intent was to change "Will send Initial packet" to "has Handshake keys", which I believe is clearer.

"Will send Initial packet" is correct.

That sounds like the approach that I originally had here, which was argued against.

Martin didn't want to allow for ACK of handshake stopping the anti-amplification retransmissions, which I think is fine, though a bit overly strict. So all of this is within the "if (endpoint is client without 1-RTT keys)" conditional, which is our proxy for: Should we keep trying to prevent a deadlock?

To be clear on what I'm suggesting:

else if (endpoint is client without 1-RTT keys):
// The client must continue sending packets until it's
// sure the server has validated the path.
if (has Handshake keys):
SendOneHandshakePacket()
else
SendOnePaddedInitialPacket()

I see. Your proposal seems good to me. @martinthomson: can you clarify what you think is missing?

Striking out the bit that we've agreed to remove, the text from the transport draft is:

Clients MUST ensure that UDP datagrams containing Initial packets are sized to at least 1200 bytes, adding padding to packets in the datagram as necessary. ~~Once a client has received an acknowledgment for a Handshake packet it MAY send smaller datagrams.~~ Sending padded datagrams ensures that the server is not overly constrained by the amplification restriction.

This contradicts that text. It's true that a server that received a Handshake packet is no longer subject to the anti-DoS limit on bytes transmitted, but it's a bit weird that we have an algorithm that directly contradicts a MUST-level requirement in the other document.

@martinthomson: AIUI, you'd remember this better since it was your PR, but wasn't the bit we agreed to remove was about special-casing padding of Initial packets? My reading of @ianswett 's proposed text doesn't contradict that.

The draft requires that clients transmit packets to prevent anti-amplification deadlock when there is no crypto data to send.

Fixes the portion of #1964 not fixed in #2281

* Don't arm the handshake timer if there's no data Fixes the portion of #1964 not fixed in #2281 * MUST send * Martin and Marten's comments * Update draft-ietf-quic-recovery.md * Update draft-ietf-quic-recovery.md * Martin and Ian's comments * Update draft-ietf-quic-recovery.md * Update draft-ietf-quic-recovery.md * Update draft-ietf-quic-recovery.md Don't set the crypto retransmission timer if the time threshold timer is set * Jana's comments * Update draft-ietf-quic-recovery.md * Update draft-ietf-quic-recovery.md * Update draft-ietf-quic-recovery.md Co-Authored-By: ianswett <ianswett@users.noreply.github.com> * Update draft-ietf-quic-recovery.md Co-Authored-By: ianswett <ianswett@users.noreply.github.com> * Update draft-ietf-quic-recovery.md * Update draft-ietf-quic-recovery.md * Update draft-ietf-quic-recovery.md * Update draft-ietf-quic-recovery.md

martinthomson reviewed Jan 1, 2019

View reviewed changes

draft-ietf-quic-recovery.md Outdated Show resolved Hide resolved

ianswett self-requested a review January 4, 2019 02:37

ianswett added the -recovery label Jan 4, 2019

ianswett reviewed Jan 6, 2019

View reviewed changes

Ralith force-pushed the anti-deadlock-pseudocode branch 2 times, most recently from 3a7dcd9 to 8088a86 Compare January 12, 2019 05:05

ianswett reviewed Jan 15, 2019

View reviewed changes

Ralith force-pushed the anti-deadlock-pseudocode branch from 8088a86 to 0eb67d4 Compare January 16, 2019 01:59

janaiyengar reviewed Jan 18, 2019

View reviewed changes

Ralith force-pushed the anti-deadlock-pseudocode branch from 0eb67d4 to 66b99f6 Compare March 31, 2019 01:40

martinthomson requested changes Mar 31, 2019

View reviewed changes

Ralith force-pushed the anti-deadlock-pseudocode branch 2 times, most recently from faf279c to 75478a7 Compare March 31, 2019 07:19

ianswett reviewed Apr 2, 2019

View reviewed changes

Ralith force-pushed the anti-deadlock-pseudocode branch from 75478a7 to 2a35f8b Compare April 3, 2019 00:12

martinthomson approved these changes Apr 4, 2019

View reviewed changes

Implement anti-deadlock in crypto retransmit timer pseudocode

7e0a5c0

The draft requires that clients transmit packets to prevent anti-amplification deadlock when there is no crypto data to send.

Ralith force-pushed the anti-deadlock-pseudocode branch from 2a35f8b to 7e0a5c0 Compare April 5, 2019 00:39

ianswett added a commit that referenced this pull request Apr 5, 2019

Don't arm the handshake timer if there's no data

308a613

Fixes the portion of #1964 not fixed in #2281

ianswett mentioned this pull request Apr 5, 2019

Don't arm the handshake timer if there's no data #2590

Merged

Update to reflect editors' discussion

5989bf1

janaiyengar merged commit b29886b into quicwg:master Apr 9, 2019

Ralith deleted the anti-deadlock-pseudocode branch April 10, 2019 00:16

Include handshake anti-deadlock logic in pseudocode #2281

Include handshake anti-deadlock logic in pseudocode #2281

Conversation

Ralith commented Jan 1, 2019

martinthomson left a comment

Choose a reason for hiding this comment

Ralith commented Jan 1, 2019

martinthomson commented Jan 1, 2019

Ralith commented Jan 1, 2019

martinthomson commented Jan 1, 2019

Ralith commented Jan 2, 2019

martinthomson commented Jan 2, 2019

Ralith commented Jan 2, 2019

Ralith commented Jan 4, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ralith commented Jan 12, 2019

ianswett left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ralith Jan 16, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ralith commented Jan 16, 2019

janaiyengar left a comment

Choose a reason for hiding this comment

Ralith commented Mar 31, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ianswett left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ralith Jan 16, 2019 •

edited

Loading