Re: [OAUTH-WG] draft-ietf-oauth-rar-08 review

[Torsten Lodderstedt <[e-mail]>]

Hi Hannes, 

> Am 21.12.2021 um 13:06 schrieb Hannes Tschofenig <[e-mail]>:
> 
> Hi all, 
>  
> thanks for writing this document. I have read through it as part of my shepherd writeup and here are a few comments and questions. 

thank you very much for your thorough review. We have tried to incorporate your comments and suggestions into revision -09 I just published. 

https://www.ietf.org/archive/id/draft-ietf-oauth-rar-09.html

>  
> Generic Comments:
>  
> As a style issue, it would be good to treat code segments as figures with a figure headings so that references in the text is easier to manage.

Added figure headings to all examples.

>  
> Editorial: s/This draft/This specification

changed 

>  
> Normative language: If you search for MUSTs in the document you will notice two things: First, they are not necessarily where you would expect them.
>  
> For example: This is from page 6, which explains an example.
>  
> "
>    The JSON objects with type fields of account_information and
>    payment_initiation represent the different authorization data to be
>    used by the AS to ask for consent and MUST subsequently also be made
>    available to the respective resource servers. 
> "  

Very good catch. The last part of the sentence is intended as a note and not normative language whereas the following sentence (not shown) represents a normative requirement. I moved the last sentence up to the beginning of this section and turned the „MUST subsequently also be made
   available to the respective resource servers.“ into a note. 

"Note: The AS will make this data subsequently available to the respective resource servers (see Section 9)."

>  
> Another example from page 29: Here are two approaches offered to mitigate a specific threat. If there are two ways then a MUST is appropriate IMHO. If there are others, maybe it is useful to tell the reader that there are other approaches and when to use those mitigations so that a developer can make an informed decision.
>  
> "
> In order to ensure their integrity, the
>    client SHOULD send authorization details in a signed request object
>    as defined in [I-D.ietf-oauth-jwsreq] or use the request_uri
>    authorization request parameter as defined in [I-D.ietf-oauth-jwsreq]
>    in conjunction with [I-D.ietf-oauth-par] to pass the URI of the
>    request object to the authorization server.
> "   

The intention of the SHOULD is to recommend clients to ensure the request integrity but leave application the room to decide to not care (which is inline with OAuth 20 and 2.1).

I suggest the following change

"If integrity of the 
authorization details is a concern, clients MUST protect authorization details against tampering and swapping. This can be achieved by signing the request using signed request objects as defined in [@I-D.ietf-oauth-jwsreq] or using the `request_uri` authorization request parameter as defined in [@I-D.ietf-oauth-jwsreq] in conjunction with [@I-D.ietf-oauth-par] to pass the URI of the request object to the authorization server."

>  
> Second, several MUST/SHOULD/MAYs are used with little guidance for the developer. Help developers to make the best decision.
>  
> For example: 
> "
>    Implementers MUST design and use authorization details in a privacy-
>    preserving manner.

The explanation is given in the following paragraphs. We could also remove the whole sentence since it states the obvious (in a privacy considerations section). 

>    
>    The AS MUST take into consideration the privacy implications when
>    sharing authorization details with the client or resource servers.

We give the advice to share on a „need to know“ basis. I don’t know what else we can state. Any ideas?


> "
>  
> In the rest of the review I will go through the individual sections and share my impressions.
>  
>  
> Abstract
>  
> The abstract says: 
>  
> "
>    This document specifies a new parameter authorization_details that is
>    used to carry fine-grained authorization data in the OAuth
>    authorization request.
> "
>  
> Later in the draft we will learn that the authorization_details parameter is not just carried in the authorization request but also in the token request.
>  
> Maybe you could say:
>  
> "
>    This document specifies a new parameter authorization_details that is
>    used to carry fine-grained authorization data in OAuth
>    messages.
> "

Good catch. That was a relict from the time where authorization details were sent in the author request only. Adopted text you proposed.

