Re: [TLS] Genart last call review of draft-ietf-tls-tls13-24

[Eric Rescorla <[e-mail]>]

Re-sending hopefully smalle.

Hi Dale,

Thanks your your review.

Please provide a PR adding yourself to contributors, as in
  https://github.com/tlswg/tls13-spec/pull/1152

Detailed responses below.

-Ekr



> - There is inexactness about "transcript", "handshake context", and
>   "context".

I'm not sure what to do with this. We have a bunch of interoperable
implementations, so this seems clear enough that people can implement
it.


> - When a server receives ClientHello, is it obligated to promptly send
>   not just the ServerHello, but all first-flight messages from
>   ServerHello through Finished?  (section 4.2.11.3)  I ask this
>   because the client is only obligated/permitted to send
>   EndOfEarlyData when it receives the server's Finished.

No, it's not obligated to every send anything. It's just that it
has to send things in a specific order.


> - It seems inconsistent that the client can send an empty Certificate
>   message, but the server cannot, even though the server can omit
>   sending the Certificate message.  (section 4.4.2.4)

This is a longstanding property of TLS that had consensus in the WG,
so I don't expect we're going to change it now.


> - Comparing sections 4.2.10 and 4.6.1, when a PSK was established in
>   an earlier connection, exactly what are the limitations on the
>   cryptographic parameters that can be used when the PSK is used in a
>   resumption connection?  4.2.10 suggests that the following must be
>   the same in both connections:  TLS version, cipher suite, ALPN.  But
>   4.6.1 suggests that different cipher suites can be used, as long as
>   they use the same hash algorithm.

4.6.1. refers to the use of PSKs for ordinary 1-RTT handshakes
whereas 4.2.10 is for 0-RTT and so also specifies the cipher suite
because this cannot be negotiated in 0-RTT. Note the title of
4.2.10: "Early Data Indication"


> - In regard to section 4.6.1, it seems to require that a client MAY
>   NOT resume a connection using a ticket issued during a connection in
>   which the server did not present a certificate for itself, because
>   in the handshake of the resumption connection, the client is required
>   to verify that the SNI is compatible with the certificate the server
>   presented in the original connection.  But that constraint isn't
>   stated in section 2.2, despite being a global constraint on the
>   structure of sessions.

Section 2 is a Protocol Overview, so it doesn't describe everything.


> - Presumably implementations MUST NOT send zero-length fragments of alert
>   messages for the same reasons that they cannot send zero-length
>   fragments of handshake messages (whatever those reasons are).

Correct. S 5.1:

   Alert messages (Section 6) MUST NOT be fragmented across records and
   multiple Alert messages MUST NOT be coalesced into a single
   TLSPlaintext record.  In other words, a record with an Alert type
   MUST contain exactly one message.


> - There are about 28 error codes but nearly 150 places where the text
>   require the connection to be aborted with an error -- and hence,
>   nearly 150 distinct constraints that can be violated.  There are 19
>   alone for "illegal_parameter".  I would like to see an "alert
>   extension value" which assigns a distinct "minor" code to each
>   statement in the text that requires an error response (with
>   implementations being allowed to be a bit sloppy in providing the
>   correct minor code).

The structure of TLS alerts dates all the way back to SSLv3, spanning
three separate IETF Standards Track RFCs, and several WGLCs for TLS 1.3,
so I don't think this is a reasonable change to propose at this time.

> - I take it that there is no "close read side" operation.  (If that
>   existed, TLS could generate the "broken pipe" error.)

Yes, it does not exist.

> There are a number of issues which span the whole text:
>
> The interaction of this draft with extensions defined for previous
> versions of TLS is not laid out clearly.  It seems safe enough for
> this draft to import data structures from earlier extensions with only
> a reference to the earlier RFC, but if an extension defines behavior
> (e.g., a negotiation process), exactly what is the specification of
> that behavior in TLS 1.3, given that the referenced RFC only defines
> its use in TLS 1.2 or earlier?  At the least, there should be an
> explicit statement that the behaviors are carried forward in the
> "obvious way".
>
> It's also not clear exactly which previously defined extensions are
> brought forward into TLS 1.3.  I suspect that they are all listed in
> section 4.2, but is it clearly stated that those, and only those, are
> grandfathered in?

The table in 4.2 is intended to be exhaustive, but I can add some
text.


> Presumably, for any referenced extension, the placement of values in
> messages in TLS 1.2 has a "natural" analog in TLS 1.3 that at most
> involves moving the value from one field to another in certain
> messages.  But it would be reassuring to have a clear statement of
> this, and an enumeration of any more complex cases.

I don't understand the question. The table in 4.2 explicitly states
which messages extensions may appear in, and each such message
includes an extension block.


> There are about 28 error codes but nearly 150 places where the text
> require the connection to be aborted with an error.  There are 19
> alone for "illegal_parameter".  I would like to see an "alert
> extension value" which assigns a distinct "minor" code to each
> statement in the text that requires an error response.  This code
> would make it a lot easier to diagnose what is going wrong with a
> handshake.  To avoid making specifying and implementing these codes
> too difficult, implementations should be allowed to deviate to a
> degree from the specification regarding minor codes, and there should
> be a range of codes reserved for implementation-defined uses.
> Conversely, there are a couple of places where the implementation MUST
> NOT distinguish between different causes of an alert, but I believe
> that those are explicitly mentioned in the text.

See above.


> The terms "side", "server side", and "client side" are used in various
> places, despite that "endpoint" is the defined term.  I suggest
> replacing these terms with "endpoint", "server", and "client".

I believe this is within editor discretion.


> There are a number of places where "fatal alert" is used, but "error
> alert" seems to be the defined term.

The Alert protocol specifically defines the term "fatal" in S 6.


> Detail items are:
>
> 1.  Introduction
>
>    TLS is application protocol independent; higher-level protocols can
>    layer on top of TLS transparently.
>
> It might be informative to describe the nature of the
> currently-defined application protocol (a bidirectional, reliable byte
> stream) and similarly, to describe the requirements on the underlying
> transport (as far as I can tell, also a bidirectional, reliable byte
> stream).

I'll see what I can do.


> flight -- it seems to be thought that messages are grouped into
> "flights", where the messages of flight N from one sender cannot be
> sent until (more or less) it receives all messages of flights M (1 <=
> M <= N) from the peer.

Disagree. I think it's clear enough.


> MAC and HMAC -- by a simple count, both of these terms are used
> exactly 11 times in the document.  Is there any functional difference
> between them?

Yes. MAC is the generic term; HMAC is a specific MAC algorithm.
These are terms of art.


> ticket -- it's not clear what this means concretely.  Abstractly, it
> seems to include the set of cryptographic parameters needed to send
> application data, but concretely I can't figure out if it is the
> entire block of parameters (struct NewSessionTicket), or whether it is
> (or can be) just a key that points to the parameters in some database
> (the ticket *field* of struct NewSessionTicket).

I'll add some text.


> A related issue is that there is a field "ticket" in the structure
> "NewSessionTicket", and, at least conceptually, that structure is also
> considered a "ticket".  I strongly suggest you change the name of the
> field, and then revise uses of "ticket" to indicate whether they refer
> to the structure or to the field.  Comparing to other parts of the
> text, I think you may have intended to call the field "identity", as
> the value in NewSessionTicket.ticket seems to be intended to be copied
> into PreSharedKeyExtension.OfferedPsks.PskIdentity.

Disagree. People don't seem to have found this confusing in practice.


> RTT, 0-RTT, 1-RTT -- Can these be defined more rigorously?

I'll add some text.


> server name, SNI -- These two terms seem to be used interchangeably
> and as if the reader already understands their meaning and use.
> Suggest you standardize on one (and list the other as a synonym in the
> glossary).  Also include how "server name" interacts with the server's
> certificate.

The first use of SNI that's not in the changelog is in Section 4.2.11,
where it's defined as follows:

   In TLS versions prior to TLS 1.3, the Server Name Identification
   (SNI) value was intended to be associated with the session (Section 3
   of [RFC6066]), with the server being required to enforce that the SNI


> advertise -- I think this means when an endpoint sends a message
> containing an extension saying that it implements a feature or option,
> soliciting the peer to send a message containing the same extension
> (and thus agreeing to use the feature/option during this connection).

Yes, we're usign the ordinary meaning of the term.


> transcript -- It might be useful to put the definition of this in this
> section.  Also, define the "context" or "handshake context", which
> seems to be the sequence of messages included in the transcript hash
> (or the concatenation thereof).  But doesn't seem clear what messages
> are in the "handshake context" for any single usage.

