Re: Benjamin Kaduk's Discuss on draft-ietf-httpbis-header-structure-18: (with DISCUSS and COMMENT)

Hi Mark,

On Tue, May 19, 2020 at 03:13:07PM +1000, Mark Nottingham wrote:
> Ben,
> 
> Thank you for the comments. Responses below. The editorial commit referred to is:
>   https://github.com/httpwg/http-extensions/commit/86dc9fb1b
> 
> 
> > On 19 May 2020, at 12:53 pm, Benjamin Kaduk via Datatracker <noreply@ietf.org> wrote:
> > 
> > ----------------------------------------------------------------------
> > DISCUSS:
> > ----------------------------------------------------------------------
> > 
> > Thanks for this document; it will be very nice to have this
> > more-structured mechanism available for future HTTP header (and trailer)
> > fields.
> > 
> > (0) There seem to still be a few lingering internal inconsistencies that
> > merit further discussion.
> > 
> > Most notably, there is the inherent risk of skew when both prose
> > algorithms and ABNF constructions are provided for the same structures.
> > While Section 1.2 is careful to disclaim that the prose algorithm takes
> > precedence over the ABNF for parsing, to my reading the coverage in the
> > following paragraph of serialization procedures imply that it is the
> > ABNF that is authoritative.  In particular, "[i]mplementations MAY vary
> > from the specified behavior so long as the output still matches the
> > ABNF" seems to admit deviations from the prose algorithms but require
> > compliance with the ABNF, in effect making the ABNF take precedence over
> > the prose algorithm.  Having a different description of the procedure
> > normative for generation vs. consumption invites
> > interoperability-affecting feature skew, such as the handling of empty
> > lists as Julian noted on the list.
> 
> I think the solution here is to change the last paragraph of Notational Conventions to:
> 
> """
> For serialization to HTTP fields, the ABNF illustrates the expected wire representation with as much fidelity as possible, and the algorithms define the recommended way to produce them. Implementations MAY vary from the specified behavior so long as the output is still correctly handled by the parsing algorithm.
> """
> 
> Does that work for you, Ben?

This makes the parsing algorithms normative for both parsing and
serialization, which alleviates the concerns about skew.

I do agree with Julian that making the (minor!) change to the ABNF to
remove that axis of skew seems worthwhile, though I do not think I can make
that a Discuss point (given that I, like him, would prefer to keep the ABNF
as-is over removing it entirely as you propose).

> > Similarly, Section 3.1.1's prose says that inner lists are delimited "by
> > a single space", but the ABNF uses (1*SP), allowing for more than
> > one space.
> 
> Discussed below.
> 
> > Additionally, when Section 4.2.3.2 discusses parsing parameter map keys,
> > the handling for duplicate map key names is specified as overwriting the
> > previous value, in contrast to the prose description (Section 3.1.2)
> > that describes these keys as "unique within the scope [of] the
> > Parameters they occur within".  (While dictionary key names might be
> > expected to have a similar behavior, I did not find conflicting text for
> > that behavior.)
> 
> That's because there's a difference between the underlying data model (specified in section 3) and the algorithms for how we get them into and out of textual HTTP headers (section 4). There might be another serialisation of the data model in the future that has a different way of handling duplicates.

Perhaps I'm just hung up on the "intentionally strict processing" preface,
but why allow duplicates at all vs. making them an error?

> > Finally, at a prose level we employ needlessly different descriptions in
> > several places for what is effectively the same procedure; while I do
> > not think any of these affect interoperability (and thus the full
> > details are in the COMMENT section), it does seem to be continuing the
> > theme.  (These are things like how we describe the algorithm to skip
> > implicit-true for booleans, whether we initialize the output string to
> > the empty string only to immediately add a constant literal character to
> > it vs. initializing the output string to that literal character, etc.)
> 
> Discussed below (Is this still part of the discuss, or just comment?).

