[Gen-art] Gen ART review of draft-nystrom-eap-potp-05.txt

[[e-mail]]

Magnus,

I am the assigned Gen-ART reviewer for draft-nystrom-eap-potp-05.txt
For background on Gen-ART, please see the FAQ at
<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.

This draft is on the right track but has open issues,
described in the review.

The draft is generally well written, and reasonably clear.
Everything I found is minor, but a couple of the concerns
rise to the level of open issues that deserve attention.

---------- Issues ----------

(A) Page 31: the definition of "msg_hash" in running text is dense,
informal and mixes the algorithm for computing the hash input
with a description of the design rationale for the algorithm.  The
result may be error-prone for an implementer.

This should be separated to explicitly specify the algorithm step by
step, and then explain why the algorithm is the way it is.  At a high
level, the algorithm has two basic steps:
	1) Concatenate specified messages excluding retransmissions.
	2) Remove a list of fields and TLVs from the concatenation,
		shortening it.
The explanation of the algorithm should explain why step 2) is a
"remove" step instead of a "set contents to zero" step.  The
description of "msg_hash'" on p.33 needs to point back to this
algorithm, as do the descriptions of "msg_hash2" and "msg_hash2'"
on p.40.  The same is probably the case for the description of
"msg_hash" on p.45, but I'm not sure about that one - it's an
example of an ambiguity that could cause interoperability problems.

It may also be useful to look at Section 3.3.3 of RFC 4302 for
an example of a well-specified similar calculation (the AH ICV). 

This may look like a nit, but this sort of sloppiness leads to
interoperability problems caused by subtle differences in the hash
inputs across implementations, hence it's tagged as an open issue.

(B) Section 4.10.7 requires that Vendor-Specific TLVs always be
Mandatory (M bit is always 1).  That's a formula for lack of
interoperability where vendor A's implementations always issue
a mandatory vendor-specific TLV that only vendor A's implementations
understand, thereby preventing interoperation with vendor B's
implementations that don't understand that TLV.

This is deja vu for me, as I've recently been through a long
discussion of this issue in the context of another draft.  In the
hope of avoiding a repeat of that discussion, here are the
conclusions, as expressed in extracts from two email messages
authored by IETF Area Directors:

-----
  The problem we're trying to avoid is Company C shipping a box
  that sends a vendor private extension that requires implementations
  that do not understand the extension to error, while company J doesn't
  implement the extension.  If C's products always send this extension,
  they will not interoperate with J's products, even though both comply
  with the spec.  Thus, the spec does not provide sufficient
  requirements to guarantee interoperability.

  [Hence] ... there must be a configuration in which no mandatory
  vendor private extensions are sent
----
   It seems to me that there are two ways to deal with vendor
   private extensions, and still allow interoperability:

  - One way is to allow use of the vendor private extension to be
  turned off via configuration.

  - Another way is to ensure that if an system sends a
  vendor-private extension, and the other system doesn't understand
  it, then it responds in a well-defined way (which I believe that the
  spec already does), and the first system is REQUIRED to continue
  in a way that makes sense (ie, follows the defined standard and
  stops using the vendor-private extension, or at least stops relying
  on it).
----

Please select one or both of the approaches in the second message
and add text to the draft to require it/them.

(C) If this were a standards-track RFC, the identifiers for the hash,
encryption and MAC algorithms defined in Section 4.10.16 would go into
an IANA registry.  Instead the document says that any addition of
algorithms requires a new RFC.  That's probably ok, but I would
advise double-checking this with Jari Arkko and/or Russ Housley.

---------- Nits ----------

(1) Page 30:
            -  For dial-up, "auth_id" SHALL either be the empty string
               or the phone number called by the peer.  The phone number
               SHALL be specified in the form of a URL conformant with
               RFC 2806 ([8]), e.g. "tel:[phone]".  Processing of
               received phone numbers SHALL be conformant with RFC 2806.

That's not a good example phone number, try [phone] - see
Section 3, subsection 6 of http://www.ietf.org/ID-Checklist.html

(2) Page 30:
            -  For IP-based EAP, "auth_id" SHALL either be the empty
               string or the IPv4 or IPv6 address of the authenticator
               as seen by the peer and in binary format (4 respectively
               16 octets).  As an example, the IPv4 address
               "10.129.13.15" would be represented as (in hex) 0A 81 0D
               0F, whereas the IPv6 address "0A0A:0B0B:0C0C:0D0D:0E0E:
               0F0F:1010:1111" would be represented as (in hex) 0A 0A 0B
               0B 0C 0C 0D 0D 0E 0E 0F 0F 10 10 11 11.

Similar problem - bad example IP addresses.  192.0.2.5 and 2001:DB8::101
would be ok.  See same reference as phone number issue above.  The same
issue occurs on p.31 at:

            auth_id = "10.129.13.1", iteration_count = 2000, and

Unfortunately, this affects the example calculation used, so its results
will need to be recalculated.

(3) p.32
            Note: To save on storage space, each EAP entity may hash
            messages as they are sent and received.  This reduces the
            amount of state needed for this purpose to the state
            required for the negotiated hash algorithm.

"hash" is not the right verb, but it's close.  Read literally, the
result over 3 messages could be H(msg3 | H(msg2 | H (msg1))) which
is not equivalent to the correct H(msg1 | msg2 | msg3).  The text
should be rewritten to talk about partial evaluation of the hash as
messages are sent and received so that the internal state of the
hash function is stored instead of the contents of all previous
messages (and the word "internal" is important).

Thanks,
--David
p.s.  And please note that Magnus didn't get a free pass, even
	though my employer (EMC) just announced that it's buying
	his employer (RSA Security) ;-).
----------------------------------------------------
David L. Black, Senior Technologist
EMC Corporation, 176 South St., Hopkinton, MA  01748
[phone]             FAX: [phone]
[e-mail]        Mobile: [phone]
----------------------------------------------------

_______________________________________________
Gen-art mailing list
[e-mail]
https://www1.ietf.org/mailman/listinfo/gen-art