Subject: [radext] [Fwd: Issues with draft-ietf-radext-radius-extensions-09]

From: Alan DeKok <[email-masked]>
Date: Thu, 31 Jan 2013 21:47:05 -0500

FYI.  I'm not sure I agree with all of the comments here.  I will have
a more detailed response later.

  This email is just to let the WG know the IESG's review of the document.

  Alan DeKok.

Subject: Issues with draft-ietf-radext-radius-extensions-09
From: Barry Leiba <[email-masked]>
To: [email-masked]
Cc: [email-masked], [email-masked]
Date: Thu, 31 Jan 2013 20:18:04 -0500
List-Id: <[email-masked]>

Hi, all.

Authors, I've been reviewing the subject document for IESG Evaluation,
and I think I've found a bunch of internal conflicts and incorrect
things, which would end up as DISCUSS positions in my ballot.  I also
think that fixing these will need some re-review in the working group
to make sure that we got them right.  Rather than have a pile of
DISCUSS ballots and possible several ADs sifting through the same set
of errors, I've asked Benoit to remove the document from next week's
telechat while we work these out.  I apologize for the delay this
creates, but I really think it's the right thing.

I'm still finishing my review -- I'm just starting Section 5.2 now --
but I don't want the comments that I currently have to have to wait,
so I'm sending you what I have up to this point.  I'm going to try to
label each point with "DISCUSS" or "COMMENT", to indicate how I think
I'd ballot it.  Of course, some (I hope not all!) of my points could
just be a result of my not understanding something, so please discuss
these with me.  Let's see if we can sort this out quickly.

Meanwhile, I'll try to finish my review today (I'm in Japan now, and
it's 10 a.m. on Friday as I send this).

Barry Leiba, Applications AD
-----------------------------------------------------------

=== COMMENT ===
-- Section 1.1.1 --

   One goal which was not met by the above modifications is to have an
   incentive for standards to use the new space.  We would ideally like
   to deprecate the "Unassigned" attributes in the standard space, which
   would permit allocation, but under more stringent guidelines.
   However, [RFC5226] has no provisions for a "Deprecated" entry in an
   IANA managed registry.

It's actually entirely possible to deprecate items in a registry, so
if you still want to handle this.  The document could add a column to
the table, perhaps called "Status", which would have the values
"current" (or blank to mean "current") or "deprecated" (or "obsolete",
or whatever).  You can define in the document exactly what these
values mean, and ask IANA to put those definitions in the header at
the top of the registry.  You can populate that column as you please,
saying that all entries get "current" except [these], which get
"deprecated".

This has been done in other documents, with other registries; see, for
example, the Message Header Fields registry:
http://www.iana.org/assignments/message-headers/perm-headers.html

In those cases, the deprecated items are not re-assigned (as 5226
notes, re-assignment can be a problem).  But your document can
explicitly allow deprecated values to be re-assigned at some point,
however you want to set that up.  Of course, this itself could get
some questions from the IESG over concerns about double usage, but you
already have such double usage (as you note in Section 5), so....

-- Section 2.1 --

=== COMMENT ===
In the description of "Type":

I suggest adding a forward reference to make an important clarification here:

NEW
      This field is identical to the Type field of the Attribute format
      defined in [RFC2865] Section 5.  It will contain one of the Extended
      Type values defined in Section 3, below.
END

=== COMMENT ===
In the description of "Length":

      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.  If a client or
      server receives an Extended Attribute with a Length of 2 or 3,
      then that Attribute MUST be considered to be an "invalid
      attribute", and handled as per Section 2.8, below.

Curious: What does a client or server do when it receives an Extended
Attribute with a length of 0 or 1?  (Same comment goes for Section
2.2.)

=== DISCUSS ===
Also, this doesn't make sense to me.  Section 2.8 starts with this:

   The term "invalid attribute" is new to this specification.  It is
   defined to mean that the Length field of an Attribute is valid (as
   per [RFC2865], Section 5, top of page 25).

These two appear to be contradictory: the first says that if the
length is invalid, it's an "invalid attribute", and the second says
that "invalid attribute" means that the length field is valid (but
there are other problems).  This comment also applies to "TLV-Length"
in Section 2.3.

=== DISCUSS ===
In the description of "Value":

      This field is similar to the Value field of the Attribute format
      defined in [RFC2865] Section 5.  The format of the data SHOULD be
      a valid RADIUS data type.

      The addition of the Extended-Type field decreases the maximum
      length for attributes of type "text" or "string" from 253 to 252
      octets.  Where an Attribute needs to carry more than 252 octets of
      data, the "Long Extended Type" format SHOULD be used.

The first SHOULD makes a subtle, but very significant change to the
RADIUS protocol, and I want to make sure what you're doing is what you
mean to do.  2865 says, "The format of the value field is one of five
data types." It has to be one of those types, and can't be anything
else -- nothing else is defined.  But you are now allowing an
implementor who understands the implications of doing so to use a
different data type.  So:

1. Do you really mean to allow that?
2. If "yes" to (1), what other data types exist that might be used?
Where are their formats defined?
3. If "yes" to (1), what *are* the implications of using other data types?

That comment also applies to "Value" in Section 2.2 and to "TLV-Value"
in Section 2.3.

