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

["Cantor, Scott" <[e-mail]>]

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.

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?

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.

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.

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.

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.

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?

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/

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

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?

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/

-- Scott