Re: [abfab] Review of draft-ietf-abfab-aaa-saml-11

Alejandro Pérez Méndez <alex@um.es> Fri, 16 October 2015 09:55 UTC

Return-Path: <alex@um.es>
X-Original-To: abfab@ietfa.amsl.com
Delivered-To: abfab@ietfa.amsl.com
Received: from localhost (ietfa.amsl.com [127.0.0.1]) by ietfa.amsl.com (Postfix) with ESMTP id A842E1A0097 for <abfab@ietfa.amsl.com>; Fri, 16 Oct 2015 02:55:07 -0700 (PDT)
X-Virus-Scanned: amavisd-new at amsl.com
X-Spam-Flag: NO
X-Spam-Score: -3.911
X-Spam-Level:
X-Spam-Status: No, score=-3.911 tagged_above=-999 required=5 tests=[BAYES_00=-1.9, MIME_8BIT_HEADER=0.3, RCVD_IN_DNSWL_MED=-2.3, SPF_PASS=-0.001, T_RP_MATCHES_RCVD=-0.01] autolearn=ham
Received: from mail.ietf.org ([4.31.198.44]) by localhost (ietfa.amsl.com [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id ivaxXMtbYyiI for <abfab@ietfa.amsl.com>; Fri, 16 Oct 2015 02:55:04 -0700 (PDT)
Received: from xenon21.um.es (xenon21.um.es [155.54.212.161]) by ietfa.amsl.com (Postfix) with ESMTP id 32CFC1A0074 for <abfab@ietf.org>; Fri, 16 Oct 2015 02:55:04 -0700 (PDT)
Received: from localhost (localhost [127.0.0.1]) by xenon21.um.es (Postfix) with ESMTP id 821B43F987 for <abfab@ietf.org>; Fri, 16 Oct 2015 11:55:02 +0200 (CEST)
X-Virus-Scanned: by antispam in UMU at xenon21.um.es
Received: from xenon21.um.es ([127.0.0.1]) by localhost (xenon21.um.es [127.0.0.1]) (amavisd-new, port 10024) with LMTP id enuT6LNbRz-N for <abfab@ietf.org>; Fri, 16 Oct 2015 11:55:02 +0200 (CEST)
Received: from [155.54.204.2] (alex.inf.um.es [155.54.204.2]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) (Authenticated sender: alex) by xenon21.um.es (Postfix) with ESMTPSA id 67FFB3F985 for <abfab@ietf.org>; Fri, 16 Oct 2015 11:55:01 +0200 (CEST)
To: abfab@ietf.org
References: <9846A6064BD102419D06814DD0D78DE112712074@CIO-TNC-D2MBX02.osuad.osu.edu>
From: =?UTF-8?Q?Alejandro_P=c3=a9rez_M=c3=a9ndez?= <alex@um.es>
Message-ID: <5620C974.30400@um.es>
Date: Fri, 16 Oct 2015 11:55:00 +0200
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.3.0
MIME-Version: 1.0
In-Reply-To: <9846A6064BD102419D06814DD0D78DE112712074@CIO-TNC-D2MBX02.osuad.osu.edu>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Transfer-Encoding: 7bit
Archived-At: <http://mailarchive.ietf.org/arch/msg/abfab/CLdwoP6P7zXvoh0ngXBzeY6cztE>
Subject: Re: [abfab] Review of draft-ietf-abfab-aaa-saml-11
X-BeenThere: abfab@ietf.org
X-Mailman-Version: 2.1.15
Precedence: list
List-Id: "Application Bridging, Federated Authentication Beyond \(the web\)" <abfab.ietf.org>
List-Unsubscribe: <https://www.ietf.org/mailman/options/abfab>, <mailto:abfab-request@ietf.org?subject=unsubscribe>
List-Archive: <https://mailarchive.ietf.org/arch/browse/abfab/>
List-Post: <mailto:abfab@ietf.org>
List-Help: <mailto:abfab-request@ietf.org?subject=help>
List-Subscribe: <https://www.ietf.org/mailman/listinfo/abfab>, <mailto:abfab-request@ietf.org?subject=subscribe>
X-List-Received-Date: Fri, 16 Oct 2015 09:55:07 -0000

Hi Scott,

thanks for this thorough review. See my comments inline.

> Sorry this is a little late. I gave it a pretty thorough read.
>
> Substantive:
>
> Is it useful to say something in section 3 along the lines that apart from the binding defined in section 4, the spec doesn't prescribe any particular pattern/usage/constraint/etc. on the use of the two RADIUS attributes it defines? I ask because of the statement in section 4 about the two attributes not being used in the same RADIUS message, and wondered if that should be stated in section 3, but then thought maybe that wasn't something being constrained by the spec, but just the binding.

I think that it would make sense to move the restriction up to section 
3, as one can see both attributes as exclusive alternatives to represent 
SAML information. If the RADIUS server decides to send a SAML Message, 
why sending a SAML Assertion aside?

>
> In 4.2 where it talks about the Status element, if the message is one that includes the Status element, then by definition it has to have a Status element in it, but you probably mean to say that the Status element should have an appropriate value in it. I'm not sure that's a SHOULD or MAY, really. Seems like a MUST, in that if you're including a response, it's going to have to have a Status whether you want one or not. And obviously it shouldn't be something silly. Just reads kind of oddly to me. Was there some point this was trying to make that I'm not getting?

I didn't write that text, so we have to hear the original authors' 
opinion here. I guess the point was that the SAML error response is 
optional, and that the RADIUS server, in presence of SAML errors, can 
just limit to provide a RADIUS-only response.

Nevertheless, I think the last two paragraphs of section 4.2 could be 
reduced to the following text, it the original authors agree:

    "In the case of a SAML processing error, the RADIUS server MAY include
     a SAML response message with an appropriate value for the 
<samlp:Status>
     element within the Access-Accept or Access-Reject packet to notify
     this fact to the Client.".

Does it read better?

>
> Some metadata corrections in section 4.3.3:
>
> In 4.3.3.1 and 4.3.3.2, it's saying that it's defining elements (and using the <element> notation), but there actually isn't much point in defining those role elements. They can't appear in a metadata file because EntityDescriptor doesn't allow for extension role *elements*, only extension role types via <RoleDescriptor xsi:type="...">
>
> Up to you guys, but I think it will create more confusion than help anything by defining the elements. And in fact, your examples in 4.3.4 are wrong for this reason, the elements can't appear in the EntityDescriptor. Should look like this:
>
>       <EntityDescriptor xmlns="urn:oasis:names:tc:SAML:2.0:metadata"
>                         entityID="https://IdentityProvider.com/SAML">
>           <RoleDescriptor xsi:type="abfab:RADIUSIDPDescriptorType" protocolSupportEnumeration=
>                                   "urn:oasis:names:tc:SAML:2.0:protocol">
> etc.
>
> If you need me to, I can edit the draft appropriately to reflect all this XML correction if it will save you time. It's critical that the examples don't have errors, obviously.

It'd be great if you could do that, thanks!

>
> Also, I don't see an actual schema appendix. As I understood it, the RFC has to stand alone, so I think the schema has to be included in full form in an appendix. That's what I did in the SAML-EC work. We need a namespace defined as well (and I guess that would also be added to the ABFAB URN registry in Section 11?), your schema fragments don't specify it. So it's not quite done. Again, I'm happy to do that bit if you wanted to pass me the pen.

Yes, please :)

>
> In 7.4.2, it mentions including an <Assertion> using the element notation. Sorry if this is old ground, but is XML Encryption intended to be ruled out? One might infer that because of the literal mention of <Assertion> but not <EncryptedAssertion>. Don't know if that's intentional.

Section 10 indicates ML signatures and encryption are optional, so I'd 
say they are not ruled out entirely.

>
> Also in 7.4.2, it mentions InResponseTo, and I think it's talking about the SubjectConfirmationData element's attribute by that name, not the Response element's attribute. You probably want both though.

Yes. It refers to the SubjectConfirmationData element attribute.

>
> In 7.4.7 and 8.3.4, I think it should read "particular to this profile", but maybe it should say something more to the effect that there are none aside from those applying to the use of the RADIUS binding?

Right.

>
> Nits:
>
> Section 1 intro:
> s/In particular where this document provides/In particular, this document provides/
>
> Section 4.2 (and 7.4.5):
> s/provide confidentiality and improve integrity protection/provide confidentiality and integrity protection/
>
> s/other arbitrary RADIUS attributes will be used/other arbitrary RADIUS attributes MAY be used/
>
> Section 4.3:
> The verbage "profiles of this binding" appears a few times. I think it's more accurate in SAML terms to say "profiles making use of this binding".
>
> Section 4.3.2:
> s/<entityId>/"entity ID"/
> s/establish a relation/establish a relationship/
> s/SAML federation metadata/SAML metadata/
>
> Section 4.3.3:
> s/represent AAA names associated to a particular/represent AAA names associated with a particular/
>
> Section 4.3.3.1/4.3.3.2:
> s/associated to this Entity/associated with the entity/

Thanks!

>
> The reference link to section 3.4 of RFC7055 is not linking to the RFC in the HTML version of the draft.

Yes, those ones are automatically generated (I just have plain text on 
the XML file).
I have to check whether xml2rfc has an option to disable automatic 
references being generated.
Otherwise, I guess I can make them an actual reference, linking to the 
right document.
>
> Section 7.3:
> "Where this specification conflicts with it, the former takes precedence."
> A little muddy which spec is meant to take precedence. I think you mean the ABFAB profile does?
Would the following be clearer?

    The ABFAB Authentication Profile is a profile of the SAML V2.0
    Authentication Request Protocol [OASIS.saml-core-2.0-os].  Where both
    specifications conflict, the ABFAB Authentication Profile takes 
precedence.

>
> Section 7.4.2:
> s/is subject conform to the following/is subject to the following constraints/
>
> Section 9:
> s/required to being able to route/required to route/
> s/Other kind of attributes/Other kinds of attributes/

Again, thank you for the review, and for your offer to help us with the 
XML corrections.

Regards,
Alejandro

>
> -- Scott
>
> _______________________________________________
> abfab mailing list
> abfab@ietf.org
> https://www.ietf.org/mailman/listinfo/abfab