Re: [radext] Review of draft-ietf-radext-radius-v11-03

[Alan DeKok <[e-mail]>]

  I've put changes into GitHub for all of these  https://github.com/radext-wg/draft-ietf-radext-radiusv11/commits/master/

  Barring minor word smithing, all of these changes have been made.

> On Dec 6, 2023, at 4:15 PM, Jan-Frederik Rieckers <[e-mail]> wrote:
> 
> Hi to all,
> 
> 
> tl;dr for those who are afraid of the long mail:
> * current version has several inconsistencies and nits that need to be fixed before moving forward, but those can be fixed
> * Expertise needed regarding TLS, esp. using the ALPN "no_application_protocol" error without previous ALPN extension.
> (jump to "Content and text review") If you are a TLS expert, please read at least that paragraph and reach out.
> 
> 
> I was appointed as Document Shepherd for the "RADIUS ALPN and removing MD5" document.
> As part of the document shepherd write-up, I've reviewed the current version (-03) and found several issues that should be fixed before the document can move forward.
> 
> 
> Before I go into the nits, I want to say that the document is well-written and, after these issues are fixed I believe that the document is ready to move forward.
> 
> 
> Some of these nits I've put in issues in the github repo for the draft for better tracking.
> https://github.com/radext-wg/draft-ietf-radext-radiusv11/issues
> 
> I'll not list typos and grammar issues, those are collected here:
> https://github.com/radext-wg/draft-ietf-radext-radiusv11/issues/6
> 
> 
> 
> **Formal Criteria/Content Guidelines**
> 
> There are some things in the document that do not meet the content guidelines, especially the required content as listed here:
> https://authors.ietf.org/required-content
> 
> * Updated RFCs
> The document metadata says that it updates RFC6614 (RADIUS/TLS), RFC7360 (RADIUS/DTLS) and RFC7930 (RADIUS Large Packets).
> Although the abstract and the introduction state, why RFC6614 and RFC7360 are updated, I have not seen any description why and what exactly in RFC7930 is updated by this document.
> 
> On the other hand, Section 6.4 (Error-Cause Attribute) explicitly updates RFC5176 (Dynamic Authorization Extensions to RADIUS) by allowing the "Error-Cause" attribute to also appear in Access-Reject packets.
> 
> * Security Considerations
> 
> The document has only one sentence of security considerations.
> Although the document talks about security considerations within the sections, I expect that the review from other teams will highlight this fact.
> As a consumer of RFCs I find it quite helpful to have a dedicated section where I can learn about all the things that I need to take care of or look into, if I operate this protocol, since I may not really want to read the whole specification.
> So, coming from this, I would recommend to add text to the security considerations section, and maybe even move some paragraphs in other sections that talk about security considerations of this specific part to the general security considerations section
> 
> * Normative/Informative References
> 
> Currently RFC6614 and RFC7360 are listed as informative references, when they should be normative, since implementing RADIUS/1.1 requires knowledge of both specifications. Maybe even RFC6613 should be listed as normative reference, but I'm not exactly sure about that.
> 
> * IANA section
> 
> Currently, the IANA section lists "radius/1.0" and "radius/1.1" as both the Protocol and the Identification Sequence.
> I believe that, for clarity, the Protocol should be "RADIUS/1.0" and "RADIUS/1.1". The Identification Sequence should remain "radius/1.0" and "radius/1.1", to remain consistent with the current registry entries.
> 
> * Note to RFC Editor for Changelog
> 
> The Changelog Section does not have a "Note to RFC Editor: remove this section before publication". As the changelog is not of interest once the document is published, this should be added to avoid confusion whether or not this should be part of the final document.
> 
> 
> 
> **Content and text review**
> 
> 
> There is a general slight inconsistency about the usage of the "no_application_protocol" alert when no ALPN extension was present. I don't consider myself enough of an expert in TLS to determine whether or not sending this alert is (or should be) allowed in such cases, so I hope that some other people can step in here and help me out.
> 
> 
> * Section 1 Introduction
> 
> In the introduction, FIPS-140 is mentioned, but there is never a reference. I think that there should be a reference for interested readers. Otherwise we simply state facts, that cannot immediately be verified.
> 
> In the list of changes for the packet format, the following sentence could lead to misunderstandings:
> "The Identifier field is no longer used, and has been replaced by the token field"
> Here, the text should be adjusted to clearly state that only the functionality the identifier field offered is replaced by the token field, but the identifier field is still there, it is only ignored.
> https://github.com/radext-wg/draft-ietf-radext-radiusv11/issues/3
> 
> The introduction also has some text that probably should be adjusted to reflect the new "radius/1.0" "radius/1.1" ALPN setup.
> (Paragraph starts with "This specification is compatible with existing implementations ...")
> There are several other cases of this, I've tried to put them all in this issue:
> https://github.com/radext-wg/draft-ietf-radext-radiusv11/issues/4
> 
> * Section 2 Terminology
> 
> After the boilerplate there should be a sentence introducing the bullet points that follow.
> 
> The use of the term "historic RADIUS/TLS" as reference to both RADIUS/TLS (RFC6614) and RADIUS/DTLS (RFC7360), while "RADIUS/TLS" should only be seen as a reference to RADIUS/TLS as per RFC6614, where as "RADIUS over TLS" is again used to reference both is a bit confusing, esp. since the "historic" may not be seen as part of the term within sentences in the later sections.
> The abstract and introduction uses "RADIUS over (D)TLS" and "(D)TLS" (which is understandable, since the terminology has not been introduced yet), but also "(D)TLS" is used in section 5.
> Unfortunately, I have no patent solution to the terminology here, but I would suggest to look at this again if we can use terms that are less likely to generate confusion.
> Possibly we could even introduce and use "RADIUS/1.0" as term for what is currently listed as "historic RADIUS/TLS".
> 
> * Section 3.3 Configuration of ALPN for RADIUS/1.1
> 
> The first mention of the "no_application_protocol" alert is in the description for the configuration setting "1.0".
> This should contain a reference to RFC7301, Section 3.2 at this point. Currently, the next (and only) reference to this specific section is later in the paragraphs after describing the behavior for the different configuration options.
> 
> The description for the configuration item "1.0, 1.1" contains a specification gap. There is no explicit mention how the server should behave if the client sends both ALPN names. Currently the spec only says (abbreviated) "If radius/1.0 is present, send radius/1.0 back. If radius/1.1 is present, send radius/1.1 back."
> There should be a paragraph here that clearly states that "radius/1.1" takes precedence.
> 
> The description for the configuration item "1.1" contains a possible missing spec for the server behavior.
> The server behavior states that the server "MAY reply with a TLS alert (...) and then close the TLS connection".
> This is probably also a dangling item from introducing "radius/1.0" as ALPN name, and it should state that the server MAY send a TLS alert, but MUST close the connection.
> 
> 
> The paragraphs after the description of the configuration options require references to "allow" and "require", which was the old terminology. This needs to be adjusted.
> 
> There are two paragraphs in this section that both talk about the use of the "no_application_protocol" alert, but have different content.
> (First starts with "While [RFC7301] does not discuss", second starts with "Unfortunately, when ALPN negotiation fails")
> Those paragraphs talk about the same thing, but come to different conclusions, one allows the server to send the error, even if no ALPN extension was received, the other one talks about how this is not possible. (Again, I'm not enough of an expert on TLS to know which one is true, if someone has more expertise, please step in.)
> https://github.com/radext-wg/draft-ietf-radext-radiusv11/issues/8
> 
> If the first paragraph remains, then there is a sentence that I tripped on: "The server MAY send this alert during the ClientHello" which seems a bit odd. I think it should say "The server MAY send this alert immediately after receiving the ClientHello"
> 
> * Section 3.5 Session Resumption
> 
> This section also references the outdated "require"/"allow" configuration options.
> 
> * Section 6.1 Status Server
> 
> The last paragraph contains a (at least a little bit) cynical comment on issues if the connection is clogged up, that may be hard to understand for people that do not operate a RADIUS server (or have never thought about the possibility that RADIUS requests may be left unanswered). The structure of the sentence is also not perfect.
> In the interest of readability I would suggest rephrasing the last few sentences, to objectively describe the issue and the fix, instead of relying on anecdotes.
> 
> I have added a suggestion for a new wording in the github issue.
> https://github.com/radext-wg/draft-ietf-radext-radiusv11/issues/9
> 
> 
> 
> 
> Cheers,
> 
> Janfred
> 
> -- 
> Herr Jan-Frederik Rieckers
> Security, Trust & Identity Services
> 
> E-Mail: [e-mail] | Fon: [phone] | Fax: [phone]
> Pronomen: er/sein | Pronouns: he/him
> __________________________________________________________________________________
> 
> DFN - Deutsches Forschungsnetz | German National Research and Education Network
> Verein zur Förderung eines Deutschen Forschungsnetzes e.V.
> Alexanderplatz 1 | 10178 Berlin
> www.dfn.de
> 
> Vorstand: Prof. Dr. Odej Kao (Vorsitzender) | Dr. Rainer Bockholt | Christian Zens
> Geschäftsführung: Dr. Christian Grimm | Jochem Pattloch
> VR AG Charlottenburg 7729B | USt.-ID. DE 1366/23822
> _______________________________________________
> radext mailing list
> [e-mail]
> https://www.ietf.org/mailman/listinfo/radext