Re: [abfab] Comments on draft-ietf-abfab-aaa-saml-10

[Alejandro Perez Mendez <[e-mail]>]

Hi Jim,

thank you for the review. See my comments inline.

El 19/02/15 a las 19:54, Jim Schaad escribió:
> I really want to see this document done as it is now the document holding up
> the ABFAB cluster of documents in the RFC editor queue.
>
> 1.  Section 1 - Paragraph 3 - There is no longer a document
> I-D.jones-diameter-abfab and there is currently no prospect of such a
> document coming back into existence anytime soon.  The paragraph and the
> reference should be killed.

Agree.

> 2.  Section 4.2 - Please change the MAY back to a can 'this profile can
> take'  There is no real protocol statement here.  If you wanted to make one
> then it should be a lot higher than MAY as the odds are that SAML statements
> are not going to be transportable in a large number of cases without either
> this or the big packet size draft being implemented.

Makes sense.

> 3.  Given the way that messages and assertions are being separated in
> section 4.2, I think that we should consider renaming SAML-Message to be
> SAML-Protocol.  This makes it clearer that protocol messages go here and
> assertions go in SAML-Assertion.   It is a bit confusing to talk about SAML
> protocol messages, SAML-Message and RADIUS Access-Request message in
> proximity.  Making both of the SAML items be protocol I think would clarify
> things.

I would like to hear the opinion of the original authors on this.

> 4. Section 4.2 - s/Therefore, other arbitrary RADIUS attributes MAY be used
> in either the request or response./Therefore, other arbitrary RADIUS
> attributes will be used in requests and responses./

Makes sense, since there will be always at least an User-Name, etc.

> 5. Section 4.3.2 - Last three paragraphs - If the SAML Issuer entityID is of
> the form of an NAI and the realms match between the IdP and the SAML NAI,
> are these statements still true or can the SAML policy still be applied or
> is that just a match for what happens in section 4.3.1?

I think this would be somehow similar to what Josh proposed in IETF 90 
(http://tools.ietf.org/agenda/90/slides/slides-90-abfab-3.pdf), but he 
was defining a different naming scheme instead of NAI name format. Then, 
the linking between SAML and AAA names would be done implicitly, when 
both names match, and no metadata or signatures would be required. But 
this proposal got some objections due to its complexity, preferring the 
"have metadata" option, if I recall correctly.

Anyway, I'm not sure whether the NAI Name format can be directly applied 
to IdPs and RPs rather and only to Clients.

> 6.  Section 4.3.3 - Can we really say that XML signatures are outside of the
> scope of the binding given that section 4.3.2 is saying that there are cases
> where signatures are going to be needed?  I think we can just delete
> everything following ", but this".

I agree.

> 7.  Section 5 - I would like to add the following paragraph:
>
> There are cases where the RP does not have a full NAI name or the IdP will
> not release a full NAI name to the RP due to policy.  In these cases, the
> domain only form of the NAI can be used.

The domain-only representation seems to be included in 
[I-D.ietf-radext-nai]. Is it really required to mention it here? I'm not 
against it, just asking.

> 8.  Section 6 - I am having a problem with the overview in this section.  It
> is quite clear from the overview that this is designed to be a
> query/response protocol.  It is not clear from the overview that any such
> thing as an IdP spontaneously returning a SAML statement or responding with
> a SAML statement that has nothing to do with the query is part of the
> profile.  I will be honest, I would prefer that the binding be exactly what
> is stated in the overview.  If the binding is to be adhered to, then the bad
> behavior should not be permitted.

Umm. The overview does not mention anything about the SAML AuthnRequest, 
does it? It only mentions that the IdP will generate an authentication 
assertion as a result. It can be as a result of a explicit 
<saml:AuthnRequest> or just spontaneously.


> 9.  Should section 6.1 Conformation Method identifiers also refer to the
> ones in section 8?  I think so

Section 8 specifies that "the Subject is the system entity (either the 
user or machine) authenticated by a previously transmitted RADIUS 
Access-Accept message". However, as section 6.1 describes the 
authentication profile, there is no previous RADIUS State attribute to 
be used for identification yet.

In my opinion, the identifiers in section 8 are more intended for the 
Assertion Query/Request profile, that happens after the authentication 
process was completed.

>
> 10.  Section 6.2 - If one is using this binding, is it really optional to
> issue the <samlp:AuthnRequest> message in step 2?  It would seem that if you
> are using this binding one is going to do the SAML stuff.   If is not doing
> the SAML stuff then one is not using this binding.

It is optional as it is stated that way in section 4.2, where two modes 
are described. The first one is a request-response model, where the 
second one is just a spontaneous response .

Said that, I see your point. However, I'm not aware of the reasons why 
this was allowed in the first place. I guess the reason is simplicity 
(as in this profile an AuthnRequest will typically contain no useful 
information other than just signalling the intention of obtaining a SAML 
Assertion as a result), and avoiding sending AuthnRequest and 
SAML-Message attributes to a RADIUS server which is not able to 
understand this, as it might return an error when the attributes cannot 
be understood.

