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