>  
> Section 1: Introduction
>  
> References to blog posts about the motivation for why this work is needed might be better replaced with a short summary, particularly if they motivate the solution in the document. If you don't want such a summary in the body of the document, then the appendix might also be a good place. 

The short summary is given in the introduction. 

The reference to the blog post is given "For a comprehensive discussion of the challenges arising from new use cases in the open banking and electronic signing spaces“.

Are you suggesting to instead include the blog post text (13 pages) as appendix? 

>  
> The last paragraph in the introduction covers one solution aspect. Is there a specific reason for putting a spotlight on it (compared to other solution aspects)? I think it comes too early in the document.
>  
> I missed one aspect in the introduction that might be relevant for the reader, namely that this specification itself is not sufficient for interoperability.  Developers and those deploying RAR need to make various agreements about the JSON structure carried inside the authorization_details parameter since the spec does not do much more than to say that you must use a JSON-based structure in this newly defined parameter. The document is more a guide on how to specify this JSON-based structure rather than mandating a specific structure. I think the introduction would be a good place to set the tone for the reader about what to expect in the rest of the document.

Moved the text to Section 2.1

>  
> Section 2: Request parameter "authorization_details"
>  
> At the start of the section I would have expected the structure of the payload being mandated. Something like "An implementation that claims conformance with this specification MUST use the following data model for the JSON structure carried inside the authorization_details parameter." I understand that you want to keep everything open for the developer.

That is correct. We define the rails for type safe authorization details. It is up to application / deployments to define the concrete authorization details types. 

>  
> Here are examples: 
>  
> - "The array [of the JSON structure] MAY contain several elements of the same type."
> - type: "This field MAY define which other elements are allowed in the request. This element is REQUIRED."
> (Is this field required to be understood by parsers or is it required to include this field in any JSON structure included in the authorization_details parameter? Interestingly, the content of the type element is not mandated either.)
> - None of the other fields seem to be required to be included nor implemented.
>  
> [Note: Throughout the spec you have used different ways to describe the "fields" (type, datatype, ) in the JSON structure. I think it would be useful to use the same term throughout the document to avoid confusion.]

Well that happens if a spec evolves over time ;-)
I refactored the section to better differentiate between authorization details, the authorization details types and common fields. I also consistently changed to text to use „fields“ for fields.  

>  
> In Section 2 you include Section 2.2 "Authorization Data Types", which is interesting since there are two elements defined: data type and type. While the subject heading of Section 2.2 gives the impression that you would be describing the data type you are actually describing the type element. IMHO the reason for this confusion is that you use the term "authorization data" to refer to authorization_details. Maybe you could use authorization_details throughout the document when you refer to the entire object.


done

>  
> Section 3: Authorization Request
>  
> You might want to give some reason to API designers about why they should be using the "scope" parameter together with the authorization_details parameter since it introduces extra complexity.

Added „Note: Combined use of `authorization_details` and `scope` is supported by this specification to allow existing OAuth-based applications to incrementally migration towards `authorization_details`. It is RECOMMENDED that a given API uses only one form of requirement specification."

The latter sentence already existed but I thought it would make sense to incorporate it. 

>  
> Section 6: Token Request
>  
> This section mostly consists of the text in Section 6.1 "Comparing authorization details", which essentially acknowledges that comparing any two arbitrary authorization details requests is difficult. Then, you give an example of how this could work. The use of RFC 2119 language is IMHO for the example not appropriate.
>  
> For example: The "MAY" in the paragraph below has to be replaced by a "may".
>  
> "
>    For our running example, the client MAY ask for all permissions of
>    the approved grant of type payment_iniation applicable to the
>    resource server residing at https://example.com/payments <https://example.com/payments>
> "

Changed to lowercased may as suggested.

>   
> Regarding the use of the MAY in this paragraph I am not sure whether it is related to the example or a generic consideration. If it is generic, maybe you want to move it into Section 2 (as a description of the element).
>  
> "
>    The predefined authorization data element locations MAY be used by
>    the client to request an access token valid for a certain resource
>    server, i.e. it is the recommended way to request issuance of
>    audience restricted access tokens.
> "  

