Subject: [quicwg/base-drafts] Conflicting error codes (#4087)

From: Tatsuhiro Tsujikawa <[email-masked]>
Date: Wed, 09 Sep 2020 22:31:27 -0700

The transport draft-30 introduced AEAD_LIMIT_REACHED(0xe) error code.
Meanwhile, the TLS draft also has KEY_UPDATE_ERROR error code (0xe), and they collide each other.
I believe they are transport error code, not application error code.

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/quicwg/base-drafts/issues/4087

---

From: Marten Seemann <[email-masked]>
Date: Wed, 09 Sep 2020 22:40:49 -0700

It would probably be a good idea to list the KEY_UPDATE_ERROR in the error code table in the transport document.

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/quicwg/base-drafts/issues/4087#issuecomment-689998083

---

From: Martin Thomson <[email-masked]>
Date: Wed, 09 Sep 2020 23:47:59 -0700

Ugh, that's unfortunate.  I think that we need to renumber the newer one (and yeah, put them all in the same document so it doesn't happen again).

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/quicwg/base-drafts/issues/4087#issuecomment-690026807

---

From: David Schinazi <[email-masked]>
Date: Thu, 10 Sep 2020 12:13:38 -0700

I believe we should fix the fact that we have two errors with the same code, however I think it would be best to just use 0xE for all these scenarios. At this point in the process, we're working on deploying h3-29 and having a breaking wire change like this would be very unfortunate. Additionally, I cannot think of any action an implementation would take that would be different between KEY_UPDATE_ERROR and AEAD_LIMIT_REACHED. Any chance we could just rename AEAD_LIMIT_REACHED to KEY_UPDATE_ERROR and call it a day?

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/quicwg/base-drafts/issues/4087#issuecomment-690646748

---

From: ekr <[email-masked]>
Date: Thu, 10 Sep 2020 13:07:52 -0700

On Thu, Sep 10, 2020 at 12:13 PM David Schinazi <[email-masked]>
wrote:

> I believe we should fix the fact that we have two errors with the same
> code, however I think it would be best to just use 0xE for all these
> scenarios. At this point in the process, we're working on deploying h3-29
> and having a breaking wire change like this would be very unfortunate.
> Additionally, I cannot think of any action an implementation would take
> that would be different between KEY_UPDATE_ERROR and AEAD_LIMIT_REACHED.
> Any chance we could just rename AEAD_LIMIT_REACHED to KEY_UPDATE_ERROR and
> call it a day?
>
I think it's useful to distinguish what happened for logging and diagnostic
purposes.

I also don't understand why this creates a significant implementation
inconvenience. On the sending side, presumably you're just sending the
wrong code for a while and it's a bug, and on the receiving side, given
that you have to handle unknown error codes anyway (because they can be
IANA Registered), what's the problem if you receive an unknown error code
in this likely quite uncommon case?

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/quicwg/base-drafts/issues/4087#issuecomment-690689612

---

From: Jana Iyengar <[email-masked]>
Date: Thu, 10 Sep 2020 17:46:57 -0700

I agree with @ekr. There is a separate question of whether -31 will be a new version or not, but let's not get ahead of ourselves.

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/quicwg/base-drafts/issues/4087#issuecomment-690806561

---

From: Christian Huitema <[email-masked]>
Date: Mon, 14 Sep 2020 08:27:33 -0700

I am trying to write a test for the generation of the key update error code. This is a bit hard, since by default the implementation will rotate keys after sending more than <close-to-the-limit> packets, and thus the condition is not seen. One could test that the peer has sent more than <limit> packets without rotating the keys, but then there is some fuzziness to what the limit is. For example, for AES GCM, should that be "received 2**25 packets" or "2**25 + 1"?

The error is also a bit silly. It requires tracking how many packets were decrypted with a given key, and then disconnecting with a specific error if the limit is exceeded. But the node that tracks the limit could also unilaterally trigger a key rotation if the receive count is getting close enough. Of course, the non-standard peer could refuse to update its own key, and never rotate the key phase. Is *that* the condition for KEY_UPDATE_ERROR?

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/quicwg/base-drafts/issues/4087#issuecomment-692132583

---

From: Jana Iyengar <[email-masked]>
Date: Mon, 14 Sep 2020 15:51:37 -0700

@huitema: this issue is merely changing the error code for an error that already exists. I read your comment as a question about testing this, but you could be asking about when this error code ought to be sent. 

My reading is that the sender is required to close the connection when it sends too many packets, and the receiver is required to close the connection when too many packets fail authentication, and I think the text is clear on precisely when both of these ought to happen.

A receiver is not required to, but could do what you're suggesting, and honestly, the precise value of "2^25 + 1" or "2^25" wouldn't matter IMO, since the receiver can only know a lower bound on the number of packets sent with the same key (since packet loss, etc). But if the receiver were to close with this error, doing it at the point that there is no ability for the sender to update the keys makes sense. The simplest thing to do would be to check if "2^25 + 1" packets have been received with the same key.

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/quicwg/base-drafts/issues/4087#issuecomment-692360022

