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

[Benjamin Kaduk <[e-mail]>]

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?

> > 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, [...]".

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

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

Thanks,

Ben