Re: [radext] Benoit Claise's Discuss on draft-ietf-radext-ip-port-radius-ext-11: (with DISCUSS and COMMENT)

[<[e-mail]>]

Hi Benoit, 

Please see inline how the updated version takes into account your review.
 
Cheers,
Med

> -----Message d'origine-----
> De : Benoit Claise [mailto:[e-mail]]
> Envoyé : jeudi 15 septembre 2016 14:26
> À : The IESG
> Cc : [e-mail]; [e-mail];
> [e-mail]; MORAND Lionel IMT/OLN; [e-mail];
> [e-mail]
> Objet : Benoit Claise's Discuss on draft-ietf-radext-ip-port-radius-ext-
> 11: (with DISCUSS and COMMENT)
> 
> Benoit Claise has entered the following ballot position for
> draft-ietf-radext-ip-port-radius-ext-11: Discuss
> 
> 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-radext-ip-port-radius-ext/
> 
> 
> 
> ----------------------------------------------------------------------
> DISCUSS:
> ----------------------------------------------------------------------
> 
> I'm late for my review here. If some points in my DISCUSS/COMMENT have
> been discussed already let me know.
> 
> I'll flag this as a DISCUSS to follow up on the IPFIX issues.
> First of, I applause the attempt to reuse/map/combine data models (IPFIX
> information element and RADIUS attribute).
> 
[Med] Thank you.

> Now, there are a couple of issues.
> The first set of issues has been sent via the IPFIX experts, copied here
> for tracking purposes.
> 
>     Dear Authors,
> 
>     The experts for the IPFIX IE registry have returned the following
>     review:
> 
>     In general, the Information Elements in draft-ietf-radext-ip-port-
>     radius-ext are so underspecified as to be unimplementable. They
> should
>     not be added to the registry in their present form. The authors are
>     advised to read RFC 7013, especially Section 4, which provides
> useful
>     information on defining Information Elements. Specifically:
> 
>     The Information Element transportType is underspecified: (a) I
> presume
>     this is in reference to sourceTransportPort and
>     destinationTransportPort, but the description must say this if it is
>     the case; (b) It's not clear at all from the description in what
>     context this distinction is useful; (c) What's an ICMP identifier?
> 
>     In addition, the description of transportType appears to create a
>     table which should probably be handled as a subregistry. See See
>     RFC7013 section 4.7. for advice on the creation of tables without
>     subregistries (in short, "don't".)
> 

[Med] The description of the attribute was updated. Basically: 
* Clarify the text
* We don't have any more sub-registries because we are relying on the IANA Protocol Numbers


>     The Information Element natTransportLimit has an inappropriate name;
>     it does not describe that which it (presumably) is supposed to
>     represent (see RFC 7013 section 4.1). In addition, it is
>     underspecified. It is impossible to implement from the description.

[Med] The attribute is renamed to "sourceTransportPortsLimit" + text updated to clarify the use of the attribute.

> Is
>     the field IPv4 specific, or is IPv6 supported as well? (If not, why
>     not?)
> 

[Med] It is valid for both IPv4 and IPv6. The text was updated to clarify that point.

>     The Information Element localID has an inappropriate name; it is far
>     too general (see RFC 7013 section 4.1). It uses an inappropriate
>     abstract data type (addresses should never be represented as UTF-8
>     strings in IPFIX, see RFC 7013 section 4.2). It is underspecified as
>     well as poorly designed. Without the ability to disambiguate the
> type
>     of information in the field, this is not a useful Information
> Element.
>     Without a complete enumeration of possible types (n.b. 'etc.' in the
>     description), it is not a useful Information Element. Its purpose is
>     unclear from its description; further, it appears to violate the
>     following guidance in RFC 7013 section 4: "The Information Element
>     must be unique within the registry, and its description must
> represent
>     a substantially different meaning from that of any existing
>     Information Element. An existing Information Element that can be
>     reused for a given purpose should be reused."
> 

