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

Fwd: draft-ietf-radext-status-server-02 editorials (fwd)



Forwarding for a non-list member:


From A.Hoenes@TR-Sys.de Thu Dec 11 16:15:59 MEZ 2008
Received: from ZEUS.TR-Sys.de by w. with ESMTP ($Revision: 1.37.109.26 $/16.3)
      id AA022718557; Thu, 11 Dec 2008 16:15:57 +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 QAA22212; Thu, 11 Dec 2008 16:15:57 +0100 (MEZ)
From: Alfred H�nes <ah@TR-Sys.de>
To: aland@freeradius.org
Cc: radiusext@ops.ietf.org
Message-Id: <200812111515.QAA22212@TR-Sys.de>
Date: Thu, 11 Dec 2008 16:15:56 +0100 (MEZ)
Subject: draft-ietf-radext-status-server-02 editorials

Hello,
after studying the Internet-Draft authored/edited by you,
          draft-ietf-radext-status-server-02,
I'd like to submit a few comments, mostly addressing textual
flaws I found in that memo.

Note on proofreading:
  Quickly browsing the radext list, I saw that there's some overlap
  of another review with the items below.  My apologies -- I didn't
  clean up that all now to avoid having to renumber the items.


The items below are presented in textual order.
To give more context, sometimes I quote larger blocks of text
literally and show the replacement proposed using the shorthand
notation:

   <original draft text>
---
   <modified text>

I use change bars ('|' in column 1) and up/down pointing marker
lines ('^^^'/'vvv') to emphasize the location of textual issues
and/or proposed corrections.  Modified text has been re-adjusted
to match RFC formatting rules, where appropriate.


(1)  Section 1, last para -- typo/grammar

      The remaining text contains ...  and discussed security ...
---                                                ^
      The remaining text contains ...  and discusses security ...
                                                   ^

(2)  Section 2.1, last para -- typo

s/qeuries/queries/
   ^^      ^^

(3)  Section 2.3.2

(3a)  headline missing  (already noted)

(3b)  requirements language

Section 2.3.1 starts:

   Status-Server SHOULD be used instead ...
                 !!!!!!

But 2.3.2, which apparently is intended to catch the alternative
case left open by that 'SHOULD', starts with:

|  Status-Server may be used instead of ...
                 ^^^
For logical consistency, this counterpoint also should use uppercase:

|  Status-Server MAY be used instead of ...
                 ^^^

(4)  Section 2.3.3

Following up from above, 2.3.3 should also be changed:

   Status-Server may be pro-actively sent ...
---
   Status-Server MAY be pro-actively sent ...
                 ^^^

(5)  Section 3 -- clarifications

(5a)  1st para below indented part 'Request Authenticator'

The draft says:

   Similarly, the Response Authenticator field of an Access-Accept
   packet sent in response to Status-Server queries MUST be generated
   using the same method as used for for calculating the Response
|  Authenticator of the Access-Accept, with the Status-Server Request
   Authenticator taking the place of the Access-Request Request
   Authenticator.

For clarity, I suggest to insert a phrase better qualifying the
Access-Accept taken as a reference:

   Similarly, the Response Authenticator field of an Access-Accept
   packet sent in response to Status-Server queries MUST be generated
   using the same method as used for for calculating the Response
|  Authenticator of the Access-Accept sent in response to an Access-
   vvvvvvvv                          ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
|  Request, with the Status-Server Request Authenticator taking the
   place of the Access-Request Request Authenticator.

(5b)  2nd para below indented part 'Request Authenticator'

Similarly, I suggest to improve the wording in the next paragraph:

   The Response Authenticator field of an Accounting-Response packet
   sent in response to Status-Server queries MUST be generated using the
   same method as used for for calculating the Response Authenticator of
|  the Accounting-Response, with the Status-Server Request Authenticator
   taking the place of the Accounting-Request Request Authenticator.
---
   The Response Authenticator field of an Accounting-Response packet
   sent in response to Status-Server queries MUST be generated using the
   same method as used for for calculating the Response Authenticator of
|  the Accounting-Response sent in response to and Accounting-Request,
                          ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
   with the Status-Server Request Authenticator taking the place of the
   Accounting-Request Request Authenticator.


(6)  Section 3.1

(6a)  headline

