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

Dale, thanks for your review. I have entered a Yes ballot and encouraged the author/WG to take a look at your comments. I suspect a lot of the stylistic/linguistic items derive from the WG participants having deep experience with the protocol and its previous versions and existing extensions.

Alissa

> On Mar 2, 2018, at 11:00 PM, Dale Worley <worley@ariadne.com> wrote:
> 
> Reviewer: Dale Worley
> Review result: Ready with Nits
> 
> I am the assigned Gen-ART reviewer for this draft.  The General Area
> Review Team (Gen-ART) reviews all IETF documents being processed
> by the IESG for the IETF Chair.  Please treat these comments just
> like any other last call comments.
> 
> For more information, please see the FAQ at
> <https://wiki.tools.ietf.org/area/gen/wiki/GenArtfaq>.
> 
> Document:  draft-ietf-tls-tls13-24
> Reviewer:  Dale R. Worley
> Review Date:  2018-03-02
> IETF LC End Date:  2018-03-02
> IESG Telechat date:  2018-03-08
> 
> Summary:
> 
>       This draft is basically ready for publication, but has nits
>       that should be fixed before publication.
> 
> This review only covers the general properties of the proposed
> protocol and the exposition, as I am unqualified to assess its
> security properties.
> 
> There are various places where the exposition could be made clearer,
> especially to readers not immersed in security matters.  In many
> places, it is mostly a matter of making clearer the connections
> between different points in the exposition.
> 
> In a few places, there seems to be ambiguity regarding the
> specification that has technical significance.  In particular:
> 
> - There is inexactness about "transcript", "handshake context", and
>  "context".
> 
> - 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.
> 
> - 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)
> 
> - 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.
> 
> - 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.
> 
> - 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).
> 
> - 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).
> 
> - I take it that there is no "close read side" operation.  (If that
>  existed, TLS could generate the "broken pipe" error.)
> 
> 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?
> 
> 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.
> 
> 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.
> 
> 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".
> 
> There are a number of places where "fatal alert" is used, but "error
> alert" seems to be the defined term.
> 
> 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).
> 
> 1.1.  Conventions and Terminology
> 
> There are a number of terms which are frequently used in the text that
> don't seem to be defined.  It's likely that they are only used in
> exposition, that if all sentences containing them were deleted, the
> document would still be complete and accurate.  But it seems like it
> would be easier on the reader to define them.  Terms that come to mind
> are:
> 
> 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.
> 
> 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?
> 
> 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).
> 
> 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.
> 
> RTT, 0-RTT, 1-RTT -- Can these be defined more rigorously?
> 
> 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.
> 
> 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).
> 
> 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.
> 
> 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.
> 
> 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.
> 
> ALPN -- This is used sporadically throughout the document.
> 
> "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.
> 
> 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".
> 
> 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 ...
> 
> 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.
> 
> 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.
> 
> 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.
> 
> 3.5.  Enumerateds
> 
>   An additional sparse data type is available called enum.  Each
> 
> You probably want s/called enum/called enum or enumerated/.
> 
>   Future extensions or additions to the protocol may define new values.
> 
> Add "... of a previously existing enumerated."
> 
> 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:/
> 
>      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.
> 
> --
> 
>      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.
> 
> 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.
> 
> 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).
> 
> --
> 
>   Servers should be prepared to receive
>   ClientHellos that include this extension but do not include 0x0304 in
>   the list of versions.
> 
> s/should/SHOULD/
> 
> 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".
> 
> 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].
> 
> --
> 
>      If the corresponding public key's parameters present, [...]
> 
> s/parameters present/parameters are present/
> 
>   -  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".
> 
>   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/).
> 
> 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."
> 
> 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 ...".
> 
>   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.
> 
> --
> 
>   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".
> 
> 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/?
> 
> 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/)
> 
> 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/
> 
> 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/.
> 
>   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/.
> 
> s/ticket_age_add/ticket_age_add in the ticket/.
> 
>   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/
> 
>   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.
> 
> --
> 
>   -  Return its own extension in EncryptedExtensions, indicating that
>      it intends to process the early data.  
> 
> s/its own extension/an early_data extension/
> 
>   "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/
> 
>   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.
> 
> --
> 
>   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/.
> 
>   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
> 
> --
> 
>   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.
> 
> --
> 
>   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.
> 
> 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.
> 
>   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.
> 
> --
> 
>   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.
> 
>   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.
> 
>   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.
> 
> 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.
> 
>   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/.
> 
>   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.)
> 
> 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.
> 
> --
> 
>   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.
> 
> 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.
> 
> 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".
> 
>   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.  But "Mode" and "Handshake
> Context" don't seem to be defined terms.  It seems to me that a better
> specification is the annotations in the state diagrams in Appendix A,
> which note for each message that is sent what key applies to it.
> 
> Only after reading the text again do I realize that the "Handshake
> Context" column is listing what the handshake context *is* at various
> points in time.  My confusion connects with the need for a more formal
> definition of "handshake context".
> 
> 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.
> 
> --
> 
>    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)
> 
> 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:
> 
> --
> 
>   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))
> 
> 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.
> 
>   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.
> 
> 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.
> 
> --
> 
>   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/.
> 
> 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/.
> 
> s/they are/the server is/
> 
>   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:
> 
>   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."
> 
> 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.
> 
> 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/.
> 
> 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) 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.
> 
> 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.
> 
>   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?  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 appropriate application traffic key.
> 
> Is there a strict accounting of what messages are encrypted using
> which key?
> 
> 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.
> 
> Does "ticket value" mean the NewSessionTicket structure or the
> "ticket" field within it.  See issues regarding "ticket" for section
> 1.1.
> 
>   (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"?
> 
> 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"?
> 
> 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.
> 
> 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 ...".
> 
> 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.
> 
> 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)?  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.
> 
>   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".
> 
> Is it intended that the "ticket" field of NewSessionTicket will become
> the "identity" field of PskIdentity.OfferedPsks.PreSharedKeyExtension?
> 
>   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.
> 
> 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.
> 
> 4.6.3.  Key and IV Update
> 
> I think you want to promote the first paragraph before the data
> structure definition.
> 
> 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.
> 
>   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.
> 
>   When record protection has not yet been engaged, TLSPlaintext
>   structures are written directly onto the wire.
> 
> Better "... sent directly using the underlying transport protocol".
> 
> 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.
> 
> (assuming we rename "content" to "fragment")
> 
> Then replace the equation by:
> 
>   encrypted_record =
>          AEAD-Encrypt(endpoint_write_key, nonce, TLSInnerPlaintext)
> 
> --
> 
>   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.
> 
> Similarly, the decryption algorithm is better expressed by
> 
>      TLSInnerPlaintext =
>          AEAD-Decrypt(peer_write_key, nonce, encrypted_record)
> 
> --
> 
>   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.
> 
> 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.
> 
> --
> 
>   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.
> 
> --
> 
>   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.
> 
> 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.
> 
> --
> 
>   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.
> 
>   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.
> 
> 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?
> 
> 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?
> 
>   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.
> 
> 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?
> 
>   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.
> 
> (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.)
> 
>   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?
> 
>   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?]
> 
> 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.
> 
> 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.
> 
>   Messages are the concatenation of the indicated handshake messages,
> 
> s/Messages are/Messages is/!
> 
>   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:
> 
> --
> 
>   -  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.
> 
> 7.3.  Traffic Key Calculation
> 
> I think you want to title the section "Write Key and IV Calculation".
> 
> And you want to (again) de-genericize the text:
> 
>   The traffic keying material (*_write_key and *_write_iv) is
>   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
> 
> 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!
> 
> 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?
> 
> 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")?
> 
> 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".
> 
> 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.
> 
> --
> 
>   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.
> 
> --
> 
>   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.
> 
> 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.
> 
> --
> 
>   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.
> 
> 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.
> 
> 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.
> 
> --
> 
>   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.
> 
> 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.
> 
>   -  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.
> 
> 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.
> 
> 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.
> 
> 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.
> 
> 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?
> 
> 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?
> 
> 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 [...]
> 
> 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".
> 
> 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.
> 
>   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.
> 
> --
> 
>   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").
> 
> 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".
> 
> 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]
> 
> 
> _______________________________________________
> Gen-art mailing list
> Gen-art@ietf.org
> https://www.ietf.org/mailman/listinfo/gen-art