Re: [TLS] WGLC: draft-ietf-tls-tls13-19

[Eric Rescorla <[e-mail]>]

> It was already mentioned that the “major differences from TLS 1.2”
> section should not be a changelog, but I agree with that.

Yes, this is on my plate.


> Should Figure 4 (“message flow for a zero round trip handshake”)
> include a “+ early_data” for the server’s flight?  (The legend for
> Figure 4 also lacks the explanation for the ‘+’ symbol.)

I see you fixed this.


> The language on page 30 is perhaps unclear:
>
>    Because TLS 1.3 forbids renegotiation, if a server receives a
>    ClientHello at any other time, it MUST terminate the connection.
>
> Is that any TLS server, or just one that has negotiated and is using
> TLS 1.3?

The latter. Adjusted.


> In the description of legacy_compression_methods on page 31, we make
> restrictions on “every TLS 1.3 ClientHello”, but do not say how such
> things are identified.  (Hmm, maybe we also do so elsewhere in the
> document, too, now that I search for where) we explicitly define what
> a client “considered to be attempting to negotiate using this
> specification (i.e., a TLS 1.3 ClientHEello) on page 87, as
> supported_versions including 1.3.  Which, is maybe not the most
> future-proof thing.

I think I feel OK about this.


> The description of version negotiation (to populate
> ServerHello.version) on page 32 seems to leave undefined what the
> server should do when receiving a ClientHello that does not contain a
> supported_versions extension.  (Also, I don’t think
> “ClientHello.supported_versions extension” is a well-defined syntax.)

I think this clear in the section on Supported Versions.


> When covering the server_random version downgrade sentinels, we do not
> mention what is to be done when downgrading to TLS 1.0, which I
> thought was still a permitted version by this spec.

Interesting point. I'm trying to remember why we did things this way.
I am tempted to just say "1.1 or 1.0". Thoughts?


> It’s a little odd that we list in enum ExtensionType on page 35 a
> strict subset of the extensions enumerated in the table on the
> following pages.

This got fixed in PR#936.



> Do we want to add some commentary about the extant SHA1 collisions
> when we say that {rsa_pkcs1,dsa,ecdsa}_sha1 are only SHOULD NOT?

Nah.


> I’ll note that we define 256 private use ECDHE group code points but
> only four such FFDHE group code points.  Probably fine, but a bit
> surprising.

Too late now!!


> Should we forbid duplicate entries in
> PreSharedKeyExtension.identities?

I don't think that's necessary.


> Conversely, we might want to explicitly say that duplicate
> OIDFilter.certificate_extension_oid fields should be expected in
> OIDFilterExtensions, to enable the case where multiple values must be
> present.  Or is that supposed to work by concatenating(?) the multiple
> values’ DER encodings in the certificate_extension_values field?

Yeah, I read this text as saying that those all go in the same
DER, not that there can be multiple copies


> I’ll call out for Russ’s attention at the end of Section 4.4.3 where
> we say that “implementations MUST NOT combine external PSKs with
> certificate-based authentication.”  Is there any reason not to qualify
> that as some sort of “don’t’ do it until it’s defined”?

I'm actually going to move and modify this section per your issue #935.


> Should Alert.level be Alert.legacy_level?

i think we went over this and decided not to.


> Appendix B has a claim that “values listed as _RESERVED were used in
> previous versions of TLS and are listed here for completeness”, though
> that is not exactly true, e.g., for ContentType.invalid_RESERVED(0)

I see you fixed this as well.


> Section C.3 notes that “Certificates should always be verified to
> ensure proper signing by a trusted Certificate Authority”, which does
> not use RFC 2119 language, but might be seen as in conflict with
> opportunistic encryption in some circumstances.  I don’t object to
> this text, but it seems worth mentioning.

I think the "Absent a specific..."


> Page 113 still has the “[[NOTE: TLS 1.3 needs a new channel binding
> definition that has not yet been defined.]]”, which should not make it
> into the final spec!

Fixed.


On Mon, Mar 27, 2017 at 11:23 PM, Kaduk, Ben <[e-mail]> wrote:

