[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Fwd: draft-zorn-radius-err-msg-10 (fwd)



Forwarding for a non-list member:


From A.Hoenes@TR-Sys.de Thu Dec 11 19:01:22 MEZ 2008
Received: from ZEUS.TR-Sys.de by w. with ESMTP ($Revision: 1.37.109.26 $/16.3)
      id AA023378481; Thu, 11 Dec 2008 19:01:21 +0100
Return-Path: <A.Hoenes@TR-Sys.de>
Received: (from ah@localhost) by z.TR-Sys.de (8.9.3 (PHNE_25183)/8.7.3)
      id TAA22555; Thu, 11 Dec 2008 19:01:20 +0100 (MEZ)
From: Alfred H�nes <ah@TR-Sys.de>
To: gwz@net-zen.net
Message-Id: <200812111801.TAA22555@TR-Sys.de>
Date: Thu, 11 Dec 2008 19:01:20 +0100 (MEZ)
Subject: draft-zorn-radius-err-msg-10

Hello again,
I also have reviewed your Internet-Draft draft-zorn-radius-err-msg-10
and would like to submit a few comments.

I have a couple of concerns regarding the structure of the document.

First of all, it looks like Section 3 was intended as a summary
of related pre-existing specifications for RADIUS.
Thus, it comes to surprise that this section contains two inserts
giving details for the new protocol elements that will be introduced
later in the memo, in Sections 4 ff.
To avoid confusion, I suggest to move these parts to the proper
places later in the document; this will save text and improve
the clarity of the document.
Very few generally applicable text should be moved in the opposite
direction, as well.
I'll give the details in the document walk-through below.

Subsequently, Sections 4..6 are structured for extensibility,
which obviously is not needed in this document.
The headlines always let the reader expect to come more, where
there's always only a single new protocol object ro define.
I strongly suggest to update the headlines and amalgamate these
sections with their single subsection each.


(1)  Section 3

The draft text starts with:

 3.  RADIUS Packet Format

   Exactly one RADIUS packet is encapsulated in the UDP Data field
   [RFC0768] where the UDP Destination Port field indicates 1812
   (decimal).

   When a reply is generated, the source and destination ports are
   reversed.

The topic of these two paragraphs, transport of radius messages,
is not directly related to the "Packet Format" announced by the
section headline.
I suggest to move these two paragraphs to the end of the section,
at best in a new subsection,

 3.1.  RADIUS Transport

There, it should at least be mentioned that TCP (and TLS-secured)
transport of RADIUS packets is currently pursued in the RADEXT WG
as well (for instance, see draft-ietf-radext-tcp-transport).

I propose to move the short text currently in Section 4 next here
in Section 3, in front of the diagram, and add a more fundamental
explanation. In the 'Administrative Note' at the end of the section,
the term "RADIUS proxy" is used as well, without priop explanation.
Therefore, I recommend to also introduce this concept here.

For instance:

|  RADIUS is a request-reply protocol, where each request packet sent
|  from a "RADIUS Client" to a "RADIUS Server" is responded to with a
|  reply packet.  A "RADIUS proxy" relays, as an intermediate agent,
|  requests and reponses between an original RADIUS client and the final
|  RADIUS server, behaving as a server for the forme and as a client for
|  the latter.  The RADIUS Packet type is determined by the Code field
|  in the first octet of the Packet.
|
|  A summary of the RADIUS packet format is shown below.  The fields are
                           ^^^^^^
   transmitted from left to right.

Having that statement in front of the diagram, the sentence immediately
below the diagram,

         The Code field is one octet, and identifies the type of RADIUS
         packet.

... could be simplified to only say:

|        The Code field is one octet, and identifies the packet type.

... or even:

|        The Code field is one octet.


The second part of the explanation for 'Code' is not of general nature.
IMHO, it should be removed from this section, and elaborated upon in
Section 4:

DELETE:

|        The RADIUS Codes (decimal) defined in this document are as
|        follows:
|
|           <MSG1> Error-Notification

Again, in the explanation of 'Authenticator', the second part with
the headline 'Notification Authenticator' comes to surprise here.
I suggest to move it entirely into Section 4:

DELETE:

|        Notification Authenticator
|
|           The value of the Authenticator field in the Error-
|           Notification packet is called the Notification
|           Authenticator, and contains a one-way MD5 hash calculated
|           over a stream of octets consisting of: the RADIUS packet,
|           beginning with the Code field, including the Identifier, the
|           Length, the Authenticator field from the packet to which
|           this packet is a response, and the response Attributes,
|           followed by the shared secret.  That is,
|
|           Notification Auth =
|                     MD5(Code+ID+Length+RequestAuth+Attributes+Secret)
|           where '+' denotes concatenation.

In the 3rd paragraph of the 'Administrative Note', I suggest to add a
few clarifying words in order to disambiguate the scenario described:

         When using a forwarding proxy, the proxy must be able to alter
         the packet as it passes through in each direction - when the
         proxy forwards the request, the proxy MAY add a Proxy-State
         Attribute, and when the proxy forwards a response, it MUST
|        remove its Proxy-State Attribute if it added one.  [...]
---
         When using a forwarding proxy, the proxy must be able to alter
         the packet as it passes through in each direction - when the
         proxy forwards the request, the proxy MAY add a Proxy-State
         Attribute, and when the proxy forwards a response, it MUST
|        remove its Proxy-State Attribute if it added one to the
         vvvvvvv                                         ^^^^^^^
|        request.  [...]


(2)  Section 4

As noted above, I propose to simplify and clarify the structure
of this section:

|4.  Packet Types
|
|  The RADIUS Packet type is determined by the Code field in the first
|  octet of the Packet.
|
|4.1.  Error-Notification

The prose already has been moved inTo section 3, so the replacement
is simply:

|4.  New Packet Type: Error-Notification

The explanation for 'Notification Authenticator' currently says:

      Notification Authenticator

|        The Notification Authenticator value is calculated from the
|        Error-Notification packet, as described above.

This sentence IMO does not match reasonably what was currently
described in Section 3.
As mentioned above, I strongly recommend to move that text (DELETED
above) to this location; doing so allows to drop the above two-liner
entirely:

      Notification Authenticator

|        The value of the Authenticator field in the Error-Notification
|        packet is called the Notification Authenticator, and contains a
|        one-way MD5 hash calculated over a stream of octets consisting
|        of: the RADIUS packet, beginning with the Code field, including
|        the Identifier, the Length, the Authenticator field from the
|        packet to which this packet is a response, and the response
|        Attributes, followed by the shared secret.  That is,
|
|           Notification Auth =
|                   MD5(Code+ID+Length+RequestAuth+Attributes+Secret)
|
|        where '+' denotes concatenation.

As I understand that (unchanged) text, with the single exception of the
original Authenticator field from the errored packet, all these fields
are the fields of the Error-Notification packet; that was not clear
from the two-liner above!

Another concern:  This new 'signature' again is stuck with the old-
fashioned Keyed-MD5 construction; obviously, doing so violates the
directive from the IESG and the IAB to equip all IETF protocols with
capabilities for crypto-algorithm agility.


(3)  Section 5

Again, as indicated above, I strongly recommend to simplify the
section structure:

|5.  Attributes
|
|5.1.  Error-Code
---
|5.  New Attribute: Error-Code


(4)  Section 6

Currently the text in Section 4 says:

                           vv      vv
|  The following sub-sections defines a new value for the Acct-Status-
   Type Attribute [RFC2866].

That's bogus grammar anyway.  However, following the spirit of the
above proposals for simplification, I suggest to change that entirely:

|6.  Attribute Values
|
|  The following sub-sections defines a new value for the Acct-Status-
|  Type Attribute [RFC2866].
|
|6.1.  Acct-Error-Notification
---
|6.  New Attribute Value: Acct-Error-Notification

   This section defines a new value for the Acct-Status-Type Attribute
   [RFC2866], 'Acct-Error-Notification' (<VAL>).


(5)  Section 7

Since this document does not define any new (sub-)namespaces to be
managed by IANA, the present text seems to be misleading.
Instead, the three specific assignments should be detailed there;
for instance, following this pattern (cf BCP 26, RFC 5226):

|  The criteria to be used by the Internet Assigned Numbers Authority
|  (IANA) for assignment of numbers within namespaces defined within
|  this document are identical to those given in [RFC3575].
---
|  IANA is requested to assign three code points within the RADIUS
|  Parameters registry defined in [RFC3575], currently located at
|  <http://www.iana.org/assignments/...> :
|
|  o  One RADIUS Packet Type
|
|     Type     Name                    Reference
|     ------------------------------------------
|     <MSG1>   Error-Notification      [RFCthis]
|
|  o  ...
|
|
|  o  ...


(6)  Section 8

|8.  Security Considerations
|
|  None.

I bet the IESG will not be a friend of such brevity ...


Kind regards,
  Alfred H�nes.

--

+------------------------+--------------------------------------------+
| TR-Sys Alfred Hoenes   |  Alfred Hoenes   Dipl.-Math., Dipl.-Phys.  |
| Gerlinger Strasse 12   |  Phone: (+49)7156/9635-0, Fax: -18         |
| D-71254  Ditzingen     |  E-Mail:  ah@TR-Sys.de                     |
+------------------------+--------------------------------------------+