Document: draft-ietf-ipsecme-ikev2bis-08.txt Reviewer: Elwyn Davies Review Date: 18 March 2010 IETF LC End Date: 18 March 2010 IESG Telechat date: (if known) - Summary: Not ready. The document contains a lot of minor niggles and nits plus a major item that I am not sure the IETF should support: this is the removal of all mention of mandatory to implement security suites from the document. I appreciate the difficulty of keeping up to the minute, but it seems to me that this is outweighed by the difficulty of guaranteeing interoperability. If the security landscape is so unstable, we have a bigger problem perhaps. Whether this change is acceptable to the IAB, the IESG and the wider IETF is not something I can resolve. One niggle that I felt represented a rather lackadaisical approach, was the retaining of the publication date of RFC 4306 as a frozen point in time for the purpose of documenting the state of the world as regards lists of configurable lists. I would have preferred the losts to be up to date at the publication of *this* RFC. There are also a number of relatively minor points where items and processes are explicitly not fully specified. Thus could lead to annoying corners where implementations are totally interoperable (e.g., what is the complete set of 'terminators' forbidden in IP_FQDNs? What is an acceptable 'sort of' email address/NAI on EAP?) Finally there are a number of points where it is recommended that network traffic needs to be limited but no concrete guidelines are given. I think that some sort of suggestions for the parameters to be applied (e.g., time constants, number of repeats, backoff algorithm) would be desirable. Major issues: s3.3.4: The draft states that the list of mandatory to implement suites has been removed due to evolution going too fast. Is this acceptable? Minor issues: s1: At para 6 in s1 the draft states: > > The first request/response of an IKE session (IKE_SA_INIT) negotiates > > security parameters for the IKE SA, sends nonces, and sends Diffie- > > Hellman values. As a (not really) naive reader, I asked myself 'Why does this text suddenly mention that we need to send nonces and Diffie-Hellman values?' Looking back at the text so far I realized that the text has not introduced the techniques that it is going to use to establish the authenticated IKE channel etc. It is assumed that readers know that as soon as D-H is mentioned we are talking public key cryptography. A paragraph explaining the underlying ideas would be useful. s1.2, last para: > > Note that IKE_AUTH messages do not contain KEi/KEr or Ni/Nr payloads. > > Thus, the SA payloads in the IKE_AUTH exchange cannot contain > > Transform Type 4 (Diffie-Hellman Group) with any value other than > > NONE. Implementations SHOULD omit the whole transform substructure > > instead of sending value NONE. Does 'cannot contain' conflict with the 'SHOULD'? I am unclear what an implementer would do if s/he chose to do otherwise than omit the transform structure in the light of the previous statement. s2.1, last sentence: > > The retransmission policy for one-way messages is somewhat different > > from that for regular messages. Because no acknowledgement is ever > > sent, there is no reason to gratuitously retransmit one-way messages. > > Given that all these messages are errors, it makes sense to send them > > only once per "offending" packet, and only retransmit if further > > offending packets are received. Still, it also makes sense to limit > > retransmissions of such error messages. Does this need to be more precise? Some explicit policy for restricting retransmissions? Possibly in s2.21.4. s2.2: Maybe should mention that retransmissions MUST use the same Message ID. s2.3: Should there be some discussion of the interaction of rekeying of the IKE_SA and windows? Presumably a rekey message should not be actioned until all previous messages have been responded to. Likewise receiving a Message ID with a sequence number bigger than that in the rekey message should be very suspect! Should the INVALID_MESSAGE_ID notification be sent in this case (and before or after the rekey?) There might be some knock on into s2.8 where rekeying is discussed. And maybe into s2.25? s2.4, para 2: > > The INITIAL_CONTACT notification, if sent, MUST > > be in the first IKE_AUTH request or response, not as a separate > > exchange afterwards; receiving parties MAY ignore it in other > > messages. What should receiving parties do if they *do* receive it and *don't* ignore it? Since it 'MUST be sent in the first IKE_AUTH' receiving at any other time is a protocol error and should cause some response (like dropping the IKE_SA perhaps). s2.4, para 4: > > Note > > that this places requirements on the failure modes of an IKE > > endpoint. An implementation MUST NOT continue sending on any SA if > > some failure prevents it from receiving on all of the associated SAs. > > If Child SAs can fail independently from one another without the > > associated IKE SA being able to send a delete message, then they MUST > > be negotiated by separate IKE SAs. I am fairly certain that is impossible for any implementation to guarantee the MUST NOT here. How does it absolutely know that it isn't able to receive anything? Absence of activity might mean failure or idleness. I *think* this is not just talking about IKE SAs but includes the Child SAs. This appears to imply that the Child SAs all have to be usable bidirectionally and have keepalives on them? And that IKE should know about the keepalives. What is the second condition saying? Does this impose some constraints on the use of Child SA deletion.. like that the IKE SA must support Child SA deletion? or is it just saying that if the IKE SA has failed you can't leave some Child SAs active (as implied by the next para)? The more I think about this the more confused I become! s2.6, next to last para: > > The initiator should limit the number of cookie exchanges it > > tries before giving up. Does anything need to be said abut exponential back-off? ss1.7/2.8: The changes documented in s1.7 state: > > In 2.8, changed "Note that, when rekeying, the new Child SA MAY have > > different traffic selectors and algorithms than the old one" to "Note > > that, when rekeying, the new Child SA SHOULD NOT have different > > traffic selectors and algorithms than the old one". This refers to para 3 of s2.8. Is the SHOULD NOT just there to cope with existing implementations? This is a binary choice - either they are the same or they aren't. If MAY is no good, I can't see that allowing the opposite choice at all makes any sense. I would argue that this ought to be MUST NOT. s2.8.1, para 2: The use of 'lexicographical' in the comparison algorithm adds confusion IMO. It implies that there is some language related total ordering on the values stored in each octet that might or might not be the natural integer ordering. Presumably the intent is that corresponding octets should be treated as 8 bit unsigned integers (in network bit order?) and compared as such. s2.15, definition of signed octets (both cases): What 'Length' is intended? And how should it be represented as a string for concatenation? Are the definitions of Nonce[IR]Payload intended to show how Nonce[IR]Data are derived ? They are not otherwise used. Similarly xxxIDPayload? How many octets of what value make up 'RESERVED' (or is it just the literal text? s2.21.2, last sentence: How would the peer expect to demonstrate understanding of extended error notifications? Protocol version number? NOTIFY payload? s2.21.4: More requests to limit message rates without specific means. s2.23, para 7 and 8 (first bullet point): I understood from earlier that the first requirement (port 4500 and responding to source address/port) applied to *all* implementations and not just those specifically supporting NAT traversal. s2.23: Should there be a discussion of NAT64? s3.1, Major Version: This definition is not entirely consistent with the major version support discussion earlier in the document. It should probably talk about implementations that support this version of the specification or any subsequent version that uses s higher version number. Reference back to s2.5 would be useful. This applies to the Minor Version also. s3.3.5, Key Length attribute: > > o The Key Length attribute MUST NOT be used with transforms that use > > a fixed length key. This includes, e.g., ENCR_DES, ENCR_IDEA, and > > all the Type 2 (Pseudo-random function) and Type 3 (Integrity > > Algorithm) transforms specified in this document. It is > > recommended that future Type 2 or 3 transforms do not use this > > attribute. I think the recommended is a RECOMMENDED here. s3.5, Identification Field table: The class of 'terminators' is not fully defined. s3.5, IP_FQDN: The specification refers to IDNA - does IDNAbis have any impact here? s3.5, last para: > > Responder implementations should not attempt to > > verify that the contents actually conform to the exact syntax given > > in [MAILFORMAT], but instead should accept any reasonable-looking > > NAI. How do I verify conformance to this statement please? s3.6, next to last para: 'also MUST be capable of being configured to send and accept the two Hash and URL formats (with HTTP URLs).' ^^^^^^^^^^^^^^^^^^^^^^^^ Only one format appears to be defined in this section. Is there another somewhere else? s3.12: > > Writers of Internet-Drafts who wish to extend this protocol MUST > > define a Vendor ID payload to announce the ability to implement the > > extension in the Internet-Draft. It is expected that Internet-Drafts > > that gain acceptance and are standardized will be given "magic > > numbers" out of the Future Use range by IANA, and the requirement to > > use a Vendor ID will go away. > > I don't understand this paragraph. I suspect I need to eat some magic mushrooms in order to be able to see the magic numbers. Can I recommend swapping over the payload identification numbers with the Delete payload so that Vendor ID is #42? But seriously, this doesn't seem to be the way we should go about standardizing an extension. s8.2 [AEAD]: I think the reference in s3.14 makes this normative. s8.2 [MAILFORMAT]: I think the statements in s3.5 make this normative. ============================================ Nits/editorial comments: General: The version of the draft in the repository is unpaginated. General: The notation used for concatenation ('|'), raising to the power ('^'), etc could be usefully defined early on in one place. s1, para 4: > > The pair is called an "exchange". We call the first > > messages establishing an IKE SA IKE_SA_INIT and IKE_AUTH exchanges > > and subsequent IKE exchanges CREATE_CHILD_SA or INFORMATIONAL > > exchanges. The start of the second sentence is confusing on a quick read: A better form might be: We call the first exchanges of messages that establish an IKE SA IKE_SA_INIT and IKE_AUTH and subsequent IKE... s1.2 (conventions): The use of [] to denote optional items in exchanges can be confusing with references. But we will probably have to live with it. s1.2 > > The keys used for the > > encryption and integrity protection are derived from SKEYSEED and are I assume *how* the derivation is done is described later (I think sa2.13/2.14 is the right answer). A forward reference would help - both to show that you shouldn't worry about how just now and to point to where it is defined. s1.3, para 3: the first sentence is redundant - it was said in para 1. s1.3: The section would be clearer if the last para was moved to be immediately after para 4 (the last para describes how and why the MAY in para 4 occurs). It would also help to change the order of the sentences in the last para, making the last sentence first. s1.3.1: The term Message ID has not been discussed on defined before its appearance here. s1.5, para 5: the last sentence > > The Initiator > > flag is set, the Response bit is set to 0, and the version flags are > > set in the normal fashion. can be omitted from the general introduction here - the various items referred to in the sentence have not been introduced at this point in the document. s1.5, last para: The previous comment applies to the last sentence here also. s2, para 1: There should be references for the port number definitions 500 is in RFC 2408 and 4500 is in RFC 3947. Arguably port 500 should be reassigned to IKE by the IKE standards since ISAKMP is no longer a well-known protocol. s2.9, last para: I believe the 'SHOULD narrow' ought to be 'should narrow'. This is is not something the protocol controls. s2.13, para 2: HMAC needs a reference. s2.13, para 3: I think the statement 'The preferred key size is used as the length..' ought to be strengthened to 'The preferred key size MUST be used as the length..'. s2.14, para 1: A reference back to Section 2.13 for the key length discussion would be useful. s2.15, para 1: IDr' is defined twice - once on its own and once in concert with IDi'. s2.17, para 7: the term ROHC_INTEG is not defined - it almost certainly needs a reference. s2.17, last bullet: s/protocol specification/the protocol specification/, s/For ESP and/For ESP and/ s2.20, para 2: Is the term 'trolling' sufficiently well-known not to need definition? s2.22: The IPCOMP algorithm table ought to be updated to the date of publication of this draft as an RFC - note to RFC Editor. s2.23, para 2: The term 'real Internet' is not well defined. The pejorative language in para 1 and here is unhelpful. s2.23, para 6: How is IPsec expected to 'discover' a NAT? (it appears that I am told a few paras later, but it isn't obvious that that is the case here!) s2.23, bullet point 7: 'these payloads'? Which payloads are they? s2.23.1, para 5: This is the first 'real' use of PAD (as opposed to in the list of changes) - it would be useful to expand it and put in a forward reference. s2.23.1: There are several missing definite articles in this section. s2.23.1, para 11: s/known NATs outer addresses/known NAT's outer addresses/ s3.1, Exchange Type: This should be updated to refer to the current document. AFAICS the table of exchange types is still valid as of today. There are several subsequent instances of the same issue. s3.1, Response Flag: s2.21.2 has a (sort of) exception to the 'no response to a response rule'. s3.1, Message ID and Length: Should be specified as unsigned integers. s3.2: The table of payload type identifiers could usefully have a forward reference to the section number where it is defined. s3.2, Payload length: Should be specified as unsigned integers. there are many other cases - it could be conveniently resolved by a global statement added to the byte ordering statement. s3.2, last para: 'Many payloads contain fields marked as "RESERVED".' So what? (well they should be treated just like you said two paras before, presumably!). s3.3, para 1: :-) 'Assembly of Security Association Payloads requires great peace of mind.' Hence any implementation must be capable of passing the Turing Test?????? s3.3, 'Proposal #': The usage of # as shorthand for number is US specific jargon and should be defined. This issue also occurs in '# of transforms' in s3.3.1. s3.3 onwards, last para: Repeating the payload type number here makes for a double maintenance problem and isn't useful within the payload. s3.3.1, SPI Size: 'the SPI is obtained from the outer header': What does 'outer' mean here? s3.5, para after ID Field types table: 'Implementations SHOULD be capable of generating and accepting all of these types.' Does 'all' here mean the four types in the previous sentence or 'all' the types in the full table? s3.5, para after ID Field types table: 'IPv6-only implementations MAY be configurable to send only ID_IPV6_ADDR instead of ID_IPV6_ADDR for IP addresses.' s/ID_IPV6_ADDR instead of ID_IPV6_ADDR/ID_IPV6_ADDR instead of ID_IPV4_ADDR/ s3.6: Probably needs a reference for ASN.1 specification. s3.9, last para: s/The size the/The size of the/ s3.16, para 1: 'The full set of acceptable values for the payload is defined elsewhere,' Where is not specified, but maybe RFC 3748. It should be explicitly stated that Type and Type_data are taken from RFC 3748 (and maybe other documents). s4, para 2: 'There are a series of optional features that can easily be ignored by a particular implementation if it does not support that feature.' I found this hard to parse. I *think* it is saying that the following list of features are ones that are prime candidates for omission from minimal implementations. How about: 'The following features are a selection of those that can be omitted from a minimal implementation while leaving it capable of interoperating satisfactorily with others:' s7, last para: Most of the references here could be usefully attached to some of the tables in s3 where the relevant specifications are called out. This would be both useful and resolve the xml2rfc nit. Appendix B, para 2: > > The strength supplied by group one may not be sufficient for the > > mandatory-to-implement encryption algorithm and is here for historic > > reasons. There are no longer any mandatory to implement encryption algorithms (at present), so this statement needs to be revised.