Re: [radext] draft-ietf-radext-ip-port-radius-ext

[Dean cheng <[e-mail]>]

Paul, please see inline below.
Thanks
Dean

> -----Original Message-----
> From: Paul Aitken [mailto:[e-mail]]
> Sent: Wednesday, October 12, 2016 1:14 AM
> To: Dean cheng; [e-mail]
> Cc: [e-mail]; [e-mail];
> [e-mail]
> Subject: Re: [radext] draft-ietf-radext-ip-port-radius-ext
> 
> Thanks for the quick response, Dean. Please find some responses inline:
> 
> 
> On 10/12/16 04:55, Dean cheng wrote:
> > Paul,
> >
> > We certainly missed this email of comments, but please see below.
> >
> > Regards
> > Dean
> >
> >> -----Original Message-----
> >> From: radext [mailto:[e-mail]] On Behalf Of Paul
> >> Aitken
> >> Sent: Tuesday, October 11, 2016 1:43 PM
> >> To: [e-mail]
> >> Cc: [e-mail]; [e-mail];
> >> [e-mail]
> >> Subject: Re: [radext] draft-ietf-radext-ip-port-radius-ext
> >>
> >> Authors I've been waiting 2 months, yet no reply?
> >>
> >> P.
> >>
> >>
> >> On 08/11/16 17:24, Paul Aitken wrote:
> >>> Dear Authors, some questions came to mind while reviewing section
> >>> 3.2 of your draft:
> >>>
> >>>
> >>> Q1: Why define a new TLV-Type 1..11 simply to encode an IPFIX
> >>> Information Element?
> >>>
> >>> Since there's a 1:1 mapping between the TLV-Type and IPFIX Element
> >>> ID, the scheme you propose would be more extensible if the IPFIX
> >>> Element ID was used instead of the TLV-Type since it would require
> >>> one less registry. True, one additional byte would be required in
> the encoding.
> >>> However, the advantage is that in future anyone could encode a new
> >>> TLV by using the relevant IPFIX ID, without having to also define a
> >>> new TLV-Type for it.
> > The definition of the TLV-Type field (refer to Section 2.3 of RFC6929)
> > is very different than that of IPFIX IE identifiers (refer to Section
> > 4 of RFC5102), not just the size (8 bits vs. 16 bits) but also
> > semantics, reserved values etc.
> 
> Nowhere in this draft says that the TLV-type field refers to RFC6929.

The reference to RFC6929 is repeated in Section 3.1.1, 3.1.2 and 3.1.3 
where the proposed Radius attributes are presented, but also in the
beginning of Section 3.2 before all sub-sections for individual 
proposed TLVs. Here is the statement at the beginning of Section 3.2:
    
   ....These TLVs use the format defined in [RFC6929]....  

> 
> Since there's a 1:1 mapping between the values of this field and IPFIX
> Information Element IDs (since most of section 3.2.x say, "This
> attribute carries IPFIX Information Element X"), it seems to be simply
> another name and another registry for the same thing.

As said, the TLV-Type field of Radius TLV (RFC6929) has totally
different size and meaning than that of the IPFIX IE identifier
(RFC5102), I'd consider it an engineering kludge to mingle them
together.

Please read Section 2.3 of RFC6929 and Section 4 of RFC5102.

> 
> 
> >>> Q2: In each TLV, the Length field is redundant since it can always
> >>> be determined from the TLV-Type: it's always 6, with special cases
> >>> for TLV-Types 5 (Length is 18) and 11 (Length could be determined
> by
> >>> a NULL terminator on the string). This suggests that the length is
> >>> not required in the protocol, so why not remove it entirely?
> > The length field is the second required one in a TLV structure, refer
> > to Section 2.3 of RFC6929.
> 
> And my point is that it's redundant.
> 
> 
> >>>
> >>> However, many of the descriptions say "contains the data ... right
> >>> justified, and the unused bits in this field MUST be set to zero. "
> >>>
> >>> Q3: Wouldn't it be better to limit the length of the field to an
> >>> appropriate size for the data, and encode that size in the Length?
> > This question was also raised by Suresh Krishnan, which was already
> > answered by Alan DeKok (see the attached).
> 
> (ie, "The limited nature of RADIUS data types.  This behaviour is
> mandated by RFC 6158")
> 
> Abstract the data type from the octets on the wire. Since the length is
> given in the packet (or can be infered from the TLV type per Q2 above),
> there's no need to transmit redundant octets.

Are you saying you want to redefine TLV as TV?

In this draft, we follow the TLV as defined in RFC6929 for
Radius protocol, and more generally, the structure of TLV
here is common across all the protocols that use TLV that
I know. I'm not sure how we could redefine the TLV here.

> 
> P.
> 
> >>> For example, the transportType in 3.2.1 is just one octet long
> >>> (unsigned8) - so there's no reason to encode it in 32 bits. Why not
> >>> encode it like so:
> >>>
> >>>      0                   1 2                   3
> >>>      0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0
> 1
> >>>     +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-
> +-+
> >>>     |       TLV-Type = TBAx1        |    Length     | transportType
> |
> >>>
> >>> +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
> >>>
> >>>
> >>> The natEvent in 3.2.8 is also an unsigned8, so it would be encoded
> >>> similarly - except that the TLV-Type would be 230, which is the IE
> >>> ID of the IPFIX natEvent.
> >>>
> >>>
> >>> The natTransportLimit in 3.2.2 is an unsigned16 - so there's no
> need
> >>> to encode it in 32 bits. Why not encode it like so:
> >>>
> >>>      0                   1 2                   3
> >>>      0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0
> 1
> >>>     +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-
> +-+
> >>>     |       TLV-Type = TBAx2        |    Length
> |natTransportLimit ...
> >>>     +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-
> +-+
> >>>    ...              |
> >>>     +-+-+-+-+-+-+-+-+
> >>>
> >>> 3.2.9 and 3.2.10 would be similar. The encoding of 3.2.3, 3.2.4,
> and
> >>> 3.2.5 would all be as shown in the draft, except that the TLV-type
> >>> would be two octets long and contain the IPFIX IE ID. The
> >>> sourceTransportPort in 3.2.6 is an unsigned16 - so there's no need
> >>> to encode it in 32 bits. Why not encode it like so:
> >>>
> >>>      0                   1 2                   3
> >>>      0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0
> 1
> >>>     +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-
> +-+
> >>>     | TLV-Type = sourceTransportPort|    Length     |
> sourceTransportPort ...
> >>>     +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-
> +-+
> >>>    ...              |
> >>>     +-+-+-+-+-+-+-+-+
> >>>
> >>> The encoding of 3.2.7 would be similar.
> >> _______________________________________________
> >> radext mailing list
> >> [e-mail]
> >> https://urldefense.proofpoint.com/v2/url?u=https-
> 3A__www.ietf.org_mai
> >>
> lman_listinfo_radext&d=DQIFAg&c=IL_XqQWOjubgfqINi2jTzg&r=Xx9729xYDYoC
> >>
> gBDdcp1FKt5PyYd1TCoXNKhyPY8CFp8&m=dEtr3XevxedcGIFIR0evA2jpVRKktTCFIE9
> >> _hDx1iO4&s=t1n5rKGP56p05NoEUsOlAkh5nDurkUeCLuNN4hVxXSU&e=