Re: [Softwires] Benjamin Kaduk's No Objection on draft-ietf-softwire-map-radius-24: (with COMMENT)

[Benjamin Kaduk <[e-mail]>]

On Fri, Jun 14, 2019 at 12:15:56PM +0000, [e-mail] wrote:
> Hi Ben, 
> 
> Please see inline.
> 
> Cheers,
> Med
> 
> > -----Message d'origine-----
> > De : Benjamin Kaduk [mailto:[e-mail]]
> > Envoyé : vendredi 14 juin 2019 01:58
> > À : BOUCADAIR Mohamed TGI/OLN
> > Cc : The IESG; [e-mail]; Yong Cui; Ian
> > Farrer; [e-mail]; [e-mail]
> > Objet : Re: Benjamin Kaduk's No Objection on draft-ietf-softwire-map-
> > radius-24: (with COMMENT)
> > 
> > On Thu, Jun 13, 2019 at 07:21:38AM +0000, [e-mail]
> > wrote:
> > > Re-,
> > >
> > > Thank you for the detailed review. Much appreciated, as usual!
> > >
> > > Please see inline.
> > >
> > > Cheers,
> > > Med
> > >
> > > > -----Message d'origine-----
> > > > De : Benjamin Kaduk via Datatracker [mailto:[e-mail]]
> > > > Envoyé : jeudi 13 juin 2019 06:03
> > > > À : The IESG
> > > > Cc : [e-mail]; Yong Cui; Ian Farrer;
> > > > [e-mail]; [e-mail]; [e-mail]
> > > > Objet : Benjamin Kaduk's No Objection on draft-ietf-softwire-map-
> > radius-
> > > > 24: (with COMMENT)
> > > >
> > > > Benjamin Kaduk has entered the following ballot position for
> > > > draft-ietf-softwire-map-radius-24: No Objection
> > > >
> > > > When responding, please keep the subject line intact and reply to all
> > > > email addresses included in the To and CC lines. (Feel free to cut
> > this
> > > > introductory paragraph, however.)
> > > >
> > > >
> > > > Please refer to https://www.ietf.org/iesg/statement/discuss-
> > criteria.html
> > > > for more information about IESG DISCUSS and COMMENT positions.
> > > >
> > > >
> > > > The document, along with other ballot positions, can be found here:
> > > > https://datatracker.ietf.org/doc/draft-ietf-softwire-map-radius/
> > > >
> > > >
> > > >
> > > > ----------------------------------------------------------------------
> > > > COMMENT:
> > > > ----------------------------------------------------------------------
> > > >
> > > > It might be worth a brief note somewhere about why attributes of this
> > > > sort can be included in Accounting-Request packets
> > >
> > > [Med] This is already covered by this text:
> > >
> > >    In some deployments, the DHCP server may use the Accounting-Request
> > >    to report to a AAA server the softwire configuration returned to a
> > >    requesting host.  It is the responsibility of the DHCP server to
> > >    ensure the consistency of the configuration provided to requesting
> > >    hosts.  Reported data to a AAA server may be required for various
> > >    operational purposes (e.g., regulatory).
> > 
> > So it is; thanks for pointing it out.  (I guess I would have expected to
> > see something earlier in the document, but this is up to your discretion.)
> > 
> > >
> > >  (and, to a lesser
> > > > extent, CoA-Request packets).
> > 
> > What about CoA-Request, though?
> > 
> 
> [Med] Added this NEW text: 
> 
>  A configuration change (e.g., BR address) may result in an exchange
>  of CoA-Requests between the BNG and the AAA server as shown in
>  Figure 3.  Concretely, when the BNG receives a CoA-Request message
>  containing Softwire46 attributes, it sends a DHCPv6 Reconfigure
>  message to the appropriate CE to inform that CE that an updated
>  configuration is available.  Upon receipt of such message, the CE
>  sends a DHCPv6 Renew or Information-Request in order to receive the
>  updated Softwire46 configuration.  In deployments where the BNG
>  embeds a DHCPv6 relay, CoA-Requests can be used following the
>  procedure specified in [RFC6977].

Thanks!

-Ben