AFAIK this is just comments.  I mention it here in case it is relevant as
part of a broader pattern (but don't know that such a broader pattern
exists or what it would be, at this time).

> > (1) What aspect(s) of structured field processing are case (in)sensitive?
> > The only mentions I see of case sensitivity are in Section 4.2 discussing
> > header field names and (implicitly) Section 4.2.2 discussing a
> > "character-for-character" comparison of dictionary key names, but of
> > course we cite RFC 5234 for ABNF, which uses case-insensitive matching.
> > On the other hand, base64 encoding requires case sensitivity for
> > successful round-tripping of arbitrary binary data.
> 
> As per S 2, it's up to each field to constrain the syntax of different types as appropriate to their semantics. The only exception is keys in dictionaries and parameters, which are explicitly lowercase (to avoid differences of only case there).

Do we want to call out case in § 2 as something that structured field
specifications should consider in their definition?  It's a bit of a
nitpicky details, admittedly, but it has caused enough problems in the past
that it may be worth it.

Also, could you help me find where we explicitly use lowercase
dictionary/parameter keys?  I don't remember seeing that.

> > (2) One of the stated goals of this document is to define intentionally
> > strict processing rules, but there are several places where we could
> > have removed additional optionality but elected to not do so.  What is
> > the criterion for "too far" towards strictness?  For example, Section
> > 4.2.7 gives optionality with respect to base64 padding (see COMMENT).
> 
> It's a judgement call in each case. As discussed below, we were lax on base64 due to common implementation limitations. In other cases, e.g., whitespace handling, it was to allow use with some existing headers, should one choose to, and because some people are still going to be creating these headers by hand (e.g., in .htaccess on Apache).
> 
> I'm happy to chat about it, but why is this last one a DISCUSS?

If there's a specific rule, it would need to be applied consistently.
Given that you say "judgement call in each case" that may be moot (in the
US sense), though I would have been happier if there was a more intentional
rule.

Also, since its vaguely topical here (but not part of the Discuss), you
note that several details were chosen so as to allow use with existing
headers.  The follow-up discussion downthread clarifies that, yes, this
spec itself only explicitly applies to new field specifications, but that
doesn't stop someone from noticing that existing fields have a compatible
encoding and can reuse the same machinery.  While I don't know of a
requirement for it, I'm used to seeing a mention in the document of the
motivation for such details, so that it puts everyone on more level ground
and doesn't require individual cleverness (and duplicated analysis work!)
to reach that conclusion.  Is there a reason that we don't want to mention
this possibility of parser/serializer reuse for existing fields in the
document itself?  I would find it helpful to understand the motivation for
certain details, though I concede that I may be unusual in that regard and
not part of the target audience.
> 
> > ----------------------------------------------------------------------
> > COMMENT:
> > ----------------------------------------------------------------------
> > 
> > Is the document toolchain used for this document responsible for the
> > rendering of em dashes as a single hyphen, as opposed to normal RFC
> > style which uses two hyphens for an em dash?
> 
> I have no idea. My assumption was that the RFC Editor would enforce whatever style was appropriate. Given that I'm already juggling at least one other (far more detailed) style guide for non-IETF work, that seems like the best use of time and mental bandwidth.

Sure.  It was distracting to me, but changing it now isn't going to
retroactively fix that :)

> > What's the motivation for "MUST omit values of boolean true" (in
> > parameters and dictionaries)?  It seems to make the output rules more
> > complicated without a significant gain in encoding size.
> 
> It allows some existing HTTP headers to be treated as structured for the purposes of serialisation.

The downthread discussion also made the claim that this is due to a desire
to have exactly one way to serialize.  But what's the good in having
exactly one way to serialize if you don't enforce that at the receiver?
(Compatibility with already-defined header fields, no doubt, but it does
kind of undercut some of the stated goals of this document.)

