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 PROTECTED]>
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 protected]
http://www.ietf.org/mailman/listinfo/gen-art
