Hi Ben, Thanks for the review. I've created GitHub issue(s) to track each comment on the QUIC WG repository, see the URL(s) in line.
On Wed, Jan 20, 2021 at 11:38 PM Benjamin Kaduk via Datatracker < [email protected]> wrote: > Benjamin Kaduk has entered the following ballot position for > draft-ietf-quic-qpack-20: No Objection > > When responding, please keep the subject line intact and reply to all > email addresses included in the To and CC lines. (Feel free to cut this > introductory paragraph, however.) > > > Please refer to https://www.ietf.org/iesg/statement/discuss-criteria.html > for more information about IESG DISCUSS and COMMENT positions. > > > The document, along with other ballot positions, can be found here: > https://datatracker.ietf.org/doc/draft-ietf-quic-qpack/ > > > > ---------------------------------------------------------------------- > COMMENT: > ---------------------------------------------------------------------- > > Thanks for another well-written document! > > I put up some editorial suggestions at > https://github.com/quicwg/base-drafts/pull/4789 > > Section 2.1.1.1 > > Figure 1 might benefit from an explicit indication of which direction is > higher (or lower) absolute index. > https://github.com/quicwg/base-drafts/issues/4791 > Section 7.1.2 > > The mitigation technique of segregating the dynamic table by entity > constructing the message seems to inherently require the encoder to have > direct knowledge of the entity on whose behalf it is constructing a > message. For the other mitigation technique we present (of always using > string literals for sensitive values), we include the 'N' bit to allow > this attribute to be propagated through intermediaries. However, I > think that in scenarios where multiple intermediaries are involved, in > the later steps in the intermediary chain the encoder will not > necessarily have knowledge of which entity created a given message, and > thus could inadvertently merge compression contexts that the original > encoder had specifically kept separate. The preconditions necessary for > this to enable an attack seem rare, with one of the originating entities > having access to observe the transport layer in a location several hops > removed, so it doesn't really seem worth attempting to add a technical > mitigation. It would probably be worth documenting the risk, though. > https://github.com/quicwg/base-drafts/issues/4792 > Section 8.1 > > (nit) In the prose we refer to the setting names with a "SETTINGS_" prefix. > Blindly adding that to the table looks like it would blow out the column > count for the plaintext output, though, so I didn't put that in my > editorial PR. > https://github.com/quicwg/base-drafts/issues/4793 > Appendix A > > (nit) At least the plaintext output might benefit from a disclaimer > about line wraps in the 'Value' column being display artifacts. > https://github.com/quicwg/base-drafts/issues/4794 > Appendix B > > I worked through the examples. The presentation format is quite nice, and > I appreciate all the detailed breakdowns! > > However, we show the dynamic table as being 1-indexed, but I'm pretty > sure the prose says it should be 0-indexed. We do it consistently, at > least, and toss some extra '1's into the math to make the numbers work > out, but since the static table is by definition 0-indexed, it's a bit > weird to show the dynamic table as 1-indexed. > > Additionally, I think that B.5 is an exception to the "we do it > consistently" -- while the 81... dynamic insert with relative index 1 > does refer to the indicated custom-key field, that would be absolute > index 3 in the 1-indexed presentation we give (though it would be > absolute index 2 if 0-indexed, if I'm getting this right). > https://github.com/quicwg/base-drafts/issues/4795 > Appendix C > > It might be worth a brief mention of the API contracts for (e.g.) the > encode*() functions. The second argument of encodeInteger() as "the > byte value used to fill in the bits of the first byte that are not > consumed by the trailing N-prefix integer" is particularly hard to infer > (if it's even the correct inference). > https://github.com/quicwg/base-drafts/issues/4796 Cheers Lucas On behalf of QUIC WG Chairs