It's in the table in S 4.4.


> handshake -- This has two meanings:  (1) messages whose content type
> is "handshake" (see section 5), and (2) messages with content type
> "handshake" that are part of the initial exchange of such messages.
> The problem is that there are messages that are (1) but not (2), and
> so you get confusing language like the title of section 4.6.

Disagree. This seems clear enough to me.


> establish vs. provision -- The document mostly adheres to the
> convention that pre-shared keys are "provisioned" whereas keys that
> are set up using TLS (possibly in a previous session) are
> "established".  In particular, "provisioned" is only used as an
> attribute of keys, whereas "establishing" is an action and
> "established" keys are ones created by that action.  But there are
> places where the two terms aren't used distinctly.

Feel free to point out some specific cases.


> ALPN -- This is used sporadically throughout the document.

Yes, I see that a reference crept in in 4.2.10 before the first citation.
I'll fix that.


> "hash over" -- A personal gripe of mine is that I look at a hash as a
> function, and so its value is a "hash of (some string)".  I don't mind
> "hash over X", "hash protects X", and "hash covers X"n when X is
> conceptualized as a substring of some larger string, that is, there's
> a Y that is explicitly being excluded.  But in a lot of cases in this
> document, X is explicitly constructed from various fragments, so
> there's no Y.  But maybe all these usages are conventional in
> cryptography.

Yes, this is conventional.


> 2.  Protocol Overview
>
>    The cryptographic parameters used by the secure channel are produced
>    by the TLS handshake protocol.  This sub-protocol of TLS [...]
>
> I think you want to s/This sub-protocol/The TLS handshake protocol/,
> since a naive reader (me!) could consider "the secure channel" as a
> plausible antecedent of "this".

Disagree. I think it's clear.


> 2.1.  Incorrect DHE Share
>
>    Note: The handshake transcript includes the initial ClientHello/
>    HelloRetryRequest exchange; it is not reset with the new ClientHello.
>
> "transcript" has not been defined yet.  Perhaps:
>
>    Note: The handshake transcript (the message input to the MAC in the
>    Finished message) starts with ...

Transcript is a term of art.


> 2.2.  Resumption and Pre-Shared Key (PSK)
>
>    Figure 3 shows a pair of handshakes in which the first establishes a
>    PSK and the second uses it:
>
> The first handshake does not point out where the PSK is established.
> Better would be
>
>    Figure 3 shows a pair of handshakes in which the first establishes
>    a PSK and the second uses it.  In the first, a PSK is carried from
>    the server to the client in the NewSessionTicket message.  In the
>    second, the identity of the PSK is sent by the client to the server
>    in the ClientHello.

Yes, I'll add something here.


> 3.1.  Basic Block Size
>
>    The representation of all data items is explicitly specified.  The
>    basic data block size is one byte (i.e., 8 bits).
>
> "byte" appears in the text 91 times, and "octet" appears 20 times.
> You probably want to change the uses of "octet" to "byte" for
> consistency.

We use them interchangeably. I think that's fine.


> 3.3.  Vectors
>
> You may want to move section 3.4 to before section 3.3, because
> section 3.3 implicitly depends on all numeric fields being unsigned,
> whereas the fact that all numeric fields are unsigned is only stated
> in section 3.4.

Done.


> 3.5.  Enumerateds
>
>    An additional sparse data type is available called enum.  Each
>
> You probably want s/called enum/called enum or enumerated/.

Done.


>    Future extensions or additions to the protocol may define new values.
>
> Add "... of a previously existing enumerated."

Not needed.


> 4.1.2.  Client Hello
>
>    In that case, the client MUST send the same
>    ClientHello (without modification) except:
>
> s/(without modification) except:/without modification, except:/

Done.


>       For every TLS 1.3 ClientHello, this vector
>       MUST contain exactly one byte set to zero, which corresponds to
>       the "null" compression method in prior versions of TLS.
>
> There is an ambiguity in English, where this might mean "the number of
> bytes in this field which are zero is exactly one".  It's a bit hard
> avoiding the ambiguity, but this seems to work:
>
>       For every TLS 1.3 ClientHello, this vector MUST have exactly one
>       member.  The member MUST be zero, which corresponds to the "null"
>       compression method in prior versions of TLS.

I did something here.



>       The actual "Extension" format is defined in Section 4.2.
>
> Probably delete "actual".  Most uses of "actual" in current writing
> (including mine) can be profitably deleted.

Disagree. This is fine.


> 4.1.4.  Hello Retry Request
>
>    This allows the client to avoid having to
>    compute partial hash transcripts for multiple hashes in the second
>    ClientHello.
>
> This seems to be correct but I found it very hard to decode.  This is
> clearer:
>
>    This allows the client to avoid having to compute hashes of partial
>    transcripts using multiple hash functions, to be used in binders in
>    the second ClientHello.

Disagree: I think this is clear enough to implementors.


> 4.2.  Extensions
>
>    In TLS 1.3, unlike TLS 1.2, extensions are negotiated for each
>    handshake even when in resumption-PSK mode.  However, 0-RTT
>    parameters are those negotiated in the previous handshake; mismatches
>    may require rejecting 0-RTT (see Section 4.2.10).
>
> I think what you mean is:
>
>    Even for a session that is resumed using a PSK established in an
>    earlier session, the applicable extensions are negotiated in its
>    initial handshake and aren't carried over from the handshake of the
>    session which established the PSK.  However, the parameters
>    applicable to 0-RTT data are those negotiated in the previous
>    handshake; if those parameters are unacceptable to the server, it
>    may reject use of 0-RTT in the session (see Section 4.2.10).
>
> --

Yes, I think this says that,


>    Servers should be prepared to receive
>    ClientHellos that include this extension but do not include 0x0304 in
>    the list of versions.
>
> s/should/SHOULD/

This ended up as a MUST due to in Adam's review.


> 4.2.2.  Cookie
>
>    Clients MUST NOT use cookies in their initial ClientHello in
>    subsequent connections.
>
> I think this becomes cleaner if you omit "in subsequent connections".

Disagree, I think this is clearer.


> 4.2.3.  Signature Algorithms
>
> Items "RSASSA-PSS RSAE algorithms" and "RSASSA-PSS PSS algorithms"
> both contain:
>
>       If the public key is carried in an X.509 certificate,
>       it MUST use the rsaEncryption OID [RFC5280].
>
> I think "it" references "certificate", so it would be clearer to say
>
>       If the public key is carried in an X.509 certificate,
>       the certificate MUST use the rsaEncryption OID [RFC5280].

Disagree. It's the public key structure (SPKI) certificate that
has the OID. Your rephrase makes it unclear which of the many
OIDs in a certificate it refers to.


> --
>
>       If the corresponding public key's parameters present, [...]
>
> s/parameters present/parameters are present/

Done.


>    -  Implementations that advertise support for RSASSA-PSS (which is
>       mandatory in TLS 1.3), [...]
>
> The phrase "Implementations that advertise" makes me think of the
> label on the box.  I think you mean "Endpoints that advertise".

Disagree. We use implementation and endpoint interchangeable for
the most part.



>    The "extension_data" field of these extension contains a
>    SignatureSchemeList value:
>
> I think s/these extension/these extensions/ (rather than s/these
> extension/this extension/).

Done.


> 4.2.7.  Negotiated Groups
>
>    Items in named_group_list are ordered according to the client's
>    preferences (most preferred choice first).
>
> This can be simplified to "Items in named_group_list are in descending
> order of the client's preferences."

This seems like a matter of editorial discretion.


> 4.2.8.  Key Share
>
>    group  The named group for the key being exchanged.  Finite Field
>       Diffie-Hellman [DH] parameters are described in Section 4.2.8.1;
>       Elliptic Curve Diffie-Hellman parameters are described in
>       Section 4.2.8.2.
>
> I think this should be capitalized as "Finite field Diffie-Hellman
> ..." and "Elliptic curve Diffie-Hellman ...".

Disagree. The convention is FFDH and ECDH


>    key_exchange  Key exchange information.  The contents of this field
>       are determined by the specified group and its corresponding
>       definition.
>
> This could be better defined by rearranging the two items, since the
> parameters don't describe the group, they describe the key being
> exchanged:
>
>    group  The value designating the named group for the keys being
>       exchanged, as defined in section 4.2.7.
>
>    key_exchange  The key exchange information.  Finite field
>       Diffie-Hellman [DH] parameters are described in Section 4.2.8.1;
>       Elliptic curve Diffie-Hellman parameters are described in
>       Section 4.2.8.2.