For the second SHOULD, there's a similar issue: What are the
alternatives to using the "Long Extended Type" format?  What are the
implications of using those alternatives?

-- Section 2.2 --

=== COMMENT ===
In the description of "Type":

As in 2.1, I suggest adding a forward reference to make an important
clarification here:

NEW
      This field is identical to the Type field of the Attribute format
      defined in [RFC2865] Section 5.  It will contain one of the Long
      Extended Type values defined in Section 3, below.
END

=== DISCUSS ===
In the description of the "M" bit:

1.  The first two paragraphs contradict each other.  The first says
that M MUST be 0 if length < 255.  The second says, "When the More
field is set (1), the attribute SHOULD have a Length field of value
255".  According to the first paragraph, that needs to be MUST, not
SHOULD.  And then with that change, the next clause (about value 4)
can be removed.  The "that is" at the beginning of the third sentence
should be removed, because the ordering has not been mentioned before.

2.  The last sentence of the second paragraph ends, "and the last
attribute in a packet MUST NOT have the More field set (1)." That's
certainly true, but isn't what you really want to say that all pieces
of the entire fragmented attribute MUST be included in one packet?

3.  The third paragraph, which contradicts the first, should be removed.

In the description of "Value":

=== DISCUSS ===
      This field is similar to the Value field of the Attribute format
      defined in [RFC2865] Section 5.  It may contain a complete set of
      data (when the Length field has value less than 255), or it may
      contain a fragment of data (when the More field is set).

Isn't it possible for the Length field to be equal to 255 and to have
M=0 (if the last fragment of the fragmented attribute takes up the
whole 255 bytes, for instance)?  And doesn't the last fragment of a
fragmented attribute contain a fragment of data, even though the More
field is 0?

=== DISCUSS ===
      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.

This is incorrect: the RADIUS packet length is limited to 4096 octets
*including* the various headers and overhead.  That puts a maximum
size of fragmented attributes as something less than 4096.  As I
compute it:

The RADIUS header is 20 octets, so that's 4096-20 = 4076 octets left
for attributes.  That will hold 15 fragments of 255 octets each, and a
16th fragment of 231 octets.  And that results in a total attribute
length (after headers are removed and the fragments are assembled) of
15*251+227 = 3992.

=== DISCUSS ===
The last paragraph of Section 2.2 says this:

   Implementations MUST be able to process non-
   contiguous fragments.  That is, fragments which are mixed together
   with other attributes of a different Type.

That contradicts what you said in the description of the "M" bit:

      That is, multiple
      fragments of the same value MUST be in order and MUST be
      consecutive attributes in the packet

=== DISCUSS ===
-- Section 2.3.1 --

   The depth of TLV nesting is limited only by the restrictions on the
   TLV-Length field.  The limit of 253 octets of data results in a limit
   of 126 levels of nesting.  However, nesting depths of more than 4 are
   NOT RECOMMENDED.

Again: Why is it NOT RECOMMENDED?  How would I know how to evaluate
the implications of going for more than 4 levels?

=== COMMENT ===
-- Section 2.7.2 --

The last paragraph seems out of place, as the following section
describes the TLV naming, basically repeating the last paragraph of
2.7.2.  I suggest correcting that organizational oddness.

=== COMMENT ===
-- Section 2.7.3 --

At the end of the first paragraph, I think "replied" should be "repeated".

=== COMMENT ===
-- Section 2.7.4 --

In the first sentence of the fourth paragraph, it seems as though "few
restrictions" is meant to be "a few restrictions", yes?

What does the example in paragraph 5 tell me that the example in
paragraph 3 doesn't?  That is, why is the second example needed?

=== COMMENT ===
-- Section 2.8 --

   For Attributes of type "Long Extended Type", an Attribute is
   considered to be an "invalid attribute" when does not match the
   criteria set out in Section 2.2, above.

1.  Typo: "when *it* does not match".

2.  I think it would be useful to remind implementors that one bad
apple spoils the whole bunch: that if one fragment of a fragmented
attribute is invalid, the entire attribute (all fragments) needs to be
considered invalid.

=== COMMENT ===
-- Sections 3 and 4 subsections --

It somewhat bothers me to have the diagrams and some information
repeated here (repeated repeatedly, in fact).  Is it not possible to
refer back to Sections 2.1 and 2.2 even more than you already do?
Perhaps like this?:

NEW
3.1.  Extended-Type-1

   Description

      This attribute encapsulates attributes of the "Extended Type"
      format, in the RADIUS Attribute Type Space of 241.{1-255}.
      See Section 2.1 for details and a diagram of the attribute
      format.

   Type

      241 for Extended-Type-1.

   Length

      >= 4

   Extended-Type

      See Section 2.1 for details.  Up-to-date values of this field
      are specified in the 241.{1-255} RADIUS Attribute Type Space.

   Value
      See Section 2.1 for details.
END

You might then also add the information in these two paragraphs to the
descriptions of "Value" in Sections 2.1 and 2.2:

      The Value field is one or more octets.  Implementations not
      supporting this specification SHOULD support the field as
      undistinguished octets.

      Implementations supporting this specification MUST use the
      Identifier of "Type.Extended-Type" to determine the interpretation
      of the Value field.

Does that make sense?

-----------------------------------------------------------