---

From: Christian Huitema <[email-masked]>
Date: Mon, 14 Sep 2020 16:05:22 -0700

@janaiyengar the sender is **NOT** required to close the connection when it sends too many packets. That's not what the TLS limits say. The requirement is to use key rotation, move to the next epoch, and start using a new encryption key.

Reading the fine print, I believe the KEY_UPDATE_ERROR error will happen in one of two conditions:

1) The node starts a key rotation but the peer keeps using the old key for too long, for example if the peer continues using the old key but acknowledges packets sent with the new key.

2) The peer transmits too many packets for the epoch.

This is different from the integrity limit, which does require closing the connection if too many decryption failures are observed.

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/quicwg/base-drafts/issues/4087#issuecomment-692364214

---

From: Jana Iyengar <[email-masked]>
Date: Mon, 14 Sep 2020 16:26:43 -0700

@huitema -- yes, I didn't read carefully; you're right.

So, as I suggested earlier IMO the best you can do at the receiver is check for 2^25 + 1, even though it is a lower bound. It does require tracking the number of packets as you say though.

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/quicwg/base-drafts/issues/4087#issuecomment-692370676

---

From: Christian Huitema <[email-masked]>
Date: Mon, 14 Sep 2020 17:39:52 -0700

I can't wait to see the first error report in which some poor dude tries to transmit more than 50GB over a QUIC connection and hits a bug in the implementation of key update...

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/quicwg/base-drafts/issues/4087#issuecomment-692391678

---

From: Marten Seemann <[email-masked]>
Date: Mon, 14 Sep 2020 18:08:40 -0700

@huitema There's another condition where KEY_UPDATE_ERROR might occur: If you acknowledge a packet in key phase N+1 using key phase N. It's just a MAY in the specification though (I don't know why, this should be trivial to implement):
> An endpoint that receives an acknowledgement that is carried in a packet protected with old keys where any acknowledged packet was protected with newer keys MAY treat that as a connection error of type KEY_UPDATE_ERROR. This indicates that a peer has received and acknowledged a packet that initiates a key update, but has not updated keys in response.

@huitema 
> I can't wait to see the first error report in which some poor dude tries to transmit more than 50GB over a QUIC connection and hits a bug in the implementation of key update...

That's why we recently added a key update test to the interop runner. Unfortunately, it looks like a number of implementations (even those that already boasted about rolling out QUIC into production) treat key updates as some kind of optional feature, which it isn't. This is bad, as this now prevents others from rolling out QUIC implementations that initiate a key update early in the connection to exercise the mechanism.

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/quicwg/base-drafts/issues/4087#issuecomment-692399479

---

From: Martin Thomson <[email-masked]>
Date: Mon, 14 Sep 2020 18:58:20 -0700

@marten-seemann, it would be non-trivial for neqo to implement that MAY.  The information about what keys are used is quite well separated from the code that manages acknowledgments.

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/quicwg/base-drafts/issues/4087#issuecomment-692414330

---

From: Marten Seemann <[email-masked]>
Date: Mon, 14 Sep 2020 19:03:43 -0700

@martinthomson That's the same in quic-go.
There's another way to determine this condition though: As you need to keep track of the largest acknowledged PN anyway, you can check when you receive an ACK for a packet sent in the current key phase that you unprotected at least one packet in the current key phase. If that's not the case, the ACK for the current key phase must have been sent in the previous key phase, and you can throw an error.

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/quicwg/base-drafts/issues/4087#issuecomment-692416009

---

From: Christian Huitema <[email-masked]>
Date: Mon, 14 Sep 2020 19:39:16 -0700

@marten-seemann you and I were thinking of the same error condition, which I described as "for example if the peer continues using the old key but acknowledges packets sent with the new key". 
And yes, I was also going to test that by implementing the "last acknowledged" check.

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/quicwg/base-drafts/issues/4087#issuecomment-692426846

---

From: Marten Seemann <[email-masked]>
Date: Mon, 14 Sep 2020 19:43:00 -0700

@huitema I just implemented this check in quic-go, and it was literally less than 10 LOC. Going to roll that out in my interop runner image soon, so we'll see if people are responding to key updates correctly.

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/quicwg/base-drafts/issues/4087#issuecomment-692427927

---

From: David Schinazi <[email-masked]>
Date: Thu, 17 Sep 2020 10:02:22 -0700

It's looking to me like I'm alone in preferring to merge the two codes, so I just wanted to say that I don't feel too strongly here. If there's WG consensus to merge #4088 then I'm OK with that.

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/quicwg/base-drafts/issues/4087#issuecomment-694368163

---

From: Martin Thomson <[email-masked]>
Date: Thu, 24 Sep 2020 02:10:49 -0700

Closed #4087 via #4088.

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/quicwg/base-drafts/issues/4087#event-3802198549