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

[Justin Richer <[e-mail]>]

Thank you, Torsten!

 — Justin

On Oct 24, 2022, at 12:18 PM, Torsten Lodderstedt <[e-mail]<mailto:[e-mail]>> wrote:

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]<mailto:[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]<mailto:[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]<mailto:[e-mail]>
https://www.ietf.org/mailman/listinfo/oauth