Subject: [Gen-art] Gen-ART review of draft-ietf-tls-rfc4346-bis-09.txt

From: Miguel Garcia <[email-masked]>
Date: Tue, 26 Feb 2008 12:53:42 +0200

I have been selected as the General Area Review Team (Gen-ART)
reviewer for this draft (for background on Gen-ART, please see
http://www.alvestrand.no/ietf/gen/art/gen-art-FAQ.html).

Please resolve these comments along with any other Last Call comments
you may receive.

Document: draft-ietf-tls-rfc4346-bis-09.txt
Reviewer: Miguel Garcia <[email-masked]>
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.

-- 
Miguel A. Garcia           tel:+358-50-4804586
Nokia Siemens Networks     Espoo, Finland

_______________________________________________
Gen-art mailing list
[email-masked]
http://www.ietf.org/mailman/listinfo/gen-art