Document: draft-ietf-tls-rfc4346-bis-09.txt Reviewer: Miguel Garcia Review Date: 2008-02-25 IETF LC End Date: 2008-02-27 IESG Telechat date: (if known) Summary: The draft is ready for publication as a proposed standard RFC. Comments: The document is well written, as one could expect from a new iteration of an existing document. I have very minor comments and suggestions, mainly trying to get clarifications for a reader who hasn't been in contact with TLS in the past. Take them at your own discretion. - expand MD5, SHA-1, and PRF at its first occurrence (Section 1.2) - Section 1.2 says: - Support for the SSLv2 backward-compatible hello is now a MAY, not a SHOULD, with sending it a SHOULD not. Support will probably become a SHOULD NOT in the future. I believe the 'not' in "SHOULD not" should be capitalized: "SHOULD NOT". - Section 4.5.1, the last 'struct'. The 'case banana:' should be indented, i.e., aligned with 'case orange:' struct { select (VariantTag) { /* value of selector is implicit */ case apple: V1; /* VariantBody, tag = apple */ case orange: ---> case banana: V2; /* VariantBody, tag = orange or banana */ } variant_body; /* optional label on variant */ } VariantRecord; - Section 6, second paragraph, says Four record protocol clients are described in this document: the handshake protocol, the alert protocol, the change cipher spec protocol, and the application data protocol. The question is what is a 'client' in the context? Is it related to the client/server architecture of most Internet protocols? I think it refers to a 'customer' of the record protocol, i.e., another protocol that communicates with the record protocol via some API. Well, it would help if the term 'client' is replaced by something less ambiguous, perhaps "Four protocols that use the TLS record protocol are described ..." or something like that. - Same second paragraph in Section 6 says: In order to allow extension of the TLS protocol, additional record types can be supported by the record protocol I think the term 'record type' hasn't be used so far, so I have no idea what it is. I suspect the text wants to say that "additional protocols that use the TLS record protocol can be supported" and the way to do it is by defining a new record type value, which is something defined elsewhere. Perhaps there is a mapping between 'record types' and 'protocol clients', but it is not written in the draft. - Following with the same paragraph in Section 6: New record type values are assigned by IANA as described in Section 12. But then, I went to the IANA section 12 and I didn't find anything that is related to "record type" or "record protocol". So I am lost. - Following up the previous comment. May I suggest to add subsections in the IANA considerations section, so that each registry gets a subsection number? Essentially, each bullet point in Section 12 should be promoted to its own subsection. Then the main text should refer to, e.g., "Section 12.4" rather than the general Section 12. This will avoid errors like the previous one. - More on Section 6, now forth paragraph: Note that because the type and length of a record are not protected by encryption, care SHOULD be taken to minimize the value of traffic analysis of these values. I have no idea how to implement the 'SHOULD'. I don't know how to implement 'care'. I don't even understand if this is an action for the TLS implementor, for the implementor of applications that use TLS, or for the human using all of them. Please explain what is needed for a TLS implementation, and cases where that makes this a SHOULD and not a MUST. - Section 6.1, page 17 contains the following enum { null, hmac_md5, hmac_sha, hmac_sha256, hmac_sha384, hmac_sha512} MACAlgorithm; /* The use of "sha" above is historical and denotes SHA-1 */ I didn't find "sha" above. Does it refer to "hmac_sha" ? - Section 7.2.1 got me very confused: "Unless some other fatal alert has been transmitted, each party...." The text implies a classification of alerts. At least, there are 'fatal alerts' some presumably 'non-fatal'. Well, after some digging and initial confusion, I found that Section 7.2 has one pseudo-code classifying the alerts: enum { warning(1), fatal(2), (255) } AlertLevel; Since this is an important aspect for the protocol, I would suggest to add one paragraph of text in Section 7.2 describing in human-readable text the alert classification. The text should also describe the implications of receiving a 'warning' alert versus a 'fatal' alert. - Section 7.3, page 34 says: The actual key exchange uses up to four messages: the server certificate, the server key exchange, the client certificate, and the client key exchange. I got confused here too. I don't know if the 'actual key exchange' is part or perhaps piggybacked to the hello messages described in the previous paragraph, or something else. Furthermore, I tried to map these four messages to the messages presented later in Figure 1, and I also failed. There isn't 'Server certificate', but 'ServerHello', for example. What about 'client certificate'? I suspect the terminology is not used consistently across the document, what makes an excellent recipe for failure to understanding. - It is a pity that Figures 1 and 2 are split in two pages. Please try to make them rendered each one in a single page. - Section 7.4.1.1 says: Hello request is a simple notification that the client should begin the negotiation process anew by sending a client hello message when convenient. The sentence does not make sense around "process anew by sending". Please clarify the sentence. - Section 7.4.1.1. says: This message may be ignored by the client if it does not wish to renegotiate a session, or the client may, if it wishes, respond with a no_renegotiation alert. s/may/MAY - Suggestion: since we don't have many formatting tricks in text document, it would be nice to distinguish the names of messages from the rest. You could, for example, capitalize the name of messages: Client Hello, Hello Request, etc. - Section 7.4.1.1 says: Note: This message MUST NOT be included in the message hashes that are maintained throughout the handshake and used in the finished messages and the certificate verify message. It is a good practice not to include normative text in notes. If it is a note, it should be informative in nature. - Section 7.4.1.2, page 40: extensions Clients MAY request extended functionality from servers by sending data in the extensions Here the new "extensions" field contains a list of extensions. The actual "Extension" format is defined in Section 7.4.1.4. There is a missing dot (.) before "Here". - Section 7.4.1.4, page 43: this protocol between new features and existing features which may result in a significant reduction in overall security, The following s/,/. - Section 9: Add a reference to the TLS_RSA_WITH_AES_128_CBC_SHA cipher suite. - Interesting... the document does not contain an "Authors" section.