Hi David,
Thanks for taking the time to read this. Some responses below.
> 1-Byte Length of Vectors: One edge-case that is implied and not
> discussed in the Presentation Language section is: "do I need to write
> the length of a vector<0...255>?". It seems logical that it will
> always be 1 byte and that it might be an implicit length. It would be
> nice to have this edge case mentioned or say something along the lines
> "there are no exceptions when encoding the length of variable-length
> vectors".
I'm not sure I understand your point here about why this is an edge
case. In order for the encoding to be unambiguous, every variable
length vectory must always be prefixed by the length, even if in
a particular message it is zero length. This section seems pretty
clear to me. Note that unconditional statement about encoding
followed by the last sentence about empty vectors.
Variable-length vectors are defined by specifying a subrange of legal
lengths, inclusively, using the notation <floor..ceiling>. When
these are encoded, the actual length precedes the vector's contents
in the byte stream. The length will be in the form of a number
consuming as many bytes as required to hold the vector's specified
maximum (ceiling) length. A variable-length vector with an actual
length field of zero is referred to as an empty vector.
Happy to look at making this clearer, but I'd rather not just
add a clarifying sentence.
> Lack of repetition: The intro diagram tells you what messages are
> encrypted with what key. But after that, no reminders are present. For
> example, look at the Finished message. How do you know that this has
> to be encrypted with the handshake key? It is not written in the
> Finished message section, it is not written in the authentication
> messages parent section and it is not written in the handshake
> protocol parent-parent section either. Only in the overview section.
I'll see what I can do about this. With that said, I'm trying to remove
unnecessary duplication, so it's a compromise.
> How to define application data in an application data message: It is
> not clear how "real" application data should appear in an application
> data message. Encrypted handshakes and alerts are clearly defined, but
> it takes some time to understand that "real" application data is
> written -- without structure or metadata -- into the content field of
> a TLSInnerPlaintext message. The current definition is "content: The
> cleartext of TLSPlaintext.fragment". I think a better one would be
> "content: the byte encoding of a handshake or alert message, or the
> raw bytes of the application's data to send".
Yeah, this seems like a good change. Feel free to send a PR or I can just
do it.
> Unencrypted Application Data: Does it say somewhere in the
> specification that real application data should never appear in a
> TLSPlaintext message? At the moment the application_data content type
> is present in the declaration of a TLSPlaintext structure, see the
> record layer section
I'm not sure I understand what you're saying here.
> What alert to use: Sometimes it is not mentioned what level of alert
> should be used. For example: "Because TLS 1.3 forbids renegotiation,
> if a server receives a ClientHello at any other time, it MUST
> terminate the connection". I'm guessing it should be a fatal alert,
> but this is open to interpretation.
Hmm... I think this is pretty clear in 6.2.
Whenever an implementation encounters a fatal error condition, it
SHOULD send an appropriate fatal alert and MUST close the connection
without sending or receiving any additional data. In the rest of
this specification, the phrase "{terminate the connection, abort the
handshake}" is used without a specific alert means that the
implementation SHOULD send the alert indicated by the descriptions
> Why force the signature algorithm extension: If the client does not
> send a signature algorithm extension, why would the server abort the
> handshake? It would make more sense to continue with the server's
> prefered certificate. Especially since currently, most webservers only
> possess one kind of certificate anyway.
The signature algorithms extension also indicates what to sign with,
and that's not completely determined by the certificate (e.g., with
RSA).
> Resumption_secret: The Key Schedule section mentions a
> resumption_secret but nowhere in the spec does this keyword
> re-appear. It should be mentioned somewhere that this is the secret to
> be used as PSK in a future resumption handshake.
Good catch. This is actually called the resumption master
secret. I'll clean this up.
> Auth_modes: there is a single mention of "auth_modes" in the
> Pre-Shared Key Extension section. No definition of this variable is to
> be found.
Yeah, this is a holdover from when this field exists. I fixed that.
> Computing Finished keys: The section on how to compute Finished keys
> should be in the cryptographic computation section in my opinion. It
> should probably be added to the key schedule diagram as well.
This made the key schedule too hard to read (I note you complain
about that below) so I'm reluctant to add it. I'd be interested
in other people's opinions about whether these computations would
be better in this section.
> Computation of the Binders: From the relevant section: "Each entry in
> the binders list is computed as an HMAC over the portion of the
> ClientHello up to and including the PreSharedKeyExtension.identities
> field. That is, it includes all of the ClientHello but not the binder
> list itself. The length fields for the message (including the overall
> length, the length of the extensions block, and the length of the
> “pre_shared_key” extension) are all set as if the binder were
> present". There are several things that are unclear to me and I think
> they should be mentioned here: the fake binder should be of size
> hash.size, it should also say that the HMAC is computed over the
> handshake message containing the client hello, not only the client
> hello structure.
I'll see what I can do here.
> What binder key to use: It is not clear which binder are to be used
> when doing a session resumption. The previous handshake one or the new
> handshake one? The confusion comes from the fact that the old
> handshake's resumption_secret/PSK is to be used in the new handshake,
> it could be thought that the old handshake's binder key should be the
> one used in the new handshake as well. I think it would help if this
> resumption_secret is called something like
> next_handshake.resumption_secret. Or make it clearer from which key
> schedule the binder key comes from.
I'm not quite following. It's not possible to compute the RMS
at this point in the handshake. The only PSK you could possibly
have is the one from the previous handshake.
> PSK and Cipher Suite: I think the following fact should be emphasized
> in more places: the client needs to advertise the single cipher suite
> they previously used when doing session resumption. It could be added
> to the cipher_suite definition of the client hello section as well as
> in the overview of session resumption.
Thanks. I can add something.
> Implicit Lengths in The Presentation Language: nested structures +
> implicit lengths = trouble. It's easy to forget parsing/writing all
> the different "sizes" when they are not clearly stated in the
> spec. Why not just having a length field in front of these
> variable-length vectors? It would add a bit of overhead but would make
> things clearer in my opinion.
Can you point to the specific structures you are concerned with
here? I think in almost every case we do have length fields. Actually,
a common complaint is we have too many redundant length fields.
> No Help For The Implementer: If you look at the Noise Protocol
> Framework, you can clearly see that the specifications were written
> with implementers in mind: several states, and objects are
> defined. For example, one way of implementing TLS is to have some sort
> of complex state that will remember what state you are in the
> handshake, what is the cryptographic context, etc... here the
> implementer has to decide everything for himself.
Most IETF specs leave the choice of how to do things up
to the implementor. As someone who has implemented a lot of
specs, I actually find it pretty frustrating when the author
gets all prescriptive about implementation. If there are
points that are unclear, I think it's fine to make them clear,
but I'd rather stick to the PDUs.
> Whitelist Of State Transitions: There is nothing about whitelisting
> state transitions. In my opinion there should be a list of allowed
> state transitions that the implementer could use. I made one here, I
> suspect it is still missing some states.
I'm going to add a state diagram in -19. This link seems like
useful input. Thanks.
> How to parse extensions: I think that IANA's consideration section's
> table would be useful to have in the earlier Extension section
> (probably only the extensions included in the RFC should be
> listed). Another useful pragraph: how to parse these extensions,
> making sure that different messages come with their own whitelist of
> extensions.
Good point. I'll make this change.
> Language <-> Bytes Translation: There are no real introduction/no
> clear section about this. I understand that everything is in the
> Presentation Language section but for someone who discovers the
> specification it takes some time to find out that this section is
> about both the presentation language and its byte encoding. A better
> introduction in this section as well as an emphasis on the byte
> representation in each of the subsections of this section would in my
> opinion benefit the whole document.
I'm not quite sure I understand what you're looking for here. Can
you send a PR?
> Asymmetrical MUSTs: some "MUST" keywords are not symmetrical. What is
> the point of them? For example, the spec says that as a client, you
> MUST use a unique 0x0303 value in the legacy_version field of the
> client hello. But if you do not, there are no problems from the server
> side in the specification and the client hello will still be well
> received. (Is this maybe for forward compatibility?)
Amongst other things. MUST do X but must allow X and Y is pretty
common in IETF spec.
> And finally, here are some minor nitpicks:
>
> PSK extension: debatable, but I would rather have the PSK section at
> the very end of the Extensions section as it MUST be the final
> extension of a client hello.
Yeah, I can do that.
> No Indication Of Encryption: This is debatable, but I find it
> inelegant that there are no indication in a record that it is
> encrypted. The implementation is supposed to keep track of it.
Actually, at present everything with first byte == application_data
is encrypted and everything else is not.
> What PSK mode does a server support: I think the server should
> indicate what mode (psk_dh_ke or psk_ke) it supports in a
> server_hello.psk_mode extension or in the new_session_ticket
> message. Like that the client would know if it has to send a key_share
> or not in the resumption. Right now, the only indication of a
> psk_dh_ke is that the server will send a key_share even when presented
> with a PSK. this is debatable.
This was a pretty explicit decision to avoid redundancy, so I think
we have WG consensus here.
> The Key Derivation: The key derivation section is the least clear
> section in this specification. The diagrams kind of help, but are
> limited by the RFC format... I have nothing else to say here, the
> whole section is confusing to say the least. No solution offered :o)
Yeah, not too sure what to do.
Thanks,
-Ekr
On Wed, Dec 14, 2016 at 3:38 PM, David Wong <davidwong.cry...@gmail.com>
wrote:
> Hello! I read the current specification and I have some feedback on it. I
> hope this can be useful. Sorry for the length or if I point to obvious
> things. Here is a link as well for readability:
> https://gist.github.com/mimoo/b6c64a133cd85da53f2f744dcdb39839
>
> 1.
>
> *1-Byte Length of Vectors*: One edge-case that is implied and not
> discussed in the Presentation Language section
> <https://tlswg.github.io/tls13-spec/#presentation-language> is: "do I
> need to write the length of a vector<0...255>?". It seems logical that it
> will always be 1 byte and that it might be an implicit length. It would be
> nice to have this edge case mentioned or say something along the lines
> "there are no exceptions when encoding the length of variable-length
> vectors".
> 2.
>
> *Lack of repetition*: The intro diagram
> <https://tlswg.github.io/tls13-spec/#rfc.section.2> tells you what
> messages are encrypted with what key. But after that, no reminders are
> present. For example, look at the Finished message. How do you know that
> this has to be encrypted with the handshake key? It is not written in the
> Finished
> message <https://tlswg.github.io/tls13-spec/#rfc.section.4.4.3>
> section, it is not written in the authentication messages
> <https://tlswg.github.io/tls13-spec/#rfc.section.4.4> parent section
> and it is not written in the handshake protocol
> <https://tlswg.github.io/tls13-spec/#rfc.section.4> parent-parent
> section either. Only in the overview section.
> 3.
>
> *How to define application data in an application data message*: It is
> not clear how "real" application data should appear in an application data
> message. Encrypted handshakes and alerts are clearly defined, but it takes
> some time to understand that "real" application data is written -- without
> structure or metadata -- into the content field of a TLSInnerPlaintext
> message. The current definition is "content: The cleartext of
> TLSPlaintext.fragment". I think a better one would be "content: the byte
> encoding of a handshake or alert message, or the raw bytes of the
> application's data to send".
> 4.
>
> *Unencrypted Application Data*: Does it say somewhere in the
> specification that real application data should never appear in a
> TLSPlaintext message? At the moment the application_data content type
> is present in the declaration of a TLSPlaintext structure, see the
> record layer section <https://tlswg.github.io/tls13-spec/#record-layer>
> 5.
>
> *What alert to use*: Sometimes it is not mentioned what level of alert
> should be used. For example: "Because TLS 1.3 forbids renegotiation, if a
> server receives a ClientHello at any other time, it MUST terminate the
> connection". I'm guessing it should be a fatal alert, but this is open to
> interpretation.
> 6.
>
> *Why force the signature algorithm extension*: If the client does not
> send a signature algorithm extension, why would the server abort the
> handshake? It would make more sense to continue with the server's prefered
> certificate. Especially since currently, most webservers only possess one
> kind of certificate anyway.
> 7.
>
> *Resumption_secret*: The Key Schedule
> <https://tlswg.github.io/tls13-spec/#rfc.section.7.1> section mentions
> a resumption_secret but nowhere in the spec does this keyword
> re-appear. It should be mentioned somewhere that this is the secret to be
> used as PSK in a future resumption handshake.
> 8.
>
> *Auth_modes*: there is a single mention of "auth_modes" in the Pre-Shared
> Key Extension
> <https://tlswg.github.io/tls13-spec/#pre-shared-key-extension>
> section. No definition of this variable is to be found.
> 9.
>
> *Computing Finished keys*: The section on how to compute Finished keys
> <https://tlswg.github.io/tls13-spec/#rfc.section.4.4.3> should be in
> the cryptographic computation
> <https://tlswg.github.io/tls13-spec/#cryptographic-computations>
> section in my opinion. It should probably be added to the key schedule
> diagram as well.
> 10.
>
> *Computation of the Binders*: From the relevant section
> <https://tlswg.github.io/tls13-spec/#psk-binder>: "*Each entry in the
> binders list is computed as an HMAC over the portion of the ClientHello up
> to and including the PreSharedKeyExtension.identities field. That is, it
> includes all of the ClientHello but not the binder list itself. The length
> fields for the message (including the overall length, the length of the
> extensions block, and the length of the “pre_shared_key” extension) are all
> set as if the binder were present*". There are several things that are
> unclear to me and I think they should be mentioned here: the fake binder
> should be of size hash.size, it should also say that the HMAC is
> computed over the handshake message containing the client hello, not only
> the client hello structure.
> 11.
>
> *What binder key to use*: It is not clear which binder are to be used
> when doing a session resumption. The previous handshake one or the new
> handshake one? The confusion comes from the fact that the old handshake's
> resumption_secret/PSK is to be used in the new handshake, it could be
> thought that the old handshake's binder key should be the one used in
> the new handshake as well. I think it would help if this
> resumption_secret is called something like next_handshake.resumption_
> secret. Or make it clearer from which key schedule the binder key
> comes from.
> 12.
>
> *PSK and Cipher Suite*: I think the following fact should be
> emphasized in more places: the client needs to advertise the single cipher
> suite they previously used when doing session resumption. It could be added
> to the cipher_suite definition of the client hello section
> <https://tlswg.github.io/tls13-spec/#client-hello> as well as in the
> overview
> of session resumption
> <https://tlswg.github.io/tls13-spec/#resumption-and-psk>.
> 13.
>
> *Implicit Lengths in The Presentation Language*: nested structures +
> implicit lengths = trouble. It's easy to forget parsing/writing all
> the different "sizes" when they are not clearly stated in the spec. Why not
> just having a length field in front of these variable-length vectors? It
> would add a bit of overhead but would make things clearer in my opinion.
> 14.
>
> *No Help For The Implementer*: If you look at the Noise Protocol
> Framework <http://www.noiseprotocol.org/noise.html>, you can clearly
> see that the specifications were written with implementers in mind: several
> states, and objects are defined. For example, one way of implementing TLS
> is to have some sort of complex state that will remember what state you are
> in the handshake, what is the cryptographic context, etc... here the
> implementer has to decide everything for himself.
> 15.
>
> *Whitelist Of State Transitions*: There is nothing about whitelisting
> state transitions. In my opinion there should be a list of allowed state
> transitions that the implementer could use. I made one here, I suspect
> it is still missing some states
> <https://gist.github.com/mimoo/779dcf8c44d80a2a34a1a2f2ed620711>.
> 16.
>
> *How to parse extensions*: I think that IANA's consideration
> <https://tlswg.github.io/tls13-spec/#iana-considerations> section's
> table would be useful to have in the earlier Extension section
> <https://tlswg.github.io/tls13-spec/#extensions> (probably only the
> extensions included in the RFC should be listed). Another useful pragraph:
> how to parse these extensions, making sure that different messages come
> with their own whitelist of extensions.
> 17.
>
> *Language <-> Bytes Translation*: There are no real introduction/no
> clear section about this. I understand that everything is in the
> Presentation
> Language <https://tlswg.github.io/tls13-spec/#presentation-language>
> section but for someone who discovers the specification it takes some time
> to find out that this section is about both the presentation language and
> its byte encoding. A better introduction in this section as well as an
> emphasis on the byte representation in each of the subsections of this
> section would in my opinion benefit the whole document.
> 18.
>
> *Asymmetrical MUSTs*: some "MUST" keywords are not symmetrical. What
> is the point of them? For example, the spec says that as a client, you MUST
> use a unique 0x0303 value in the legacy_version field of the client
> hello <https://tlswg.github.io/tls13-spec/#client-hello>. But if you
> do not, there are no problems from the server side in the specification and
> the client hello will still be well received. (Is this maybe for forward
> compatibility?)
>
> And finally, here are some minor nitpicks:
>
> 1.
>
> *PSK extension*: *debatable*, but I would rather have the PSK section
> <https://tlswg.github.io/tls13-spec/#pre-shared-key-extension> at the
> very end of the Extensions section
> <https://tlswg.github.io/tls13-spec/#extensions> as it MUST be the
> final extension of a client hello.
> 2.
>
> *No Indication Of Encryption*: This is *debatable*, but I find it
> inelegant that there are no indication in a record that it is encrypted.
> The implementation is supposed to keep track of it.
> 3.
>
> *What PSK mode does a server support*: I think the server should
> indicate what mode (psk_dh_ke or psk_ke) it supports in a
> server_hello.psk_mode extension or in the new_session_ticket message.
> Like that the client would know if it has to send a key_share or not
> in the resumption. Right now, the only indication of a psk_dh_ke is
> that the server will send a key_share even when presented with a PSK. *this
> is debatable*.
> 4.
>
> *The Key Derivation*: The key derivation section is the least clear
> section in this specification. The diagrams kind of help, but are limited
> by the RFC format... I have nothing else to say here, the whole section is
> confusing to say the least. No solution offered :o)
>
>
> _______________________________________________
> TLS mailing list
> TLS@ietf.org
> https://www.ietf.org/mailman/listinfo/tls
>
>
_______________________________________________
TLS mailing list
TLS@ietf.org
https://www.ietf.org/mailman/listinfo/tls
