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 >