Subject: [radext] Review of draft-ietf-radext-datatypes-02

From: Stefan Winter <[email-masked]>
Date: Tue, 3 May 2016 15:55:35 +0200

Hello,

I'm currently reviewing draft-ietf-radext-datatypes-02 as part of the
shepherd write-up. I have a few questions/change requests below:

* The introduction mentions a number of "war stories" of specifications
that have left data type gaps or introduced ambiguities. The I-D
correspondingly has an Updates relationship to these RFCs. I wonder: is
this list meant to be exhaustive? I.e. are there no other RFCs with
similar problems? Obviously, if there are others, then there should also
be an Updates relationship to them.

* 2.1.2 para 3 states that attributes can be allocated from standard or
extended space. I believe we are at a point where the standard space is
full and no allocations are made to standard space any more. I think the
ambiguity in this paragraph has been overtaken by events and can simply
go away.

* 2.2 contains a link to "Section 2.6". That section does not exist;
please update the link to point to something existing.

* Section 3, last para is a bit confusing. You state that you do not
create a data type for Location-Capable; but later in section 4, the
data type "enum" is assigned to this attribute. Worse, the last sentence
of the para speaks of "it's better to treat complex attributes as 'string'".
enum requires (MUST) all enumerated values to be defined. Looking at
RFC5580, this is not really an enumeration, rather a bit field. 4 of the
bits are defined, resulting in 16 different enumerated values, all of
which need a unique name. Is that what you want to happen? Then the I-D
at hand needs to make that enumerated list for Location-Capable since
the original RFC did not.
Or maybe this is actually a symptom of needing a data type "bit field"
instead, where only the bits have a unique name?

3.8 ifid: any particular reason why interface IDs are limited to 8
octets length? Just wondering because I named the network interface of
my workstation "workstation" and would not be able to use such a name in
RADIUS with this definition. Looking at our firewall, we even have
actual production interfaces with names such as "10G.privatetest" (for
some VLAN).

3.10 Prefix - this appears to be variable length, and the length of the
prefix is defined by Refix-Length.
I don't understand why deployments are given the degree of freedom to
decide whether they want to pad the prefix with trailing zeroes or not
(which is enabled by the formulation "Bits outside of the Prefix-Length,
** if included **, MUST be zero.". Maybe that's just my naive thinking,
but wouldn't it be more straightforward to mandate the Prefix sub-field
to have exactly the length as specified in Prefix-Length, padded to
*exactly* the next octet border?

3.11 Length is indicated as variable from 2 - 18 octets here. It looks
to me like the max length possible is 4 octets (Prefix) + 1 octet
(Reserved) + 6 bits (Prefix-Length), in total 5 octets and 6 bits. The
Length description should be updated accordingly.
As a second remark, I understand that the structure of the data types is
taken from existing specs. However the structure of this data type is
really awkward from a parsing point of view - fields delimited a full
octet (like making Prefix-Length *8* bits long even if only values until
32 are used) would make the data type much prettier.

* related to the same section: The RADIUS data type registry marks
RFC6572 as the source of the ipv4prefix data type. I can't find a
Prefix-Length which is 6 bit long in that RFC? Where exactly do I find
the original 6-bit length in this complex data type?

* 3.13: TLV-Length is (correctly) 3 or more octets, because TLV-Data
must be at least one byte. An empty TLV-Data would lead to a TLV-Length
of *2* though - but in this situation, the entire TLV is to be omitted.
This makes the last sentence in the section a bit awkward:
"TLVs having TLV-Length of zero (0) MUST NOT be sent; omit the entire
TLV instead."
This should either start with "TLVs having a TLV-Data of zero bytes
length" or it should stat "TLVs having TLV-Length of two (2)" - both
sentences are equivalent.

* IANA considerations point to a wrong section: 3.1 is not the
description of the new registry; you probably meant to xref to section 4.1.

* same for the second para which should refer to 4.2, not 3.2.

* 4.1: We occasionally make beauty contests for the numbers we assign
;-) So here is one: wouldn't it be much more pretty if "ipv4prefix" and
"ipv4addr" were grouped together, like "ipv6prefix" and "ipv6addr" are?
Since the "Value" column doesn't have an on-the-wire effect, it should
be easy to shuffle those around.

* I think we need to do something about "Acknowledgements" prior to
sending this to IETF LC. The single word "Stuff" does not tell readers a
whole lot ;-)

* nit: s/Ascii/ASCII/g

* nit: Section 3 : "We do not create specific data types for the
'tagged' attributes defines in" -> s/defines/defined/

I'm ending my review here, but will have another check of at least
samples of all the attribute, data type and reference tuples in 4.2 at a
later time.

I guess some of the comments above will need a new I-D to tackle; I'll
hold the shepherd write-up and the 4.2 checks until the new I-D is out.

Greetings,

Stefan Winter

-- 
Stefan WINTER
Ingenieur de Recherche
Fondation RESTENA - Réseau Téléinformatique de l'Education Nationale et
de la Recherche
2, avenue de l'Université
L-4365 Esch-sur-Alzette

Tel: +352 424409 1
Fax: +352 422473

