Subject: [Gen-art] Gen-art review of draft-ietf-ipsecme-ikev2bis-08.txt

From: Elwyn Davies <[email-masked]>
Date: Tue, 04 May 2010 13:17:59 +0100

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 wait for direction from your document shepherd
or AD before posting a new version of the draft.

Document: draft-ietf-ipsecme-ikev2bis-10.txt
Reviewer: Elwyn Davies
Review Date: 4 May 2010
IETF LC End Date: 18 March 2010
IESG Telechat date: 6 May 2010

Summary:
When I reviewed this document at IETF Last call, I discovered that compared to
previous documents, it contains no mention of mandatory to implement
ciphersuites.  In a discussion with Paul Hoffmann, I was pointed at various
other RFCs that specify ciphersuites, and informed that the IESG and WG had
approved the current situation.  However it seems to me that somebody looking at
the IKE documents would be likely to come to this document first and would find
it difficult to locate the auxiliary documents without considerable effort (and
risk of missing some).  I continue to believe that f there is not to be a list
of appropriate ciphersuites here, there needs to be some pointers to places to look.

A number of my comments have been implemented, but there are quite a few that I
have seen no response to amd have not been implemented.  The list below
represents the remaining comments. I have left in a number of comments regarding
the cut off time (publication date of RFC 3406) for updating of registration
tables.  It strikes me that it would be relatively little work and quite helpful
to bring these tables up to date.

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.  However there are effectively some
mandatory to implement suites; they are listed in other documents.  There should
be a way of finding them so that users and implmenters can find them easily.

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.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.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? [A suggestion of using
the Vendor ID payload has been added.  I am not sure if this adequately deals
with the situation.]

s2.21.4: More requests to limit message rates without specific means.

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.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.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.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.

2.14, para 1: A reference back to Section 2.13 for the key length discussion
would be useful.

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, 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.

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.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.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.6: Probably needs a reference for ASN.1 specification.

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.

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