Re: [Last-Call] [Gen-art] Genart last call review of draft-ietf-oauth-rar-15

[Robert Sparks <[e-mail]>]

On 11/17/22 4:44 PM, Robert Sparks wrote:
> Apologies - an upload fumble left -00 empty. I've revised it to have 
> content at -01.

Which I'll also paste here for convenience:

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-rar-14 (<- that was a typo - I really did 
review 15)
Reviewer: Robert Sparks
Review Date: 2022-11-17
IETF LC End Date: 2022-11-17
IESG Telechat date: Not scheduled for a telechat

Summary:

Major issues: Essentially ready for publication as a Proposed Standard 
RFC, but with issues to discuss before publication

I have two major issues that I think need discussion:

Major Issue 1) The document seems to be specifying a new way of 
comparing json names, claiming it is what RFC8259 requires, but I 
disagree that RFC8259 says what this document is claiming. If I'm 
correct, the document is trying to rely on the text in section 7 of 
RFC8259 to support the idea that implementation MUST NOT alter the json 
names (such as applying Unicode normalization) before comparison and 
that the comparison MUST be performed octet-by-octet. Rather, section 7 
says something more like "you better escape and unescape stuff correctly 
if you’re going to do unicode codepoint by codepoint comparison" which 
is a completely different statement.

If I'm right, and this is a new comparison rule that goes beyond what 
JSON itself defines, I think the group should seek extra guidance from 
Unicode experts. If I'm wrong and this behavior is defined somewhere 
else, please provide a better pointer to the definition.

In many environments, its unusual for an implementation relying on a 
stack below it to have any say at all on whether normalization is going 
to be applied to the unicode before the application gets to look. Rather 
than trying to work around the problem you've identified with 
normalization by specifying the comparison algorithm, consider just 
making stronger statements about the strings used in the json names the 
document defines. Why _can't_ you restrict the authorization_details 
values to ascii? If it's because you want to present the string to a 
user, consider putting a presentation string elsewhere in the json that 
is not used for comparison.

Major Issue 2) The suggested pattern demonstrated starting in figure 16 
(using [] to mean "let the user choose") seems underspecified. If the 
point is that different APIs may invent different mechanics _like_ this, 
and that this is only an example. Make it much clearer that this is an 
example. If this is a pattern you expect all APIs to follow, then more 
description is warranted. Is it intended that a user could add and 
remove things arbitrarily to such lists? For instance is it intended 
that this support an interaction where the client is asking for 
permission to operate on account A, and the user can say "no, but you 
can operate on account B"?

Minor issues:

Section 2: What does "The AS MUST ensure that there is no collision 
between different authorization details types that it supports." mean? 
How is this a 2119 requirement? I _think_ you're trying to point out 
that the authorization_details string is a unique-key at any AS and that 
that has consequences on the API _designer_, but I don't know what is 
expected of an AS coder here. Some clearer words are needed.

Section 7: Please expand on, or rephrase, "if these are deemed of no 
intended use for the client." Could you just delete the phrase? If you 
are only explaining why an AS might do this, make it clear that it's an 
example (and split the example into a separate sentence). If this is the 
_only_ reason an AS might omit values in the token Response, then more 
detail is needed.


Nits/editorial comments:

Throughout the document, there is a mix of "authorization_details" and 
"authorization details". It is not completely consistent using one when 
talking about the parameter and the other when talking about the general 
concept of details about authorization. Please inspect each occurrence 
and verify that it is using the correct form.

Introduction: suggest changing 'defines the parameter scope' to 'defines 
the parameter "scope"'

Introduction just after figure 1 - expand AS (first use).

Section 2.2: Please rework the sentence where you mention "the cartesian 
product" and remove the reference - it is not helpful. Instead of "That 
is to say," just say it.

Section 6.1, discussion just after Figure 13. Consider rewriting the 
sentence to avoid the term "non-subsuming". Think about translation into 
other languages.

Section 9.2 intro to Figure 21: something is missing from this sentence. 
The RS: at the end is either not needed or needs more words?

Section 10: Please reconsider the first sentence. Write it in a way that 
doesn't imply the AS has agency (the AS can't "want"), and avoid the 
current "If...then MUST" construction. It would be better to say 
something like "To signal (foo) the AS (does these things)".

Section 10 just before Figure 23: Please clarify what it means for a 
client to announce types that they "use"? Do you mean "support"? Do you 
mean "intend to use in this interaction"? Or something else? Consider 
saying more about why allowing the client to announce these things is 
useful.

Appendix A.1 : expand ACR

Figure 30 caption : typo at eHaelth.


>
> RjS
>
> On 11/17/22 4:36 PM, Robert Sparks via Datatracker wrote:
>> Reviewer: Robert Sparks
>> 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-rar-??
>> Reviewer: Robert Sparks
>> Review Date: 2022-11-17
>> IETF LC End Date: 2022-11-17
>> IESG Telechat date: Not scheduled for a telechat
>>
>> Summary:
>>
>> Major issues:
>>
>> Minor issues:
>>
>> Nits/editorial comments:
>>
>>
>>
>
> _______________________________________________
> Gen-art mailing list
> [e-mail]
> https://www.ietf.org/mailman/listinfo/gen-art