I made a change here.

>    Each KeyShareEntry value MUST correspond to a
>    group offered in the "supported_groups" extension and MUST appear in
>    the same order.
>
> I think you mean to require that for a given group there can be only
> one KeyShareEntry.  So you could say
>
>    Each KeyShareEntry value MUST correspond to a distinct group
>    offered in the "supported_groups" extension, and the KeyShareEntrys
>    MUST appear in the order their groups appear (possibly
>    non-consecutively) in "supported_groups".

We state later that you can't offer duplicates:

  Clients MUST NOT offer multiple
   KeyShareEntry values for the same group.


> 4.2.8.1.  Diffie-Hellman Parameters
>
>    This check ensures that the remote peer is properly behaved
>    and isn't forcing the local system into a small subgroup.
>
> s/into a small subgroup/into insecure operation/?

No. A small subgroup is a specific condition.


> 4.2.8.2.  ECDHE Parameters
>
>    For the curves secp256r1, secp384r1 and secp521r1, peers MUST
>    validate each other's public value Y by ensuring that the point is a
>    valid point on the elliptic curve.  The appropriate validation
>    procedures are defined in Section 4.3.7 of [X962] and alternatively
>    in Section 5.6.2.3 of [KEYAGREEMENT].  This process consists of three
>    steps: (1) verify that Y is not the point at infinity (O), (2) verify
>    that for Y = (x, y) both integers are in the correct interval, (3)
>    ensure that (x, y) is a correct solution to the elliptic curve
>    equation.  For these curves, implementers do not need to verify
>    membership in the correct subgroup.
>
> It seems that the language of this paragraph is a version behind,
> particularly in that this paragraph seems to use "Y" differently from
> the definition of UncompressedPointRepresentation.  Comparing with
> [KEYAGREEMENT], it seems like it ought to read (with changes marked
> >>>...<<<):
>
>    For the curves secp256r1, secp384r1 and secp521r1, peers MUST
>    validate each other's public value >>>Q = (X, Y)<<< by ensuring
>    that the point is a valid point on the elliptic curve.  The
>    appropriate validation procedures are defined in Section 4.3.7 of
>    [X962] >>>or<<< alternatively in Section >>>5.6.2.3.3<<< of
>    [KEYAGREEMENT].  This process consists of three steps: (1) verify
>    that >>>Q<<< is not the point at infinity (O), (2) verify that
>    >>>both integers X and Y <<< are in the correct interval, >>>and<<<
>    (3) ensure that >>>Q<<< is a correct solution to the elliptic curve
>    equation.  For these curves, implementers do not need to verify
>    membership in the correct subgroup.
>
> (You can s/or alternatively/and also/)

I did some of this,


> In particular, 5.6.2.3.3 of [KEYAGREEMENT] is "validation without
> verifying subgroup membership", so it needs to be verified that this
> procedure expresses the author's intent.
>
> 4.2.9.  Pre-Shared Key Exchange Modes
>
>    psk_dhe_ke  PSK with (EC)DHE key establishment.  In this mode, the
>       client and servers MUST supply "key_share" values as described in
>       Section 4.2.8.
>
> s/servers/server/

Done.


> 4.2.10.  Early Data Indication
>
>    For
>    externally established PSKs, the associated values are those
>    provisioned along with the key.
>
> Probably s/externally established/provisioned/.

Done.


>    For PSKs provisioned via NewSessionTicket, a server MUST validate
>    that the ticket age for the selected PSK identity (computed by
>    subtracting ticket_age_add from PskIdentity.obfuscated_ticket_age
>    modulo 2^32) is within a small tolerance of the time since the ticket
>    was issued (see Section 8).
>
> s/provisioned/established/.

I actually think provisioned is better here, b/c NST doesn't
quite establish them.


> s/ticket_age_add/ticket_age_add in the ticket/.

Disagree. I think this is clear enough.


>    0-RTT messages sent in the first flight have the same (encrypted)
>    content types as their corresponding messages sent in other flights
>    (handshake and application_data) but are protected under different
>    keys.
>
> s/as their corresponding messages sent in/as messages of the same types
sent in/

Done.


>    After receiving the server's Finished message, if the server
>    has accepted early data, an EndOfEarlyData message will be sent to
>    indicate the key change.  This message will be encrypted with the
>    0-RTT traffic keys.
>
> This is awkward.  Perhaps
>
>    After receiving the server's Finished message, if the server
>    has accepted early data, the client will send an EndOfEarlyData message
>    indicate that following (non-early) application data uses the
>    negotiated keys.  The EndOfEarlyData message is be encrypted with the
>    0-RTT traffic keys.

Disagree.

> --
>
>    -  Return its own extension in EncryptedExtensions, indicating that
>       it intends to process the early data.
>
> s/its own extension/an early_data extension/

Done.

>
>    "pre_shared_key" extension.  In addition, it MUST verify that the
>    following values are consistent with those associated with the
>    selected PSK:
>
> s/consistent with/the same as/

Done.

>    If the server chooses to accept the "early_data" extension, then it
>    MUST comply with the same error handling requirements specified for
>    all records when processing early data records.
>
> It seems like this could be misread by binding "when processing..." to
> "specified".  This avoids that:
>
>    If the server chooses to accept the "early_data" extension, then it
>    MUST apply to early data records the same error handling
>    requirements specified for other data records.

I think this is clear enough.


> --
>
>    Specifically, if the
>    server fails to decrypt any 0-RTT record following an accepted
>    "early_data" extension it MUST terminate the connection with a
>    "bad_record_mac" alert as per Section 5.2.
>
> But probably better to s/any/an/.

Actually "a"


>    If the server rejects the "early_data" extension, the client
>    application MAY opt to retransmit early data once the handshake has
>    been completed.
>
> Better:
>
>    [...] MAY opt to retransmit as non-early data the application data
>    contained in the early data records

Done.

> --
>
>    Note that automatic re-transmission of early data
>    could result in assumptions about the status of the connection being
>    incorrect.
>
> This doesn't quite say what you want.  Better
>
>    Note that after connection establishment, the application may
>    consider the status of the connection to be different than it was
>    for early data, and so transmitting the same bytes as non-early
>    application data may not have the same effect as transmitting them
>    as early application data.

Disagree.


>    Similarly,
>    if early data assumes anything about the connection state, it might
>    be sent in error after the handshake completes.
>
> A bit awkward.  Perhaps
>
>    Similarly,
>    if early data assumes anything about the connection state, it might
>    be erroneous to re-send the same data after the handshake completes.

Disagree.


> 4.2.11.  Pre-Shared Key Extension
>
>    The "pre_shared_key" extension is used to indicate the identity of
>    the pre-shared key to be used with a given handshake in association
>    with PSK key establishment.
>
> s/indicate/negotiate/ -- because more than one can be offered.

Done.


>    selected_identity  The server's chosen identity expressed as a
>       (0-based) index into the identities in the client's list.
>
> I think this is intended as an index into the (abstract) vectors
> OfferedPsks.identities and OfferedPsks.binders, as opposed to an
> offset into the serialized data structures.  You could be clearer with
>
>    selected_identity  The server's chosen identity expressed as a
>       (0-based) index into the vector of identities in OfferedPsks.

I think this is clear enough.

> --
>
>    identity  A label for a key.  For instance, a ticket defined in
>       Appendix B.3.4 or a label for a pre-shared key established
>       externally.
>
> See issues regarding "ticket" in section 1.1.

See above response.


>    In TLS versions prior to TLS 1.3, the Server Name Identification
>    (SNI) value was intended to be associated with the session (Section 3
>    of [RFC6066]), with the server being required to enforce that the SNI
>    value associated with the session matches the one specified in the
>    resumption handshake.  However, in reality the implementations were
>    not consistent on which of two supplied SNI values they would use,
>    leading to the consistency requirement being de-facto enforced by the
>    clients.  In TLS 1.3, the SNI value is always explicitly specified in
>    the resumption handshake, and there is no need for the server to
>    associate an SNI value with the ticket.  Clients, however, SHOULD
>    store the SNI with the PSK to fulfill the requirements of
>    Section 4.6.1.
>
> See issue regarding "SNI" in section 1.1.

See above response.


