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

" Yu Fu " <eleven711711@foxmail.com> Tue, 22 January 2019 07:45 UTC

Return-Path: <eleven711711@foxmail.com>
X-Original-To: radext@ietfa.amsl.com
Delivered-To: radext@ietfa.amsl.com
Received: from localhost (localhost [127.0.0.1]) by ietfa.amsl.com (Postfix) with ESMTP id 9D37B130E9C for <radext@ietfa.amsl.com>; Mon, 21 Jan 2019 23:45:24 -0800 (PST)
X-Virus-Scanned: amavisd-new at amsl.com
X-Spam-Flag: NO
X-Spam-Score: -0.768
X-Spam-Level:
X-Spam-Status: No, score=-0.768 tagged_above=-999 required=5 tests=[BAYES_00=-1.9, DKIM_SIGNED=0.1, DKIM_VALID=-0.1, DKIM_VALID_AU=-0.1, FREEMAIL_ENVFROM_END_DIGIT=0.25, FREEMAIL_FROM=0.001, FROM_EXCESS_BASE64=0.979, HTML_MESSAGE=0.001, MSGID_FROM_MTA_HEADER=0.001, RCVD_IN_DNSWL_NONE=-0.0001, SPF_PASS=-0.001, URIBL_BLOCKED=0.001] autolearn=no autolearn_force=no
Authentication-Results: ietfa.amsl.com (amavisd-new); dkim=pass (1024-bit key) header.d=foxmail.com
Received: from mail.ietf.org ([4.31.198.44]) by localhost (ietfa.amsl.com [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id I7h4VzjofkT3 for <radext@ietfa.amsl.com>; Mon, 21 Jan 2019 23:45:17 -0800 (PST)
Received: from smtpbgeu1.qq.com (smtpbgeu1.qq.com [52.59.177.22]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ietfa.amsl.com (Postfix) with ESMTPS id D75E2130E8E for <radext@ietf.org>; Mon, 21 Jan 2019 23:45:16 -0800 (PST)
DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=foxmail.com; s=s201512; t=1548143110; bh=+9FuooC1XDlSPhaRu7qbSe5/vvvQOKdwdzK2ub4EN7I=; h=From:To:Cc:Subject:Mime-Version:Content-Type:Content-Transfer-Encoding:Date:Message-ID; b=s5j/cOqMF16gGyTcF0s/oMfLDsvD3syH9qhmk1CalZBVA9JT6qEXG1vNaxQJ0sRgr RRrIl5omrhwH1sHUiCE5gDl0+p3NWoMmfaxD7hA+WplUYlprCHQBCSnDU7X8D8ZLuX twtSKHWWtHDIKeTnhCkGGnY8Qi5efz1mjzlCkQcE=
X-QQ-FEAT: AXXY5CIMcyfFy2OQBPlWtelPcD5o3TIZezMaXRJb+C8vOHl/ThlLLc8IOqnhk fGTHDmgDGJZxQ5mxoTnjQDeoBSQOxpMzVOeAHKfNbs9X3/NE21IZSWU1C9XWVysl2SleRG4 8WotiZKW4ens158PV3p8OBEGNYbqQcLdT05G+7ohyOuXuwHpnRQzipY+0qEtl86SRar4vP+ sr0kqCQmFL8HBmtpYbZvPmHZ7Nafz3rEG+/KgNpExEJ7bZl6Gk1G6sXTz7hPM61qjkUfdy+ 8SeNLc74LbSbH6F72ERMb3fq3ShyrtEWUTuGyDz/BRr44TGEG0geguppU=
X-QQ-SSF: 00000000000000F000000000000000Z
X-HAS-ATTACH: no
X-QQ-BUSINESS-ORIGIN: 2
X-Originating-IP: 202.45.191.87
X-QQ-STYLE:
X-QQ-mid: webmail9t1548143109t476850
From: Yu Fu <eleven711711@foxmail.com>
To: Alan DeKok <aland@deployingradius.com>, Winter Stefan <stefan.winter@restena.lu>
Cc: "radext@ietf.org" <radext@ietf.org>, draft-ietf-softwire-map-radius <draft-ietf-softwire-map-radius@ietf.org>
Mime-Version: 1.0
Content-Type: multipart/alternative; boundary="----=_NextPart_5C46CA05_0B7663C8_19F6EDEF"
Content-Transfer-Encoding: 8bit
Date: Tue, 22 Jan 2019 15:45:09 +0800
X-Priority: 3
Message-ID: <tencent_78E51C7F15B91CDD02A9E15A2517A70AAF0A@qq.com>
X-QQ-MIME: TCMime 1.0 by Tencent
X-Mailer: QQMail 2.x
X-QQ-Mailer: QQMail 2.x
X-QQ-SENDSIZE: 520
Received: from qq.com (unknown [127.0.0.1]) by smtp.qq.com (ESMTP) with SMTP id ; Tue, 22 Jan 2019 15:45:10 +0800 (CST)
Feedback-ID: webmail:foxmail.com:bgforeign:bgforeign2
X-QQ-Bgrelay: 1
Archived-At: <https://mailarchive.ietf.org/arch/msg/radext/JKJzIKvWBqhAAFMkPcDhd49kFO0>
Subject: Re: [radext] Review request for draft-ietf-softwire-map-radius
X-BeenThere: radext@ietf.org
X-Mailman-Version: 2.1.29
Precedence: list
List-Id: RADIUS EXTensions working group discussion list <radext.ietf.org>
List-Unsubscribe: <https://www.ietf.org/mailman/options/radext>, <mailto:radext-request@ietf.org?subject=unsubscribe>
List-Archive: <https://mailarchive.ietf.org/arch/browse/radext/>
List-Post: <mailto:radext@ietf.org>
List-Help: <mailto:radext-request@ietf.org?subject=help>
List-Subscribe: <https://www.ietf.org/mailman/listinfo/radext>, <mailto:radext-request@ietf.org?subject=subscribe>
X-List-Received-Date: Tue, 22 Jan 2019 07:45:25 -0000

Hi Alan,

Thanks a lot for your comments.


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


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



 Please check whether this version have addressed  all you comments.


Your further comments are also appreciated.


Thanks again


BR







------------------ Original ------------------
From:  "Alan DeKok"<aland@deployingradius.com>;
Date:  Mon, Nov 12, 2018 09:37 PM
To:  "Winter Stefan"<stefan.winter@restena.lu>;
Cc:  "radext@ietf.org"<radext@ietf.org>; "draft-ietf-softwire-map-radius"<draft-ietf-softwire-map-radius@ietf.org>; 
Subject:  Re: [radext] Review request for 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.

  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.

* 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"


  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.

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


  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.


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.

  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.


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.


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.