Subject: [quicwg/base-drafts] Editorial suggestions from Benjamin Kaduk's IESG review (qpack) (#4789)

From: kaduk <[email-masked]>
Date: Wed, 20 Jan 2021 15:36:30 -0800

You can view, comment on, or merge this pull request online at:

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

-- Commit Summary --

  * Editorial suggestions from IESG review

-- File Changes --

    M draft-ietf-quic-qpack.md (56)

-- Patch Links --

https://github.com/quicwg/base-drafts/pull/4789.patch
https://github.com/quicwg/base-drafts/pull/4789.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/4789

---

From: kaduk <[email-masked]>
Date: Wed, 20 Jan 2021 15:41:35 -0800

@kaduk commented on this pull request.

> @@ -1203,9 +1204,9 @@ the dynamic table state. If a guess is compressed into a shorter length, the
 attacker can observe the encoded length and infer that the guess was correct.
 
 This is possible even over the Transport Layer Security Protocol (TLS, see
-{{?TLS=RFC8446}}), because while TLS provides confidentiality protection for
-content, it only provides a limited amount of protection for the length of that
-content.
+{{?TLS=RFC8446}}) and QUIC Transport Protocol (see {{QUIC-TRANSPORT}}), because
+while TLS and QUIC provide confidentiality protection for content, they only
+provides a limited amount of protection for the length of that content.

Perhaps we should just not mention TLS at all?

> @@ -1235,12 +1236,12 @@ recovered successfully. However, values with low entropy remain vulnerable.
 Attacks of this nature are possible any time that two mutually distrustful
 entities control requests or responses that are placed onto a single HTTP/3
 connection. If the shared QPACK compressor permits one entity to add entries to
-the dynamic table, and the other to access those entries to encode chosen field

If you can "access" those entries, you're done already :)

> @@ -1277,6 +1279,11 @@ different values.  This penalty could cause a large number of attempts to guess
 a field value to result in the field not being compared to the dynamic table
 entries in future messages, effectively preventing further guesses.
 
+This response might be made inversely proportional to the length of the
+field value. Disabling access to the dynamic table for a given field name might
+occur for shorter values more quickly or with higher probability than for longer
+values.
+

This is just  moving the related paragraph up so the note appears at the end.

> @@ -1339,7 +1341,7 @@ An attacker can try to cause an endpoint to exhaust its memory. QPACK is
 designed to limit both the peak and stable amounts of memory allocated by an
 endpoint.
 
-The amount of memory used by the encoder is limited by the protocol using
+The amount of memory used by the decoder is limited by the protocol using

In some sense this is true as it is, but that's not a terribly interesting statement to make -- the encoder can limit its memory usage unilaterally, and could be sloppily implemented and exceed the protocol-conveyed limit.

> @@ -1717,7 +1719,7 @@ Stream: Decoder
                                3   0  custom-key  custom-value
                               ^-- acknowledged --^
                                4   0  :authority  www.example.com
-                              Size=215
+                              Size=217

The table hasn't changed from the previous state, properly shown as 217 bytes.

> @@ -1738,7 +1740,7 @@ Stream: Encoder
                               ^-- acknowledged --^
                                4   0  :authority  www.example.com
                                5   0  custom-key  custom-value2
-                              Size=215
+                              Size=214

I attempted to count the difference between `:authoritywww.example.com` and `custom-keycustom-value2`.

> @@ -1787,8 +1789,8 @@ for line in field_lines:
 
 # encode the prefix
 if requiredInsertCount == 0:
-  encodeIndexReference(prefixBuffer, 0, 0, 8)
-  encodeIndexReference(prefixBuffer, 0, 0, 7)
+  encodeInteger(prefixBuffer, 0, 0, 8)
+  encodeInteger(prefixBuffer, 0, 0, 7)

encodeIndexReference() is shown earlier with a different signature.  I know pythonic things do have optional parameters, but this seems to match the encodeInteger() signature perfectly, so I assume it was just an incomplete edit.  Though, stylistically perhaps the second argument should be `0x00` rather than a literal `0`.

-- 
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/4789#pullrequestreview-572785617

---

From: Martin Thomson <[email-masked]>
Date: Wed, 20 Jan 2021 15:44:10 -0800

@martinthomson approved this pull request.

```suggestion
provide a limited amount of protection for the length of that content.
```

👍 

I didn't catch this when @afrind updated examples at my request.  Thanks.

-- 
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/4789#pullrequestreview-572787322

---

From: kaduk <[email-masked]>
Date: Thu, 21 Jan 2021 08:57:35 -0800

@kaduk pushed 1 commit.

91e2df8b2811ab45ddcc395b7370df2f7a3b402b  Apply suggestion from code review

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/4789/files/5086a869ca64273a20bd44e9fdabf953f7bfb388..91e2df8b2811ab45ddcc395b7370df2f7a3b402b

---

From: afrind <[email-masked]>
Date: Fri, 22 Jan 2021 14:05:42 -0800

@afrind commented on this pull request.

Thank you for your detailed review and this PR.  I have one suggestion.

> +state memory it uses and signal a lower dynamic table size than the decoder
+allows to propagate that fact to the decoder (see {{eviction}}).

Suggestion:

 An encoder can limit the amount of state memory it uses by choosing a smaller dynamic table size than the decoder allows, and signaling this to the decoder (see {{set-dynamic-table-capacity}}).

                               ^-- acknowledged --^

