Hi Russ,
thank you for your feedback on the OAuth Security BCP draft! I incorporated most of the proposed changes into the latest version (-26).
Some comments below:
Reviewer: Russ Housley Review result: Almost Ready I am the assigned ART-ART reviewer for this draft. Please treat these comments just like any other last call comments. Document: draft-ietf-oauth-security-topics-25 Reviewer: Russ Housley Review Date: 2024-02-18 IETF LC End Date: 2024-02-22 IESG Telechat date: Unknown Summary: Almost Ready Major Concerns: Section 4: Some of the subsections contain MUST, MUST NOT, SHOULD, and MAY statements. Given the structure of the document, I expected all of the words that are defined in RFC 2119 to appear is Section 2, with discussion of attacks in Section 4. Please consider lower case words.
I absolutely see where you are coming from, but Section 4 intentionally provides normative requirements on top of those in Section 2. Our intention is to provide a brief rundown of the most important Best Practices in Section 2. However, some requirements are only applicable in certain cases (e.g., protocol options not commonly used), some require a lengthy detailed explanation, and some require explaining the context of a certain attack in order to make sense. Those are listed in Section 4.
Removing all requirements from Section 4 would effectively mean merging Sections 2 and 4. Since Section 2 was introduced in reponse to demands for a summary of the most important measures, we would prefer not to do that.
We did check carefully that the requirements defined in Section 4 complement, but never contradict, those in Section 2.
I noticed, however, that the introductions to Sections 2 and 4 did not provide the full picture - I therefore changed the introduction to Section 2 to the following:
This section describes the core set of security mechanisms and measures the
OAuth working group considers best practices at the time of writing. Details
about these security mechanisms and measures (including detailed attack
descriptions) and requirements for less commonly used options are provided in
(#attacks_and_mitigations).
And the one of Section 4 to the following:
This section gives a detailed description of attacks on OAuth implementations,
along with potential countermeasures. Attacks and mitigations already covered in
[@!RFC6819] are not listed here, except where new recommendations are made.
This section further defines additional requirements beyond those defined in
Section (#recommendations) for certain cases and protocol options.
Done!Minor Concerns: Section 1: I recognize that the term "Anti-patterns" is gaining favor, but I still think that you should provide a brif introduction to the concept. Perhaps: Anti-patterns are patterns in software development that are considered bad programming practices, but they seem to happen over and over.
Good catch, thank you! I expanded the "If not" to "If it was not", making clear that it refers to this part of the previous sentence: "... whether (...) was meant to be used for that particular resource server."Section 2.3: It says: ... If not, the resource server must refuse to serve the respective request. ... The "If not" is ambiguous. Which aspects of the previous sentence does this cover? Part is implemented by the access server and part is implemented by the resource server. Can the resource server always determin whether the "the authorization server associates the access token with the respective resource and actions"? Please clarify.
Section 2.5: When is client authentication not possible? In other words, under what circumstances does an authorization server implementer ignore this SHOULD statement?
Thanks, changed to:
Authorization servers SHOULD enforce client authentication if
it is feasible, in the particular deployment, to establish a process for
issuance/registration of credentials for clients and ensuring the
confidentiality of those credentials.
Section 4.1.1: The text says: "First, the attacker ..." However, there is not "Second" or "Then" to go wit the "First". Please reword. Perhaps: s/First/To begin/
Changed as proposed.
Section 4.1.2: The text says: "First, the attacker ..." However, there is not "Second" or "Then" to go wit the "First". Please reword. Perhaps: s/First/To begin/
Changed as proposed.
Nits:
Abstract: Because the Abstract is not allowed to contain references:
s/[RFC6749], [RFC6750], and [RFC6819]/
/RFC 6749, RFC 6750, and RFC 6819/
fixed
Section 1: s/client talks to/client is talking to/
fixed
fixedSection 1: s/when feasible/as soon as feasible/
Changed as shown aboveSection 2.5: s/authentication if possible./authentication./
Section 3: s/attacker model/threat model/ (multiple places)
In most cases, the use of 'attacker model' is intentional and correct, as the document does not define a threat model (as RFC6819 did for OAuth) by listing individual threats, but describes an attacker model, i.e., broad capabilities of attackers from which threats can be derived.
This is in line with how the research that (partly) led to the
creation of the OAuth Security BCP (see https://www.sec.uni-stuttgart.de/research/wim/)
defines attackers. The main difference to a threat model is that
attackers are mostly protocol-agnostic capabilities, while threats
are usually specific to some protocol step, message, or
participant. Ideally, during a formal analysis, all existing
threats can be derived from the attacker model and the analysis
can ensure that no threat is overlooked.
Having said that, I also noticed that the introduction to Section
3 uses the words interchangeably, which is not correct. I changed
the paragraph to the following:
In [@RFC6819], a threat model is laid out that describes the threats against
which OAuth deployments must be protected. While doing so, [@RFC6819] makes
certain assumptions about attackers and their capabilities, i.e., implicitly
establishes an attacker model. In the following, this attacker model is made
explicit and is updated and expanded to account for the potentially dynamic
relationships involving multiple parties (as described in (#Introduction)), to
include new types of attackers and to define the attacker model more clearly.
fixedSection 3: s/(wifi)/(Wi-Fi)/
fixedSection 4.4.1: I do not think the bolding in the Variants bullets is helpful to the reader, especially in the plaintext document.
Section 4.10.1: I do not think the bolding at the send of the section is helpful to the reader, especially in the plaintext document.
fixed
Thanks again for your review,
-Daniel
-- last-call mailing list last-call@xxxxxxxx https://www.ietf.org/mailman/listinfo/last-call