Changed wording and added reference to section 2.2 (which now has a more detailed definition of „locations“)
>  
> Section 10.  Metadata
>  
> In this section I have expected to see an example, as present in all other sections. For some reasons you decided not to include one.

Good point, I added examples. 
>  
> Section 11.  Scope value "openid" and "claims" parameter
>  
> This section feels a bit out of place. It almost sounds like a design idea for use in the OpenID foundation.

The „claims“ Parameter is defined in OpenID Connect. We try to convey that „claims“ can still be used. On the other hand, we would also like to get across that „claims“ could be transformed into an authorization details type in the future (that’s the design idea).

I merged Section 11 into the exiting addendum for OpenID Connect. 

>  
> Section 12.  Implementation Considerations
>  
> This section is good.

Thank you :-)

> Just a minor remark: In this paragraph the SHOULD appears to be inappropriate. I think you just want to make the reader aware of existing specifications to deal with concerns regarding large request sizes.
>  
>    Implementers SHOULD therefore consider using the request_uri
>    parameter as defined in [I-D.ietf-oauth-jwsreq] in combination with
>    the pushed request object mechanism as defined in
>    [I-D.ietf-oauth-par] to pass authorization details in a reliable and
>    secure manner.

Lowercased the should
 
>    
> Section 13.  Security Considerations
>  
> This paragraph is a repetition of text in Section 2.1:
>  
>    All strings MUST be compared using the exact byte representation of
>    the characters as defined by [RFC8259].  This is especially true for
>    the type field, which dictates which other fields and functions are
>    allowed in the request.  The server MUST NOT perform any form of
>    collation, transformation, or equivalence on the string values.

It is more generic than the text in section 2.

>    
> 14.  Privacy Considerations
>  
> In this paragraph (reformatted) you are turning some of the informative references into normative:
>   
>    Any sensitive personal data included in authorization details MUST be
>    prevented from leaking, e.g., through referrer headers.
>    Implementation options include 
>    
>    1) encrypted request objects as defined in [I-D.ietf-oauth-jwsreq],
>    2) transmission of authorization details via end-to-end encrypted connections between client and authorization
>    server by utilizing [I-D.ietf-oauth-par], and
>    3) the request_uri authorization request parameter as defined in
>    [I-D.ietf-oauth-jwsreq].

Why does the text turn this references into normative references? The specification requires the application to protect the data but it does not require any of the referenced mechanisms in particular. If the application decides to use a proprietary way that would fulfil the MUST as well. 

>  
> IMHO [I-D.ietf-oauth-jwsreq] needs to become a normative reference. [I-D.ietf-oauth-jwsreq] has been published as RFC 9101.

Yep, changed references to JAR and PAR to references to the actual RFCs.

> When you mandate the use of encryption (either through [I-D.ietf-oauth-jwsreq] or by [I-D.ietf-oauth-par]) then some guidance on when to use what mechanism would be good and how credential types influence the deployment.

Does this work? 

„The later does not require application level encryption but it requires another message exchange between client and AS. „

>  
> The following paragraph makes me wonder how developers make design decisions since they have
> to balance the privacy implications and the desire to make fine-grained authorization decisions:
>  
>    The AS MUST take into consideration the privacy implications when
>    sharing authorization details with the client or resource servers.
>    The AS SHOULD share this data with those parties on a "need to know"
>    basis.
>    
> Is there any guidance you could give to developers to help them make a good decision?

I honestly don’t know what advise to give beyond „need to know“. In the end the AS could share any detail with the client and/or RS as long as it ensures the legal basis. But work along the „need to know“ basis is I think, a reasonable advise. 

best regards,
Torsten. 

>  
> Ciao
> Hannes
>  
> IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you. _______________________________________________
> OAuth mailing list
> [e-mail] <mailto:[e-mail]>
> https://www.ietf.org/mailman/listinfo/oauth <https://www.ietf.org/mailman/listinfo/oauth>