Subject: [quicwg/base-drafts] allow CRYPTO_BUFFER_EXCEEDED when overflowing a CRYPTO frame (#3186)

From: Marten Seemann <[email-masked]>
Date: Sun, 03 Nov 2019 21:12:31 -0800

As @kazuho pointed out in https://github.com/quicwg/base-drafts/pull/3042#issuecomment-549225351, I missed that there&#39;s a CRYPTO_BUFFER_EXCEEDED that would equally valid to send when a CRYPTO frame exceeds the 2^62-1 limit.
You can view, comment on, or merge this pull request online at:

  https://github.com/quicwg/base-drafts/pull/3186

-- Commit Summary --

  * allow CRYPTO_BUFFER_EXCEEDED when overflowing a CRYPTO frame

-- File Changes --

    M draft-ietf-quic-transport.md (3)

-- Patch Links --

https://github.com/quicwg/base-drafts/pull/3186.patch
https://github.com/quicwg/base-drafts/pull/3186.diff

-- 
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/pull/3186

---

From: Kazuho Oku <[email-masked]>
Date: Sun, 03 Nov 2019 21:13:52 -0800

kazuho approved this pull request.

Thank you! LGTM.

-- 
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/pull/3186#pullrequestreview-310881295

---

From: ianswett <[email-masked]>
Date: Mon, 04 Nov 2019 13:14:43 -0800

ianswett approved this pull request.

-- 
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/pull/3186#pullrequestreview-311362417

---

From: MikkelFJ <[email-masked]>
Date: Mon, 04 Nov 2019 13:45:07 -0800

I can see how this can simplify some decoding logic, but I am not sure we should consider such a value valid in the first place. If the value is invalid it is meaningless to discuss length and then there is only a frame error left. Otherwise we could probably revisit a gazillion other places where this principle could also apply.

-- 
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/pull/3186#issuecomment-549561406

---

From: ianswett <[email-masked]>
Date: Mon, 04 Nov 2019 13:48:59 -0800

ianswett requested changes on this pull request.

Actually, I thought the principle we have is that if you could identify a frame as being invalid with no other information, it should be a FRAME_ENCODING_ERROR, so the existing text is correct?

-- 
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/pull/3186#pullrequestreview-311381191

---

From: MikkelFJ <[email-masked]>
Date: Mon, 04 Nov 2019 13:50:10 -0800

@ianswett yes this is largely what I meant. The PR introduces a new principle.

-- 
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/pull/3186#issuecomment-549563261

---

From: Jana Iyengar <[email-masked]>
Date: Mon, 04 Nov 2019 17:56:48 -0800

@marten-seemann : I'm confused too. For consistency,  I would keep it at FRAME_ENCODING_ERROR, since that's what we do for the flow control error too.

-- 
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/pull/3186#issuecomment-549628891

---

From: Kazuho Oku <[email-masked]>
Date: Mon, 04 Nov 2019 18:37:14 -0800

@ianswett @janaiyengar I am with @marten-seemann. Let me explain why.

There are two reasons we decided to allow use of FLOW_CONTROL_ERROR for STREAM frames:
* The overflow can be detected by the flow control logic. Then, there is not need to have a logic to detect the error in the frame decoder.
* The flow control logic might be capable of returning only one error.
* FLOW_CONTROL_ERROR is more specific than FRAME_ENCODING_ERROR.

These reasons equally hold for CRYPTO streams sending CRYPTO_BUFFER_EXCEEDED, as CRYPTO_BUFFER_EXCEEDED is the error code used to indicate flow control error with CRYPTO streams.

-- 
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/pull/3186#issuecomment-549637548

---

From: Jana Iyengar <[email-masked]>
Date: Mon, 04 Nov 2019 18:47:35 -0800

Thanks for clarifying, @kazuho. This LGTM.

-- 
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/pull/3186#issuecomment-549639666

---

From: Jana Iyengar <[email-masked]>
Date: Mon, 04 Nov 2019 18:47:44 -0800

janaiyengar approved this pull request.

-- 
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/pull/3186#pullrequestreview-311476940

---

From: ianswett <[email-masked]>
Date: Mon, 04 Nov 2019 19:13:58 -0800

>From the original issue, the proposed principle was: "If you can detect an error while parsing the frame, and without consulting any connection state, it should be a FRAME_ENCODING_ERROR."

It seems clear you can do that in this case, but maybe we changed the principle?

-- 
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/pull/3186#issuecomment-549644796

---

From: Marten Seemann <[email-masked]>
Date: Mon, 04 Nov 2019 19:51:41 -0800

@ianswett That would also apply to the STREAM frame then. I'd be fine just using the FRAME_ENCODING_ERROR in all these cases, but people pushed back against 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/pull/3186#issuecomment-549651826

---

From: Kazuho Oku <[email-masked]>
Date: Mon, 04 Nov 2019 19:58:29 -0800

@ianswett Yeah I think I should have said that the exceptional rule within the principles.

Specifically, I mean the discussion starting from https://github.com/quicwg/base-drafts/pull/3042#pullrequestreview-289402001, that talks if we should allow FLOW_CONTROL_ERROR and why.

-- 
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/pull/3186#issuecomment-549652995

---

From: ianswett <[email-masked]>
Date: Tue, 05 Nov 2019 08:24:29 -0800

@kazuho I thought that might have been the relevant discussion.  In general, I'd prefer to have principles decided on issues, but I understand these things happen.

I don't think this is the end of the world, but I prefer the original principle because I think it's easy to reason about.  If we've decided against that principle, I'd suggest we state a new principle.

-- 
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/pull/3186#issuecomment-549885922

---

From: Marten Seemann <[email-masked]>
Date: Tue, 05 Nov 2019 08:49:47 -0800

@ianswett I don't think we ever actually discussed this in the working group. If I remember correctly, using two error codes was a suggestion that came up on my original PR, and then the editors decided in Cupertino that I should update the PR accordingly.

Actually, I'd prefer to take the principled approach, and just use FRAME_ENCODING_ERROR wherever possible.

-- 
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/pull/3186#issuecomment-549899953

---

From: Kazuho Oku <[email-masked]>
Date: Tue, 05 Nov 2019 09:22:29 -0800

@marten-seemann I think we did discuss at the interim. https://github.com/quicwg/wg-materials/blob/master/interim-19-10/minutes.md#issue-3027-codes-for-frame-encoding-errors

-- 
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/pull/3186#issuecomment-549919076

---

From: Kazuho Oku <[email-masked]>
Date: Tue, 05 Nov 2019 17:25:00 +0000 (UTC)

To be precise, we discussed that two error codes were OK at the interim, then declared that as a consensus in the call started on Oct 22 https://mailarchive.ietf.org/arch/msg/quic/LrM0w3L3mHSLMn1mH98Xq1E-DQs. I do not think we should be revisiting the discussion.

-- 
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/pull/3186#issuecomment-549920654

---

From: ianswett <[email-masked]>
Date: Tue, 05 Nov 2019 11:02:12 -0800

I think we should either:
1) Espouse a new principle that fits the existing text and this PR and use that going forward 
2) Stick with the original principle and remove the OR FLOW_CONTROL_ERROR from the text.