> > Section 1.2
> > 
> >   When parsing from HTTP fields, implementations MUST follow the
> >   algorithms, but MAY vary in implementation so as the behaviors are
> >   indistinguishable from specified behavior.  If there is disagreement
> > 
> > nit: was this intended to be "so long as"?
> 
> Fixed in editorial commit.
> 
> > Section 2
> > 
> >   o  Define the semantics of those structures.
> > 
> > nit: it's not clear to me what the referent for "those" is intended to
> > be (as while the previous bullet point does give a list of types of
> > structures, it seems to be saying that any given field has to pick
> > exactly one, which is inconsistent with the plural "those").
> 
> Fixed in editorial commit.
> 
> >   o  Specify any additional constraints upon the structures used, as
> >      well as the consequences when those constraints are violated.
> > 
> > nit: similarly, the plural "structures" may not be needed here.
> 
> Fixed in editorial commit.
> 
> >   Typically, this means that a field definition will specify the top-
> >   level type - List, Dictionary or Item - and then define its allowable
> >   types, and constraints upon them.  For example, a header defined as a
> > 
> > It's a little unfortunate that we task the word "type" with two
> > different meanings (the top-level List/Dictionary/Item and also the more
> > fine-grained Integer/String/etc.).  We also haven't mentioned the latter
> > type of "type" yet, though perhaps that's not an issue.
> 
> I don't see anything actionable here.
> 
> >   List might have all Integer members, or a mix of types; a header
> >   defined as an Item might allow only Strings, and additionally only
> >   strings beginning with the letter "Q".  Likewise, Inner Lists are
> >   only valid when a field definition explicitly allows them.
> > 
> > (We haven't mentioned Inner Lists yet, so the reader might not know what
> > to expect from them.)
> 
> Fixed in editorial commit.
> 
> >   When parsing fails, the entire field is ignored (see Section 4.2); in
> >   most situations, violating field-specific constraints should have the
> >   same effect.  Thus, if a header is defined as an Item and required to
> >   be an Integer, but a String is received, the field will by default be
> >   ignored.  If the field requires different error handling, this should
> >   be explicitly specified.
> > 
> > Explicitly specified in the specification that defines the structured
> > field, I trust?
> 
> Yes.
> 
> > Also, I worry that just leaving this as "in most cases" is a bit
> > antithetical to the goals of structured headers -- don't we want to nail
> > things down as tightly as we can?  If someone wants an exception, they
> > don't have to use structured headers...
> 
> We don't want to be so slavish to those goals as to make them unusable; consider the history of XML.
> 
> >   To further assure that this extensibility is available in the future,
> >   and to encourage consumers to use a complete parser implementation, a
> >   field definition can specify that "grease" Parameters be added by
> > 
> > Do you want to reference RFC 8701 here?
> 
> I don't think a reference to a TLS document will be helpful to many readers, especially when its definition of grease is so brief.

Okay.

> >   An extension to a structured field can then require that an entire
> >   field value be ignored by a recipient that understands the extension
> >   if constraints on the value it defines are not met.
> > 
> > This seems to set us up for a situation in which an implementation that
> > does not understand the extension will happily process the header field
> > and ignore the extension, but implementations that do understand the
> > extension will ignore the header field entirely.  Is that really what's
> > intended?  (When might this be desirable?)
> 
> I agree it's odd; I don't remember the discussion that got us to that text, but it certainly is possible to construct an extension that does that. Desirable, well, *shrug*.

I suppose we can hope to whatever degree we find appropriate that no
extensions find need to make use of this option.

> > Section 3
> > 
> >   This section defines the abstract value types that can be composed
> >   into Structured Fields.  The ABNF provided represents the on-wire
> > 
> > nit: I don't understand the sense in which "composed into" is used --
> > don't individual Structured Fields need to be exactly one of these three
> > types, irregardless of whether there is additional substructure?
> 
> That text pre-dates a rearrangement; I've massaged it in the editorial commit.
> 
> > Section 3.1
> > 
> >   An empty List is denoted by not serializing the field at all.
> > 
> > So any structured field definition that allows for an empty list has to
> > define its semantics to be equivalent to the semantics used for when the
> > peer does not support that structured field at all?
> 
> Yes; this point was discussed (a lot, if memory serves).