Nevertheless, the original authors might provide a better response.

>
> 11.  Section 6.2 - Bindings - s/not require the use of/not use/  The use of
> require seems to me to imply there are cases where HTTP based bindings could
> be used.

Right.

>
> 12. Section 6.2 step 2 - s/may optionally issue/issues/
> 13.  Section 6.3.2  - s/MAY include/includes/
>          s/this RADIUS/the RADIUS/
>          s/SAML RADIUS binding/SAML-Message RADIUS attribute/
> 	s/destination MAY be/destination may be/

They are related to the previous comments. We need some discussion on this.

>
> 14.  Section 6.3.3 - What is "RADIUS authentication"?  We have said that you
> need to do EAP for ABFAB however no restrictions for non-ABFAB methods.  Do
> we consider any RADIUS authentication method to be sufficient?

Section 6.2 states that
"This profile does not require the use of any
       particular authentication method.  The ABFAB architecture does
       require the use of EAP [RFC3579], but this specification may be
       used in other non-ABFAB scenarios."

I think this would be similar to what happens in the Web world. When the 
User is redirected to the IdP, there isn't any restriction on how the 
IDP authenticates the User.
The SAML Web-SSO profile states that "....the identity provider may use 
any means to authenticate the user agent, subject to any requirements 
included in the <AuthnRequest> in the form of the 
<RequestedAuthnContext> element."

Although, in the context of ABFAB, we need an EAP authentication.

>
> 13.  Section 6.3.4 - I would prefer that this read
> The IdP MUST issue a <samlp:Response> message to the RP that is consistent
> with the authentication results, as described in [OASIS.saml-core-2.0-os].
> This SAML response is delivered to the
>     Relying Party using the SAML RADIUS binding described in Section 4.  If a
> response message is not returned by the IdP, the RP MUST NOT assume that the
> authentication was performed in a manner consistent with the request.

I agree with this new text, but only if we agree on making the 
AuthnRequest mandatory. Otherwise, the RP might not receive a SAML 
response when no request has been sent, and still being consistent with 
the request.


>    Section 6.4.2 -  If the IdP cannot or will not satisfy the request, it
> MUST either respond with a <samlp:Response> message containing an
> appropriate error status code or codes and/or respond with a RADIUS access
> denied message.

I think it makes sense to make this mandatory rather than optional.

>
> 15.  Section 6.4.2 - Is the last bullet item normal for a SAML provider?
> Specifically, that one can authenticate, but ignore any of the actual
> requests from the RP.  If not, should we make some statements about telling
> the RP that we did not follow the instructions?  Possibly by either an error
> or no response (if the IdP does not support the binding).

I think that's allowed by SAML specification.
In fact, this paragraph is almost word-by-word copied from section 
4.1.4.2 of the saml-profiles-2.0-os.

> 16.  Section 6.4.4 - I think that this should become a completely separate
> binding.   Let's make the spontaneous stuff be a single separate binding.

Currently, spontaneous stuff is a separated model of the same binding. 
We could make this more clear by creating subsections in section 4.2. 
I'm not sure how creating an additional binding would increase the 
complexity of the document, since we might need also to duplicate 
profiles, etc.

>
> 17.  Section 7.3.1 - Bullet #1 - I am having problems with this because it
> would potentially kill the machine confirmation type as a request.   I am
> not even sure what it means for the identifiers to be different, you might
> want to discuss that instead.   Do you really mean to say that these names
> need to be, in some way, consistent instead?

Yes, I think that makes more sense.

>
> 18.  Neither in section 6 or section 7 is it made clear which RADIUS
> attribute is to be used in sending the request and the response.  This
> should be made explicit even if we don't create a new binding for the
> spontaneous sending of SAML from the IdP to the RP.

Right, that needs to be clarified.

>
> 19.  These are not RADIUS State confirmation methods - they are SAML Subject
> Confirmation methods.  As it currently stands, it is not clear where these
> go.

I think a better name would be "SAML V2.0 RADIUS State Subject 
Confirmation Methods", being more consistent with others such as the 
"SAML V2.0 Kerberos Subject Confirmation Method".


> 20.  I want to sit down with the authors in Dallas and talk about the change
> in names that was made for revision -10.   I think that many of these
> changes are potentially harmful for clarity of the document.

The idea behind this change was to make it consistent with the ABFAB 
terminology (as this is an ABFAB WG document). Before the change, we had 
in the document:

Client = Principal/User Agent/Client
IDP = SAML responder/SAML Issuer/IDP
RP = SAML requester/SAML comsumer/RP

At some points it was difficult for a non SAML expert to know who the 
SAML Issuer or the requester was, for instance.

Sadly, I will not be in Dallas. I don't know whether Sam or Josh will be 
there. In any case, we can make an audio conference at a time that suits 
us both.

Regards,
Alejandro


>
> Jim
>
>
>
>