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

From: "Cantor, Scott" <[email-masked]>
Date: Fri, 16 Oct 2015 03:12:19 +0000

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

---

From: Alejandro Pérez Méndez <[email-masked]>
Date: Fri, 16 Oct 2015 11:55:00 +0200

Hi Scott,

thanks for this thorough review. See my comments inline.

> 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 th…

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 t…

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?

> 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 w…

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 m…

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 th…

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…

Right.

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

> 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
> [email-masked]
> https://www.ietf.org/mailman/listinfo/abfab

---

From: Sam Hartman <[email-masked]>
Date: Fri, 16 Oct 2015 08:55:38 -0400

>>>>> "Alejandro" == Alejandro Pérez Méndez <[email-masked]> writes:

    Alejandro> Hi Scott, thanks for this thorough review. See my
    Alejandro> comments inline.

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

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

Agreed.

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

Yes, that's the intent as I understand it.

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

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

I think the following would be more clear:

    "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 the client.
 Alternatively, the RADIUS server can respond without a SAML-Message attribute.".

Or did we end up calling it SAML-Protocol?

--Sam

---

From: "Cantor, Scott" <[email-masked]>
Date: Fri, 16 Oct 2015 13:16:59 +0000

> Does it read better?

I think Sam's suggestion works, and now I understand what you were trying to say, thanks.

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

I can take an edit pass within the next few days to get the XML fixed, can you pass me a copy of the WD XML source? Or if you want to finish up your edits and then pass it along, that's fine.

> say they are not ruled out entirely.

In that case I'd use wording that says "<saml:Assertion> or <saml:EncryptedAssertion>". Or just use "assertion" in lower case.

>     specifications conflict, the ABFAB Authentication Profile takes

That's fine.

-- Scott

---

From: Alejandro Pérez Méndez <[email-masked]>
Date: Fri, 16 Oct 2015 16:27:02 +0200

>   Alternatively, the RADIUS server can respond without a SAML-Message attribute.".

That sounds better, yes.

> Or did we end up calling it SAML-Protocol?

Oh, right. I'll change that.

Regards,
Alejandro

---

From: Alejandro Pérez Méndez <[email-masked]>
Date: Fri, 16 Oct 2015 16:28:54 +0200

> I think Sam's suggestion works, and now I understand what you were trying to say, thanks.

I agree.

> I can take an edit pass within the next few days to get the XML fixed, can you pass me a copy of the WD XML source? Or if you want to finish up your edits and then pass it along, that's fine.
Whichever works better for you. It might be better if you do the XML 
fixes before, so I can adapt the text afterwards. Let me send you the 
XML file in a private mail.

> In that case I'd use wording that says "<saml:Assertion> or <saml:EncryptedAssertion>". Or just use "assertion" in lower case.

I think using assertion might be better, since this might happen in 
other places along the document.

Regards,
Alejandro

---

From: Stefan Paetow <[email-masked]>
Date: Mon, 19 Oct 2015 21:38:48 +0000

>Or did we end up calling it SAML-Protocol?

Which? The RADIUS attribute? SAML-AAA-Assertion. I don't see any other
SAML-named attributes anywhere in a FR dictionary.

Stefan Paetow
Moonshot Industry & Research Liaison Coordinator

t: +44 (0)1235 822 125
gpg: 0x3FCE5142
xmpp: [email-masked]
skype: stefan.paetow.janet

jisc.ac.uk

Jisc is a registered charity (number 1149740) and a company limited by
guarantee which is registered in England under Company No. 5747339, VAT
No. GB 197 0632 86. Jisc¹s registered office is: One Castlepark, Tower
Hill, Bristol, BS2 0JA. T 0203 697 5800.

Jisc Collections and Janet Ltd. is a wholly owned Jisc subsidiary and a
company limited by guarantee which is registered in England under Company
No. number 2881024, VAT No. GB 197 0632 86. The registered office is:
Lumen House, Library Avenue, Harwell, Didcot, Oxfordshire, OX11 0SG. T
01235 822200.

---

From: Alejandro Pérez Méndez <[email-masked]>
Date: Tue, 20 Oct 2015 09:27:39 +0200

> SAML-named attributes anywhere in a FR dictionary.

The "SAML-Message" attribute is now called "SAML-Protocol". The 
"SAML-Assertion" keeps the name.

The attribute in FR is a Vendor-Specific one, assigned to UKERNA, so it 
can be called whichever they want, since the one in the IETF draft has 
not been standardized yet.
When the RFC is published, that will change to the proper name. 
Something similar happended with RFC 7055's attributes. They were moved 
from dictionary.ukerna to dictionary.7055, with proper asignements of 
attribute numbers.

Regards,
Alejandro

---

From: Stefan Paetow <[email-masked]>
Date: Tue, 20 Oct 2015 07:58:44 +0000

> The "SAML-Message" attribute is now called "SAML-Protocol". The "SAML-Assertion" keeps the name.
> 
> The attribute in FR is a Vendor-Specific one, assigned to UKERNA, so it can be called whichever they want, since the one in the IETF draft has not been standardized yet.
> When the RFC is published, that will change to the proper name. Something similar happended with RFC 7055's attributes. They were moved from dictionary.ukerna to dictionary.7055, with proper asignements of attribute numbers.

That may be the case, but I checked the UKERNA dictionary and didn't find it there... In fact that's the first place I looked because I assumed the above.

So it's probably a good idea to submit a pull request to make sure this is in FR.

Stefan Paetow

t: +44 (0)1235 822 125
gpg: 0x3FCE5142
xmpp: [email-masked]
skype: stefan.paetow.janet
Lumen House, Library Avenue, Harwell Oxford, Didcot, OX11 0SG

jisc.ac.uk

Jisc is a registered charity (number 1149740) and a company limited by guarantee which is registered in England under Company No. 5747339, VAT No. GB 197 0632 86. Jisc’s registered office is: One Castlepark, Tower Hill, Bristol, BS2 0JA. T 0203 697 5800.
Jisc Collections and Janet Ltd. is a wholly owned Jisc subsidiary and a company limited by guarantee which is registered in England under Company No. number 2881024, VAT No. GB 197 0632 86. The registered office is: Lumen House, Library Avenue, Harwell, Didcot, Oxfordshire, OX11 0SG. T 01235 822200.