[Med] We made the following changes: 
* We changed the Value field to be still a "string" but per definition of ietf-draft-radext-datatype and not to propose as a new IPFIX IE.
* We added a new text to clarify the intent of the attribute: 

   The primary issue addressed by this TLV is that there are CGN
   deployments that do not distinguish internal hosts by their internal
   IP address alone, but use further identifiers for unique subscriber
   identification.  For example, this is the case if a CGN supports
   overlapping private or shared IP address spaces (refer to [RFC1918]
   and [RFC6598]) for internal hosts of different subscribers.  In such
   cases, different internal hosts are identified and mapped at the CGN
   by their IP address and/or another identifier, for example, the
   identifier of a tunnel between the CGN and the subscriber.  In these
   scenarios (and similar ones), the internal IP address is not
   sufficient to demultiplex connections from internal hosts.  An
   additional identifier needs to be present in the IP-Port-Range
   Attribute and IP-Port-Forwarding-Mapping Attribute in order to
   uniquely identify an internal host.  The IP-Port-Local-Id TLV is used
   to carry this identifier.

> I have some more issues I would like to discuss:
> - section 3.2.5, as an example, contains a list of sourceIPv6Address. In
> IPFIX, we would export those as a list [RFC 6313]
> Does this RADIUS document want to only reuse the semantic of individual
> IPFIX information element?
> And also the semantic of multiple information elements [RFC 6313]?

[Med] The intent is to reuse the semantic of individual IEs. The rationale basically was that IPFIX offers a rich IE registry that can be reused in our context.

> And also the IPFIX encoding? For example, some IPFIX fields are "right
> justified" (sourceTransportPort), while some others are not (localID)
> The document should be clear.

[Med] localId is not defined as an IPFIX attribute.

> 
> 
> ----------------------------------------------------------------------
> COMMENT:
> ----------------------------------------------------------------------
> 
> -
> 
>    1.  IP-Port-Limit-Info: This attribute may be carried in RADIUS
>        Access-Accept, Access-Request, Accounting-Request or CoA-Request
>        packet.  The purpose of this attribute is to limit the total
>        number of TCP/UDP ports and/or ICMP identifiers allocated to a
>        user, associated with one or more IPv4 addresses.
> 
> I see "may be carried". Could it be carried in other packets ... was my
> first question.
> It was not clear (until I read the rest of the document) that it's a non
> compulsory attribute only in
> Access-Accept, Access-Request, Accounting-Request or CoA-Request
> packets.
> Proposal:
>    1.  IP-Port-Limit-Info: The purpose of this attribute is to limit the
> total
>        number of TCP/UDP ports and/or ICMP identifiers allocated to a
>        user, associated with one or more IPv4 addresses. This attribute
> is
>        carried in RADIUS Access-Accept, Access-Request,
> Accounting-Request or
>        CoA-Request packets.

[Med] The initial wording is correct. It is aligned with the text in Section 3.1.1.

> 
> Same remark for the other two points below.
> 
>  2.  IP-Port-Range: This attribute may be carried in RADIUS
>        Accounting-Request packet.  The purpose of this attribute is to
>        report by an address sharing device (e.g., a CGN) to the RADIUS
>        server the range of TCP/UDP ports and/or ICMP identifiers that
>        have been allocated or deallocated associated with a given IPv4
>        address for a user.
> 
>    3.  IP-Port-Forwarding-Map: This attribute may be carried in RADIUS
>        Access-Accept, Access-Request, Accounting-Request or CoA-Request
>        packet.  The purpose of this attribute is to specify how an IPv4
>        address and a TCP/ UDP port (or an ICMP identifier) is mapped to
>        another IPv4 address and a TCP/UDP port (or an ICMP identifier).
> 
> -
> 
>    1.  IP-Port-Limit-Info: This attribute may be carried in RADIUS
>        Access-Accept, Access-Request, Accounting-Request or CoA-Request
>        packet.  The purpose of this attribute is to limit the total
>        number of TCP/UDP ports and/or ICMP identifiers allocated to a
>        user, associated with one or more IPv4 addresses.
> 
> 
> Only for TCP/UDP? I believe Mirja had the same question.

[Med] We updated the text so that any transport protocol can be used. The new text says the following:

   The format of IP-Port-Type TLV is shown in Figure 4.  This attribute
   carries the IP transport protocol number defined by IANA (refer to
   [ProtocolNumbers])

> 
> - Section 2. Terminology.
> Typically, you want to have that note at the beginning:
> 
>    Note that the definitions of "internal IP address", "internal port",
>    "internal realm", "external IP address", "external port", "external
>    realm", and "mapping" are the same as defined in Port Control
>    Protocol (PCP) [RFC6887], and the Common Requirements for Carrier-
>    Grade NATs (CGNs) [RFC6888].
> 
> If I know of RFC6887 and RFC6888, I don't read the definitions.

