Subject: [radext] AD review: draft-ietf-radext-radius-extensions

From: Benoit Claise <[email-masked]>
Date: Fri, 07 Dec 2012 16:53:28 +0100

Dear all,

Here is my AD review.
3 categories: problems, clarifying questions, and editorial comments

_Problems:_
1.
Section 2.1
  Length

       This field is identical to the Length field of the Attribute
       format defined in[RFC2865] Section 5  <http://tools.ietf.org/html/rfc2865#section-5>.

[RFC2865] Section 5  <http://tools.ietf.org/html/rfc2865#section-5>  says:
Length

       The Length field is one octet, and indicates the length of this
       Attribute_including_the Type, Length and Value fields.

So the length doesn't include the one octet from Extended-Type, if I read the text correctly!
But I see "Permitted values are between_4_  and_255_." So it seems that the Extended-Type length is
included when I see 255.
Please improve the text.
Same remark for the other types.

2.
What is the semantic of TLV Data type in section 2.3?
I mean: how do we define the relationship between sub-TLV?
Always logical AND? Or logical OR, or something else such as nested?

Attribute encapsulating two TLVs, one after the other.

      241.2 { 1 23 45 } { 2 67 89 }
        -> f1 0b 02 01 04 23 45 02 04 67 89

What if
      241.2 { 1 23 45 } { 1 23 46 }

RFC 2865:
    Some Attributes MAY be included more than once.  The effect of this
    is Attribute specific, and is specified in each Attribute
    description.  A summary table is provided at the end of the
    "Attributes" section.

    If multiple Attributes with the same Type are present, the order of
    Attributes with the same Type MUST be preserved by any proxies.  The
    order of Attributes of different Types is not required to be
    preserved.  A RADIUS server or client MUST NOT have any dependencies
    on the order of attributes of different types.  A RADIUS server or
    client MUST NOT require attributes of the same type to be contiguous.

What is the impact for sub-TLV?
Do you want to have a table like in RFC 2865 section 5.44 with the list
of attributes, explaining whether or not they can be part of the sub-TLV, and
with which multiplicity.

Note: I spent quite some time on structured data semantic with RFC 6313, the IPFIX structured data

3.
Section 2.8
    When an implementation receives an "invalid attribute", it SHOULD be
    silently discarded.  If it is not discarded, it MUST NOT be handled
    in the same manner as a well-formed attribute.

Section 5.2
    Proxy servers SHOULD forward
    attributes, even ones which they do not understand, or which are not
    in a local dictionary.

Question: what should proxy servers do with invalid attributes?
Maybe, clarify "implementation" as it means client and server, and not proxy which should forward "invalid attribute"

_Clarifying questions_

1.
What's the max length of Long Extended Type?

    * defining a method which uses the additional octet to indicate data
      fragmentation across multiple Attributes.  This method provides a
      standard way for an Attribute to carry more than 253 octets of
      data.

The max length is the one from the RADIUS packet, ie 4K, correct?
I propose to mention this.
I was wondering what's the link with this new entry in the charter:
Feb 2013   RADIUS packet fragmentation submitted as an Experimental RFC
Apparently, none.

2. In section 1.1, called "Unmet Requirements", why do you speak about:
    It is RECOMMENDED that implementations support this specification.
    It is RECOMMENDED that new specifications use the formats defined in
    this specification.

    The alternative to the above recommendations is a circular argument
    of not implementing this specification because no other standards
    reference it, and also not defining new standards referencing this
    specification because no implementations exist.

Maybe a new section title for the 2 paragraphs above?

3.
    For example, Livingston has Vendor-Id 307, and has defined an
    attribute "IP-Pool" as number 6.  This VSA can be uniquely identified
    as 26.307.6.

Don't you want to mention "Livingston-IP-Pool" instead?
Similar remark for
    For example, the company USR has been allocated Vendor-Id 429, and
    has defined a "Version-Id" attribute as number 32768.  This VSA can
    be uniquely identified as 26.429.32768.

4.
Section 2.7.1
    We RECOMMEND that vendors use their name as a unique prefix for
    attribute names.

Should RADIUS server implementations follow this rules when receiving attributes,
specifically when the same attribute name from different vendors is used.
So basically, enforcing global uniqueness of name, and an advice for the RADIUS server.

