Last Call Review of draft-ietf-radext-radiusv11-07
review-ietf-radext-radiusv11-07-artart-lc-allocchio-2024-06-25-00
Request | Review of | draft-ietf-radext-radiusv11 |
---|---|---|
Requested revision | No specific revision (document currently at 11) | |
Type | Last Call Review | |
Team | ART Area Review Team (artart) | |
Deadline | 2024-06-26 | |
Requested | 2024-06-12 | |
Authors | Alan DeKok | |
I-D last updated | 2024-06-25 | |
Completed reviews |
Genart Last Call review of -08
by Christer Holmberg
(diff)
Artart Last Call review of -07 by Claudio Allocchio (diff) Secdir Last Call review of -08 by Barry Leiba (diff) Opsdir Last Call review of -10 by Susan Hares (diff) Secdir Telechat review of -10 by Barry Leiba (diff) |
|
Assignment | Reviewer | Claudio Allocchio |
State | Completed | |
Request | Last Call review on draft-ietf-radext-radiusv11 by ART Area Review Team Assigned | |
Posted at | https://mailarchive.ietf.org/arch/msg/art/T6s2PIOiL_d1QM6k-Z0wcDTyHrE | |
Reviewed revision | 07 (document currently at 11) | |
Result | Ready w/nits | |
Completed | 2024-06-25 |
review-ietf-radext-radiusv11-07-artart-lc-allocchio-2024-06-25-00
Hello, Here is my list of observations on this document. nits: in the 1. Introduction section you repeat a large number of times that this experimental specification requires only a small set of modifications to existing implementations, and then finally you list the only 3 needed. I may suggest just stating this "need for small set of modification" just once, where you list them. You also repeat a number of times that MD5 use is deprecated, and shared secret should not be used anymore: same comment as above... just state it once.In the Introduction this sort of "excessive over-statement of concepts" happens also elsewhere. As this is a technical specifications, I suggest avoiding these repetitions, "as if you need to strongly defend the idea" that it is not going to make any harm. nits: sections 3.2 Do we really need this digest of RFC7303 here? it makes no harm, but it seems a bit odd this "support for readers not familiar" in a specification. issue (maybe?): section 3.3. here you make it possible for an implementation not to be backward compatible and just support radius/1.1 (even if not recommended). This is against the principle "be liberal in what you accept", and could create non interoperable situations. Are we sure we really want this to happen? how can we be sure that "all (or nearly all) RADIUS clients have been updated to support RADIUS/1.1." nits: section 3.5 again the text re-states the concept a number of time. Maybe it is just a matter of style, which pervdes the whole specification. suggestion: 4.2.1 the proposed method to generate an initial Token value has of course a 1/(2^32) possibility to generate the same initial value. This has no effect of course (besides the rare chance that a debugging ends into 2 overlapping series f tokens. Why not also add this to the explanation? this is already partially there in the explanation, tough... so just a suggestion. Session 6 nits: nit: session 6.1 describes and tries to give a fix to a known problem which is not specific to the new extension, but general. Shall we make it more explicit somewhere n the text that here we are trying to retro-fit a general issue? nit: also 6.2 gives recommendations to update historic implementations, which is fine, but maybe "hidden", "not visible" in a specification like this which in the end is a "profile" (and experimental for now). 6.5 does the same for another issue. Suggestion: make evident the attempt of all section 6 to fix existing issues, besides describing them here, or implementers may risk to ignore what's written in section 6. For all the rest, the document is ok and ready to go forward. all the best!