Hi all, My review of this is below. I do think a couple of things at least(1st two) need fixing before IETF LC, so I'll mark this as revised ID needed unless you want to argue with me about that.
Note that I sent an early version of this to the authors and chairs, and have incorporated some feedback I got from Sam below. Cheers, S. AD review of draft-ietf-abfab-gss-eap-naming-04 2012-09-06 General, but I think need fixing before IETF LC: - Examples are very very badly needed. Maybe some GSS calls and showing the values that'll be produced. - 6.2, can't there be more spaces after the first two in this case? Calling that out would seem better. General, comment-like: - As presented, this looks very complex. I think the presentation could do with work to make this an easier read. - Ought this be made specific to the currently known GSS mechtypes that use RADIUS or SAML? I mean for example a statment like "This specification only applies for GSS-EAP or SAML-EC." That might well simplify a lot of things since I reckon some of the complexity mentioned above comes from what might be premature generalisation. questions: - 2nd para of section 4: what does "MUST make a policy decision" mean? Maybe you mean that a service MUST have a way to decide which IdPs are trusted for which "asserted values"? (And what exactly are "asserted values"?) - How do I implement the "MUST NOT" in the last para of section 4? I'm told that's meant for mechanism spec. writers so making that clear would be good. "Closely associated" seems too vague. "Can assume" in the example also seems pretty vague. - Section 5, 1st para: what does "undefined" mean? If I call the relevant GSS function too early (before access-accept) what do I get? Maybe say here which GSS calls are meant? - 6.1, How can a GSS API extension spec assert that authentication only succeeds if the SAML or AAA exchange is successfully authenticated? Sam suggested: "This attribute MUST always be authenticated when present in mechanism names complying with this specification." which I think would be better. - 6.2: Maybe I need to check SAML again, but the "<saml:Attribute> element's Name XML attribute" is used at the end of para 1, but later paras talks about the <saml:AttributeValue> - are they the same or not? - 6.2: What does "sufficiently validated" mean? Adding more clarity here would be good. Sam suggested: "what we're saying or trying to say here is that a mechanism of GSS-EAP should mark attributes carried according to draft-ietf-abfab-aaa-saml as authenticated in the return from GSS_Get_Name_attribute without needing to do normal SAML validation." - 6.2: last sentence talks about "this assertion" buts its not clear to me if you mean the entire assertion or just the attribute? - 6.3: Why the SHOULD in the first para? - section 8, what is "paramname"? nits and whinges: - ID-nits complains about a few things. No show stoppers at this point but please fix after IETF LC. - worth calling out that %20 is not a separator in 2nd para of section 3? - Section 4, 2nd last para: whenever academics (like me:-) throw in "context" or "policy" alarm bells ought go off. Here you're saying a trust-context is a type of context and context is a property of an attribute. Don't you find that all a bit overly-generic? Then you say attributes with different contexts need different names. Does that mean that the name is also a property of the attribute? Is there no way to get rid of some of that veribage and reduce this to what a programmer needs to know? (Ok, you can feel free to ignore this, its just a whinge and I'm sorry;-) - section 8, all but 1st sentence of 1st para should go _______________________________________________ abfab mailing list [email protected] https://www.ietf.org/mailman/listinfo/abfab
