Re: Gen-ART LC review of draft-ietf-abfab-aaa-saml-12

Alejandro Pérez Méndez <alex@xxxxx> · Thu, 10 Dec 2015 08:32:02 +0100



    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?

    
        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.

    
        Nits/editorial comments:
        
          In 
            section 1 please expand ABFAB
        
      
    Ok

    
          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