Re: [TLS] AD review of draft-ietf-tls-grease-02

[David Benjamin <[e-mail]>]

Thanks for the comments! I've addressed them in
https://github.com/tlswg/draft-ietf-tls-grease/pull/10.

On Wed, Jul 3, 2019 at 1:11 PM Benjamin Kaduk <[e-mail]> wrote:

> Section 1
>
>    The TLS protocol [RFC8446] includes several points of extensibility,
>    including the list of cipher suites and the list of extensions.  The
>    values in these lists identify implementation capabilities.  TLS
>
> We could probably make this text a little more precise (for one, there's
> not a single list of extensions since many messages can carry
> extensions).  So, maybe "the list of cipher suites and several lists of
> extensions" and "The values transmitted in these lists"?
>

Done.


> Section 2
>
> Can we add an editorial note that values prefaced with "{TBD}" are
> suggested values but subject to change prior to final allocation by
> IANA?
>

Done. Also made the other such notes match the style used in the TLS 1.3
draft.


>    Future versions of TLS or DTLS [RFC6347] MUST NOT use any of the
>    above values as versions.
>
> Process-wise, this feels like an attempt to Update: the (D)TLS core
> specs, which we can't do in an Informational document.  So it would
> probably be better to say something "The values thusly allocated are no
> longer available for use as version numbers by (D)TLS implemnetations".
> Things are made somewhat awkward by there not being a registry of
> protocol versions, sadly...
>

Done.


> Section 3.1
>
> Are there any of these for which we want to say "the client MUST NOT
> advertise a list consisting solely of GREASE values"?  It would probably
> be fine to do this for, say, key_share, but not for, say, cipher_suites.
> But perhaps the reader will be smart enough to figure out what works
> without prodding from us...
>

I dunno, I feel like that's a bit overkill, but I can include something in
that vein if others disagree. A cipher suite list full of GREASE is
functionally equivalent to a list containing some weird cipher no one
implements.


>    Clients MUST reject GREASE values when negotiated by the server.
>    Specifically, the client MUST fail the connection if a GREASE value
>    appears any in the following:
>
> I did not attempt to audit the (omitted) list for completeness; the
> first sentence should cover any situations that are not specifically
> listed, right?
>

It should. I replaced "Specifically" with "In particular" so that's clearer.


> Section 3.2
>
>    When processing a ClientHello, servers MUST NOT treat GREASE values
>    differently from any unknown value.  Servers MUST NOT negotiate any
>    GREASE value when offered in a ClientHello.  Servers MUST correctly
>    ignore unknown values in a ClientHello and attempt to negotiate with
>    one of the remaining parameters.
>
> Similarly to the above, we might consider adding a parenthetical noting
> that there may not be any remaining valid parameters, and that's not
> necessarily fatal.
>

Done.


>    Note that these requirements are restatements or corollaries of
>    existing server requirements in TLS.
>
> (side note) Some future reviewers might complain about using normative
> language to duplicate exisiting requirements from other documents; in
> this case, I don't mind, myself.
>
> Section 4.1
>
>    o  A server MAY select one or more GREASE extension values and
>       advertise corresponding extensions with varying length and
>       contents.
>
> nit: I don't think "corresponding" is quite the right word; maybe
> "advertise those extensions"?
>

Rephrased this and elsewhere.


>    o  A server MAY select one or more GREASE signature algorithm values
>       and advertise them in the "signature_algorithms" extension.
>
> I'm not necessarily expecting any action based on this comment, but I
> note that status_request, signed_certificate_timestamp,
> certificate_authorities, oid_filters, and signature_algorithms_cert are
> also currently defined for CertificateRequest but we do not call out any
> extension-specific greasing for them.  Of that list, only
> signature_algorithms_cert seems like it might be calling out for special
> handling, to me...
>

Added signature_algorithms_cert.


> Section 4.2
>
>    When processing a CertificateRequest or NewSessionTicket, clients
>    MUST NOT treat GREASE values differently from any unknown value.
>    Clients MUST NOT negotiate any GREASE value when offered by the
>    server.  Clients MUST correctly ignore unknown values offered by the
>    server and attempt to negotiate with one of the remaining parameters.
>
> (following the theme) I don't remember any cases where the client can
> succeed if the list becomes empty after pruning unknown values ... if we
> are deciding that we want to say anything on this topic at all.
>

Added a similar parenthetical.


> Section 5
>
>    Implementations advertising GREASE values SHOULD select them at
>    random.  This is intended to encourage implementations to ignore all
>    unknown values rather than any individual value.  Implementations
>    MUST honor protocol specifications when sending GREASE values.  For
>    instance, implementations sending multiple GREASE values as
>    extensions MUST NOT send the same GREASE value twice.
>
> Feel free to tell me that I'm being internally inconsistent, but in this
> case "MUST NOT send the same GREASE value twice" does not seem like a
> good place to use normative language to restate an existing requirement.
> So I'd rather see lowercase "must not" and possibly a section reference
> to 8446 § 4.2 ("[t]here MUST NOT be more than one extension of the same
> type in a given extension block.").
>

Rephrased this.


> Section 6
>    [[TODO: Update IANA considerations for TLS 1.3 and rebase over draft-
>    ietf-tls-iana-registry-updates.]]
>
> Can the shepherd please work with the author to make the needed changes?
>
> IIRC the main change for TLS 1.3 is the "TLS 1.3" column for
> extensiontype values.
>
> Since this document is Informational, we have to be Recommended "N" for
> everything.
>

Oh oops, I must have missed this when rebasing over TLS 1.3. Added the
relevant columns.


> Thanks for the note about the specific values listed being just
> suggestions.
>
>