Re: [abfab] AD review of draft-ietf-abfab-gss-eap-naming

[Stephen Farrell <[e-mail]>]

Hi Jim,

On 09/08/2012 12:40 AM, Jim Schaad wrote:
> Some comments inline
> 
>> -----Original Message-----
>> From: [e-mail] [mailto:[e-mail]] On Behalf
>> Of Stephen Farrell
>> Sent: Thursday, September 06, 2012 1:49 AM
>> To: <[e-mail]>
>> Subject: [abfab] AD review of draft-ietf-abfab-gss-eap-naming
>>
>>
>> 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.
> 
> I am not sure what you mean here.   It seems to me that we have 
> <string> <space> <string> <space> <string>

The last string can have a space embedded in some cases I think.
It says: "The final part is the <saml:Attribute> element's Name
XML attribute" which as I read it says this 3rd part is an
XML attribute value, which can have spaces. If that's right then
I think we need to tell coders about it.

> 
> Where are you planning to put the additional spaces?  After the last string?
> It might be interesting to say if you could put multiple space characters in
> the location where the <space> is, but that is not something that I would
> necessarily expect.  
> 
> Yes I think it would be reasonable to state that spaces in URIs must be
> encoded as %20, but I thought that was obvious, although it might be an
> error that I made at some point in the future if I just did a copy of the
> string from the XML to here without checking for embedded whitespace.
> 
>>
>> General, comment-like:
>>
>> - As presented, this looks very complex. I think the presentation could do
>> with work to make this an easier read.
> 
> In order to access this, I would need to know where you felt it was complex.
> At this point it is, for better or worse, rather easy to read.

Our reading experiences differ then. If its just me then that's
fine but I think (as I say below) that the attempt to be generic
here causes fairly tortured language to be needed.

I'll leave the rest of the comments for the list, since they're
just comments and I'm not sure Jim is really talking to me so
much as the list for some of 'em:-)

Cheers,
S.

>> - 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"?)
> 
> Thus, a service MUST make a policy based decision:  Is this IdP permitted to
> assert this attribute and is the value permitted.
> 
>>
>> - 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.
> 
> 
> I don't have good text here at this point.
> 
>>
>> - 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?
> 
> If you make the call too early, you might get an answer that would change
> after the access-accept message.  You might get an error.  We are not saying
> what the correct behavior is.  While the current deployments are setup to
> just get information back, for Plasma we are looking at situations where we
> are going to want to ask more questions and the way to do this is not
> currently defined.
> 
>>
>> - 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.
>>
> 
> I had asked a similar question in the past, and I think that Sam's response
> is incorrect in that it does not match with what he told me in the past.
> There is a possibility that some mechanism will have a SAML statement it
> receives that it cannot authenticate either via the AAA exchange or by
> validating the signature on the SAML statement.  In this case the attribute
> would not be returned as authenticated.  However I would be willing for this
> behavior to be dropped.  But given that we are talking about how to query
> for the attribute, I don't think that the suggested sentence from Sam would
> be correct.
> 
> Perhaps
> 1.  Make this a new paragraph
> 2.  Updated text
> 
> This attribute is authenticated only when the mechanism can successfully
> authenticated the SAML statement.  For the GSS-EAP mechanism this is true if
> the AAA exchange has successfully authenticated.  However, uses of the
> GSS-API MUST confirm that the attribute is marked authenticated as other
> mechanisms MAY permit an initiator to provide an unauthenticated SAML
> statement.
> 
>> - 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?
> 
> They are not the same.
> 
>>
>> - 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."
>>
> 
> That probably is what Sam said, not what he suggested as text.
> 
> Text: In the GSS-EAP mechanism, a SAML assert carried in an
> integrity-protected and authenticated AAA protocol SHALL be considered
> successfully validated.
> 
>> - 6.2: last sentence talks about "this assertion" buts its not clear to me
> if you
>> mean the entire assertion or just the attribute?
> 
> I think that when Sam wrote the text he was thinking of the SAML assertion.
> I think however it would be more correct in the current content to
> substitute attribute for assertion as it needs to be clear than local policy
> may kill an attribute.  I would suggest the following changes:
> 
> 1.  Add the following to the end of section 6.1
>    "An implementation MAY apply local policy checks to a SAML assertion and
> discard it if it fails the local policy checks."
> 2.  Change the text at the end of section 6.2 to
>   "An implementation MAY apply local policy checks to each attribute
> contained in a SAML assertion and discard the attribute if it fails the
> local policy checks.
> 
>>
>> - 6.3: Why the SHOULD in the first para?
> 
> A mechanism can choose not to expose some name forms to the acceptor.  In
> this case the GSS name attribute would not be present.
> 
>>
>> - section 8, what is "paramname"?
> 
> If a parameter "paramname" is registered in this registry then its URN will
> be "urn:ietf:gss: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
>> [e-mail]
>> https://www.ietf.org/mailman/listinfo/abfab
> 
> 
>