This seems like the sort of (potentially security relevant) detail that's
worth noting explicitly.

> > Section 3.1.1
> > 
> >   Inner Lists are denoted by surrounding parenthesis, and have their
> >   values delimited by a single space.  A field whose value is defined
> > 
> > The ABNF does not seem to support the "single" in "single space".
> 
> Fixed in editorial commit.
> 
> >   A header field whose value is defined as a List of Inner Lists with
> >   Parameters at both levels could look like:
> > 
> > Is this also an inner list of strings?
> 
> It could be.

I mean, this particular example looks to my eye to be an inner list of
strings.  The previous example specifically called out that its example
used an inner list of strings, so a reader might notice the lack of such
callout and wonder if this one is supposed to be different.

> > Section 3.1.2
> > 
> >   Parameters are an ordered map of key-values pairs that are associated
> > 
> > nit: is the plural "values" intentional?
> 
> Fixed in editorial commit.
> 
> >   parameters    = *( ";" *SP parameter )
> >   parameter     = param-name [ "=" param-value ]
> >   param-name    = key
> >   key           = ( lcalpha / "*" )
> >                   *( lcalpha / DIGIT / "_" / "-" / "." / "*" )
> > 
> > Huh, so I could have a key that was "*" or "*****"?  (I assume that no
> > special "wildcard" semantics are afforded to the asterisk in either
> > usage...)
> > 
> >   Example-ParamListHeader: abc;a=1;b=2; cde_456, (ghi;jk=4 l);q="9";r=w
> > 
> > Is it worth writing out which item/list each parameter applies to?
> 
> I don't think so.

I can defer to your judgment, but note that I'm only about 90% sure I'd get
it right if forced to guess.

> >   Parsers MUST support at least 256 parameters on an Item or Inner
> >   List, and support parameter keys with at least 64 characters.  Field
> >   specifications can constrain the types and cardinality of individual
> >   parameter names and values as they require.
> > 
> > In what way might I further constrain the *type* of a parameter *name*?
> 
> This text has already been rewritten; please see the editors' draft.
> 
> > Section 3.2
> > 
> >   Implementations MUST provide access to Dictionaries both by index and
> >   by name.  Specifications MAY use either means of accessing the
> >   members.
> > 
> >   Example-DictHeader: en="Applepie", da=:w4ZibGV0w6ZydGU=:
> > 
> > I, for one, would appreciate a bit of commentary on the interpretation
> > of the final '=', even if just a forward reference to §3.3.5.
> 
> Fixed in editorial commit.
> 
> >   Typically, a field specification will define the semantics of
> >   Dictionaries by specifying the allowed type(s) for individual member
> >   names, as well as whether their presence is required or optional.
> > 
> > Similarly to my previous comment, I'm not sure I understand what is
> > meant by the "type" of a member name.
> 
> Fixed in editorial commit.
> 
> > Section 3.3.3
> > 
> > Are the parentheses around "chr" in the sh-string construction doing
> > anything?
> 
> Fixed in editorial commit.
> 
> > Section 3.3.5
> > 
> >   A Byte Sequence is delimited with colons and encoded using base64
> >   ([RFC4648], Section 4).  For example:
> > 
> > Thank you for the section reference to disambiguate base64 and
> > base64url!
> 
> You're welcome.
> 
> > Section 4
> > 
> >   This section defines how to serialize and parse Structured Fields in
> >   field values, and protocols compatible with them (e.g., in HTTP/2
> >   [RFC7540] before HPACK [RFC7541] is applied).
> > 
> > nit: I'm not sure I'm parsing this sentence correctly: what does
> > "protocols compatible with them" bind to -- is it part of what "this
> > section defines"?
> > (I don't actually see any text anywhere in this document that I would
> > consider to be described by "protocols compatible with them".)
> 
> Fixed in editorial commit (this is a leftover from a previous rewrite).

