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

["Jim Schaad" <[e-mail]>]

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>

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.

> 
> - 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