Issue 101: WG Last Call Review

["Congdon, Paul T (ProCurve)" <paul.congdon@hp.com>]

[pc] Here are some proposed resolutions to Pasi's Issue 101 WG review
comments:

Overall impression: NAS/QoS-Filter-Rule part is quite messy, other parts
are reasonably OK. I'll send my comments about NAS-Filter-Rule in a
separate email.

1) Section 1.1 contains some unnecessary terminology that is not used
anywhere later in the document: "Access Point (AP)", "Association",
"Port Access Entity (PAE)", "Station (STA)"

[pc] - agreed, to be removed.

2) Section 2.2: "If accounting is enabled on the NAS, it MUST generate
an Accounting-Request (Stop) message upon session termination."

So the NAS must send a Stop message, even though it has not sent a Start
message for this session yet? RFC 2866 seems suggest that a session
always has a Start message before Stop, but does not seem to outright
prohibit sending a stand-alone Stop message. Is this a common practice,
or are there any problems associated with it?

[pc] I believe that this should not cause any problem, but to be honest,
we could use some input from others.  I'm aware of some implementations
that send a start and then immediately follow with a stop, but this
could be more resource intensive than needed.  The stop without a start
would either be ignored by the server or appropriately acknowledge the
termination of the session that didn't start.  This could be useful for
diagnostic purposes.  I'd love to hear from other Radius vendors as to
whether this would be a problem or not.  Again, my assumption is that it
isn't.

3) Section 3.1: 802.1Q does not use the abbreviation "VLANID"
but rather "VID". Should this document also follow that convention?
(but maybe VLANID is more understandable)

[pc] Yes, 802.1Q most often uses VID for VLAN Identifier. It sometimes
uses VLAN ID (with a space).  However, RFC 3580 uses VLANID and since
this document is most likely used in conjunction with RFC 3580, I
suggest we just leave VLANID as is. 

4) Section 3.1: "Padding bits SHOULD be 0 (zero)." Why "SHOULD"
instead of "MUST"? (according to RFC 2119, it would mean there are valid
reasons to send something else in some circumstances?) (This same issue
occurs also in Section 4.2.)

[pc] This can be changed to MUST.  The desired behavior, IMO, is to
require the bits to be set to 0, but not require them to be checked on
receipt.  The simple MUST or SHOULD wording doesn't really reflect this,
so it is probably best that we make this a MUST.  Agree to change this
SHOULD to a MUST here and in 4.2 

5) Section 3.3: If VLAN-Name has basically the same meaning as
Egress-VLANID, IMHO their names should reflect this. Perhaps
"Egress-VLAN-Name"?

[pc] Sure, no objection.  Change the name of the VLAN-Name attribute to
Egress-VLAN-Name everywhere in the document.

6) Section 3.3: Using "START OF HEADING" (0x01) and "START OF TEXT"
(0x02) control characters to indicate tagged/untagged might make it
unnecessarily difficult to enter this attribute in a management
interface... maybe something like 0x74/0x75
("t"/"u") or 0x31/0x32 ("1"/"2") would be easier?

[pc] We were trying to get Egress-VLANID and (now called)
Egress-VLAN-Name to have as similar a format as possible.  While
Egress-VLAN-Name could be made to be an all text type of management
interface, Egress-VLANID can not.  The VLANID is a simple 12-bit number
and won't map well to the keyboard input optimization no matter what we
do, so some form of special formatting for the management interface will
be required to support these two attributes.  Also, tagged and untagged
are the only flags we need today, but perhaps this field would need to
be extended someday (e.g. 802.1ad support), and using more bits to
represent the two values needed today would seems to be wasteful.  I
propose no change here.

7) Section 3.3: Probably should have reference to UTF-8? [RFC3629]

[pc] yes, Add a reference to [RFC3629] here and then a formal reference
in section 8.

8) Section 3.4: "The table, expressed as an 8 octet string, maps the
incoming priority (if one exists - the default is 0) into one of seven
regenerated priorities." If the regenerated priority is an integer in
range 0-7, should that be "eight regenerated priorities"?

[pc] yes, change to "eight regenerated priorities"

9) A comment about the bit figures in Section 3 and 4. 
Currently, all the figures are basically identical: they contain the
RADIUS Type and Length fields, plus one other field (variably called
either "Integer", "String" or "Text"). 
The contents of this field are then described in English.

I think it would be much more understandable if the figure also showed
the structure. For instance, instead of this (Section 3.1),

+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+ 
|     Type      |    Length     |            Integer          
+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+ 
|        Integer(cont.)         |  
+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+ 

this would IMHO be more readable:

+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+ 
|     Type      |    Length     |   Tag Option  |     (zero)   
+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+ 
 (zero) |   VLANID (12 bits)    |  
+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+ 

(Note that I'm not proposing changing the attribute formats at all -- so
no changes to any software --, just how they are presented in the
document. And we can keep all the zero padding there if we want...)

[pc] To make this clean and follow your recommendation I believe it has
significant structural changes to the document.  Most of the attributes
do not have diagrams at all.  There would need to be a lot of new ASCII
art added and it would be difficult for string based fields like
NAS-Filter-Rule to accommodate this.  If we put all the internal field
names in the base attribute diagram along with Type and Length, it would
seem that we would need to describe each of the other fields
individually rather than indicating how to format the Integer, Text or
String portion.  I'd rather keep the bulk of the attribute sections
intact and only provide a diagram describing how to format the Integer,
Text or String porition - as is done in 3.1.  We could certainly make
3.2, 3.3 and 3.4 more consistent with 3.1 and add a diagram on how to
format the Integer, Text and/or String component.  Would this work?  

10) Section 5: If the semantics of Acct-EAP-Auth-Method are identical to
Diameter EAP (as Section 4.2 claims), then it can also be included in
Access-Accept messages. (Or if it can't, then the semantics are not
identical, and need to be described in this document.)

[pc] agreed, but I believe this attribute is going to be removed from
the draft per issue 114.

11) Section 7: It looks like this security considerations section was
just copied from somewhere without much thought for how well it actually
applies to this document. Most of the text looks redundant to me; we
don't need to describe how to use IPsec with RADIUS in every document
that defines a new RADIUS attribute.

On the other hand, there's exactly zero discussion about new threats
caused by the attributes defined in _this_ document.
And to me it looks like there are new attacks that e.g. a malicious or
compromised RADIUS server or proxy could do with these attributes
(compared to the situation where NAS does not support these attributes).
At least Egress-VLANID, Ingress-Filters and NAS-Filter-Rule have some
pretty obvious possibilities for abuse... (and maybe some non-obvious as
well)

[pc] Yes, much of this text was copied from 3580, but short of trying to
imagine all the things that could happen if a RADIUS server is
comprimized, I'm not sure what else to put here.  We could puts some
words here about how modified settings of things like Ingress-Filters or
Filter-Rules could change the expected behavior, but the discussion
would not be exhaustive.  I thought the point of this section was to
document what the vulnerabilities are, not how they might be used.  For
example, the scheme is vulnerable to packet  modification attacks, but
we can't document what might happen with every possible modification of
a packet.  Please provide some more explanation or examples of what you
are looking for here. 

12) References: Both 802.1Q and .1D look normative to me, but
RFC1321 and 802.11i probably are just informative.

[pc] agreed.  Swap these reference locations.

13) References: Document cites [DiamEAP] and [NASREQ], but they're not
included in the references section. Both are normative.

[pc] agreed.  Add these references.

--
to unsubscribe send a message to radiusext-request@ops.ietf.org with
the word 'unsubscribe' in a single line as the message text body.
archive: <http://psg.com/lists/radiusext/>