5.
    * New specifications SHOULD NOT request allocation from the standard
      Attribute Type Space (i.e. Attributes 1 through 255).  That space
      is deprecated.

Deprecated: is it the right term?
Maybe you meant: Assignment from that space is not to be used.
In this case, this contradicts
       * specifications which allocate a small number of attributes
         (i.e. less than ten) should have all allocations made from
         the standard space.

6.
section 6.7
    * RADIUS proxy servers SHOULD forward all attributes, even ones
      which they do not understand, or which are not in a local
      dictionary.  These attributes SHOULD be forwarded verbatim, with
      no modifications or changes
Why do you repeat what you have written in section 5.2?

Proposal:
OLD:
    * RADIUS proxy servers SHOULD forward all attributes, even ones
      which they do not understand, or which are not in a local
      dictionary.  These attributes SHOULD be forwarded verbatim, with
      no modifications or changes.

    * When forwarding attributes, RADIUS proxy servers SHOULD forward
      the "Reserved" field unchanged.  That is, they SHOULD NOT assume
      that the "Reserved" field can always be set to zero.
NEW:
    * RADIUS proxy servers MUST follow the specifications in section 5.2

8.
section 10.3.4
       * specifications which allocate a small number of attributes
         (i.e. less than ten) should have all allocations made from
         the standard space.

       * specifications which allocate a large number of attributes
         (i.e. ten or more) should have all allocations made from the
         extended space.
Instead of using a number, which would anyway not be relevant very long, why don't you have a relative number?
For example, instead of "i.e. ten or more", "compared to the remaining available number of entries in the standard space"

9.
Section 10.3.4
       * specifications which request allocation of an attribute
         which can transport 253 or more octets of data should have
         that attribute allocated from within the long extended space,
         We note thatSection 6.5  <http://tools.ietf.org/html/draft-ietf-radext-radius-extensions-06#section-6.5>, above requires specifications to
         request this allocation.

Are we ever going to face the issue that an attribute is right now below 253, but
there is a probability that it could be extended in the future?
Do we want to advice that such an attribute be allocated to the long extended space right now?
Or do we advice to allocate a separate attribute in the future?

_Editorial comments_
1.
Group the entry logically, like for the two entries related to "Extended Type"
OLD:
    * defining a "Long Extended Type" format, which inserts
      an additional octet between the "Extended Type" octet,
      and the "Value" field.  This method gives us a general way of
      adding additional functionality to the protocol.

    * defining a method which uses the additional octet to indicate data
      fragmentation across multiple Attributes.  This method provides a
      standard way for an Attribute to carry more than 253 octets of
      data.

    * allocating 2 attributes as using the format "Long Extended Type".
      This allocation extends the RADIUS Attribute Type Space
      by an additional 500 values.

NEW:
    * defining a "Long Extended Type" format, which inserts
      an additional octet between the "Extended Type" octet,
      and the "Value" field.  This method gives us a general way of
      adding additional functionality to the protocol.

    * allocating 2 attributes as using the format "Long Extended Type".
      This allocation extends the RADIUS Attribute Type Space
      by an additional 500 values.

    * defining a method which uses the additional octet to indicate data
      fragmentation across multiple Attributes.  This method provides a
      standard way for an Attribute to carry more than 253 octets of
      data.

2.
Vendor-Id

       The 4 octets are the SMI Network Management Private Enterprise
       Code of the Vendor in network byte order.

Give a reference tohttp://www.iana.org/assignments/enterprise-numbers
Note: there are multiple instances of that sentence.

