Telechat Review of draft-ietf-tls-record-limit-02

Request Review of draft-ietf-tls-record-limit
Requested rev. no specific revision (document currently at 03)
Type Telechat Review
Team Security Area Directorate (secdir)
Deadline 2018-03-06
Requested 2018-02-16
Authors Martin Thomson
Draft last updated 2018-03-01
Completed reviews Opsdir Telechat review of -02 by √Čric Vyncke (diff)
Secdir Telechat review of -02 by Alan DeKok (diff)
Genart Telechat review of -02 by Francis Dupont (diff)
Assignment Reviewer Alan DeKok 
State Completed
Review review-ietf-tls-record-limit-02-secdir-telechat-dekok-2018-03-01
Reviewed rev. 02 (document currently at 03)
Review result Has Nits
Review completed: 2018-03-01


  I have reviewed this document as part of the security directorate's ongoing effort to review all IETF documents being processed by the IESG. These comments were written primarily for the benefit of the security area directors. Document editors and WG chairs should treat these comments just like any other last call comments.

  The summary of the review is ready with nits.

4.  The "record_size_limit" Extension
	... an endpoint
   MUST NOT send a value higher than the protocol-defined maximum record
   size ...

Comment: That's good, so later we have:

   The record size limit can interact with the maximum transmission unit
   (MTU) in DTLS, but it is a separate and independent constraint on
   record size.

Nit:  perhaps say that this is an *additional* constraint.  Which is (to me) a clearer indication that both constraints must be matched.  "independent" constraints sound like not only they vary independently, but that they can be applied independently (i.e. separately).  Saying "additional" constraint is a clearer indication that both constraints must be applied at the same time.

   In particular, it is not appropriate to use the record
   size limit in place of path MTU detection. 

Q: How would that be done?  I don't mean that the document needs to explain how to do something wrong.  I mean that it would be good to explain the misunderstanding which would lead to using record size limit in place of path MTU detection.

  e.g. "the reception of an illegal_parameter error on a session gives no information about the allowed MTU size"

   The record size limit is
   a fixed property of an endpoint that is set during the handshake and
   fixed thereafter.  In comparison, the MTU is determined by the
   network path and can change dynamically over time.

Comment:  it would be good to give guidance on what to do here, and what happens in error cases.

  e.g. should the record size limit to be set at the start of a session to the *smallest expected MTU*?  If so, what are the side effects?

  Or what about this - the MTU is large at the start of a session, and a record size limit is negotiated which matches that MTU.  At some later time, the MTU changes to a lower value than the negotiated record size limit.

  What happens then?  The application may receive an ICMP message indicating destination unreachable, with a code indicating fragmentation needed.  If Don't Fragment (DF) is not set.. the packet cannot be sent.

  Should the session be closed?  If not, why not?  If so, should the application keep track of minimum MTU across multiple sessions, and negotiate a value of record size limit which is more likely to work?

  It would be good to have guidance for edge / error conditions.  They tend to be a source of network problems and interoperability issues.

5.  Deprecating "max_fragment_length"
	...A server that supports the "record_size_limit"
   extension MUST ignore and "max_fragment_length"

Nit: this is probably "any" instead of "and".

  7.  IANA Considerations
   This document registers the "record_size_limit" extension in the TLS
   "ExtensionType Values" registry ...

   In the same registry, the "max_fragment_length" [[has been|will be]]
   changed to a status of not recommended.

Comment: the registry has no "status" column.

  It may be useful to update that registry to add a "deprecated" note, perhaps as is done with the ICMP parameters registry:

  In which case the "max_fragment_length" entry should be changed to:

Value				1
Extension name		max_fragment_length (deprecated)
Reference			RFC6066, draft-ietf-tls-record-limit