> > > > Section 1
> > > >
> > > >    Since IPv4-in-IPv6 softwire configuration information is stored in
> > an
> > > >    AAA server, and user configuration information is mainly
> > transmitted
> > > >    through DHCPv6 between the BNGs and Customer Premises Equipment
> > (CEs,
> > > >    a.k.a., CPE), new RADIUS attributes are needed to propagate the
> > > >    information from the AAA servers to BNGs.
> > > >
> > > > nit: maybe "from the AAA servers to BNGs so that they can be
> > propagated
> > > > to CPE over the existing DHCPv6 options."?
> > > >
> > >
> > > [Med] Updated as follows:
> > >
> > > "from the AAA servers to BNGs so that they can be provided to CEs using
> > the existing DHCPv6 options."
> > 
> > Thanks
> > 
> > > > Section 2
> > > >
> > > > Please use the boilerplate text from RFC 8174 (including BCP 14
> > > > mention).
> > > >
> > >
> > > [Med] Added to BCP 14 mention.
> > >
> > > > Section 3.1
> > > >
> > > >       The Softwire46-Configuration Attribute conveys the configuration
> > > >       information for MAP-E, MAP-T, or Lightweight 4over6.  The BNG
> > > >       SHALL use the configuration information returned in the RADIUS
> > > >       attribute to populate the DHCPv6 Softwire46 Container Option
> > > >       defined in Section 5 of [RFC7598].
> > > >
> > > > nit: "Option(s)" seems more appropriate, since that section defines
> > > > separate options for MAP-E and MAP-T.
> > >
> > > [Med] OK.
> > >
> > > >
> > > > Section 3.1.1.1
> > > >
> > > > Just to double-check my understanding: since this is an "attribute",
> > >
> > > [Med] This is a sub-attribute that appears under Softwire46-
> > Configuration. This is discussed in the introduction text of Section
> > 3.1.1.
> > >
> > > > it's a top-level container with the 'type' value bearing universal
> > > > semantics for all RADIUS packets, as opposed to a "tlv" that appears
> > > > within a given attribute and whose 'type' values only have meaning in
> > > > the context of that attribute?  But this interpretation doesn't hold
> > up
> > > > for (e.g.) Section 3.3.3, which defines an "attribute" but uses a
> > > > "TLV-Type" that is in the range of values scoped only to the
> > structures
> > > > defined in this document.
> > > >
> > > > Section 3.1.3.2
> > > >
> > > >    There MUST be at least one Softwire46-BR included in each
> > > >    Softwire46-MAP-E or Softwire46-Lightweight-4over6.
> > > >
> > > > This seems to be in conflict with Table 2, which says that exactly one
> > > > Softwire-BR is included in each MAP-E or Lightweight-4over6 attribute.
> > >
> > > [Med] Good catch. Updated the table: s/1/1+.
> > >
> > > >
> > > > Section 3.1.6.1
> > > >
> > > >                          This field that specifies the
> > > >          numeric value for the Softwire46 algorithm's excluded
> > > >          port range/offset bits (a bits), as per Section 5.1
> > > >          of [RFC7597]. Allowed values are between 0 and 15.
> > > >
> > > > nit: "This field that specifies" is redundant; just "This field
> > > > specifies" would be  fine.
> > >
> > > [Med] OK.
> > >
> > > >
> > > > I'm also not sure I understand the range being between 0 and 15, when
> > we
> > > > previously talk about this being an 8-bit value ("right justified").
> > >
> > > [Med] We used to have a 8-bit field that we then updated to 32-bit to be
> > aligned with radext reco. Fixed.
> > >
> > > >
> > > > Section 3.1.6.3
> > > >
> > > > This format seems needlessly confusing.  We encode as an integer (32-
> > bit
> > > > unsigned), but then state that this 32-bit integer contains a 16-bit
> > > > value, right justified.  And then we only interpret the f irst 'k'
> > bits
> > > > on the left of the 16-bit value.  Isn't there a simpler way to encode
> > a
> > > > 'k' bit value in a 32-bit field?
> > >
> > > [Med] We used to have a 16-bit field but changed it to 32-bit to be
> > aligned with RFC6158. The proposed approach is straightforward for
> > extracting the psid bits.
> > >
> > > >
> > > > Section 3.2
> > > >
> > > >           Softwire46 mechanisms are prioritized in the appearance
> > order
> > > >           of the in the Softwire46-Priority Attribute.
> > > >
> > > > Do we want to say explicitly that the first-appearing mechanism is
> > most
> > > > preferred?
> > >
> > > [Med] We can.
> > >
> > > >
> > > > Section 3.3.1
> > > >
> > > >     TLV-Length
> > > >        16 octets. The length of asm-prefix64 must be to 96 [RFC8115].
> > > >
> > > > nit(?): I can't parse the grammar here -- what does "must be to" mean?
> > > > (Same question for following section.)
> > >
> > > [Med] Updated to: "The length of asm-prefix64 must be /96 [RFC8115]."
> > >
> > > >
> > > > Please expand "ASM" on first use.
> > >
> > > [Med] Done.
> > >
> > > >
> > > > Section 4
> > > >
> > > >    1.  The CE creates a DHCPv6 Solicit message.  For unicast softwire
> > > >        configuration, the message includes an OPTION_REQUEST_OPTION
> > (6)
> > > >        with the Softwire46 Container option codes as defined in
> > > >        [RFC7598].  [...]
> > > >
> > > > nit: with all of them (the Softwire46 Container option codes)?
> > >
> > > [Med] Not all of them because some mechanisms may not be supported. I
> > updated the text to:
> > >
> > > " Softwire46 Container option code(s)"
> > >
> > > >
> > > >    2.  On receipt of the Solicit message, the BNG constructs a RADIUS
> > > >        Access-Request message containing a User-Name Attribute (1)
> > > >        (containing either a CE MAC address, interface-id or both), a
> > > >        User-Password Attribute (2) (with a pre-configured shared
> > > >        password as defined in [RFC2865].  The Softwire46-Configuration
> > > >        Attribute and/or Softwire46-Multicast Attribute are also
> > included
> > > >        (as requested by the client).  The resulting message is sent to
> > > >        the AAA server.
> > > >
> > > > Perhaps clarify whether the shared password is shared between BNG/AAA
> > > > server, or CE/AAA server?
> > >
> > > [Med] Updated with an explicit mention of CE-AAA server.
> > 
> > Thanks!
> > 
> > > >
> > > >    6.  The BNG sends a Reply message to the client containing the
> > > >        softwire container options enumerated in the ORO.
> > > >
> > > > nit: maybe "DHCPv6 Reply" to match the "DHCPv6 Request" in step (5)?
> > >
> > > [Med] Fixed.
> > >
> > > >
> > > >    The authorization operation could also be done independently, after
> > > >    the authentication process.  In this case, steps 1-5 are completed
> > as
> > > >    above, then the following steps are performed:
> > > >
> > > > nit: "authorization" or "authorize" do not appear in the previous
> > > > procedure anywhere; it would be rhetorically more clear what this
> > > > statement refers to if there was explicit mention in the prior
> > > > procedure.
> > >
> > > [Med] Will double check this one.
> > >
> > > >
> > > >    o  In both the configuration message flows described above the
> > > >       Message-authenticator (type 80) [RFC2869] SHOULD be used to
> > > >       protect both Access-Request and Access-Accept messages.
> > > >
> > > > Why is this SHOULD and not MUST?  Maybe an example of when it would
> > not
> > > > be needed would be helpful?
> > >
> > > [Med] The set of requirements are deployment-oriented. I already changed
> > my local copy to avoid the use of normative language. This applies also
> > for the following comments.
> > 
> > Okay
> > 
> > > > nit: Message-Authenticator has two capital letters.
> > > [Med] Fixed.
> > >
> > > >
> > > >    o  As specified in [RFC8415], Section 18.2.5, "Creation and
> > > >       Transmission of Rebind Messages", if the DHCPv6 server to which
> > > >       the DHCPv6 Renew message was sent at time T1 has not responded
> > by
> > > >       time T2, the CE (DHCPv6 client) SHOULD enter the Rebind state
> > and
> > > >       attempt to contact any available server.  In this situation, a
> > > >
> > > > This seems to just be restating the requiremnets from RFC 8415, in
> > which
> > > > case the normative language is not needed...
> > > >
> > > >       secondary BNG receiving the DHCPv6 message MUST initiate a new
> > > >       Access-Request message towards the AAA server.  The secondary
> > BNG
> > > >       includes the Softwire46-Configuration Attribute in this Access-
> > > >       Request message.
> > > >
> > > > ... but this MUST does need to use normative language (as it currently
> > > > does).
> > > >
> > > >    o  For Lightweight 4over6, the subscriber's binding state needs to
> > be
> > > >       synchronized between the clients and the Lightweight AFTR
> > > >       (lwAFTR)/BR.  This can be achieved in two ways: static pre-
> > > >
> > > > We have not previously talked about the "subscriber"; is there some
> > > > realignment of terminology or earlier definition that would help
> > clarify
> > > > how the subscriber relates to the other entities in play?
> > > >
> > >
> > > [Med] Changed to "CE".
> > >
> > > >       configuration of the bindings on both the AAA server and lwAFTR,
> > > >       or on-demand whereby the AAA server updates the lwAFTR with the
> > > >       subscriber's binding state as it is created or deleted.
> > > >
> > > > (I assume this is done "out of band" from the perspective of this
> > > > document.)
> > >
> > > [Med] Yes.
> > >
> > > >
> > > > Section 6
> > > >
> > > > As the secdir reviewer notes, there are a lot of references here.
> > > > That's usually good, avoiding duplication of content/etc., but this
> > text
> > > > seems to claim that there is discussion of known security
> > > > vulnerabilities in RADIUS discussed in RFCs 2607, 2865, and 2869, and
> > a
> > > > quick review of those documents does not find extensive discussion of
> > > > such vulnerabilities (and not much in the Security Considerations
> > > > section where one might expect to find it).  I think that just those
> > > > references may not provide a comprehensive picture of the state of
> > > > RADIUS (in)security, and so some additional exposition to tie together
> > > > what those documents are saying (including Section references as
> > > > appropriate), as well as potential other information, would be in
> > order.
> > >
> > > [Med] Will update the text with the exact section references.
> > >
> > > > I note that there was some fairly recent discussion of the general
> > state
> > > > of RADIUS security in the IESG processing of
> > > > draft-ietf-radext-coa-proxy, along the lines of "it basically boils
> > down
> > > > to you have to trust your neighbor/contractual arrangement".  I do
> > *not*
> > > > think that the three RFCs from 2000 convey that sentiment well, and so
> > > > the following paragraph's statement about targetting deployments with
> > a
> > > > trust relationship is quite important.  Perhaps we can tweak the
> > writing
> > > > a bit so that these two points tie together more clearly.
> > >
> > > [Med] Any suggestion to better convey this message?
> > 
> > I'd just add another sentence at the end of the second paragraph: "These
> > well-established properties of the RADIUS protocol place some limitations
> > on how it can safely be used, since there is some inherent requirement to
> > trust the counterparty to not misbehave." and start the third paragraph
> > with "Accordingly, [...]".
> > 
> 
> [Med] Works for me. Updated the text accordingly. 
> 
> > > >
> > > > It may be worth mentioning earlier (in the Introduction?) that this
> > > > document only targets deployments with a trusted relationship between
> > > > RADIUS client/server (that can use IPsec/TLS as appropriate).
> > >
> > > [Med] We can but IMHO this will be redundant.
> > 
> > It's your call.
> > 
> > > >
> > > > I am also not entirely sure about the "does not introduce any security
> > > > issue other than the ones already identified in RADIUS documents",
> > > > though admittedly the additional exposure seems quite minimal.
> > > > Specifcally, without these RADIUS attributes, DHCPv6 negotiation of
> > > > softwires includes in-band protocol flows conveying softwire
> > > > configuration information between DHCP client and BNG, but now that
> > > > information flows additionally to the AAA server.  The additional
> > > > exposure seems minimal, though, since the necessary softwire
> > > > configuration information inherently needs to be in *some* central
> > > > location, so we're just exchanging one way of configuring the BNG
> > ("out
> > > > of band") for another (RADIUS).  The most notable difference would
> > seem
> > > > to be that now we have to trust the AAA server to not leak the
> > softwire
> > > > configuration details (as opposed to whatever central configuration
> > > > server would otherwise be used), but since both are rather inherently
> > > > trusted roles, there's not in the general case more attack surface to
> > > > worry about.
> > >
> > > [Med] How is this specific to the attributes defined in the document?
> > 
> > It's only specific to the attributes in this document in that it's the
> > information content of these attributes getting a slightly different
> > exposure.  I think I recently suggested on a different document a
> > rewording
> > like "does not introduce any security issues inherently different from
> > those already identified in [other documents]" for a different document,
> > if
> > you were interested in something like that.
> > 
> 
> [Med] Ok, thanks.
> 
> > > >
> > > > Section 7.3
> > > >
> > > > If the registry is to be "Option Codes Permitted in the
> > > > Softwire46-Priority Attribute", that seems to imply that there are
> > some
> > > > option codes *not* permitted.  Where are those option codes
> > enumerated?
> > > > (I would have guessed at
> > > > https://www.iana.org/assignments/dhcpv6-parameters/dhcpv6-
> > > > parameters.xhtml#dhcpv6-parameters-2
> > > > , but the values don't seem to match up.)  Do we need to add a note
> > > > somewhere about future allocations of such option codes needing to
> > > > decide whether or not they are permitted in the Softwire46-Priority
> > > > Attritube?
> > >
> > > [Med] New (attribute) codes that can be listed will naturally include a
> > IANA request to be added to the registry. I'm not sure a note is needed so
> > that each new attribute has to declare whether it can be listed or not.
> > 
> > I think my point here is that I'm confused whether this is a registry of
> > freshly allocated values or a subset of values allocated elsewhere (and
> > thus reusing the numerical value from the other registry).  It sounds like
> > it's the former, though, so it's just the "Permitted in" in the title that
> > confuses me (vs. "for" or similar) ... but looking at the IANA page this
> > wording seems to be pretty widespread already, so I don't see any grounds
> > to change this one just on my account.
> > 
> 
> [Med] Thank you for clarifying. I maintained the text as it is. 
> 
> > Thanks,
> > 
> > Ben