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

[David Benjamin <[e-mail]>]

Changes in draft-ietf-tls-grease-03.

On Mon, Jul 8, 2019 at 6:23 PM David Benjamin <[e-mail]> wrote:

> 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.
>>
>>