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

[Torsten Lodderstedt <[e-mail]>]

Hi Roman, 

I just published revision -14 with the all the agreed changes.

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

best regards,
Torsten. 

> Am 24.10.2022 um 13:07 schrieb Roman Danyliw <[e-mail]>:
> 
> Hi Torsten!
> 
> Thanks for the response.  More inline ...
> 
>> -----Original Message-----
>> From: Torsten Lodderstedt <[e-mail] <mailto:[e-mail]>>
>> Sent: Tuesday, October 18, 2022 9:00 AM
>> To: Roman Danyliw <[e-mail] <mailto:[e-mail]>>
>> Cc: [e-mail] <mailto:[e-mail]>; Brian Campbell <[e-mail] <mailto:[e-mail]>>;
>> [e-mail] <mailto:[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