Document: draft-ietf-radext-rfc3576bis-10 Reviewer: Ben Campbell Review Date: 16 October 2007 IESG Telechat date: 18 October 2007 Summary: > This draft is basically ready for publication as an informational RFC, > but has nits that should be fixed before publication. Comments: I reviewed the 09 version of this draft for IETF last call. If there was a response to that review, I missed it. Rather than review this version from scratch, I looked to see if my comments from the 09 review were addressed. In all fairness, most of my comments are pedantic nits. Please see imbedded comments. Thanks! Ben. On Sep 20, 2007, at 5:04 PM, Ben Campbell wrote: > 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-radext-rfc3576bis-09.txt > Reviewer: Ben Campbell > Review Date: 9/20/07 > IETF LC End Date: 9/24/07 > > Summary: > > This draft is basically ready for publication as an informational RFC, > but has nits that should be fixed before publication. > > Comments: > > idnits reports two relevant issues: > >> -- The exact meaning of the all-uppercase expression 'MAY NOT' >> is not >> defined in RFC 2119. If it is intended as a requirements >> expression, it >> should be rewritten using one of the combinations defined in RFC >> 2119; >> otherwise it should not be all-uppercase. This has been fixed in version 10. >> >> ** Obsolete normative reference: RFC 2409 (Obsoleted by RFC 4306) > > No change in version 10. It may be that reference to the old IKE RFC is intentional, in which case I am fine with it. I do not have enough expertise in IKE to judge that for myself, so perhaps the authors will comment. > Section 2.2, diagram: > > I'm not sure that the term "authz" is well known enough to use without > definition. Consider spelling out "authorization" or adding "authz" to > the terminology list. This was fixed in version 10. > > Section 2.3: Definition of RADIUS code values > > This section references RFC3575 for the IANA registrations. RFC3575 > references RFC3576, which is being obsoleted by _this_ spec, for the > value definitions. Is that the intent? No change. I'm okay with this if this was intentional, but would like to see some comment to that effect. > > Section 3, first paragraph: > > "All NAS and session identification attributes included in a > CoA-Request or Disconnect-Request packet MUST match at least one > session in order for a Request to be successful; otherwise a > Disconnect-NAK or CoA-NAK MUST be sent." > > Is the above text correct? If so, what does it mean for a NAS > identification attribute to match a _session_? I assume we are > using the NAS identifiers as a scope in which the session > identifiers have meaning, so that all sessions could be considered > of having an implied NAS property? A little more explanation might > be helpful. No apparent change in the text. I think the authors mean by "all" to say that the combination of NAS and session identification attributes MUST match a session--but the text still seems ambiguous on this point. > > Section 3, 5th paragraph from end: > > "Where a Diameter client utilizes the same Session-Id for both > authorization and accounting, inclusion of an Acct-Session-Id > Attribute in a Disconnect-Request or CoA-Request can assist with > Diameter/RADIUS translation, since Diameter RAR and ASR commands > include a Session-Id AVP. An Acct-Session-Id Attribute SHOULD be > included in Disconnect-Request and CoA-Request packets." > > I find the above text to be confusing. I _think_ you intend to > compare the difference between an NAC behavior and a Diameter > client behavior, but on the first read I it seemed like you were > proposing suggested RADIUS behavior as conditional based on what a > particular Diameter client does. I propose changing the first few > words to "While Diameter clients... " No change, but after re-reading, I think I understand the meaning--no change needed other than perhaps finding a reviewer who can read more carefully :-) > > Section 3.1, last paragraph: > > Should the following sentence us normative language? > > "Since > Disconnect and CoA responses are authenticated on the entire packet > contents, the stripping of the Proxy-State Attribute invalidates > the > integrity check - so the proxy needs to recompute it." No change. If I am reading this right, they are saying that if a proxy does not recompute the integrity check, things will break. That seems like a good place to say the proxy MUST recompute... > > Section 3.2, entire section: > > It would be helpful to mention in this section how Authorize Only > is used to ease mapping to Diameter, and reference the Diameter > Considerations section. As it is, I spent some time wondering what > the semantic effect of the resulting Access-Request message was > supposed to be. I don't find any change related to this comment. However, it is not a critical comment; it was merely a personal opinion of a way to improve readability of the spec. > > Section 3.5 Paragraph 5: > > "When the HMAC-MD5 message integrity check is calculated the > Request Authenticator field and Message-Authenticator Attribute > should be considered to be sixteen octets of zero." > > Is that 16 octets _combined_ or _each_? I originally assumed > combined, but the calculation for use in an ACK/NAK the Message- > Authenticator is treated as 16 octets of zero all by itself. No change in the draft. I think this needs to be more precise, and could easily be fixed by inserting the word "each" somewhere. > > Section 6.2 paragraph 4: > > Can a proxy be expected to easily know if it is one-hop away from > the NAS? Is the mechanism for determining this well-known or > documented somewhere that could be referenced here? (Sorry if I am > showing my RADIUS ignorance here.) No change in the draft. In all fairness, this was a question more than a comment, but I have not seen a response.