I have just scanned through version -10 of this draft, posted couple of hours ago.
This version addresses my comments 5 and 6; and comments 4 and 10 are obsolete since the text I commented has been removed. The remaining comments are still valid. One additional comment for version -10: 16) Section 5.3.2, "EMSKname is in the username part of the NAI and is encoded in hexadecimal values. The EMSKname is 64-bits in length and so the username portion takes up 128 octets." EMSKname is not defined in this document (or any of its references, as far as I can tell); and encoding 64 bits as hexadecimal doesn't take 128 octets anyway. Best regards, Pasi > -----Original Message----- > From: Eronen Pasi (Nokia-NRC/Helsinki) > Sent: 05 February, 2008 14:30 > To: '[EMAIL PROTECTED]'; 'ext Tim Polk'; '[EMAIL PROTECTED]' > Cc: '[EMAIL PROTECTED]'; 'ietf@ietf.org'; 'ext Lakshminath > Dondeti'; [EMAIL PROTECTED] > Subject: Gen-ART review of draft-ietf-hokey-erx-09 > > > 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-hokey-erx-09 > Reviewer: Pasi Eronen > Review Date: 2008-02-05 > IETF LC End Date: 2008-02-07 > IESG Telechat date: 2008-02-07 > > Summary: This draft is on the right track but has open issues, > described in the review. > > Comments: > > Most serious comments first: > > 1) The document should contain explicit text about the relationship > between ERP and the lower layers; for example, what would need to be > changed in lower layers that use EAP to add support for ERP. > (E.g. with parallel EAP-Request/Identity+EAP-Initiate/Reauth-Start > the protocol is no longer lock-step; the authenticator is no longer > responsible for all the retransmissions, etc.) > > 2) The document specifies message fields for so-called "channel > binding" information, but contains basically no text about > what to put in the field, or how to process them. > > Note that just saying "RADIUS Calling-Station-Id" is not very helpful, > since peers don't usually implement RADIUS. The spec either needs to > describe what the field should contain, or should tell where that is > described. > > Also, the semantics are highly unclear: the spec says these attributes > can be included in EAP-Initiate/Re-Auth and EAP-Finish/Re-Auth > messages -- but how do the peers know when to include them? Or what to > do with them when received? E.g. if the EAP-Initiate/Re-Auth contains > some of these attributes, should the EAP-Finish/Re-Auth also contain > them? With same values? > > (Answers to some of these questions may be "obvious" to people who > participated in the channel binding discussions 3..5 years ago; > but they're not in the current specification. And if there's any > difficulty in writing text about them, it IMHO suggests they > are not that obvious.) > > 3) The document uses terms EAP Peer-ID and EAP Session-ID which > are not part of RFC 3748; they are defined in draft-ietf-eap-keying, > which needs to be (normatively) referenced. > > 4) Section 4.1.1 defines "NameDerivationKey = EAP Session-ID, when K > used in rRK derivation is the EMSK"; however, existing EAP methods are > not required to export a Session-Id. This document needs to specify > what is done when no Session-ID is exported, or explicitly say that it > works only with EAP methods that export a session id. > > 5) Section 5.1, "In this case, the lower layer may already have > derived the TSKs based on the MSK received earlier. The lower layer > may then choose to ignore the rMSK that was received with the ER > bootstrapping exchange. Alternatively, the lower layer may choose to > generate a TSK from the rMSK." > > Who/what coordinates this; that is, ensures that both peer > and authenticator use the same key (MSK or rMSK)? > > > > The following comments are basically nits that should be easy > to fix: > > 6) Section 4.1.1 specifies rRK derivation seed as "S = rRK Label > + "\0" + NULL + length". It's not clear what "NULL" means here; > IMHO one obvious interpretation would be a single zero octet > (same as "\0"), but then again, perhaps an empty (zero-length) > string is intended, since a different notation was used? > > 7) Section 4.1.1 specifies the rRK label as "EAP Re-(newline)(white > space)authentication Root [EMAIL PROTECTED]"; this is a rather > unfortunate place to break a line, as the hyphen could be > interpreted in two different ways. > > 8) Inconsistent IANA considerations: slightly different USRK label > string is used in Sections 4.1.1 and 8. > > 9) Section 4.1.5 says "the most significant k octets of the output > are used"; the term "most significant" makes sense when talking > about integers. When talking about octet strings, I'd find "first" > or "last" less ambiguous. > > 10) Sections 5.2 and 5.3.2.2, "If rIKname-NAI is present, the > authenticator MUST use that NAI to route the message. If the > rIKname-NAI is not present, the authenticator MUST use the NAI in > the Peer-ID to forward the message via AAA. If neither are > available, the authenticator MUST forward the ERP messages to the > local ER server. If none of these rules apply, the authenticator > MUST drop the packets silently." > > Let's see; it seems the logic is as follows: > > if (rIKname-NAI is present) { > use rIKname-NAI > } else if (rIKname-NAI not present && Peer-ID present) { > use Peer-ID NAI > } else if (rIKname-NAI not present && Peer-ID not present) { > use local > } else { > drop silently; > } > > I can't quite figure out when the "If none of these rules apply" > text would be used. Perhaps the intent was to drop the packet > if it can't be parsed at all? If so, this should be described > more clearly. > > 11) Sections 5.3.1, 5.3.2, and 5.3.3 do describe TVs/TLVs which > can be included in the messages, but don't clearly specify > which of them are always present, and which may be omitted > in some circumstances. > > 12) Section 5.4, "The server expects a sequence number of zero or > higher. When the server receives an EAP- Initiate/Re-auth message, > it uses the same sequence number in the EAP-Finish Re-auth message. > It increments the expected sequence number by 1." > > The last sentence is obviously wrong; the text probably intended to > say that the server sets the expected sequence number to the received > sequence number plus 1. > > 13) Section 6, "Transport of ERP messages is specified in [10] and > [11]"; this presumably means "Transport of ERP messages between the > ER Authenticator and ER Server", as neither draft seems to cover > transport between ER Peer and ER Authenticator. > > 14) Section 7, "..is indicated in the EAP re-authentication Response > message" Does this mean EAP-Initiate/Re-auth message, or something > else? > > 15) Section 8 (IANA considerations) could be clearer about where IANA > is expected to assign values from existing registry, and where > creating a new registry is required. This section should also provide > names for the new registries. > > Best regards, > Pasi > _______________________________________________ Ietf mailing list Ietf@ietf.org http://www.ietf.org/mailman/listinfo/ietf