>    Implementor's note: the most straightforward way to implement the
>    PSK/cipher suite matching requirements is to negotiate the cipher
>    suite first and then exclude any incompatible PSKs.
>
> I think you mean:
>
>    Implementor's note: the most straightforward way for the server to
>    implement the PSK/cipher suite choice requirements is to choose the
>    cipher suite first and then exclude any PSKs incompatible with the
>    chosen cipher suite.

I believe it is sufficiently clear.

>
> since this doesn't seem to describe an interaction between the server
> and the client, but simply how the server responds to one message.
>
>    In order to accept PSK key
>    establishment, the server sends a "pre_shared_key" extension
>    indicating the selected identity.
>
> I think this sentence would read better as a separate paragraph.

Disagree.


>    This extension MUST be the last extension in the ClientHello. (This
>    facilitates implementation as described below.)
>
> Given that the previous paragraph discussed the early_data extension,
> "This extension" isn't clear.  So s/This extension/The
> "pre_shared_key" extension/.

Done.

>
>    If this
>    value is not present or does not validate, the server MUST abort the
>    handshake.
>
> s/does not validate/is not valid/  (An actor validates a value; a
> value is validated.)

Disagree.


> 4.2.11.1.  Ticket Age
>
>    The "obfuscated_ticket_age"
>    field of each PskIdentity contains an obfuscated version of the
>    ticket age formed by taking the age in milliseconds and adding the
>    "ticket_age_add" value that was included with the ticket, see
>    Section 4.6.1 modulo 2^32.
>
> Clearer would be
>
>    [...] "ticket_age_add" value that was in the NewSessionTicket for
>    the ticket, modulo 2^32.

This was fixed in a different way.


>    Note that
>    the "ticket_lifetime" field in the NewSessionTicket message is in
>    seconds but the "obfuscated_ticket_age" is in milliseconds.
>
> Expand to
>
>    Note that
>    the "ticket_lifetime" field in the NewSessionTicket message is in
>    seconds but the "obfuscated_ticket_age" and "ticket_age_add" fields
>    are in milliseconds.

No, ticket_age_add is not in any units. It's just a number.



> 4.2.11.3.  Processing Order
>
>    Clients are permitted to "stream" 0-RTT data until they receive the
>    server's Finished, only then sending the EndOfEarlyData message,
>    followed by the rest of the handshake.  In order to avoid deadlocks,
>    when accepting "early_data", servers MUST process the client's
>    ClientHello and then immediately send the ServerHello, rather than
>    waiting for the client's EndOfEarlyData message.
>
> This is awkward, and omits the remainder of the servers' first flight
> messages.  Better is
>
>    Clients are permitted to "stream" 0-RTT data until they receive the
>    server's Finished message, only then sending the EndOfEarlyData
>    message, and the rest of the handshake.  In order to avoid
>    deadlocks, when accepting early data, servers MUST process the
>    client's ClientHello immediately upon receipt, and immediately send
>    all of its first flight messages from ServerHello through Finished,
>    rather than waiting for the client's EndOfEarlyData message.

Changed differently.


> 4.4.  Authentication Messages
>
>    Certificate  The certificate to be used for authentication, and any
>       supporting certificates in the chain.  Note that certificate-based
>       client authentication is not available in 0-RTT mode.
>
> Probably better to say s/in 0-RTT mode/for 0-RTT data/ -- or perhaps
> "early data".

No. It's not available at all if you do 0-RTT, or even in PSK. I
updated accordingly.


>
>    The following table defines the Handshake Context and MAC Base Key
>    for each scenario:
>
> Eh, what?  I think what you mean is that this table specifies what
> base keys are used for which messages.

No. It specifies which messages go into the various CV and
Finished computations.


> But "Mode" and "Handshake
> Context" don't seem to be defined terms.

Handshake context is defined immediately above.

"A Handshake Context consisting of the set of messages to be
  included in the transcript hash."

> 4.4.1.  The Transcript Hash
>
>    Many of the cryptographic computations in TLS make use of a
>    transcript hash.  This value is computed by hashing the concatenation
>    of each included handshake message, including the handshake message
>    header carrying the handshake message type and length fields, but not
>    including record layer headers.  I.e.,
>
> There are a number of awkward spots in how this is phrased.  Better:
>
>    Many of the cryptographic computations in TLS make use of a
>    transcript hash.  This value is computed by hashing the concatenation
>    of a sequence of messages in the handshake, with each message
>    including the TLS message type and length fields, but not any
>    headers of the underlying transport protocol.

No. This is incorrect, because the TLS records are not included.
Left as-is.



>     Transcript-Hash(M1, M2, ... MN) = Hash(M1 || M2 ... MN)
>
> Usually, symbol for "the n-th message" would use lower-case "n" as the
> index, and one usually puts the operator before and after "...".  Also
> add verbal explanation:
>
>    The transcript hash of messages M1 through Mn is:
>
>       Transcript-Hash(M1, M2, ... Mn) = Hash(M1 || M2 || ... || Mn)

I changed to Mn and added the ||s. I don't think the text is needed.



> Continue,
>
>    As an exception to this general rule, when the server has responded to
a
>    ClientHello with a HelloRetryRequest, the first ClientHello is
>    replaced with a special synthetic handshake message of message type
>    "message_hash" whose data part is Hash(first ClientHello).  I.e.,
>
>   Transcript-Hash(ClientHello1, HelloRetryRequest, ... Mn) =
>       Hash(message_hash ||        /* Handshake type (1 byte) */
>            00 00 Hash.length ||   /* Handshake message length, in bytes
(3 bytes) */
>            Hash(ClientHello1) ||  /* Hash of ClientHello1 */
>            HelloRetryRequest || ... || Mn)
>
>    The reason for this construction is to allow the server not
>    store state after sending HelloRetryRequest by storing just the
>    hash of the first ClientHello in the cookie, rather than requiring
>    it to store all of the ClientHello or the entire intermediate hash
>    state (see Section 4.2.2).
>
> --
>
>    For concreteness, the transcript hash is always taken from the
>    following sequence of handshake messages, starting at the first
>
> This is awkward.  Perhaps
>
>    the transcript hash is always of the following sequence of
>    handshake messages, starting at the first ClientHello and including
>    only those messages that we sent/received:

Disagree.


>    In general, implementations can implement the transcript by keeping a
>    running transcript hash value based on the negotiated hash.
>
> Probably s/negotiated hash/negotiated hash function/.
>
> Also, this needs to include the modification of truncating the last
> message if it is to include the transcript hash.  I think you need
> something like:
>
>    If the last message, Mn, is to include the transcript hash, then
>    the transcript hash is always the last field of the message, and
>    that message is first truncated by removing that field from the
>    message.  (The message length field of Mn is unmodified; it includes
>    the length of the transcript hash.)
>
>       Transcript-Hash(M1, M2, ... Mn) = Hash(M1 || M2 || ... ||
Truncate(Mn))

Messages never include themselves in the transcript hash. There
is a truncated case, but it is handled differently, with the
message passed in as pre-truncated.

>
> 4.4.2.  Certificate
>
>    If the corresponding certificate type extension
>    ("server_certificate_type" or "client_certificate_type") was not
>    negotiated in Encrypted Extensions, or the X.509 certificate type was
>    negotiated, then each CertificateEntry contains a DER-encoded X.509
>    certificate.
>
> This needs a reference to RFC 7250 to define certificate type
> extension.  Also, see the general issues regarding extensions.

Disagree. It is cited earlier.

>    Each following certificate SHOULD
>    directly certify one preceding it.
>
> The phrase "one preceding it" allows extraneous certificates in the
> list, as "one preceding it" doesn't usually require that it be
> immediately preceding.  I think you mean "the one preceding it", which
> does require it to be immediately preceding, and thus does not allow
> extraneous certificates in the chain.

Yes, although the real world is much messier.


> 4.4.2.1.  OCSP Status and SCT Extensions
>
>    CertificateStatus message.  In TLS 1.3, the server's OCSP information
>    is carried in an extension in the CertificateEntry containing the
>    associated certificate.
>
> Clearer to phrase it:
>
>    [...] in the CertificateEntry containing the
>    associated certificate in the Certificate message.

Disagree.

> --
>
>    CertificateRequest message.  If the client opts to send an OCSP
>    response, the body of its "status_request" extension MUST be a
>    CertificateStatus structure as defined in [RFC6066].
>
> s/its "status_request" extension"/the "status_request" extension in
> its Certificate message/.

Disagree.


> 4.4.2.2.  Server Certificate Selection
>
>    All certificates provided by the server MUST be signed by a signature
>    algorithm advertised by the client, if they are able to provide such
>    a chain (see Section 4.2.3).
>
> Probably better a/All certificates/Each certificate/.