This looks right too, but it makes me want to re-run the examples through the code I wrote to verify (I made @martinthomson's changes by hand).

 # encode the prefix

This is a good catch, thanks.  The prefix is integers, not references.

-- 
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/4789#pullrequestreview-574654039

---

From: kaduk <[email-masked]>
Date: Fri, 22 Jan 2021 17:40:14 -0800

@kaduk pushed 1 commit.

532638be3d7067b572c1100fb6045c252d9de59e  apply suggestion from @afrind

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/4789/files/91e2df8b2811ab45ddcc395b7370df2f7a3b402b..532638be3d7067b572c1100fb6045c252d9de59e

---

From: kaduk <[email-masked]>
Date: Fri, 22 Jan 2021 17:40:56 -0800

Thanks; I pushed that with a slight modification (the signalling is not part of the act of limiting the encoder's state memory, but rather a related but separate act).

-- 
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/4789#discussion_r562998146

---

From: afrind <[email-masked]>
Date: Mon, 25 Jan 2021 15:49:09 -0800

@afrind pushed 1 commit.

2084871b0a237dcfc4ffc09e3cd67954bf8009a1  Update draft-ietf-quic-qpack.md

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/4789/files/532638be3d7067b572c1100fb6045c252d9de59e..2084871b0a237dcfc4ffc09e3cd67954bf8009a1

---

From: Mike Bishop <[email-masked]>
Date: Tue, 26 Jan 2021 09:00:31 -0800

@MikeBishop approved this pull request.

```suggestion
{{?TLS=RFC8446}}) and the QUIC Transport Protocol (see {{QUIC-TRANSPORT}}), because
```
Will probably also need a re-wrap.

-Having requests or responses from mutually distrustful entities occurs when an
-intermediary either:
+For example, requests or responses from mutually distrustful entities occurs

```suggestion
For example, requests or responses from mutually distrustful entities can occur
```

> +The amount of memory used by the decoder is limited by the protocol using
 QPACK through the definition of the maximum size of the dynamic table, and the
 maximum number of blocking streams. In HTTP/3, these values are controlled by

I think this is really dependent on the sense of "used by."  The protocol limits the amount of memory the encoder can cause to be consumed at the decoder.  Maybe something like....

```suggestion
QPACK uses the definition of the maximum size of the dynamic table and the
maximum number of blocking streams to limit the amount of memory the
encoder can cause the decode to consume. In HTTP/3, these values are
controlled by
```

> @@ -1355,14 +1357,14 @@ A decoder can limit the amount of state memory used for the dynamic table by
 setting an appropriate value for the maximum size of the dynamic table. In
 HTTP/3, this is realized by setting an appropriate value for the
 SETTINGS_QPACK_MAX_TABLE_CAPACITY parameter. An encoder can limit the amount of
-state memory it uses by signaling a lower dynamic table size than the decoder
-allows (see {{eviction}}).
+state memory it uses by choosing a smaller dynamic table size than the decoder
+allows, and signaling this to the decoder (see {{set-dynamic-table-capacity}}).

```suggestion
allows and signaling this to the decoder (see {{set-dynamic-table-capacity}}).
```

-- 
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/4789#pullrequestreview-576555819

---

From: afrind <[email-masked]>
Date: Wed, 27 Jan 2021 10:18:20 -0800

^-- acknowledged --^

Wait, let's go to the video tape!  I think 215 is correct.

I re-ran my decoder against the input, and it gets 215.

~~~
DecoderTable:

[1] (s=55) custom-key: custom-value2
[2] (s=57) :authority: www.example.com
[3] (s=54) custom-key: custom-value
[4] (s=49) :path: /sample/path
total size: 215
~~~
[Note: the decoder table prints with HPACK relative indices, hence they are 1 based and in reverse order]

Also verified in the shell:

~~~
# echo -n :authoritywww.example.com | wc -c
25
# echo -n custom-keycustom-value2 | wc -c
23
~~~

And third, I examined the table which has the two values in consecutive rows, and it appears only the "om" hangs over the 'custom-value2'.

-- 
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/4789#discussion_r565530977

---

From: kaduk <[email-masked]>
Date: Wed, 27 Jan 2021 10:22:42 -0800

^-- acknowledged --^

Well crap, I can only assume that I typo'd something when checking in the shell.  Sorry about that, and thanks for double-checking.

-- 
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/4789#discussion_r565533963

---

From: afrind <[email-masked]>
Date: Thu, 28 Jan 2021 09:13:43 -0800

@afrind pushed 1 commit.

3e9662066dea422df2b8b1f56ad0c8c3fcb29583  Mike's suggestions, wrapped.

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/4789/files/2084871b0a237dcfc4ffc09e3cd67954bf8009a1..3e9662066dea422df2b8b1f56ad0c8c3fcb29583

---

From: afrind <[email-masked]>
Date: Thu, 28 Jan 2021 09:26:26 -0800

@afrind pushed 1 commit.

a3d2fa909b434fb7a2ec276044d92979da4ec92c  Merge branch 'master' into qpack-iesg-eval

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/4789/files/3e9662066dea422df2b8b1f56ad0c8c3fcb29583..a3d2fa909b434fb7a2ec276044d92979da4ec92c

---

From: afrind <[email-masked]>
Date: Thu, 28 Jan 2021 09:35:28 -0800

Sorry I missed this comment earlier and basically reverted to my original text.  While the acts themselves are separate, the second is directly tied to the first -- the encoder can't change the size it is using without also signaling the decoder.

-- 
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/4789#discussion_r566279622

---

From: afrind <[email-masked]>
Date: Thu, 28 Jan 2021 09:36:50 -0800

Merged #4789 into master.

-- 
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/4789#event-4262323580