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

From: "Jim Schaad" <[email-masked]>
Date: Thu, 19 Feb 2015 10:54:07 -0800

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.

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.

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.

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

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?

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

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.

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.

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

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.

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.

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/

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?

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.

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

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

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. 

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?

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.

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.

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.

Jim

---

From: Alejandro Perez Mendez <[email-masked]>
Date: Thu, 26 Feb 2015 12:00:45 +0100

Hi Jim,

thank you for the review. See my comments inline.

> reference should be killed.

Agree.

> this or the big packet size draft being implemented.

Makes sense.

> proximity.  Making both of the SAML items be protocol I think would clarify

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

> attributes will be used in requests and responses./

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

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

> everything following ", but this".

I agree.

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

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

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

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

> require seems to me to imply there are cases where HTTP based bindings could

Right.

> 	s/destination MAY be/destination may be/

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

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

> 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

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

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

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

> need to be, in some way, consistent instead?

Yes, I think that makes more sense.

> spontaneous sending of SAML from the IdP to the RP.

Right, that needs to be clarified.

> Confirmation methods.  As it currently stands, it is not clear where these

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

> 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