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

[Benjamin Kaduk <[e-mail]>]

On 04/11/2017 12:32 PM, Eric Rescorla wrote:
> > 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.
>

I guess I didn't consult back with what I had put in this mail when
preparing my editorial-changes pull request :)

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

(For the gallery, there were some tweaks in this area in #936)

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

It looks like this changed a little via #936 as well; I'm fine with your
followup change there.

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

That's probably fine; I expect there is some additional attack in there
where an attacker could force 1.0 if 1.1 would otherwise have been used,
but both of those are not in a great place right now, so we don't need
to try too hard to help them out.

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

Okay.

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

Yeah, I missed the bits I called out in #935 when I was doing my WGLC
review, but the two are related and can be handled together.

>
> > Should Alert.level be Alert.legacy_level?
>
> i think we went over this and decided not to.
>

There was a pull request from not-me, yes.  Though IIRC it only changed
the name locally when describing alerts, and was rejected on the grounds
that "we don't use the legacy_level version for this anywhere else in
the spec", which is a little bit of circular reasoning.  I'm okay with
leaving it as-is.

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

Well, maybe.  ContentType.invalid is the only one that I marked up in
red pen on my paper copy, but I can't certify that I compared the entire
list against the IANA registry.  I did look at the extensions registry
when preparing #936, though.

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

Yeah, I guess I snuck that fix into #936.  So much for keeping things
separate...

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

It looks like that was just by removing the note.  Is a channel binding
spec for 1.3 still a needed work item, then?

-Ben

>
> On Mon, Mar 27, 2017 at 11:23 PM, Kaduk, Ben <[e-mail]
> <mailto:[e-mail]>> wrote:
>
>     On 3/13/17, 12:30, "Sean Turner" <[e-mail]
>     <mailto:[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] <mailto:[e-mail]>
>     https://www.ietf.org/mailman/listinfo/tls
>     <https://urldefense.proofpoint.com/v2/url?u=https-3A__www.ietf.org_mailman_listinfo_tls&d=DwMFaQ&c=96ZbZZcaMF4w0F4jpN6LZg&r=sssDLkeEEBWNIXmTsdpw8TZ3tAJx-Job4p1unc7rOhM&m=zDb_mbfXWxowypc9E4E6zZZ_lXTab2DcV9qm--twWoM&s=WD0bv4QMm5OHI8RqolDFScW-e1jMk-YNzkVmbC4cmEw&e=>
>
>