> On 3/13/17, 12:30, "Sean Turner" <[e-mail]> wrote:
>
>     This is a working group last call announcement for
> draft-ietf-tls-tls13-19, to run through March 27.  Please send your reviews
> to the list as soon as possible so we can prepare for any discussion of
> open issues at IETF 98 in Chicago.
>
> As the price of running the WGLC right during the meeting lead-up, my
> review comes in at the last minute.
>
> Generally, it is in good shape.  I think I still owe some text about what
> we aim for and expect to achieve with respect to side channel resistance,
> though at this point it may be too late to get that text in :(
>
> The following is basically a laundry list of the minor issues; I will send
> editorial notes under separate cover, probably as a pull request.
>
> It was already mentioned that the “major differences from TLS 1.2” section
> should not be a changelog, but I agree with that.
>
> Should Figure 4 (“message flow for a zero round trip handshake”) include a
> “+ early_data” for the server’s flight?  (The legend for Figure 4 also
> lacks the explanation for the ‘+’ symbol.)
>
> The language on page 30 is perhaps unclear:
>
>    Because TLS 1.3 forbids renegotiation, if a server receives a
>    ClientHello at any other time, it MUST terminate the connection.
>
> Is that any TLS server, or just one that has negotiated and is using TLS
> 1.3?
>
> In the description of legacy_compression_methods on page 31, we make
> restrictions on “every TLS 1.3 ClientHello”, but do not say how such things
> are identified.  (Hmm, maybe we also do so elsewhere in the document, too,
> now that I search for where)  we explicitly define what a client
> “considered to be attempting to negotiate using this specification (i.e., a
> TLS 1.3 ClientHEello) on page 87, as supported_versions including 1.3.
> Which, is maybe not the most future-proof thing.
>
> The description of version negotiation (to populate ServerHello.version)
> on page 32 seems to leave undefined what the server should do when
> receiving a ClientHello that does not contain a supported_versions
> extension.  (Also, I don’t think “ClientHello.supported_versions
> extension” is a well-defined syntax.)
>
> When covering the server_random version downgrade sentinels, we do not
> mention what is to be done when downgrading to TLS 1.0, which I thought was
> still a permitted version by this spec.
>
> It’s a little odd that we list in enum ExtensionType on page 35 a strict
> subset of the extensions enumerated in the table on the following pages.
>
> Do we want to add some commentary about the extant SHA1 collisions when we
> say that {rsa_pkcs1,dsa,ecdsa}_sha1 are only SHOULD NOT?
>
> I’ll note that we define 256 private use ECDHE group code points but only
> four such FFDHE group code points.  Probably fine, but a bit surprising.
>
> Should we forbid duplicate entries in PreSharedKeyExtension.identities?
>
> Conversely, we might want to explicitly say that duplicate
> OIDFilter.certificate_extension_oid fields should be expected in
> OIDFilterExtensions, to enable the case where multiple values must be
> present.  Or is that supposed to work by concatenating(?) the multiple
> values’ DER encodings in the certificate_extension_values field?
>
> I’ll call out for Russ’s attention at the end of Section 4.4.3 where we
> say that “implementations MUST NOT combine external PSKs with
> certificate-based authentication.”  Is there any reason not to qualify that
> as some sort of “don’t’ do it until it’s defined”?
>
> Should Alert.level be Alert.legacy_level?
>
> The editors copy has already removed the reference to RFC 4507, which is
> obsoleted by RFC 5077 (and was not cited anywhere, anyway).
>
> Appendix B has a claim that “values listed as _RESERVED were used in
> previous versions of TLS and are listed here for completeness”, though that
> is not exactly true, e.g., for ContentType.invalid_RESERVED(0)
>
> Section C.3 notes that “Certificates should always be verified to ensure
> proper signing by a trusted Certificate Authority”, which does not use RFC
> 2119 language, but might be seen as in conflict with opportunistic
> encryption in some circumstances.  I don’t object to this text, but it
> seems worth mentioning.
>
> Page 113 still has the “[[NOTE: TLS 1.3 needs a new channel binding
> definition that has not yet been defined.]]”, which should not make it into
> the final spec!
>
> -Ben
>
> _______________________________________________
> TLS mailing list
> [e-mail]
> https://www.ietf.org/mailman/listinfo/tls
>