Thanks for the review Roman and thanks Justin for the responses. I took the liberty of going ahead and making some of the more straightforward changes suggested by the review in the document source https://github.com/oauthstuff/draft-oauth-rar/commit/575f21bf6369c609f673062ec2083215c797b4c2 so as to hopefully free up this thread to focus on the things that need more discussion.
On Thu, Sep 15, 2022 at 9:20 AM Justin Richer <jric...@mit.edu> wrote: > Hi Roman, some responses inline. > > > On Sep 14, 2022, at 6:30 PM, Roman Danyliw <r...@cert.org> wrote: > > > > Hi! > > > > I performed an AD review of draft-ietf-oauth-rar-12. Thanks for this > document. My feedback is as follows: > > > > ** Section 2. Editorial > > > > This field MUST be compared using an exact byte match of the string > > value against known types by the AS. > > > > Consider if you want to introduce how the lack of match will be handled > here - it is covered later. > > > > ** Section 2.2. > > > > All data fields are OPTIONAL for use by a given API > > definition. > > > > I don't follow how this is true in the general case. I was under the > impression that this section defined what is expected to be common fields. > Couldn't some AS with a particular type require their presence? > > Yes — a particular “type” value can require any of these fields, but a > type is itself not *required* to use any of these fields. That’s what we > were trying to convey with the OPTIONAL here. Is there a better way to > communicate this? Maybe we should just leave off the “OPTIONAL” flag here, > but we wanted people to realize that the common elements don’t have to be > used by each “type”, while every request needs to have a “type”. > > > > > ** Section 3. Editorial > > > > OLD > > In case of authorization requests as defined in [RFC6749], > > implementors MAY consider to use > > > > NEW > > In case of authorization requests as defined in [RFC6749], > > implementors MAY consider using ... > > > > ** Section 3. Typo. s/intiate/initiate/ > > > > ** Section 5. Typo. > > > > OLD > > authorization details type > > or authorization details > > > > NEW > > authorization_details type > > or authorization_details > > > > ** Section 6.1 > > > > However, when comparing a new request to an existing request, > > authorization servers can use the same processing techniques as used > > in granting the request in the first place to determine if a resource > > owner needs to authorize the request. > > > > Why is it possible to assess two arbitrary requests in this case to > determine "if a resource owner needs to authorize the request", but the > prior paragraph explicitly calls out that comparing two arbitrary requests > is not feasible? To me is seems like comparing two requests to understand > if more or less permissions are being requested is equivalent to > determining if a new request exceed the current request to determine if > going back to the resource owner is needed. > > > > It might be possible to do such a comparison in a specific case, but we > can’t add logic in the general case. In OAuth, scopes are supposed to be > purely additive, so if you have another scope it’s for “more” things. We > know in practice that that’s not always how it works. Things get much more > complex with RAR because you could have an object with :more: fields in it > that makes things more :strict: by the presence of those fields. That’s all > going to be up to the “type” definition though, so if you understand the > “type” definition you could do a comparison based on that. To me the text > is clear, can you suggest how we could clarify this? > > > ** Section 6.1. Typo. s/isaunce/issuance/ > > > > ** Section 7. > > > > If the client does not specify > > the authorization_details token request parameters, the AS determines > > the resulting authorization details at its discretion. The > > authorization server MAY consider the values of other parameters such > > as resource and scope if they are present during this processing, and > > the details of such considerations are outside the scope of this > > specification. > > > > This guidance seems to indicate the use of the scope parameter is > optional in determining the authorization details. Section 3.1 says "The > AS MUST consider both sets of requirements in combination with each other > for the given authorization request." My read is that this is conflicting > guidance and Section 3.1 is correct. > > You aren’t required to use “scope” in order to use > “authorization_details”, but we do want to say that the AS is allowed to > (or even is supposed to) consider both “scope” and “authorization_details” > when determining the resulting access for any given request that might have > both. The guidance in 3.1 should probably say “the AS MUST consider all > requirements present on a request” or something like that? > > > > > ** Figure 15. The text prior to this figure says that for "For our > running example, this would look like this" indicating that this figure is > similar to previous examples. There is one key different - this is the > first use of a "payment_initiator" type with the API URL prepended. > > > > ** Section 7.1. Typo. s/sub set/subset/ > > > > ** Section 8. What is the difference between this section and Section 5 > beyond this text explicitly stating the name of the error value > (invalid_authorization_details). I'd recommend stating the normative > behavior twice; that is, why are both sections needed? > > > > ** Section 9.2. Editorial. There is some kind of rendering issues in > the RFC7622 reference. It reads "[!@RFC7662]". > > > > ** Section 11.2. > > > > Products supporting this specification should provide the following > > basic functions: > > > > Should this section be more tightly scoped to AS behavior instead of a > "products"? > > > > ** Section 11.2. > > > > Accept authorization_details parameter in authorization requests > > including basic syntax check for compliance with this > > specification > > > > Why only "basic syntax checking"? Perhaps "syntax checking"? > > I’m not positive, but I think the guidance here is meant for “basic” to > mean more like “make sure it’s a JSON object and that it has a type field” > as opposed to “check the type field’s value and run it against a JSON > Schema definition”. > > > > > ** Section 11.2 > > > > One option would be to have a mechanism allowing the registration of > > extension modules, each of them responsible for rendering the > > respective user consent and any transformation needed to provide the > > data needed to the resource server by way of structured access tokens > > or token introspection responses. > > > > I don't follow the flexibility being described here. "One option ..." > with respect to what? > > With respect to having certain types hard-coded (like someone like > Facebook or GitHub might do because their API is specific) or having some > kind of mechanism that just prints out the RAR objects verbatim. > > > > > ** Section 11.3. Could this section provide an example of what it would > mean to use JSON schema ids in the type value. > > > > ** Section 12. Please note that the Security Considerations of RFC6749, > RFC7662, and RFC8414 apply. > > > > ** Section 13. > > > > Implementers MUST design and use authorization details in a privacy- > > preserving manner. > > > > I completely agree with the principle, but this design guidance cannot > be enforced without specifics. I recommend s/MUST/must/. The more > specific text in this section can use the normative MUST statements. > > > > ** Section 13. > > > > Any sensitive personal data included in authorization details MUST be > > prevented from leaking, e.g., through referrer headers. > > > > Is "leaking" the same as being sent unencrypted? Recommend being clear. > > Not just that, but anything that shows up as a URL parameter (which this > can) has the ability to be unwittingly transmitted to third parties in > weird ways — thus the referrer example. This is in line with other OAuth > security recommendations, do you have any suggestions to make this advice > more clear? > > > > > ** Section 13. > > > > The AS SHOULD share this data with those parties on a "need to know" > > basis. > > > > Completely agree. Consider ending this sentence with "... as determined > by local policy", or the equivalent to make it clear that it will not be > document in this specification. > > > > Thanks, > > Roman > > > > _______________________________________________ > > OAuth mailing list > > OAuth@ietf.org > > https://www.ietf.org/mailman/listinfo/oauth > > > — Justin > _______________________________________________ > OAuth mailing list > OAuth@ietf.org > https://www.ietf.org/mailman/listinfo/oauth > -- _CONFIDENTIALITY NOTICE: This email may contain confidential and privileged material for the sole use of the intended recipient(s). Any review, use, distribution or disclosure by others is strictly prohibited. If you have received this communication in error, please notify the sender immediately by e-mail and delete the message and any file attachments from your computer. Thank you._
_______________________________________________ OAuth mailing list OAuth@ietf.org https://www.ietf.org/mailman/listinfo/oauth
