Re: [OAUTH-WG] draft-ietf-oauth-rar-08 review
Hi Hannes, > Am 21.12.2021 um 13:06 schrieb Hannes Tschofenig : > > 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
[OAUTH-WG] draft-ietf-oauth-rar-08 review
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. 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. Editorial: s/This draft/This specification 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. " 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. " 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 AS MUST take into consideration the privacy implications when sharing authorization details with the client or resource servers. " 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. " 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 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. 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. 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.] In Section 2 you include Section 2.2 "Authorization Data Types", which is interesting since there are two