PGP key updated to 4096 Bit RSA - I will encrypt all mails if the
recipient's key is known to me

http://pgp.mit.edu:11371/pks/lookup?op=get&search=0xC0DE6A358A39DC66

---

From: Alan DeKok <[email-masked]>
Date: Fri, 6 May 2016 20:44:35 -0400

> be an Updates relationship to them.

  The "Updates" line should reference all RFCs it updates.  Mainly the ones which have incomplete and/or inconsistent data types.

  But to be pedantic, the document updates *all* RFCs which define a RADIUS attribute.  It has to do this (arguably) in order to define a data type for each attribute.

  Exactly what goes into the "Updates" line is a subject best left for the list.

> ambiguity in this paragraph has been overtaken by events and can simply

  Point taken.  I'll re-word it.

> please update the link to point to something existing.

  That should be Section 3.6

> create a data type for Location-Capable;

  That reference should be to "Location-Information", which addresses the later comments.

> octets length?

  RFC 4291 Section 2.5.1

https://tools.ietf.org/html/rfc4291#section-2.5.1

> Just wondering because I named

  Local interface names are not "interface identifiers".

> (which is enabled by the formulation "Bits outside of the Prefix-Length,<ipv4

  The definition mirrors RFC 3162  I didn't want to re-define it in an incompatible way here.

> Length description should be updated accordingly.

  That's a cut & paste error from Section 3.10.  Length for ipv4prefix should be fixed to 6 octets, as per RFC 6572 Section 4.12.

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

> 32 are used) would make the data type much prettier.

  I think that's a safe change to make here.

> the original 6-bit length in this complex data type?

  RFC 6572.

  To be realistic... I think it's OK to update the "Reserved" field to be 8 bits instead of 10.  And to increase the size of the "Prefix-Length" field to be 8 bits instead of 6, with the upper 2 bits mandated to be zero.

> sentences are equivalent.

  I'll use the second phrasing.

> description of the new registry; you probably meant to xref to section 4.1.

  Fixed.

> * same for the second para which should refer to 4.2, not 3.2

  Fixed.

> be easy to shuffle those around.

  Sure... the intention was mostly to define simple types before complex types.  But that's fine.

> sending this to IETF LC. The single word "Stuff" does not tell readers a

  Fixed.

> * nit: s/Ascii/ASCII/g

  Fixed.

> 'tagged' attributes defines in" -> s/defines/defined/

  Fixed.

> samples of all the attribute, data type and reference tuples in 4.2 at a

  Yes.

  The "integer" and "ipv4addr" ones should be fine.  The "octets" and "string" ones may be harder... I picked things which make sense, but... that's just my $0.02.

> hold the shepherd write-up and the 4.2 checks until the new I-D is out.

  Done (momentarily)

  Alan DeKok.

---

From: Stefan Winter <[email-masked]>
Date: Mon, 9 May 2016 09:15:00 +0200

Hi,

>   Exactly what goes into the "Updates" line is a subject best left for the list.

Good point. FWIW, my take on it is:

* if the only update to an attribute is the addition of the "data type"
column, that's a trivial case and does not warrant an Updates relationship.
* everything else does.

>   That reference should be to "Location-Information", which addresses the later comments.

most of them, yes. One point remains, now only pertaining to the mention
of Location-Capable in section 4, where it's an "enum":

This part of my review still isn't covered IMHO:

"Looking at
instead, where only the bits have a unique name?"

> Local interface names are not "interface identifiers". 

Thanks. That explains it nicely :-)

>   The definition mirrors RFC 3162  I didn't want to re-define it in an incompatible way here.

Ok.

>   I think that's a safe change to make here.

Great.

>   To be realistic... I think it's OK to update the "Reserved" field to be 8 bits instead of 10.  And to increase the size of the "Prefix-Length" field to be 8 bits instead of 6, with the upper 2 bi…

Definitely looks like a better choice parsing-wise. I guess if the PMIP
people have a problem with that backwards-compatible change, they can
complain during IETF LC.

Do you want to explain why the difference in definition is a
non-difference; or shall I just add a note to the shepherd review in
case this comes up later?

As one last comment on -03: you introduced a new nit :-)

s/esstentially/essentially/

Greetings,

Stefan Winter

-- 
Stefan WINTER
Ingenieur de Recherche
Fondation RESTENA - Réseau Téléinformatique de l'Education Nationale et de la Recherche
2, avenue de l'Université
L-4365 Esch-sur-Alzette

Tel: +352 424409 1
Fax: +352 422473

PGP key updated to 4096 Bit RSA - I will encrypt all mails if the recipient's key is known to me

---

From: Alan DeKok <[email-masked]>
Date: Tue, 10 May 2016 09:43:37 -0400

> column, that's a trivial case and does not warrant an Updates relationship.

  Sounds good to me.  I'll take a look through the draft.

> instead, where only the bits have a unique name?"

  No, it's probably best to just make it an "integer" type.

  Yes.

> case this comes up later?

  I'll add a note to the document.

> s/esstentially/essentially/

  Fixed, thanks.

  Alan DeKok.