Disagree.

> s/they are/the server is/

s/they/it/

>    If the client cannot construct an acceptable chain using [...]
>
> The purpose of this paragraph is not clear.  Was "server" meant?  If
> so, it seems to be redundant.  I think it is intended to discuss how
> the client processes the (alleged) certificate chain presented by the
> server, in which case, it's a sharp change of focus for this section.
> That could be aided by moving this paragraph to the end of the section
> and adding some words:

It's not. This is about how the server decides what to send and
directly follows from the previous paragraph.

>    If the client cannot construct an acceptable chain from the
>    certificates provided by the server and decides to abort the
>    handshake, then it MUST abort the handshake with an appropriate
>    certificate-related alert (by default, "unsupported_certificate";
>    see Section 6.2 for more).
>
> But it would probably be better to integrate it into section 4.4.2.4.
>
> 4.4.2.3.  Client Certificate Selection
>
>    -  If the CertificateRequest message contained a non-empty
>       "oid_filters" extension, the end-entity certificate MUST match the
>       extension OIDs recognized by the client, as described in
>       Section 4.2.5.
>
> More exact would be "must match the extension OID/value pairs that are
> recognized by the client."

Done.

> 4.4.2.4.  Receiving a Certificate Message
>
> It seems inconsistent that the client can send an empty Certificate
> message, but the server cannot, even though the server can omit
> sending the Certificate message.

That's the way it has been since TLS 1.0 at least. We're not changing
it now.



> 4.4.3.  Certificate Verify
>
>    This message is used to provide explicit proof that an endpoint
>    possesses the private key corresponding to its certificate.
>
> I'd prefer s/to its certificate/to the certificate it has presented/.

Disagree.


> The discussion of how "signature" is formed is awkward and I'm not
> sure I understand it.  E.g., the digital signature is computed "over"
> a string, but one part of that string is "the content to be signed".
> I think it could be made clearer as:
>
>    The algorithm field specifies the signature algorithm used (see
>    Section 4.2.3 for the definition of this field).  The signature is a
>    digital signature using that algorithm.  The string that is signed
>    is the concatenation of:
>
>    -  A string that consists of octet 32 (0x20) repeated 64 times
>
>    -  The context string
>
>    -  A single 0 byte which serves as a separator
>
>    -  Transcript-Hash(Handshake Context, Certificate)
>
> But that leaves unclear what "context string" and "Handshake Context"
> are.  I think you want to define those back in 4.4.1 (and probably
> also in 1.1)

The both are defined clearly in 4.4.1.


> as both being the concatenation of the messages that are
> in the transcript to this point.  I also assume that the Certificate
> Verify message is truncated when it is put into the Transcript-Hash
> and "context string", but that needs to be stated.

That's not how it's done. I believe a close reading of 4.4.1 will
make this clear.


> 4.4.4.  Finished
>
>    The key used to compute the finished message is computed from the
>
> s/finished/Finished/
>
>    The key used to compute the finished message is computed from the
>    Base key defined in Section 4.4 using HKDF (see Section 7.1).
>
> This is correct, but you have to read further to understand the key
> described here isn't the key that encrypts the finished message.  It
> would be easier to understand if the text was rearranged:
>
>    Structure of this message:
>
>       struct {
>           opaque verify_data[Hash.length];
>       } Finished;
>
>    The verify_data value is computed as follows:
>
>       finished_key =
>           HKDF-Expand-Label(BaseKey, "finished", "", Hash.length)
>
>       verify_data =
>           HMAC(finished_key,
>                Transcript-Hash(Handshake Context,
>                                Certificate*, CertificateVerify*))
>
>       * Only included if present.
>
> And this is another instance where the poorly-defined "Handshake
> Context" appears.

Disagree. 4.4.1 appropriately sets the context for this.


>    Any records following a 1-RTT Finished message MUST be encrypted
>    under the appropriate application traffic key as described in
>    Section 7.2.
>
> Are there any non-1-RTT Finished messages?

There used to be. I updated a bit.

> And aren't all application
> data records encrypted under the "appropriate" key?  Or is an
> "application traffic key" different from the keys used for application
> data early in the connection?  This needs to be rephrased somehow, but
> I can't guess in what way.
>
> 4.6.  Post-Handshake Messages
>
> This section name is awkward.  Of course, there are messages after the
> handshake.  I think the problem is that there are "handshake
> messages", messages with handshake types (or content type
> "handshake"), that are not part of "the handshake", the initial
> exchange of handshake-type messages.  In the end, you need to decide
> what terminology to use so that the title of this section makes sense.

The WG decided on the terminology that appears here.

>
>    the appropriate application traffic key.
>
> Is there a strict accounting of what messages are encrypted using
> which key?

I think so.


> 4.6.1.  New Session Ticket Message
>
>    message, it MAY send a NewSessionTicket message.  This message
>    creates a unique association between the ticket value and a secret
>    PSK derived from the resumption master secret.
>
> It would be useful to mention that the resumption_master_secret is
> defined/computed in section 7.1.

Done.


> Does "ticket value" mean the NewSessionTicket structure or the
> "ticket" field within it.  See issues regarding "ticket" for section
> 1.1.

It means the ticket field. I don't think this is particularly confusing.



>    (Section 4.2.11).  Servers MAY send multiple tickets on a single
>
> Note the conflation here of "ticket" with "NewSessionTicket message".
>
>    Any ticket MUST only be resumed with a cipher suite that has the same
>    KDF hash algorithm as that used to establish the original connection.
>
> How is a ticket "resumed"?

See S 2.2, where we define "resumption" I added some text there.


> Also, since in a "resumption" connection, the ticket that is used is
> (or refers to) a PSK, the above statement corresponds to
> the statement "In addition, it MUST verify that the
> following values are consistent with those associated with the
> selected PSK:" in section 4.2.10.
>
>    Clients MUST only resume if the new SNI value is valid for the server
>    certificate presented in the original session, and SHOULD only resume
>    if the SNI value matches the one used in the original session.
>
> What does it mean to say a client "resumes"?

See above.


> Here we suddenly descend into the usage of what seems to be an
> extension, server_name, which is presumably optional and logically
> added on to TLS rather than being an integral part of it.

As with many IETF protocols, extensions are an integral part
of TLS, though some extensions are actually not needed
for basic operations.


> Also the logic isn't described very cleanly; I think it means "A
> client must abort resuming a connection if the ServerHello message
> does not contain a server_name extension whose value is a valid SNI
> for the server certificate presented in the original session ...".

That's not how SNI works. It's not a negotiation, it's a statement.

> All of this seems to require that a client MAY NOT resume a connection
> using a ticket issued during a connection in which the server did not
> present a certificate for itself.  But that constraint wasn't stated
> in section 2.2, despite being a global constraint on the structure of
> sessions.

That's not a requirement, and I'm not sure how you're getting
that.


> Or am I wrong in believing that the client chooses to resume the
> connection by placing the ticket in the ClientHello *before* it
> receives the ServerHello (which contains the SNI)?

You're right about how the client does it, but wrong about where
SNI goes. It's a statement by the client.


> This paragraph
> seems to be written as if the client decides to resume after receiving
> ServerHello.
>
>    ticket_lifetime  Indicates the lifetime in seconds as a 32-bit
>       unsigned integer in network byte order from the time of ticket
>       issuance.
>
> Probably better to s/the time of ticket issuance/the time the
> NewSessionTicket was sent to the client/, unless "issuance"/"to issue"
> is explicitly defined somewhere.

Disagree. This is plenty clear.


>    ticket  The value of the ticket to be used as the PSK identity.  The
>       ticket itself is an opaque label.
>
> This shows the ambiguities around "ticket"; this specification says
> that "'ticket' is the value of the ticket to be used as the PSK
> identity".

Yes, that's what it is.


> Is it intended that the "ticket" field of NewSessionTicket will become
> the "identity" field of PskIdentity.OfferedPsks.PreSharedKeyExtension?

Yep.


