Sooooo, you can ignore my message from last night. These comments need to be dealt with. I have already replied to Elwyn about the "major" issue, but need to plow through the minor issues and nits. Given travel to the meeting, I doubt I'll have a new draft on Monday as planned, but some of these definitely need WG discussion.
--Paul Hoffman >Date: Fri, 19 Mar 2010 14:37:19 +0000 >From: Elwyn Davies <elw...@dial.pipex.com> >To: General Area Review Team <gen-...@ietf.org> >CC: Mary Barnes <mary.ietf.bar...@gmail.com>, > draft-ietf-ipsecme-ikev2bis....@tools.ietf.org, > IETF Discussion <i...@ietf.org> >X-SA-Exim-Connect-IP: 2001:8b0:0:30::51bb:1e34 >X-SA-Exim-Rcpt-To: draft-ietf-ipsecme-ikev2bis....@tools.ietf.org >X-SA-Exim-Mail-From: elw...@dial.pipex.com >X-Spam-Checker-Version: SpamAssassin 3.3.0 (2010-01-18) on > merlot.tools.ietf.org >X-Spam-Level: >X-Spam-Status: No, score=-1.9 required=3.0 tests=BAYES_00 autolearn=no > version=3.3.0 >Subject: Gen-art review of draft-ietf-ipsecme-ikev2bis-08.txt >X-SA-Exim-Version: 4.2.1 (built Sat, 01 Aug 2009 12:09:26 +0000) >X-SA-Exim-Scanned: Yes (on merlot.tools.ietf.org) > >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-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. _______________________________________________ IPsec mailing list IPsec@ietf.org https://www.ietf.org/mailman/listinfo/ipsec