To better indicate the simplification for implementations, which seems
to be the spirit of this detail, I suggest to change the headline
(this will be even more become clear by the subsequent modification
I'll propose below):

|3.1.  Consistent definition for Status-Server
---
|3.1.  Single Definition for Status-Server
       ^^^^^^^^

(6b)  2nd paragraph -- clarification / textual improvement

I suggest to replace "one" by "a single":

          vvv
|  Having one definition for Status-Server packets is simpler than
   having different definitions for different destination ports.  [...]
---       vvvvvvvvv
|  Having a single definition for Status-Server packets is simpler than
   having different definitions for different destination ports.  [...]


(7)  Section 4 -- consistent spelling of abbreviation

For uniformity and consistency, I suggest to    s/coa/CoA/
(2 instances in Section 4).


(8)  Section 4.2

(8a)  need to update the metadata (front matter)

The third para says:

   We note that [RFC2865] Section 3 defines a number of RADIUS Codes,
   but does not make statements about which Codes are valid for port
|  1812.  In contrast, [RFC2866] Section 3 specifies that only RADIUS
|  Accounting packets are to be sent to port 1813.  This specification
   is compatible with [RFC2865], as it uses a known Code for packets to
   port 1812.  This specification is not compatible with [RFC2866], as
   it adds a new code (Status-Server) that is valid for port 1812.
|  However, as the category of [RFC2866] is Informational, this conflict
|  is acceptable.

This statement clearly indicates that this specification
*** Updates RFC 2866 ***.

This relationship really should be noted in the front matter of the
draft.

(8b)  7th para -- word omission

Please correct:
                                         v
|  The server MAY prioritize the handling Status-Server queries over the
   handling of other requests, subject to the rate limiting described
   above.
---                                      vvvv
|  The server MAY prioritize the handling of Status-Server queries over
   the handling of other requests, subject to the rate limiting
   described above.


(9)  Section 4.3

(9a)  1st para -- improvement #1

I suggest to improver the wording:

   When a Dynamic Authorization Client ([RFC5176] Section 1.3) reboots,
|  it SHOULD send a Status-Server packet to a CoA port to IP addresses
                                         !!            !!
   that are configured as both Dynamic Authorization Servers and RADIUS
   clients.  [...]
---
   When a Dynamic Authorization Client ([RFC5176] Section 1.3) reboots,
|  it SHOULD send a Status-Server packet to a CoA port at IP addresses
                                                       ^^
   that are configured as both Dynamic Authorization Servers and RADIUS
   clients.  [...]

(9b)  1st para -- improvement #2 (missing word)

                [...]  When a response is received, the Dynamic
   Authorization Client MUST NOT send further Status-Server packets to
|  the CoA port of any Dynamic Authorization until it next reboots.
---                                         ^
                [...]  When a response is received, the Dynamic
   Authorization Client MUST NOT send further Status-Server packets to
|  the CoA port of any Dynamic Authorization Server until it next
   reboots.
                                            ^^^^^^^^
(9c)  4th para -- inconsistency with 1st para

The final part of the 4th para apparently contradicts the last
sentence of the first para (quoted in (9b) above):

|                                              [...].  The NAS MAY
|  otherwise decide to receive multiple packets to it's CoA port before
|  marking the RADIUS server as responsive.  This behavior is
|  implementation-defined, and SHOULD be configurable.

(9d)  last para -- punctuation

The last comma (after "port 3799") IMO distorts the sentence
and should better be deleted.


(10)  Section 4.4, 2nd para -- improvement

I suggest to slightly change the wording:

        [...].  If the server is still unresponsive, however, the result
   may be user login failures.  The Status-Server solution is an ideal
|  one to solve this problem.
---^^^
        [...].  If the server is still unresponsive, however, the result
   may be user login failures.  The Status-Server solution is an ideal
|  way to solve this problem.
   ^^^


(11)  Section 4.6

(11a)  4th para below first figure -- improvement/grammar

I suggest to change:

           [...].  If the implementation fails to respond, then the NAS
   cannot distinguish between the Proxy Server being down, or the next
|  server along along the proxy chain is unreachable.
--                                    ^^
           [...].  If the implementation fails to respond, then the NAS
   cannot distinguish between the Proxy Server being down, or the next
|  server along along the proxy chain being unreachable.
                                      ^^^^^

(11b)  5th para below first figure -- missing word

I suggest to insert "in" as follows:

   In the worst case, failures in routing for Realm A may affect users
|  Realm B.  [...]
---^
   In the worst case, failures in routing for Realm A may affect users
|  in Realm B.  [...]
   ^^^^

(11c)  denomination of entities

>From the second figure ...

                /-> Proxy Server P -----> Home Server P
               /                    \ /
            NAS                      X
               \                    / \
                \-> Proxy Server S -----> Home Server S

... it becomes apparent that the denominations of the entities are
not chosen carefully; in this case, 'P' and 'S' are both overloaded.

To disambiguate the whole stuff, here's my proposal:

Use 'P1' and 'P2' (in place of 'P' and 'S') for the two Proxy Servers
in both scenarios (figures and text); use 'H1' and 'H2'  for the Home
Servers in the second scenario.


(12)  Section 4.7 -- terminology

The draft uses the colloquial, but incorrect short term "MIB" whenever
in wants to indicate a "MIB Module".  Please see the BCP 111, RFC 4181
for the detailed rationale of the precise terminology.

Therefore,  please change     MIB[s]   -->   MIB module[s]
throughout the text.

(13)  Section 4.7.1

(13a)  need to update the metadata (front matter)

The 1st para clearly indicates that this draft
***  Updates RFC 4669 and RFC 4671  ***

To make it easier for developers and users to find this update,
please amend the draft front matter accordingly.

(13b)  last para

The draft text is confusingly complicated, yet inprecise.
The draft text could be misunderstood as recommending to
define alternate MIB modules where it better should recommend
 using extensions for standard MIB tables.

I suggest to modify this paragraph as follows:

   If a server supports Status-Server and the [RFC4669] or [RFC4671]
|  MIBs, then it SHOULD also support vendor-specific MIBs containing
|  similar information as the standard MIBs, but which are instead
   dedicated solely to tracking Status-Server requests and responses.
|  Any definition of the server MIBs for Status-Server is outside of the
   scope of this document.
---
|  If a server supports Status-Server and the [RFC4669] or [RFC4671] MIB
|  modules, then it SHOULD also support vendor-specific MIB extensions
   ^^^^^^^                                              ^^^^^^^^^^^^^^^^
   dedicated solely to tracking Status-Server requests and responses.
|  Any definition of the server MIB module extensions for Status-Server
                                ^^^^^^^^^^^^^^^^^^^^^
   is outside of the scope of this document.

(Please note the deletion of the full 3rd line!)


(14)   Section 4.7.2

(14a)
Similar as noted in (13a) above, the first para of 4.7.2 clearly
indicates that this specification  *** updates RFCs 4668 and 4670 ***.

(14b)
Similarly as in (13b) above, the last para of 4.7.2 should be modified
as follows:

   If an implementation supports Status-Server and the [RFC4668] or
|  [RFC4670] MIBs, then it SHOULD also support vendor-specific MIBs
|  containing similar information as those MIBs, but which are instead
   dedicated solely to tracking Status-Server requests and responses.
|  Any definition of the client MIBs for Status-Server is outside of the
   scope of this document.
---
   If an implementation supports Status-Server and the [RFC4668] or
|  [RFC4670] MIB modules, then it SHOULD also support vendor-specific
             ^^^^^^^^^^^
|  MIB extensions dedicated solely to tracking Status-Server requests
   ^^^^^^^^^^^^^^^
|  and responses.  Any definition of the client MIB module extensions
                                                ^^^^^^^^^^^^^^^^^^^^^
   for Status-Server is outside of the scope of this document.


(15)  Section 5.1

(15a)  2nd para, last sentence -- typo/grammar

Please   s/server/servers/  in ...

                     [...].  These queries are most useful in
|  deployments where an administrator has internal RADIUS servers that
                                                                ^
   proxy to other internal RADIUS servers, such as for load balancing or
   fail over.

(15b)  3rd para -- improvement

To improve the readability, I suggest varying the multiple uses of
"use", e.g.:
      vvvv            vvvv                vvvvv
   If used, the names used for these test users SHOULD be difficult to
   guess by an attacker.  [...]
---                   vvvvvvvv
   If used, the names utilized for these test users SHOULD be difficult
   to guess by an attacker.  [...]


(16)  Section 5.2  -- outdated text

The first paragraph seems to be outdated now by the RADIUS-over-TCP
draft!


(17)  Section 5.3, 1st para

The draft says:
                                                  [...].  It may be
   tempting to increase the utility of Status-Server by having the
|  responses carry additional information, implementors are warned that
   such uses have not been analyzed for potential security issues or
   network problems.

To better reflect the relationship between the two (half-)sentences
connected with a comma, I propose to either use a semicolon instead
of the comma, or insert  " but" after the comma:

                                                  [...].  It may be
   tempting to increase the utility of Status-Server by having the
|  responses carry additional information, but implementors are warned
   that such uses have not been analyzed for potential security issues
   or network problems.


Best 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                     |
+------------------------+--------------------------------------------+