Re: [Softwires] Review of draft-ietf-softwire-map-radius-08

[tianxiang li <[e-mail]>]

Hi Ian,

Many thanks for providing such a thorough review of the document.
We really appreciate your comments. Please see inline(TX>).

There are some issues in the draft which need editing, we will update a new
version soon, with these bits cleaned up.

Thank you and best regards,
Tianxiang

2016-11-15 17:23 GMT+08:00 Ian Farrer <[e-mail]>:

> Hi,
>
> Here’s a review of the updated version of the draft.
>
> Thanks,
> Ian
>
>
> General Comments:
> Please do language review throughout.
>
> Throughout the document, there are multiple places where the client is described as sending a ORO containing the ’S46 container options’, or similar (e.g. Figure 1, description text under figure 1). This should be 'S46 container option code’. Please revise throughout.
>
> TX>There are indeed a number of language problems, we will do a thorough
language review of the document.

>
> Figure 1
> 4. The DHCPv6 advertise message will have an S46 container option as well.
>
> General comment on figure 1 text
> There is a lot of requirements language here that I think is unnecessary. The purpose of the text is to describe how the message flow works and this can be done without the need for RFC2119 language. The use of SHOULD, particularly in point 2 leaves questions like: under what conditions is it acceptable for the DHCPv6 server NOT to fill in the shared password, and what are the expected effects of this?
>
> TX>Agree, we will remove some of the  requirements language, and make it
more concise. I think some wordings which are necessary should remain, e.g.
the MUST in point 3.

> Figure 1 Text
> 4.  Suggest "After receiving the Access-Accept message with the corresponding Attribute, the BNG SHOULD respond the user an Advertisement message.” is replaced with: "After receiving the Access-Accept message with the corresponding Attribute, the BNG SHOULD respond to the DHCPv6 client with an Advertisement message.”
>
> 6. The inclusion of DHCPv4o6 as an option here complicates things as it has it’s own set of message flows which are separate to this process. I’m not sure if this is also the case for PCP based provisioning. It may be best to limit the scope of this to the S46 options described in RFC7598 for clarity.
>
> TX>It's true, PCP also uses different message flows. We will limit the
scope of this to the S46 options described in RFC7598, and remove
descriptions of DHCP4o6 and PCP.

>
> Section 4.2
> As draft-ietf-softwire-unified-cpe-08 is just about to be published, and the draft allows for multiple S46 attributes, it would make sense to also have an attribute for OPTION_S46_PRIORITY so that operator prioritisation for multiple mechanisms can be supplied. This may need an IANA registry for the allowed values for this new RADIUS attribute with the same contents as Option Codes Permitted in the S46 Priority Option.
> A pointer and some text for RFC6519 (DSLite attribute) could be included will also be needed.
>
> TX>I think the solution here would be to consider OPTION_S46_PRIORITY as a
type of S46 Container Option in section 4.2, and define the s46-option-code
field as a Sub Option in section 4.3.

>
> Across all of the following attribute definitions, it might be helpful to ‘borrow’ the definitions in RFC7598 for consistency (e.g. which parts are variable, where 0 padding is used).
> 4.3.2
> The Sublen is defined as being 18. Shouldn’t this be variable (but a multiple of 16) if it is a list of addresses?
>
> BR-ipv6-address - This describes a single address, but the diagram shows a list of addresses.
>
> 4.3.3
> The Sublen is given as 8. Shouldn’t this be variable (1 for prefix len, variable for the prefix).
>
> TX>Agree, this should be varable length.

> dmr-ipv6-prefix-len
> (Note on this one, the RFC7598 text is wrong. It should allow values of 0-96 as described in RFC7599 Section 5.1. I have raised an errata on this)
>
> TX>Thanks for pointing that out, we will use 0-96 here.

> dmr-ipv6-prefix
> This states it is a 32 bit filed, but it’s meant to contain an IPv6 prefix.
>
> TX> Agree, this should be a variable field, which could be an IPv6 prefix
or address of the BR.
We will do a thorough edit of section 4 to keep the description unified
with RFC7598.

> 4.3.4
> There should only be a single
>
> TX>There is a typo here, it was meant to say "There MUST be at most one
S46-V4V6Bind Sub Option included in each Lighweight4over6 Container
Option".  This correlates to the phrase "There MUST be at most one
OPTION_S46_V4V6BIND option" in section 5.3 of RFC 7598.

>
> The Sublen should be variable.
>
> TX>Agree.

> 4.3.5
> Is 6 the correct SubLen here (I don’t know if the SubType and SubLen fields should be counted in the SubLen)?
>
> TX>Yes, the SubType and SubLen fields should be counted. We will also make
sure it's correct for the other options defined.

>
> Section 7.
> The word ‘proofing’ should probably be replaced with ‘spoofing’
>
> Can the dictionary attack on the shared password be mitigated? As this communication takes place between the DHCPv6 server and the RADIUS server, wouldn’t the use of ACLs preventing users being able to send requests directly to the RADIUS server prevent this type of attack?
>
> TX>  I think the dictionary attack here is used by the attacker on the BNG
server, to get the password which is pre-configured on the BNG. The
attacker could  then masquerading as a legitimate user to access
configuration information by requesting the BNG server, the BNG server
would then communicate with the AAA server. Would ACL prevent this?