Ah, and that fix makes it clear that I was mostly just stumbling over the
comma :)

> > Section 4.1
> > 
> > It's interesting to note that some of the subsections treat "input item
> > of wrong type" as an explicit failure case (e.g., 4.1.3.1) whereas
> > others implicitly assume that the input is one of the appropriate types
> > (e.g., 4.1.1).
> 
> Yes. Doing a type check at every conceivable point would make the specification more verbose for little gain in interoperability. We targeted places that are more likely to cause problems.

I'm glad to hear that these were intentional choices!

> >   Given a structure defined in this specification, return an ASCII
> >   string suitable for use in a HTTP field value.\
> > 
> > nit: I would have expected wording like "the following procedure
> > returns" or "return [...] value as follows" or similar; the current
> > sentence is an imperative that does not indicate how to fulfil it.
> > (The same comment applies to basically all of the subsections, and a
> > similar one to § 4.2 and its subsections.)
> 
> This is the first time anyone has commented on that style. As it's a matter of English usage, I'd prefer to wait for the input of the RFC Editor.

Sure.

> >   6.  Return output_string converted into an array of bytes, using
> >       ASCII encoding [RFC0020].
> > 
> > This implicitly assumes that output_string contains only characters
> > representable in ASCII.  I believe this is currently true based on the
> > relevant ABNF constructions and serialization procedures, but it still
> > feels like a needless dangling edge case.
> 
> Why? HTTP fields are already strongly encouraged to be ASCII.

If someone tries to hand me an output_string for which this encoding fails,
am I expected to fail to emit the entire structured field?

> > Section 4.1.2
> > 
> > Is the nesting level correct for these (sub)procedures?  It seems like
> > top-level steps 3, 4, and 5 are logically children of step 2.
> 
> Already fixed.
> 
> > Section 4.1.6
> > 
> >   3.  Let output be an empty string.
> > 
> >   4.  Append DQUOTE to output.
> > 
> > Can't we consolidate steps 3 and 4 (as is done in § 4.1.1.1 where output
> > is initialized to "("?
> 
> Fixed in editorial commit.
> 
> > Section 4.1.8
> > 
> >   1.  If input_bytes is not a sequence of bytes, fail serialization.
> > 
> > side note: perhaps this is just my roots as a C programmer showing
> > through, but how could you end up with something that's not a sequence
> > of bytes? :)
> 
> We expect a fair amount of diversity in implementation; some might use functions, others objects, etc. While the parent algorithm _should_ assure that the input is correct, double-checking does no harm.
> 
> > Section 4.2
> > 
> >   Strings split across multiple field lines will have unpredictable
> >   results, because comma(s) and whitespace inserted upon combination
> >   will become part of the string output by the parser.  Since
> >   concatenation might be done by an upstream intermediary, the results
> >   are not under the control of the serializer or the parser.
> > 
> > This seems to have the implication that the results are unpredictable
> > even if a serializer and parser collaborate to use a procedure that, in
> > disregard of this specification, splits individual list and/or
> > dictionary entries across multiple field lines, due to the potential for
> > an intermediary that is not complicit in the deviation from the spec.
> > In some sense, that might be the key implication of this statement, in
> > which case it's surprising to not see it spelled out more clearly.
> 
> Do you have a suggestion?

I'd consider adding a clause at the end, "even when the serializer and
parser are under the control of the same party", though that may have flaws
of its own.

> > Section 4.2.1.2
> > 
> >   3.  While input_string is not empty:
> > 
> >       1.  Discard any leading SP characters from input_string.
> > 
> >       2.  If the first character of input_string is ")":
> > 
> > Don't we need to check if the input_string is empty again after
> > consuming leading SP but before attempting to consume anything else?
> 
> In that case, it will fall through to (3) - running Parsing an Item - and fail there.