[Med] OK.

> 
> - Section 3.1.3
> 
>    This attribute is of type "TLV" as defined in the RADIUS Protocol
>    Extensions [RFC6929].  It contains the following sub-attributes:
> 
>    ...
> 
>    o  either an IP-Port-Int-IPv4-Addr TLV (see Section 3.2.4) or an IP-
>       Port-Local-Id TLV (see Section 3.2.11),
> 
>    o  either an IP-Port-Int-IPv6-Addr TLV (see Section 3.2.5) or an IP-
>       Port-Local-Id TLV (see Section 3.2.11),
> 
> Depending on the IP-Port-Type value, it's one of the two above, not
> both.

[Med] Fair. Changed to: 

   o  If the internal realm is with IPv4 address family, the IP-Port-
      Forwarding-Map Attribute MUST contain the IP-Port-Int-IPv4-Addr
      TLV (see Section 3.2.4); if the internal realm is with IPv6
      address family, the IP-Port-Forwarding-Map Attribute MUST contain
      the IP-Port-Int-IPv6-Addr TLV (see Section 3.2.5).

> 
> - I believe the RFC 2119 keywords usage is sometimes weak (generic
> comment for the entire document)
> Example1: section 3.1.3
> 
>    The attribute contains a 2-byte IP internal port number that is
>    associated with an internal IPv4 or IPv6 address, or a locally
>    significant identifier at the customer site, and a 2-byte IP external
>    port number that is associated with an external IPv4 address.  The
>    internal IPv4 or IPv6 address, or the local identifier must be
>    included; the external IPv4 address may also be included.
> 
> Not RFC 2119 keywords. Shoud they be MUST and MAY?

[Med] Fixed. The NEW text is provided below: 

NEW:

   The attribute contains a 2-byte IP internal port number and a 2-byte
   IP external port number.  The internal port number is associated with
   an internal IPv4 or IPv6 address that MUST always be included.  The
   external port number is associated with a specific external IPv4
   address if included, but otherwise with all external IPv4 addresses
   for the end user.

> And the next paragraph contains RFC 2119 keywords.
> 
>    The IP-Port-Forwarding-Map Attribute MAY appear in an Access-Accept
>    packet.  It MAY also appear in an Access-Request packet to indicate a
>    preferred port mapping by the device co-located with NAS.  However
>    the server is not required to honor such a preference.
> 
> Example2:
> 
>    IP-Port-Ext-IPv4-Addr TLV MAY be included in the following
>    Attributes:
> 
>    o  IP-Port-Limit-Info Attribute, identified as 241.TBD1.3 (see
>       Section 3.1.1).
> 
>    o  IP-Port-Range Attribute, identified as 241.TBD2.3 (see
>       Section 3.1.2).
> 
>    o  IP-Port-Forwarding-Mapping Attribute, identified as 241.TBD3.3
>       (see Section 3.1.3).
> 
> Don't you need the RFC2119 keyword in the reverse situation, in section
> 3.1.1 (IP-Port-Limit-Info Attribute)?
> OLD:
>    o  an optional IP-Port-Ext-IPv4-Addr TLV (see Section 3.2.3).
> 
> NEW:
>    o The IP-Port-Limit-Info Attribute MAY contain the
> IP-Port-Ext-IPv4-Addr (see Section 3.2.3).

[Med] Fixed.

> 
> This logic would not only be clearer, but would also avoid
> inconsistencies.
> See section 3.1.3
> OLD:
> 
> It contains the following sub-attributes:
>    ...
>    o  an IP-Port-Ext-IPv4-Addr TLV (see Section 3.2.3).
> 
> 
> NEW:
> 
> It contains the following sub-attributes:
> 
>   ...
>    o  an optional IP-Port-Ext-IPv4-Addr TLV (see Section 3.2.3).
> 
> Example 3:
> 
>    natEvent
> 
>       Integer.  This field contains the data (unsigned8) of natEvent
>       (230) defined in IPFIX, right justified, and unused bits must be
>       set to zero.  It indicates the allocation or deallocation of a
>       range of IP ports as follows:
> 
> MUST?

[Med] Yes. Fixed in the new version.