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

From: Stephen Farrell <[email-masked]>
Date: Thu, 06 Sep 2012 09:49:06 +0100

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

---

From: "Jim Schaad" <[email-masked]>
Date: Fri, 7 Sep 2012 16:40:13 -0700

Some comments inline

> -----Original Message-----
> From: [email-masked] [mailto:[email-masked]] On Behalf
> Of Stephen Farrell
> Sent: Thursday, September 06, 2012 1:49 AM
> To: <[email-masked]>
> 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.
> 
> 
> - 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
> 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;-)
> 
> _______________________________________________
> abfab mailing list
> [email-masked]
> https://www.ietf.org/mailman/listinfo/abfab

---

From: Stephen Farrell <[email-masked]>
Date: Sun, 09 Sep 2012 15:08:19 +0100

Hi Jim,

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

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

---

From: Sam Hartman <[email-masked]>
Date: Thu, 13 Sep 2012 14:58:21 -0000

As a FYI, there has been action on this.  I'm updating the Moonshot
implementation to make sure it is consistent with this draft and with
the approved draft-ietf-abfab-gss-eap so I can generate examples.
Mostly done.  One more change; hope to have examples Friday or this
weekend.

---

From: Sam Hartman <[email-masked]>
Date: Wed, 19 Sep 2012 14:43:58 -0400

>>>>> "Jim" == Jim Schaad <[email-masked]> writes:

    Jim> Some comments inline
    >> -----Original Message----- From: [email-masked]
    >> - Examples are very very badly needed. Maybe some GSS calls and
    >> showing the values that'll be produced.
    >> 

I've tried to add some.

    >>     >> - 6.2, can't there be more spaces after the first two in this
    Jim> Calling that
    >> out would seem better.

    Jim> I am not sure what you mean here.  It seems to me that we have
    Jim> <string> <space> <string> <space> <string>

    Jim> Where are you planning to put the additional spaces?  After the
    Jim> last string?  It might be interesting to say if you could put
    Jim> multiple space characters in the location where the <space> is,
    Jim> but that is not something that I would necessarily expect.

I agree with Stephen that the last string can for some name formats
contain spaces.
Text clarified.

Tried to clarify this too.

    >> 
    >> General, comment-like:
    >> 
    >> - As presented, this looks very complex. I think the presentation
    >> could do with work to make this an easier read.

    Jim> In order to access this, I would need to know where you felt it
    Jim> was complex.  At this point it is, for better or worse, rather
    Jim> easy to read.

    >> mechtypes that
    Jim> use
    >> RADIUS or SAML? I mean for example a statment like "This
    Jim> only
    >> applies for GSS-EAP or SAML-EC." That might well simplify a lot
    Jim> since
    >> I reckon some of the complexity mentioned above comes from what

I think there's a huge cost for future applications if we do that and
come up with related SAML mechanisms or stack these SAML mechanisms in
for example a GSS mechanism providing data compression or the like.
Also, consider the interaction with NegoEx or SPNEGO.
Applications can and will find these attributes with mechanism names
with a much broader set of underlying mechanism.

    >> questions:

I've added text that hopefully clarifies.

    >> - How do I implement the "MUST NOT" in the last para of section
    Jim> 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.

    Jim> I don't have good text here at this point.

I've proposed something in the soon-to-be-uploaded 05.

    >> - Section 5, 1st para: what does "undefined" mean? If I call the
    Jim> GSS
    >> function too early (before access-accept) what do I get? Maybe
    >> say here which GSS calls are meant?

    Jim> If you make the call too early, you might get an answer that
    Jim> would change after the access-accept message.  You might get an
    Jim> error.  We are not saying what the correct behavior is.  While
    Jim> the current deployments are setup to just get information back,
    Jim> for Plasma we are looking at situations where we are going to
    Jim> want to ask more questions and the way to do this is not
    Jim> currently defined.

I've tried to clarify that we're specifying behavior for what happens if
you look for these attributes on the src_name returned from
GSS_Accept_Sec_context after GSS_S_COMPLETE.

    >> 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.
    >> 
Jim>
Jim>I had asked a similar question in the past, and I think that Sam's response
    Jim> is incorrect in that it does not match with what he told me in
    Jim> the past.  There is a possibility that some mechanism will have
    Jim> a SAML statement it receives that it cannot authenticate either
    Jim> via the AAA exchange or by validating the signature on the SAML
    Jim> statement.  In this case the attribute would not be returned as
    Jim> authenticated.  However I would be willing for this behavior to
    Jim> be dropped.  But given that we are talking about how to query
    Jim> for the attribute, I don't think that the suggested sentence
    Jim> from Sam would be correct.

Hmm.
I thought what I said in the past is that  the SAML assertion attribute
would be dropped if the mechanism didn't like the assertion, but that
you'd typically expect the RADIUS attribute carrying the assertion to
remain at least for GSS-EAP.

Although I guess that was more about local policy checks than validation
faliures.

    Jim> Perhaps 1.  Make this a new paragraph 2.  Updated text

    Jim> This attribute is authenticated only when the mechanism can
    Jim> successfully authenticated the SAML statement.  For the GSS-EAP
    Jim> mechanism this is true if the AAA exchange has successfully
    Jim> authenticated.  However, uses of the GSS-API MUST confirm that
    Jim> the attribute is marked authenticated as other mechanisms MAY
    Jim> permit an initiator to provide an unauthenticated SAML
    Jim> statement.

    >> "<saml:Attribute> element's Name XML attribute" is used at the
    >> end of para 1, but later
    Jim> paras
    >> talks about the <saml:AttributeValue> - are they the same or not?

    Jim> They are not the same.

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

    Jim> Text: In the GSS-EAP mechanism, a SAML assert carried in an
    Jim> integrity-protected and authenticated AAA protocol SHALL be
    Jim> considered successfully validated.

    >> - 6.2: last sentence talks about "this assertion" buts its not
    Jim> if you
    >> mean the entire assertion or just the attribute?

    Jim> I think that when Sam wrote the text he was thinking of the
    Jim> SAML assertion.  I think however it would be more correct in
    Jim> the current content to substitute attribute for assertion as it
    Jim> needs to be clear than local policy may kill an attribute.  I
    Jim> would suggest the following changes:

    Jim> 1.  Add the following to the end of section 6.1 "An
    Jim> implementation MAY apply local policy checks to a SAML
    Jim> assertion and discard it if it fails the local policy checks."
    Jim> 2.  Change the text at the end of section 6.2 to "An
    Jim> implementation MAY apply local policy checks to each attribute
    Jim> contained in a SAML assertion and discard the attribute if it
    Jim> fails the local policy checks.

    >> - 6.3: Why the SHOULD in the first para?

    Jim> A mechanism can choose not to expose some name forms to the
    Jim> acceptor.  In this case the GSS name attribute would not be
    Jim> present.

    >> - section 8, what is "paramname"?

    Jim> If a parameter "paramname" is registered in this registry then
    Jim> its URN will be "urn:ietf:gss:paramname".

    >> - section 8, all but 1st sentence of 1st para should go

Will do, but Stephen is taking full responsibility for resolving any
confusion by people trying to put it somewhere else.:-)