If we don't have a principle that matches the text, we're prone to having this discussion a few more times.

-- 
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/pull/3186#issuecomment-549970173

---

From: ianswett <[email-masked]>
Date: Tue, 05 Nov 2019 16:04:11 -0800

@martinthomson kindly shared our principles, and this PR aligns with that.
https://quicwg.org/base-drafts/draft-ietf-quic-transport.html#error-handling

-- 
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/pull/3186#issuecomment-550079795

---

From: ianswett <[email-masked]>
Date: Tue, 05 Nov 2019 16:04:21 -0800

-- 
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/pull/3186#pullrequestreview-312123846

---

From: Kazuho Oku <[email-masked]>
Date: Thu, 14 Nov 2019 03:44:36 -0800

Chairs and editors, I think we agree that this is an amendment to #3042 that has been shipped as part of -24.

Does that mean that this would be merged as an editorial correction? Or if not, do we need an issue?

-- 
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/pull/3186#issuecomment-553851912

---

From: ianswett <[email-masked]>
Date: Thu, 14 Nov 2019 06:46:59 -0800

I think we can merge it because it agrees with the stated principles.  But its @martinthomson call

-- 
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/pull/3186#issuecomment-553919563

---

From: Martin Thomson <[email-masked]>
Date: Sun, 17 Nov 2019 15:51:47 -0800

Let's open an issue.  At this stage, I want to be very particular about this sort of change, even if it is quite clearly within the bounds of established principles.

-- 
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/pull/3186#issuecomment-554802226

---

From: Kazuho Oku <[email-masked]>
Date: Sun, 17 Nov 2019 23:59:25 -0800

@marten-seemann did as #3258.

-- 
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/pull/3186#issuecomment-554898128