Thanks Christer for your thorough review! A new draft ( https://www.ietf.org/archive/id/draft-ietf-oauth-step-up-authn-challenge-12.html) reflecting changes resulting from the feedback has been published. Comments inline
On Thu, Feb 23, 2023 at 9:13 AM Christer Holmberg via Datatracker < nore...@ietf.org> wrote: > > This message originated outside your organization. > > > Reviewer: Christer Holmberg > Review result: Ready with Issues > > I am the assigned Gen-ART reviewer for this draft. The General Area > Review Team (Gen-ART) reviews all IETF documents being processed > by the IESG for the IETF Chair. Please treat these comments just > like any other last call comments. > > For more information, please see the FAQ at > > <https://trac.ietf.org/trac/gen/wiki/GenArtfaq>. > > Document: draft-ietf-oauth-step-up-authn-challenge-11 > Reviewer: Christer Holmberg > Review Date: 2023-02-23 > IETF LC End Date: 2023-03-03 > IESG Telechat date: Not scheduled for a telechat > > Summary: The document is well written, and easy to read. However, I have > found > some issues that I would like the authors to address. > > Major issues: > > QMa1: General > > As the document defines a new error code, and define new > WWW-Authenticate > parameters, should the document not be an Update to RFC 6750? > > It's a good point to consider. Our rationale is that this document leverages many different extension points baked in a number of existing specs, hence it doesn't seem a slam dunk to determine which one we should "inherit" from. > ---- > > QMa2: Section 4 > > The text defines the procedures for the client. > > But, what if the client does not support the new error code and the new > WWW-A parameters? I think there should be some backward compatibility > text > (or reference, if defined elsewhere). > > Especially it should be clear that the server will not receive the > WWW-A > parameters in the new request if the client does not support them. > > This is a tricky one. Technically, every extension starts its existence with zero support from existing clients. Implementers should be familiar with this, and specs stipulate that any entity receiving a parameter it doesn't understand to ignore that parameter, from which it follows that whatever semantic or directive that parameter carries will remain not executed. Saying in the document that clients not supporting the parameters we are defining here will not achieve the goals of the spec seem somewhat redundant. What do you think? > ---- > > Minor issues: > > QMi1: Section 3 > > Can the new WWW-Authenticate parameters only be used with the new error > code? If so, please indicate it. > > There doesn't seem to be any obvious reasons to ban future extensions from using those parameters, nor there appear to be obvious scenarios where we would proactively suggest doing so: that's our rationale for not having commented on this aspect in the document. > --- > > QMi2: Section 3 > > Is the max_age value required to be given as a string value (as in the > example)? If so, please indicate it. > > Good question. It doesn't have to be a string value. It can be a token or quoted-string. We will clarify. --- > > Nits/editorial comments: > > QNi1: General > > Throughout the document uses "doesn't", "isn't" and "it's". I suggest > replacing those with "does not", "is not" and "it is". > > Alright. That will definitely make the document more formal sounding. ---- > > QNi2: Abstract > > The text starts by talking about resource servers, requests etc. > Eventhough > the document title mentions OAuth 2.0, I think it would also be good to > mention it in the beginning of the Abstract. > > E.g., > > "When OAuth 2.0 is used, it is not uncommon for..." > > The scenario we describe is a generic occurrence for any API surfacing resources requiring different access levels. We chose OAuth as the framework within which we specify a method to address the scenario, and there will be no doubt in the mind of the reader about that (the title and the concrete guidance take care of it), however the scenario remains generic hence restricting it to OAuth in the description at the abstract wouldn't necessarily add to that. > ---- > > QNi3: Introduction > > Similar to the Abstract, I think it would be good to mention OAuth 2.0 > in > the beginning. > > See above for a clarification on the pattern operating at the generic API level, regardless of implementation details, and guidance on how to implement it (OAuth specific). > Also, I am not sure what "API authorization scenario" means. > Could you say "OAuth 2.0 authorization scenario"? > > The need for API to implement authorization logic is independent on the specific protocols the API implementers decided to use. What makes this OAuth specific is the fact that we use OAuth framework constructs to get the job done, but the scenario remains a high level pattern that makes sense regardless of the tech. While still describing the scenario at high level, adding OAuth to the description would narrow the scope of the scenario more than it is warranted, given that when the rubber hits the road (actual protocol guidance) the relationship to OAuth is unmistakable. . > ---- > > QNi4: Introduction > > The text says: "An API might also determine" > > Should it say "authorization server" instead of "API"? > > The API behavior is what we really intend to describe there - the fact that it is the API (and not the AS) to make determinations based on authentication properties is the novel mechanism we want to point the reader's attention to.
_______________________________________________ Gen-art mailing list Gen-art@ietf.org https://www.ietf.org/mailman/listinfo/gen-art