>    max_early_data_size  The maximum amount of 0-RTT data that the client
>       is allowed to send when using this ticket, in bytes.  Only
>       Application Data payload (i.e., plaintext but not padding or the
>       inner content type byte) is counted.  A server receiving more than
>       max_early_data_size bytes of 0-RTT data SHOULD terminate the
>       connection with an "unexpected_message" alert.  Note that servers
>       that reject early data due to lack of cryptographic material will
>       be unable to differentiate padding from content, so clients SHOULD
>       NOT depend on being able to send large quantities of padding in
>       early data records.
>
> The last sentence assumes that a server that "reject early data due to
> lack of cryptographic material" will be strict and count all bytes in
> early data messages against the max_early_data_size quota.  However, a
> server in such a situation could be liberal and not bother counting
> any bytes -- since it will be discarding early data messages
> (immediately after discovering that it can't decrypt them), it never
> has to buffer more than one of them.  Unless I'm overlooking
> something, this advice isn't needed, since a server in this situation
> isn't buffering early data.

No, because you don't know what a server will do.


> 4.6.2.  Post-Handshake Authentication
>
>    All of the client's
>    messages for a given response MUST appear consecutively on the wire
>    with no intervening messages of other types.
>
> Better, "consecutively in the underlying transport stream".  But that
> is a little vague given the message/fragment/record mechanism.  More
> exactly,
>
>    All of the client's messages for a given response MUST appear
>    consecutively on the wire, that is, the records containing the
>    fragments of the messages composing the client's response must
>    appear consecutively in the underlying transport stream.

Disagree.


> 4.6.3.  Key and IV Update
>
> I think you want to promote the first paragraph before the data
> structure definition.

Disagree.


> 5.  Record Protocol
> and
> 5.1.  Record Layer
>
> The text isn't very clear about the message/fragment/record mechanism.
> The text wants to consider the data for each content type to consist
> of a series of messages.  The messages are cut into fragments.
> Adjacent fragments within one content type stream can be concatenated
> to form the content of TLSPlaintext structures.
>
> One problem is that despite this model, the boundary between messages
> isn't carried through the transport.  For application data, message
> boundaries are lost entirely.  For handshake and alert content types,
> there are some complex restrictions how their message boundaries show
> up as record boundaries, but the actual framing of messages is done
> "in band" by the message length fields.  A closer description of the
> services TLS provides is that the data for each content type is a
> stream that can be broken arbitrarily into fragments that are the
> content of records, except:
>
> - The boundaries of alert messages must be boundaries of the records
>   that carry them and no record boundary can be introduced into an
>   alert message.
>
> - If two records contain fragments of the same handshake message, all
>   records between them must contain only fragments of that handshake
>   message.
>
> - If there is a key change between the sending of two handshake
>   messages, there must be a record boundary between them.
>
>    -  Handshake messages MUST NOT span key changes.  Implementations
>       MUST verify that all messages immediately preceding a key change
>       align with a record boundary; if not, then they MUST terminate the
>       connection with an "unexpected_message" alert.  Because the
>       ClientHello, EndOfEarlyData, ServerHello, Finished, and KeyUpdate
>       messages can immediately precede a key change, implementations
>       MUST send these messages in alignment with a record boundary.
>
> Is this description correct?  As written, it says "because a key
> change can happen after message X, there must be a record boundary
> after message X", which isn't exactly the same as "Handshake messages
> MUST NOT span key changes" -- unless there is always a key change
> following these message types, in which case s/can immediately
> precede/always precede/.  I think the three points listed above give a
> clearer and more accurate version.

ClientHello and ServerHello may not appear before a key change.
Left as-is.


>    Implementations MUST NOT send zero-length fragments of Handshake
>    types, even if those fragments contain padding.
>
> Presumably implementations MUST NOT send zero-length fragments of alert
> messages also.

That's stated elsewhere.


>    When record protection has not yet been engaged, TLSPlaintext
>    structures are written directly onto the wire.
>
> Better "... sent directly using the underlying transport protocol".

Editor discretion.


> 5.2.  Record Payload Protection
>
> I *think* what's going on with record protection is:
>
> - The TLSPlaintext is turned into a TLSInnerPlaintext by moving the
>   type field, removing the length field, and copying "fragment" to
>   "content".  (Why do the structures use different names for the data
>   fragment?)
>
> - The encryption is done by:
>
>       AEADEncrypted =
>           AEAD-Encrypt(write_key, nonce, plaintext)
>
>   where plaintext is TLSInnerPlaintext and AEADEncrypted becomes
>   TLSCiphertext.encrypted_record.
>
> - TLSCiphertext is transmitted.
>
> But the text doesn't say this at all plainly.  I would add before
> "AEAD algorithms take...":
>
>    The TLSPlaintext is converted into a TLSInnerPlaintext by copying the
>    type field, removing the length field, and copying the fragment
>    field.

I made a few changes, but I don't believe this text is necessary.


> (assuming we rename "content" to "fragment")
>
> Then replace the equation by:
>
>    encrypted_record =
>           AEAD-Encrypt(endpoint_write_key, nonce, TLSInnerPlaintext)

Disagree.

> --
>
>    The key is either the
>    client_write_key or the server_write_key, the nonce is derived from
>    the sequence number (see Section 5.3) and the client_write_iv or
>    server_write_iv, and the additional data input is empty (zero
>    length).
>
> It would be clearer to move the reference to read "the nonce is
> derived from the sequence number and the client_write_iv or
> server_write_iv (see Section 5.3)" as section 5.3 describes both the
> sequence number and the derivation.

Done.

> Similarly, the decryption algorithm is better expressed by
>
>       TLSInnerPlaintext =
>           AEAD-Decrypt(peer_write_key, nonce, encrypted_record)

Disagree.

> --
>
>    This limit
>    is derived from the maximum TLSPlaintext length of 2^14 octets + 1
>    octet for ContentType + the maximum AEAD expansion of 255 octets.
>
> s/TLSPlaintext/TLSInnerPlaintext/ -- the maximum length of
> TLSPlaintext is 2^14 + 5.

Done.


> 5.3.  Per-Record Nonce
>
>    The appropriate sequence number is incremented by one after reading
>    or writing each record.  The first record transmitted under a
>    particular traffic key MUST use sequence number 0.
>
> I think the first and second paragraphs could be profitably combined:
>
>    A 64-bit sequence number is maintained separately for reading and
>    writing records and is incremented by one after reading or writing
>    each record.  Each sequence number is set to zero at the beginning
>    of a connection and whenever the key for that direction is changed;
>    the first record transmitted under a particular traffic key uses
>    sequence number 0.
Done.


> --
>
>    Because the size of sequence numbers is 64-bit, they should not wrap.
>
> The sense of "should" is not clear.  I think what you want to say is
>
>    Because the size of sequence numbers is 64 bits, there is no need to
>    allow sequence numbers to wrap.

Disagree. This is a statement about the future.


>    Each AEAD algorithm will specify a range of possible lengths for the
>    per-record nonce, from N_MIN bytes to N_MAX bytes of input
>
> I think this is clearer (as it makes clear where N_MIN comes from):
>
>    Each AEAD algorithm specifies an N_MIN and N_MAX, which give the
>    range of possible lengths in bytes of the per-record nonce.

Disagree.


> 5.4.  Record Padding
>
>    Padding is a string of zero-valued bytes appended to the
>    ContentType field before encryption.
>
> More exact would be
>
>    Padding is a string of zero-valued bytes following the type field
>    in TLSInnerPlaintext.

Disagree.


>    The presence of padding does not change the overall record size
>    limitations - the full encoded TLSInnerPlaintext MUST NOT exceed 2^14
>    + 1 octets.  If the maximum fragment length is reduced, as for
>    example by the max_fragment_length extension from [RFC6066], then the
>    reduced limit applies to the full plaintext, including the content
>    type and padding.
>
> I think you want s/encoded TLSInnerPlaintext/TLSInnerPlaintext/ -- all
> data structures are defined with a concrete representation, so
> "encoded" is redundant, but "encoded" could be confused with
> "encrypted" and the encrypted plaintext can be longer than the plaintext.

People know what encoded versus encrypted means.


>    If the maximum fragment length is reduced, as for
>    example by the max_fragment_length extension from [RFC6066], then the
>    reduced limit applies to the full plaintext, including the content
>    type and padding.
>
> This needs clarification.  I think you mean that if the m.f.l. is
> reduced, then the limit on TLSInnerPlaintext is reduced to m.f.l.+1,
> but that's not what this says.  OTOH, if this text is accurate, the
> maximum length of the fragment proper is m.f.l.-1.

No, that's never been the case. MFL starts as 2^14 + 1.


> 6.  Alert Protocol
>
>    Alert messages convey a description of the alert and a legacy field
>    that conveyed the severity of the message in previous versions of
>    TLS.  In TLS 1.3, the severity is implicit in the type of alert being
>    sent, and the 'level' field can safely be ignored.
>
> I think at this point you want to insert "Alerts are divided into two
> classes: closure alerts and error alerts."
>
>    Stateful implementations of tickets (as in many clients)
>    SHOULD discard tickets associated with failed connections.
>
> What is a "ticket" here?

It's synechdoche for "PSKs and the associated ticket name"


> And what are the "associations" in question?  E.g., presumably it
> includes the ticket used when attempting to establish a connection if
> the attempt fails.  But if a NewSessionTicket is received during a
> connection, and the connection is later aborted, does the client have
> to discard the remembered NewSessionTicket?

I clarified this somewhat.


>    All the alerts listed in Section 6.2 MUST be sent as fatal and MUST
>    be treated as fatal regardless of the AlertLevel in the message.
>    Unknown alert types MUST be treated as fatal.
>
> This was remarkably confusing until I figured out that the first
> "fatal" means "with AlertLevel "fatal"" and the second and third
> "fatal" mean "indicates abortive closure"!  Better:
>
>    All the alerts listed in Section 6.2 MUST be sent with AlertLevel
>    "fatal" and when received MUST be treated as error alerts
>    regardless of the AlertLevel in the message.  Unknown alert types
>    MUST be treated as error alerts.

Done.

> 6.1.  Closure Alerts
>
>    user_canceled  This alert notifies the recipient that the sender is
>       canceling the handshake for some reason unrelated to a protocol
>       failure.  If a user cancels an operation after the handshake is
>       complete, just closing the connection by sending a "close_notify"
>       is more appropriate.  This alert SHOULD be followed by a
>       "close_notify".  This alert is generally a warning.
>
> What does "This alert is generally a warning." mean?  What is it a
> warning of?

Changed.

>    Each party MUST send a "close_notify" alert before closing its write
>    side of the connection, unless it has already sent some other fatal
>    alert.
>
> This implies that close_notify is a "fatal alert" (properly, error
> alert).  And "before closing the write side of the connection" is not
> clear, since sending close_notify *is* closing the write side of the
> connection.  Better:
>
>    Each party MUST send a "close_notify" alert before closing the
>    write side of the underlying transport connection, unless it has
>    already sent some other error alert.

Done.


> (Unless I'm mistaken regarding what is intended.)
>
> I take it that there is no "close read side" operation.  (If that
> existed, TLS could generate the "broken pipe" error -- the sender
> wants to transmit more data but the receiver is unwilling to receive
> it.)

Yes.


>    If the application protocol using TLS provides that any data may be
>    carried over the underlying transport after the TLS connection is
>    closed, the TLS implementation MUST receive a "close_notify" alert
>    before indicating end-of-data to the application-layer.  No part of
>    this standard should be taken to dictate the manner in which a usage
>    profile for TLS manages its data transport, including when
>    connections are opened or closed.
>
> This isn't clear too me.  The second sentence seems to mean:
>
>    A usage profile for TLS specified how it manages its data
>    transport, including when connections are opened or closed.  No
>    part of this standard should be taken to dictate the manner in
>    which a usage profile for TLS manages its data transport.
>
> But I can't figure out what the first sentence means.  It seems to
> mean "If ... a TLS implementation MUST receive a "close_notify" alert
> before indicating end-of-data to its application-layer.", but that's
> obvious behavior, what else would cause it to signal EOD?

Some non-TLS traffic.


>    Note: It is assumed that closing the write side of a connection
>    reliably delivers pending data before destroying the transport.
>
> I think you mean
>
>    Note: It is assumed that closing the write side of a connection
>    will cause the peer TLS implementation to reliably deliver all
>    transmitted data before [what?]

No, that's not what it means. It means that you deliver the
close_notify before sending FIN (if on TCP).

> 7.  Cryptographic Computations
>
>    The TLS handshake establishes one or more input secrets which are
>    combined to create the actual working keying material, as detailed
>    below.
>
> Probably delete "actual".  Most uses of "actual" in current writing
> (including mine) can be profitably deleted.

Disagree.


> 7.1.  Key Schedule
>
> Given the terminology in RFC 5869, struct HkdfLabel probably should be
> called HkdfInfo, as it is the "info" argument to HKDF-Expand.

Disagree.


>    Messages are the concatenation of the indicated handshake messages,
>
> s/Messages are/Messages is/!

Done.


>    Given a set of n InputSecrets, the final "master secret" is computed
>    by iteratively invoking HKDF-Extract with InputSecret_1, [...]
>
> This is hard to follow.  If I understand correctly, this is a general
> description of the mechanism that is diagrammed below.  But, e.g.,
> "secret" is used for at least two categories of quantities.  It would
> be clearer to phrase this:
>
>    Generally, we use a construction that takes as input a sequence of
>    n InputSecrets and from them computes a sequence of derived
>    secrets.  The initial derived secret is simply a string of
>    Hash.length bytes set to zeros.  Each successive derived secret is
>    computed by invoking HKDF-Extract with an InputSecret and the
>    preceding derived secret as inputs.
>
>    Concretely, for the present version of TLS 1.3, the construction
>    proceeds as diagrammed below, with the InputSecrets on the left
>    side and the derived secrets passing as shown by the downward
>    arrows.  The InputSecrets are added in the following order, where
>    if a given InputSecret is not available, then the 0-value is used
>    in its place:

<---

I have updated this somewhat.


> --
>
>    -  HKDF-Extract is drawn as taking the Salt argument from the top and
>       the IKM argument from the left.
>
> Append "with its output to the bottom and the name of the output at
> the right".
>
> 7.2.  Updating Traffic Keys and IVs
>
> I think you want to remove "and IVs" here.  IVs aren't mentioned in
> this section.  Of course, changing the traffic key changes the IV, but
> it also changes the write key, and the write key isn't mentioned in
> the section title.

Done.


> 7.3.  Traffic Key Calculation
>
> I think you want to title the section "Write Key and IV Calculation".

No. These are the traffic keys, which are derived from traffic secrets.

> And you want to (again) de-genericize the text:
>
>    The traffic keying material (*_write_key and *_write_iv) is
o>    generated from the following input values:
>
>    -  A secret value, the applicable *_traffic_secret
>
>    -  A purpose value indicating the specific value being generated
>
>    -  The length of the key

Disagree.


> 7.4.1.  Finite Field Diffie-Hellman
>
>    For finite field groups, a conventional Diffie-Hellman computation is
>    performed.
>
> I think you need a reference for this!

TODO.


> 7.5.  Exporters
>
>    A separate
>    interface for the early exporter is RECOMMENDED, especially on a
>    server where a single interface can make the early exporter
>    inaccessible.
>
> What does "where a single interface can make the early exporter
> inaccessible" mean?  If you don't have a separate interface for the
> early exporter, how does "a single interface" make it inaccessible?

The handshake might complete in the meantime.


> 8.1.  Single-Use Tickets
>
>    If the tickets are not self-contained but rather are database keys,
>    and the corresponding PSKs are deleted upon use, then connections
>    established using one PSK enjoy forward secrecy.
>
> What does "one PSK" mean here?  Do you mean "established using such a
> PSK" (equivalently "established using such a ticket")?

I just changed it to "a PSK"



> Also, the question again arises as to what a "ticket" *is*.
>
>    Because this mechanism requires sharing the session database between
>    server nodes in environments with multiple distributed servers, it
>
> Probably more conventional to say "requires a shared database between
> all server instances".

Disagree.


> 8.2.  Client Hello Recording
>
> The first paragraph is hard to follow.  I think it could be clarified
> along these lines:
>
>    An alternative form of anti-replay is to record a unique value
>    derived from the ClientHello (generally either the random value or
>    the PSK binder) and reject duplicates.  Recording all ClientHellos
>    causes state to grow without bound, but a server can instead retain
>    ClientHellos only within a given time window and use the
>    "obfuscated_ticket_age" to determine whether the ClientHello was
>    generated by the client recently.  Thus, the server can ensure that
>    it only allows 0-RTT data in connections established by
>    non-duplicate ClientHellos which were generated by the client
>    within the recording window.

Disagree.


>
>    The server MUST derive the storage key only from validated sections
>    of the ClientHello.  If the ClientHello contains multiple PSK
>    identities, then an attacker can create multiple ClientHellos with
>    different binder values for the less-preferred identity on the
>    assumption that the server will not verify it, as recommended by
>    Section 4.2.11.  I.e., if the client sends PSKs A and B but the
>    server prefers A, then the attacker can change the binder for B
>    without affecting the binder for A.
>
> At this point, a conditional needs to be inserted; otherwise the
> argument you're making is only implicit.
>
>    If the server uses the binder for B as part of the storage key,
>    these variations on the ClientHello will not be detected by the
>    server as duplicates of each other, and the server will accept all
>    of them.
>
> Then continue with:
>
>    This may cause side effects such as replay cache
>    pollution, although any 0-RTT data will not be decryptable because it
>    will use different keys.  If the validated binder or the
>    ClientHello.random are used as the storage key, then this attack is
>    not possible.

I rewrote this a little bit.

>
>    When implementations are freshly started, they SHOULD reject 0-RTT as
>    long as any portion of their recording window overlaps the startup
>    time.  Otherwise, they run the risk of accepting replays which were
>    originally sent during that period.
>
> I think this needs a couple of changes of phrasing:
>
>    When an implementation is restarted with a cleared recording
>    memory, it SHOULD reject 0-RTT as long as the startup time is
>    within the recording window.  Otherwise, it runs the risk of
>    accepting replays of ClientHellos which were sent during the
>    previous execution.

Disagree.


> 8.3.  Freshness Checks
>
>    Variations in client and server clock rates are likely to be minimal,
>    though potentially with gross time corrections.
>
> What does "gross time corrections" mean?  I think you mean "with
> moments of large change in the clock time", but that isn't a feature
> of the clock *rate*.  I think this is more accurate:
>
>    Differences between client and server clock times are likely to be
>    minimal, though there will sometimes be gross differences due to
>    uninitialized clocks and misconfigured time zones.

I rewrote this a little bit


>    After early data is accepted, records may continue to be streamed
>    to the server over a longer time period.
>
> More clear as
>
>    After the server accepts early data is accepted, the client may
>    continue to send early data to the server over a longer time period
>    than the freshness window for ClientHellos.

Not necessary.


> 9.1.  Mandatory-to-Implement Cipher Suites
>
>    A TLS-compliant application MUST support digital signatures with
>    rsa_pkcs1_sha256 (for certificates), rsa_pss_rsae_sha256 (for
>    CertificateVerify and certificates), and ecdsa_secp256r1_sha256.
>
> It seems that ecdsa_secp256r1_sha256 is missing a statement of what
> uses it must be supported for.

That is because it must simply be supported.


> 9.2.  Mandatory-to-Implement Extensions
>
>    -  If containing a "supported_groups" extension, it MUST also contain
>       a "key_share" extension, and vice versa.  An empty
>       KeyShare.client_shares vector is permitted.
>
> I think this is a bit better expressed as:
>
>    -  If containing a "supported_groups" extension, it MUST also contain
>       a "key_share" extension (which may contain an empty
>       KeyShare.client_shares vector), and vice versa.

Disagree.

> --
>
>    Additionally, all implementations MUST support use of the
>    "server_name" extension with applications capable of using it.
>
> I'm not clear what the test is for "applications capable of using
> SNI".  I think you want to turn the conditional around:
>
>    An application profile MAY require that the endpoint's TLS
>    implementation supports use of the "server_name" extension.

Disagree. This was subject to WG consensus.


> 9.3.  Protocol Invariants
>
>    -  A server receiving a ClientHello MUST correctly ignore all
>       unrecognized cipher suites, extensions, and other parameters.
>       Otherwise, it may fail to interoperate with newer clients.  In TLS
>       1.3, a client receiving a CertificateRequest or NewSessionTicket
>       MUST also ignore all unrecognized extensions.
>
> This needs to be split, because the two parts are about different roles:
>
>    -  A server receiving a ClientHello MUST correctly ignore all
>       unrecognized cipher suites, extensions, and other parameters.
>       Otherwise, it may fail to interoperate with newer clients.

Disagree.


>    -  In TLS 1.3, a client receiving a CertificateRequest or
>       NewSessionTicket MUST also ignore all unrecognized extensions.
>
> 11.  IANA Considerations
>
>    -  TLS Alert Registry: Future values are allocated via Standards
>       Action [RFC8126].  IANA [SHALL update/has updated] this registry
>       to include values for "missing_extension" and
>       "certificate_required".
>
> It would be nice to add a finer-grained "minor alert code" registry.

As noted, I do not believe this change is appropriate.


> Appendix A.  State Machine
>
> It would be helpful if the state diagrams were extended to describe
> the activity on the "control channel" (handshake and alert content
> types) after CONNECTED, that is, what happens while the connection is
> established and how connections are shut down.

I took a look at this and concluded it would make matters worse.


> Appendix B.  Protocol Data Structures and Constant Values
>
> There is no listing of the value structure corresponding to each
> extension type.  Extensions collectively are only defined as:
>
>    struct {
>        ExtensionType extension_type;
>        opaque extension_data<0..2^16-1>;
>    } Extension;
>
> This is a problem because the name of the value struct is not
> systematically derived from the name of the extension_type value!
> E.g., "signature_algorithms" and "signature_algorithms_cert" use
> SignatureSchemeList as a value, and you can only reliably discover
> that by searching through the whole of the text.

I have not found other places where we did this particularly
helpful.


> B.4.  Cipher Suites
>
>    A symmetric cipher suite defines the pair of the AEAD algorithm and
>    hash algorithm to be used with HKDF.
>
> Better phrased:
>
>    A symmetric cipher suite is the pair of an AEAD algorithm and a
>    hash algorithm to be used with HKDF.

Disagree.


> C.3.  Implementation Pitfalls
>
>    -  As a server, do you send a HelloRetryRequest to clients which
>       support a compatible (EC)DHE group but do not predict it in the
>       "key_share" extension?o
>
> This needs an additional condition:
>
>    -  As a server, if you select an (EC)DHE group which the client
>       supports but for which the client did not provide a
>       KeyShareEntry, do you send a HelloRetryRequest?

These are the same.



> Appendix D.  Backward Compatibility
>
>    Prior versions of TLS used the record layer version number for
>    various purposes.  (TLSPlaintext.legacy_record_version and
>    TLSCiphertext.legacy_record_version) As of TLS 1.3, this field is
>    [...]
>
> I think this was intended to be formatted thusly:
>
>    Prior versions of TLS used the record layer version number
>    (TLSPlaintext.legacy_record_version and
>    TLSCiphertext.legacy_record_version) for various purposes.  As of
>    TLS 1.3, this field is [...]

Done.


> D.5.  Backwards Compatibility Security Restrictions
>
>    The security of SSL 3.0 [SSL3] is considered insufficient for the
>    reasons enumerated in [RFC7568], and MUST NOT be negotiated for any
>    reason.
>
> s/and MUST NOT/and it MUST NOT/, with "it" referring to "SSL 3.0", as
> otherwise the verb "MUST NOT" is parallel to the verb "is" and the
> subject of "MUST NOT" is "the security of SSL 3.0".

Done.


> E.1.  Handshake
>
>    -  A set of "session keys" (the various secrets derived from the
>       master secret) from which can be derived a set of working keys.
>
> Is this consistent with the usual meaning of "session key"?  My
> understanding (which may be wrong) is that a "session key" is the key
> for a "session", i.e., an entire connection.  Perhaps there is already
> a defined term in the text that covers the use you intend.

The reason for scare quotes is that this is the term of art in
the cryptographic literature.


>
>    Note that these
>    properties are not necessarily independent, but reflect the protocol
>    consumers' needs.
>
> The significance of "but" is not clear here, as it seems to be placing
> in opposition "reflect the ... needs" and "independent".  I think this
> is probably closer to what you meant:
>
>    Note that these properties are not necessarily independent, but
>    together they cover the protocol consumers' needs.

Disagree. The point is that this is from the consumer's perspective.


> --
>
>    Uniqueness of the session keys:  Any two distinct handshakes should
>       produce distinct, unrelated session keys.  Individual session keys
>       produced by a handshake should also be distinct and unrelated.
>
> It's not clear how two session keys produced by a single handshake can
> be "unrelated".  I suspect there's a known technical term for this,
> like "cryptographically independent" (to parallel "cryptographically
> random").

I've changed this to "independent"


>
> A similar term is needed in section E.1.4.
>
>    If fresh (EC)DHE keys are used for each
>    connection, then the output keys are forward secret.
>
> When it is used as an adjective, you hyphenate "forward-secret".

Diagree.



> E.1.3.  0-RTT
>
>    See Section 4.2.10 for one mechanism to limit the exposure to replay.
>
> This discussion is now in section 8.
>
> [END]
>