3.
Section2.6  <http://tools.ietf.org/html/draft-ietf-radext-radius-extensions-06#section-2.6>.  Vendor-ID Field

    We define the Vendor-ID field of Vendor-Specific Attributes to
    encompass the entire 4 octets of the Vendor field.  [RFC2865  <http://tools.ietf.org/html/rfc2865>]Section  <http://tools.ietf.org/html/draft-ietf-radext-radius-extensions-06#section-5.26>
    5.26  <http://tools.ietf.org/html/draft-ietf-radext-radius-extensions-06#section-5.26>  defined it to be 3 octets, with the high octet being zero.  This
    change has no immediate impact on RADIUS, as the maximum Private
    Enterprise Code defined is still within 16 bits.

There is something wrong with the reference: [RFC2865  <http://tools.ietf.org/html/rfc2865>]Section  <http://tools.ietf.org/html/draft-ietf-radext-radius-extensions-06#section-5.26>  5.26  <http://tools.ietf.org/html/draft-ietf-radext-radius-extensions-06#section-5.26>

4.
Section2.3.1  <http://tools.ietf.org/html/draft-ietf-radext-radius-extensions-06#section-2.3.1>.  TLV Nesting
    TLVs may contain other TLVs.  When this occurs, the "container" TLV
    MUST be completely filled by the "contained" TLVs.  That is, the
    "container" TLV-Length field MUST be exactly two (2) more than the
    sum of the "contained" TLV-Length fields.  If the "contained" TLVs
    over-fill the "container" TLV, the "container" TLV MUST be considered
    to be an "invalid attribute", and handled as described inSection  <http://tools.ietf.org/html/draft-ietf-radext-radius-extensions-06#section-2.7>
    2.7  <http://tools.ietf.org/html/draft-ietf-radext-radius-extensions-06#section-2.7>, below.

Section 2.7 ->Section 2.8

5.
    * Specifications of an ttribute which encodes 252 octets or less
=> attribute

6.
In section 1, please introduce with one sentence or two the guidelines in section 6.
Actually, it would also fit the abstract.
Reason: while I was reading through all these extra encodings, I had a lot questions that were answered in section 6.
I could have paid more attention to the table of content, sure but ...

7.
section 7 Rationale
Rationale for what? "Specifications rationale" maybe?

8.
section 10.3.4
       * specifications which request allocation of an Attribute of
         data type TLV should have that attribute allocated from the
         extended space.
Attribute -> attribute

9.
This document updates:2865  <http://tools.ietf.org/html/rfc2865>,2866  <http://tools.ietf.org/html/rfc2866>,3575  <http://tools.ietf.org/html/rfc3575>,5176  <http://tools.ietf.org/html/rfc5176>,6158  <http://tools.ietf.org/html/rfc6158>    
My guess is that those should be normative references

Regards, Benoit

---

From: Alan DeKok <[email-masked]>
Date: Wed, 26 Dec 2012 14:27:05 -0500

Benoit Claise wrote:
>       format defined in [RFC2865] Section 5 <http://tools.ietf.org/html/rfc2865#section-5>. 
> 
>       Attribute _including _the Type, Length and Value fields. 
> 
> But I see "Permitted values are between _4_ and _255_." So it seems that the Extended-Type length is 
> included when I see 255. 
> Please improve the text.

  I've updated the intro text:

The new attribute formats are designed to be compatible with the
attribute format given in [RFC2865] Section 5.  The meaning and
interpretation of the Type and Length fields is unchanged from that
specification.  This reuse allows the new formats to be compatible
RADIUS implementations which do not implement this specification.
Those implementations can simply ignore the Value field of an
attribute, or forward it verbatim.

The changes to the attribute format come about by "stealing" one or
more octets from the Value field.  This change has the effect that the
Value field of [RFC2865] Section 5 contains both the new octets given
here, and any attribute-specific Value.  The result is that Values in
this specification are limited to less than 253 octets in size.  This
limitation is overcome through the use of the "Long Extended Type"
format.

We reiterate that the formats given in this document do not insert new
data into an attribute.  Instead, we "steal" one octet of Value, so
that the definition of the Length field remains unchanged.

  and updated the text for the new formats:

The Length field is one octet, and indicates the length of this
Attribute including the Type, Length, Extended-Type, and Value fields.
Permitted values are between 4 and 255.

> Always logical AND? Or logical OR, or something else such as nested?

  RADIUS has always considered them to be a logical set, i.e. "and":

If multiple TLVs with the same TLV-Type are present, the order of TLVs
with the same TLV-Type MUST be preserved by any proxies.  The order of
TLVs of different TLV-Types is not required to be preserved.  A RADIUS
server or client MUST NOT have any dependencies on the order of TLVs
of different TLV-Types.  A RADIUS server or client MUST NOT require
TLVs of the same TLV-type to be contiguous.

The interpretation of multiple TLVs of the same TLV-Type MUST be that
of a logical "and", unless otherwise specified.  That is, multiple
TLVs are interpreted as specifying a set or a list of values.
Specifications SHOULD NOT define TLVs to be interpreted as a logical
"or".  Doing so would mean that a RADIUS client or server would make
an arbitrary, and non-deterministic choice among the values.

> What is the impact for sub-TLV?

  It should be the same as RFC 2865 Section 5 comments in multiple
attributes.  I've copied && re-worded the text above.

> of attributes, explaining whether or not they can be part of the sub-TLV, and 

  Quite likely, yes.

> Question: what should proxy servers do with invalid attributes?

  Forwarded verbatim, I think.

> Maybe, clarify "implementation" as it means client and server, and not proxy which should forward "invalid attribute"

  Proxies should forward them verbatim.  For the simple reason that
there is a *lot* of broken networking equipment.  That equipment poaches
on the IANA attribute space (1..255).  So that equipment sends "invalid
attributes".

  Maybe some other system in the network will understand them.

> The max length is the one from the RADIUS packet, ie 4K, correct?

  Yes.

> I propose to mention this.

  Suggested text:

We note that the maximum size of a fragmented attribute is limited
only by the RADIUS packet length limitation (i.e. 4096 octets, not
counting various headers and overhead).  Implementations SHOULD be
able to handle fragmented attributes of 4096 octets.

> Feb 2013   RADIUS packet fragmentation submitted as an Experimental RFC

  Not really, no.

> Maybe a new section title for the 2 paragraphs above?

  OK.

> Don't you want to mention "Livingston-IP-Pool" instead?

  I refer to the attribute by it's historical name.

>    be uniquely identified as 26.429.32768.

  Yes.

> So basically, enforcing global uniqueness of name, and an advice for the RADIUS server.

  The attribute names are local to the implementation.  It's up to the
implementation to enforce local name uniqueness.

> Maybe you meant: Assignment from that space is not to be used.

  IANA can't do that.

>         (i.e. less than ten) should have all allocations made from

  I'll update that to change the allocation strategy to be a bit more
flexible.

> Why do you repeat what you have written in section 5.2?

  Just to give checklists for people.

> Proposal:
...
>    * RADIUS proxy servers MUST follow the specifications in section 5.2

  Sure.

> For example, instead of "i.e. ten or more", "compared to the remaining available number of entries in the standard space"

  OK.  I'll just change it to "many" attributes.

>         We note that Section 6.5 <http://tools.ietf.org/html/draft-ietf-radext-radius-extensions-06#section-6.5>, above requires specifications to
>         request this allocation.
> 
> 

  I doubt it.  The attribute audit in the doc shows about 25 attributes
out of ~5000 can be longer than 253 octets.  So I'm not worried.

> Or do we advice to allocate a separate attribute in the future?

  If there's any chance it will be longer than 253 octets, it should be
allocated as a "long" attribute.

> Group the entry logically, like for the two entries related to "Extended Type"

  Sure.

> Give a reference to http://www.iana.org/assignments/enterprise-numbers

 OK.

> 3. 
> Section 2.6 <http://tools.ietf.org/html/draft-ietf-radext-radius-extensions-06#section-2.6>.  Vendor-ID Field
> 
>    encompass the entire 4 octets of the Vendor field.  [RFC2865 <http://tools.ietf.org/html/rfc2865>] Section <http://tools.ietf.org/html/draft-ietf-radext-radius-extensions-06#section-5.26>
> There is something wrong with the reference: [RFC2865 <http://tools.ietf.org/html/rfc2865>] Section <http://tools.ietf.org/html/draft-ietf-radext-radius-extensions-06#section-5.26> 5.26 <http://tools.ietf.org/html/draft-ietf-radext-radius-extensions-06#section-5.26>

  That seems to be an artifact of the text to HTML conversion on
tools.ietf.org.

...
> Section 2.7 ->Section 2.8

  Fixed.

>    * Specifications of an ttribute which encodes 252 octets or less

   Fixed.

> I could have paid more attention to the table of content, sure but ...

  OK.

> Rationale for what? "Specifications rationale" maybe?

   Maybe "Rationale for this design"

>         data type TLV should have that attribute allocated from the

  OK.

> 9. 
> This document updates: 2865 <http://tools.ietf.org/html/rfc2865>, 2866 <http://tools.ietf.org/html/rfc2866>, 3575 <http://tools.ietf.org/html/rfc3575>, 5176 <http://tools.ietf.org/html/rfc5176>, 6158 <http://tools.ietf.org/html/rfc6158>   

  Sure... it's an informational document, but OK.

  Alan DeKok.