Re: [OAUTH-WG] AD review of draft-ietf-oauth-rar-12

[Roman Danyliw <[e-mail]>]

Hi Torsten!

Thanks for the response.  More inline ...

> -----Original Message-----
> From: Torsten Lodderstedt <[e-mail]>
> Sent: Tuesday, October 18, 2022 9:00 AM
> To: Roman Danyliw <[e-mail]>
> Cc: [e-mail]; Brian Campbell <[e-mail]>;
> [e-mail]
> Subject: Re: [OAUTH-WG] AD review of draft-ietf-oauth-rar-12
> 
> Hi Roman,
> 
> thanks for you review.
> 
> I’m trying to do the next round for the outstanding issues you raised.
> 
> > Am 15.09.2022 um 00:30 schrieb Roman Danyliw <[e-mail]>:
> >
> > 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.
> 
> I’m not sure how to handle this as there is no direct consequence (in the sense
> of error condition) other then that different byte arrays constitute different
> types.
> 
> We could state something like this: "String values with different byte
> representations constitute different types.“ ?

Works for me.

> >
> > ** 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?
> >
> > ** 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
> 
> The spec uses authorization details types to refer to the general concept and
> authorization_details to refer to the parameter. I think the former is
> appropriate at this place.

Understood.

> >
> > ** 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.
> >
> > ** 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.
> 
> Justin: „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?“
> 
> Section 3 already states that the AS must consider scope and resource in
> addition to authorisation details. I suggest to drop that sentence entirely.

Works for me.

> >
> > ** 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?
> 
> 5 and 8 define the error behaviour of different endpoint. But you are right, the
> text is the same (with 5 being even more detailed). Suggest to remove the text
> in 8 with a reference to 5.

Works for me.

Thanks,
Roman

> best regards,
> Torsten.
> 
> >
> > ** 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"?
> >
> > ** 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?
> >
> > ** 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.
> >
> > ** 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
> > [e-mail]
> > https://www.ietf.org/mailman/listinfo/oauth