Subject: Re: [radext] Review request for draft-ietf-softwire-map-radius

From: "Yu Fu" <[email-masked]>
Date: Tue, 22 Jan 2019 16:01:36 +0800

Hi Stefan, 

Thank you very much for your comments.

We have added a paragraph at the end of section 4 to clarify your questions in the 18 version.

Please see the updated version as below:

https://datatracker.ietf.org/doc/draft-ietf-softwire-map-radius/  

Hope the updated version could answer your questions.

Comments are also welcomed.

BR
Yu

------------------ Original ------------------
From:  "Winter Stefan"<[email-masked]>;
Date:  Mon, Nov 12, 2018 08:29 PM
To:  "[email-masked]"<[email-masked]>;
Cc:  "draft-ietf-softwire-map-radius"<[email-masked]>; 
Subject:  Re: Review request for draft-ietf-softwire-map-radius

Hello,

and here is a quick question to begin with: the document states that the
attributes can occur in an Accounting-Request packet. However, nowhere
in the document is the corresponding semantics of that transmission defined.

I.e. why would a BNG or other RADIUS Client send this in an
Accounting-Request? What would it want to achieve? And how does the
RADIUS server react if it gets conflicting information there (say, IP
addresses in the sub-TLVs differ in the Accounting-Request from what was
sent in the preceding Access-Accept)?

Greetings,

Stefan Winter

Am 12.11.18 um 13:25 schrieb Stefan Winter:
> Hello radext,
>
> we have received a request for review of an I-D from the softwire
> working group, please see below.
>
> I would appreciate if you could take a few moments of your time to
> review this document and respond here to the working group, with the
> draft's mail address in cc.
>
> Greetings,
>
> Stefan Winter
>
> -------- Weitergeleitete Nachricht --------
>
> We have the following wg document, RADIUS Attributes for Address plus
> Port (A+P) based Software Mechanisms.
> https://datatracker.ietf.org/doc/draft-ietf-softwire-map-radius/
>
> There has been a lot of work on getting draft ready for publication from
> the opinion of softwire wg. However, because there are several new
> attributes and a lot of sub-TLVs, we think we need to get RADEXT to help
> review, before we advance this doc to IESG.
>

---

From: "Yu Fu" <[email-masked]>
Date: Thu, 31 Jan 2019 09:57:01 +0800

Hi Alan,

We have updated the 18 version based on all your comments at the high level and in detail.

Please find the corresponding changes as  below:

>  At a high level:

>* Please remove all ASCII art.  RFC 8044 (RADIUS data types) Section 2.1 suggests that this is no longer necessary.

>* Please use the attribute definition format suggested in RFC 8044 Section 2.1.3.  Doing so will make the document clearer

>* Most of the data types in this document fit the data types in RFC 8044.  If not, the considerations of RFC 6158 Section 3.2.3 on "Complex >Data Types" need to be addressed.

[fuyu]: We have referenced the RFC 8044 for the attribute format and cited it in the definition. 

>* Please decrease the use of "TLV"  An attribute can be referred to by name. e.g. "S46-MAP-E".  It does not need to be referred to as "S46-
>MAP-E TLV"

[fuyu]: We have decrease the use of TLV in the section 3 of the 18 version. 

>  And a review in detail:

>3.  Extensions of RADIUS Attributes and TLVs

>I suggest just calling this "New RADIUS Attributes".  There already exists "RADIUS extensions" documents, which do radically different things.

>      The Softwire46-Configuration Attribute MUST contain one or more of
>      the following: S46-MAP-E TLV, S46-MAP-T TLV, and/or S46-

>      Lightweight-4over6 TLV.

>  "the following... " what?  I suggest:

>      The Softwire46-Configuration Attribute MUST contain one or more of
>      the following attributes: S46-MAP-E, S46-MAP-T, and/or S46-
>      Lightweight-4over6.

>  Similar comments apply to many other places in the document.

[fuyu]: Please see that we have deleted "TLV" in the corresponding place.

>  Also, is there any reason to shorten the attribute names?  "Softwire46-MAP-E" would be clearer than "S46-MAP-E".

[fuyu]: Please see that we have updated "Softwire46" instead of "S46" in the whole document.

>  Figure 1 is useful and informative.  NIT: I would remove the repeated use of "TLV" and "sub-TLV".  Which attribute contains what is clear from >the diagram.

[fuyu]: Done

>3.1.1.1.  S46-MAP-E TLV Format

>  I would rename this section "S46-MAP-E Attribute".  The section describes the attribute, not just it's format.  If RFC 8044 is referenced, the >formats are defined there.  And don't need to be copied here.

[fuyu]: Please see that we don't copy the format here and just cite it from the RFC 8044 in the new version.

>  The section also doesn't say what the attribute is *for*.  While that description is alluded to in the rest of the document, it it not clear.  Again, >using the layout recommended by RFC 8044 would be useful.  That layout includes a portion for a description of what the attribute does.

>  Similar comments apply to many other places in the document.

[fuyu]: We have added one sentence to describe each attribute is "for"...

>Table 2: Softwire46 Sub-TLVs

>  The use of "M - Mandatory, O - Optional, N/P - Not Permitted" is unusual.  I suggest using a table format similar to RFC 2865 Section 5.44, as > is done with other RADIUS RFCs.  e.g. use "0", "0-1", "0+". etc.

 > For number assignment, the TLVs create a new number space, and can likely be assigned directly in the document.  e.g. if "Foo" contains >"Bar" and "Baz", then you can assign "1" to "Bar", and "2" to "Baz".  This is what IANA will do anyways, and it helps clarify the document, and > prevent mistakes.

[fuyu]: Thanks for the suggestions.  We have changed it into "0, 0+ ....."

>3.1.4.3.  EA-Length Sub-TLV

>  This section defines an attribute that contains a 16 bit number.  Such attributes are discouraged by RFC 6158.  This definition should be >corrected to be a 32-bit integer, or an explanation should be given as to why 16 bits is required here.

>  Similar comments apply for Sections 3.1.6.1, 3.1.6.2, and 3.1.6.3.
>  Section 3.2.  defines an attribute that contains an array of 16-bit integers.  Please follow the guidelines of RFC 6158 here.

[fuyu]：Thanks, We have corrected them into 32 bit. 

Thanks again,

Cheers

Yu