Subject: Re: [radext] OPS-DIR review of draft-ietf-radext-datatypes-06

From: Alan DeKok <[email-masked]>
Date: Sat, 20 Aug 2016 11:57:16 +0200

On Aug 20, 2016, at 9:35 AM, Liushucheng (Will) <[email-masked]> wrote:
> * Section 1, page 1:
> >
> > RADIUS specifications have historically defined attributes in terms of
> > name, type value, and data type.
> 
> The term "type value" sounds confusing, particularly when one is used to terms such as "TLV" meaning "Type, Length, Value". Did you mean just "type"? Something else?

  I'll remove "type" and clarify the resulting text.

> 
> * Section 1, page 1:
> > There is no management of data type name or definition.
> 
> Same here. Not sure if you just meant data types, or what. But this is confusing to me.

  I've reworded the paragraph to:

RADIUS specifications have historically defined attributes in terms of
name, value, and data type.  Of these three pieces of information, the
name is recorded by IANA in the RADIUS Attribute Type registry, but
not otherwise managed or restricted, as discussed in [RFC6929] Section
2.7.1.  The value is managed by IANA, and recorded in that registry.
The data type is not managed or recorded in the RADIUS Attribute Type
registry.  Experience has shown that there is a need to create well
known data types, and have them managed by IANA.

> * Section 2.1.1, Page 7:
> > For example, the data type "vsa" will contain a data field called
> > "VSA-Data".
> 
> Not sure what this means. Is the data type "vsa" supposed to contain multiple "fields" (a la "C-language structures"), or what?  -- i.e., it's not clear what you mean by "data type" and "data field". Given the goal of this document, such definitions should be clearly spelled out.

  This text is intended to describe the naming convention, not define the data types.  The later text defines the data types.

  I've tried to clarify this by the following text:

We consistently use "Value" to refer to the contents of a data type,
where that data type is simple.  For example, an "integer" can have a
"Value".  In contrast, a Vendor-Specific attribute carries complex
information, and thus cannot have a "Value".

For data types which carry complex information, we name the fields
based on the data type.  For example, a Vendor-Specific attribute is
defined to carry a "vsa" data type, and the contents of that data type
are described herein as "VSA-Data".

> 
> * Section 2.1.2, page 7:
> > Attributes can usually be completely described via the Attribute Type
> > code, name, and data type.
> 
> The use of "type" here is confusing. Could "type" be omitted from this sentence? Besides, it seems it's the first time you refer to the "code".
> Is the reader expected to be familiar with what you're referring to by "code", here?

 The Attribute Type refers to the IANA RADIUS Attribute Type registry.

  I'll change "code" to "value".

> 
> * Section 2.1.3, page 8:
> > The Attribute Type code, given in the "dotted number" notation from
> > [RFC6929].
> 
> How about s/code/number/ or just remove the term "code"?

  I'll just use "value".

> * Section 2.2, page 10:
> > Instead, specifications which define new meaning for "reserved"
> > fields SHOULD describe how older implementations process those fields.
> 
> What's the point of specifying how an older implementation would behave? i.e., if this spec is going to apply to older implementations, such implementations are, by definition, no longer "older" (if they end up employing with this spec)

  The intention is to describe how the new definition is compatible with existing implementations.  I'll update the text.

  i.e. defining new mandated behaviour for a "reserved" field means that existing implementations won't follow that mandated behaviour.  This is bad.

> * Section 3.11, page 21:
> > If the address is all zeros (i.e. "0.0.0.0", then the Prefix-Length
> > MUST be set to 32.
> 
> Would you clarify why?

  It's taken from previous specifications, which don't explain why.  I'm hesitant to give a new explanation here.

https://tools.ietf.org/html/rfc6572#section-4.12

> * Section 3.15, page 25:
> >
> > Ext-Data
> >
> > The contents of this field MUST be a valid data type as defined in the
> > RADIUS Data Type registry.  The Ext-Data field MUST NOT contain any of
> > the following data types: "concat", "vsa", "extended",
> > "long-extended", or "evs".
> 
> How can you tell what's the data type stored in the Ext-Data field?

  Via the attribute definition.  It defines a value / OID, name, and data type for the contents.  The data type is what goes into the Ext-Data field, as discussed in the text.

  Remember, the "extended" type is a way to  extend the Attribute Type value from an 8 bit space to "dotted number" space.  The real contents of that type are attribute data, just like the Attr-Data data defined earlier.  But the "extended" type is defined *within* the "Attr-Data" field, so we have to have a different and unique name for it.

> * Section 3.16, page 26:
> > The More field is one (1) bit in length, and indicates whether or not
> > the current attribute contains "more" than 251 octets of data.  The
> > More field MUST be clear (0) if the Length field has value less than
> > 255.  The More field MAY be set (1) if the Length field has value of
> > 255.
> >
> > If the More field is set (1), it indicates that the Ext-Data field has
> > been fragmented across multiple RADIUS attributes. When the More field
> > is set (1), the attribute MUST have a Length field of value 255; there
> > MUST be an attribute following this one; and the next attribute MUST
> > have both the same Type and Extended Type.
> 
> There seems to be conflicting RFC2119 for Length == 255 && M=1 -- one is a MAY, the other one is a MUST

  This is text dealing with fragments.  A fragment of Length 255 is allowed, and can have M=0.

  If Length=255 and M=1, then there MUST be a subsequent fragment.

  Both situations are allowed.  The first one MAY have M=0, because there are no subsequent fragments.  If there are multiple fragments, it MUST have M=1, and conversely, if M=1, there MUST be subsequent fragments.

> ** Editorial **
> 
> * Abstract:
> > updates the specifications to better follow established practice.  We
> > do this by naming the data types defined in RFC 6158, which have been
> > used since at least RFC 2865.
> 
> Please change to "since at least the publication of RFC2865" or ...

  OK.

> 
> * Section 1.1, page 4:
> >
> > A number of data type names and definitions are given in [RFC2865]
> > Section 5, at the bottom of page 25.
> 
> Please rephrase as "A number of data type names and definitions are given in Section 5 of [RFC2865], at the bottom of page 25"

  Sure.

> * Section 2.2, page 9:
> > It is RECOMMENDED that such attributes be treated as "invalid
> > attributes", as defined in [RFC6929] Section 2.8.
> 
> Throughout the document, please replace instances like this with something like "...Section x.x of [RFCxxxx]", instead.

  I'm not sure this is necessary.  A scan of previous specifications show both uses with about equal weighting.

> * Section 3.5, page 15:
> > s, and TLVs cannot be used, the "string" data type MUST be used.
> > This requirement include encapsulation of data structure
> 
> s/include/includes/

  OK.

> * Section 3.11, page 20:
> >
> > 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
> > +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+ |
> > Reserved   | Prefix-Length |  Prefix ...
> > +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+ ...
> > Prefix                 | +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
> 
> There seems to be an alignment problem with the Prefix-Length field?

  Fixed.

> 
> 
> * Section 3.16, page 26:
> >
> > Extended-Type
> >
> > This field is identical to the Extended-Type field defined above in
> > Section 2.13.
> 
> There's no such Section.

  Updated.

> 
> * Section 4.1 and Section 4.2:
> 
> Shouldn't these two sections be part of the "IANA Considerations" section?

  I"m Ok with it either way, and there hasn't been feedback from IANA that it's required.

  Alan DeKok.