Hiya,
On 10/12/15 07:32, Alejandro Pérez Méndez wrote:
Dear Roni and chairs of the ABFAB WG,
thank you for the revision. Please, see my responses inline (specially
the one related to point #2)
El 03/12/15 a las 15:31, Roni Even escribió:
I am the assigned Gen-ART reviewer for this draft. For background on
Gen-ART, please see the FAQ at
<http://wiki.tools.ietf.org/area/gen/trac/wiki/GenArtfaq>.
Please resolve these comments along with any other Last Call comments
you may receive.
Document: draft-ietf-abfab-aaa-saml-12
Reviewer: Roni Even
Review Date:2015–12-3
IETF LC End Date: 2015–12-4
IESG Telechat date:
Summary: This draft is almost ready for publication as an
Informational RFC.
Major issues:
Minor issues:
1.Why is the RADIUSNasIpAddress a string and not as specified in for
example in RFC2865
The RADIUSNasIpAddress is a SAML metadata element, thus it has to comply
with existing SAML types. The string type allows to encode the "display"
value of these RADIUS attributes (e.g. "192.168.1.1", or "::1"). Note
that current text specifies that the element contains an acceptable
value for RADIUS NAS-IP-Address or RADIUS NAS-IPv6-Address attributes,
so no arbitrary values are accepted nonetheless.
2.In general I was wondering why this is an Informational document. It
defines procedures and has normative language.
That sounds like kind of an unfortunate bug. For some reason, it changed
from Standards Track to Informational between versions -00 and -01.
However, we want it standards-track with a normative downreference to
radsec. Can it be done at this moment or does it require a more complex
process?
Hmm. The shepherd write-up says informational is correct. If the WG
chairs want to, we can re-spin the IETF LC. But this has been so
long in the process and has slowly so I'd prefer to not do that
unless someone really cares, and it makes a difference.
For now, I've kept this on the Dec17 IESG telechat as informational
but if needed we can push it into the new year.
3.In the IANA consideration in section 11.1, as far as I understand
the IANA attribute type registry you need to ask for values for TBD1
and TBD2 from the unassigned space (and not the reserved space)
I agree. I cannot find where we state otherwise, though. Could you point
the specific text where we say it?
4.In step 2 of figure 7 (section 7.2) the text says “In step 2, the
Relying Party may optionally issue a <samlp:AuthnRequest> message to
be delivered to the Identity Provider using the SAML-Protocol RADIUS
attribute.” My reading is that the rest of the steps are when this
message is sent, since it is “may” what happens if the message is not
sent?
If the <AuthnRequest> is not sent, the procedure follows the
"unsolicited response" (explained in 7.4.4), where the IdP deliveres a
<samlp:Assertion> element. I agree that step 4 needs to include this
clarification. The new text should read as:
4. Identity Provider issues <samlp:Response> to Relying Party
(Section 7.3.4). In step 4, the Identity Provider issues a
<samlp:Response> message to the Relying Party using the SAML
RADIUS binding. The response either indicates an error or
includes a SAML Authentication Statement in exactly one SAML
Assertion. If the RP did not send an <samlp:AuthnRequest>, the IdP
issues an unsolicited <samlp:Assertion>, as described in section 7.4.4.
I'll add the above as an RFC editor note.
Cheers,
S.
Nits/editorial comments:
1. In section 1 please expand ABFAB
Ok
1.
2. In section 7.2, the text says “To implement this scenario, a
profile of the SAML Authentication Request protocol is used in
conjunction with the SAML RADIUS binding defined in Section 4.” I
think that the language should be more normative maybe it should
say “To implement this scenario, this profile of the SAML
Authentication Request protocol MUST Be (or SHOULD if there are
other options) used in conjunction with the SAML RADIUS binding
defined in Section 4.”
Agree. I think "MUST be" is the one to be used.
Best regards,
Alejandro