If we ask people to inspect the first character of an empty string, is
everybody going to get it right and avoid invoking undefined behavior in
their implementation language of choice?  To my eye there is some risk of
security-relevant implementation errors here.

> > Section 4.2.2
> > 
> >   Given an ASCII string as input_string, return an ordered map whose
> >   values are (item_or_inner_list, parameters) tuples. input_string is
> > 
> > Should we say anything about the map keys?
> 
> I'm struggling to come up with text that is worth the extra space taken; "keys that are keys" is hardly informative, and going into more detail seems to be re-specifying in the summary.

"an ordered map whose keyed values are (item_or_inner_list, parameters)
tuples", maybe?

> 
> > Section 4.2.3.2
> > 
> > Why do we use a different procedure to implement the "implicit true"
> > property of booleans than we did in Section 4.2.2?
> 
> They're doing slightly different things. We could rewrite 4.2.3.2 to use the same approach, but I'd prefer not to, as the risk of adding unintentional bugs seems greater than the benefit of consistency.

I confess that now I'm curious what is different, though it may not be
worth the time to get into.

> > Section 4.2.4
> > 
> >        4.  Otherwise, prepend char to input_string, and exit the loop.
> > 
> > This is the only place in the document where we use "prepend" in an
> > algorithm.  That might make it surprising to the reader; is it easy to
> > reformulate this without greedily consuming input that does not match?
> > 
> >        2.  If output_number is outside the range -999,999,999,999,999
> >            to 999,999,999,999,999 inclusive, fail parsing.
> > 
> > [I'm not sure how this check could trigger given the length check in 7.5]
> 
> Same answer as for Byte Sequences.
> 
> > Section 4.2.7
> > 
> >   Because some implementations of base64 do not allow reject of encoded
> >   data that is not properly "=" padded (see [RFC4648], Section 3.2),
> >   parsers SHOULD NOT fail when it is not present, unless they cannot be
> >   configured to do so.
> > 
> > This is a new protocol mechanism; why do we need to be so accommodating
> > to inflexible implementations?
> 
> Because some languages have implementations of base64 that don't expose the ability to control this. While we could require from-scratch implementation to conform, that seems unlikely to succeed.
> 
> > (Also, nit: "rejection")
> 
> Fixed in editorial commit.
> 
> >   Because some implementations of base64 do not allow rejection of
> >   encoded data that has non-zero pad bits (see [RFC4648], Section 3.5),
> >   parsers SHOULD NOT fail when it is present, unless they cannot be
> >   configured to do so.
> > 
> > (ibid)
> > Also, nit: singular/plural mismatch "bits"/"it is"
> 
> Fixed in editorial commit.
> 
> > Section 6
> > 
> > It seems worth mentioning the handling for duplicated key names (e.g.,
> > in parameters and dictionaries) w.r.t. overwrite or must-be-unique, and
> > how there have been previous vulnerabilities relating to different
> > implementations choosing "first one wins" vs. "last one wins".
> 
> That doesn't seem to apply to a correct implementation, only to headers that *aren't* structured fields.

It's still motivation for why we are making the choices we did and a
benefit that structured headers have over the existing mechanisms.

Also, it seems to explicitly apply to parameter map keys (per the earlier
discussion).

Thanks for the updates,

Ben

> > Appendix B
> > 
> >   Implementers should note that Dictionaries and Parameters are order-
> >   preserving maps.  Some fields may not convey meaning in the ordering
> >   of these data types, but it should still be exposed so that
> >   applications which need to use it will have it available.
> > 
> > This far through the document I still have no idea when the order might
> > actually be useful; even this text is only noting that (at least
> > sometimes) the order does not convey meaning.
> 
> As discussed previously, we got substantial feedback that it was useful for header authors interested in using this specification.
> 
> Cheers,
> 
> --
> Mark Nottingham   https://www.mnot.net/
>

Received on Tuesday, 19 May 2020 17:01:05 UTC