Subject: [OAUTH-WG] WGLC review of PAR

From: Neil Madden <[email-masked]>
Date: Thu, 20 Aug 2020 12:30:28 +0100

As promised in the last interim meeting, I’ve reviewed the current (03) draft-ietf-oauth-par document. Overall it looks close to ready to me, with mostly minor comments and one security-relevant comment on section 2.1 that should be discussed further, and one additional proposed security consideration:

Abstract:
The wording here could be improved - “allows clients to push an authorization request […] used as a reference to the data in a subsequent authorization request.” Both the pushed data and the call to the authorization endpoint are referred to as an “authorization request”. Maybe change the second usage to “in a subsequent call to the authorization endpoint”.

Section 1:
The introductory part here is quite long. Maybe adding a new sub-heading before the example would make it flow better.

The end of the introduction uses the acronym “PAR” for the first time, but never explicitly defines it.

I agree with Justin that ACR is not the best example to lead with. If it stays there should be a reference to OIDC to explain what this means.

The paragraph that begins “It also allows clients to push the form encoded …” is confusing because the use of “also” suggests this is different from the previous paragraph, but it seems to actually be saying the same thing?

“[…] but it also allows clients requiring
   an even higher security level, especially cryptographically confirmed
   non-repudiation, to explicitly adopt JWT-based request objects”

The “but” should be an “and” in this paragraph. It’s also not clear what is being said here - isn’t JAR what provides JWT-based request objects? 

The paragraph beginning “As a further benefit, …” is a little bit of a Columbo moment (“Just one more thing…”) at the end of the introduction. Maybe move this as another bullet point at the start of the section. The following paragraph can then be rewritten as “The increased confidence in the identity of the client during the authorization process allows confidential clients to provide a different redirect_uri on every request. […]”

Section 2:
The 3rd paragraph contains statements like 
The endpoint also supports sending all authorization
   request parameters as request object according to
   [I-D.ietf-oauth-jwsreq <https://tools.ietf.org/html/draft-ietf-oauth-par-03#ref-I-D.ietf-oauth-jwsreq>].
presumably this is not a normative requirement that any PAR implementation has to also support JAR, or is it? Maybe change the wording to “MAY also support …”.

The first mention of “token_endpoint_auth_method” and client metadata should have a reference to RFC 7591 - currently it’s only referenced later in section 2.1.

2.1:
I’m a little bit wary of the relaxing of the redirect_uri processing rules, because this removes a layer of defence in depth. With the current requirement for pre-registered URIs an attacker needs to compromise the redirect endpoint *and* the client credentials in order to steal an authorization code and use it. With this change, compromising the client credentials alone would be enough. If the use-case is specifically in a PKI environment, could the redirect_uri be baked into the cert? Maybe this use-case could be better addressed in a separate draft.

2.2:
The definition of “expires_in” as a "JSON number" suggests that fractional/floating-point values are allowed. Presumably this is intended to be an integer? Is there a recommended minimum/maximum? Suggested wording:

===
   *  "expires_in" : A JSON number that represents the lifetime of the
      request URI in seconds as a positive integer. The lifetime SHOULD 
      be at least 5 seconds and at most 600 seconds, but other values are
      permitted at the discretion of the authorization server.
===

Those values are fairly arbitrary - 5 seconds seems a reasonable lower bound for a client with a bad network connection, and 10 minutes seems more than adequate upper bound for the typical uses.

3:
I confirmed that the request JWT verifies with the given JWK using our internal JWT library :-)

5:
“require_pushed_authorization_requests” might be better named “requires_pushed_authorization_requests” given that it is describing the server’s policy rather than telling the client to require them, but this is a really pedantic point. Same for the client metadata in section 6.

7:
I would like to propose an additional security consideration, with the following wording:

===
7.5. Request URI swapping

An attacker could capture the request URI from one request and then substitute it into a different authorization request. For example, in the context of OpenID Connect, an attacker could replace a request URI asking for a high level of authentication assurance with one that requires a lower level of assurance. Clients SHOULD make use of PKCE, a unique “state" parameter, or the OIDC “nonce” parameter in the pushed request object to prevent this attack.
===

— Neil

---

From: Brian Campbell <[email-masked]>
Date: Wed, 26 Aug 2020 14:04:47 -0600

Thanks Neil. Appreciate the review and feedback. My attempts at responses
are inline below.

On Thu, Aug 20, 2020 at 5:30 AM Neil Madden <[email-masked]>
wrote:

> As promised in the last interim meeting, I’ve reviewed the current (03)
> draft-ietf-oauth-par document. Overall it looks close to ready to me, with
> mostly minor comments and one security-relevant comment on section 2.1 that
> should be discussed further, and one additional proposed security
> consideration:
>
> Abstract:
> The wording here could be improved - “allows clients to push an
> authorization request […] used as a reference to the data in a subsequent
> authorization request.” Both the pushed data and the call to the
> authorization endpoint are referred to as an “authorization request”. Maybe
> change the second usage to “in a subsequent call to the authorization
> endpoint”.
>

Makes sense.

>
> Section 1:
> The introductory part here is quite long. Maybe adding a new sub-heading
> before the example would make it flow better.
>

Will look at breaking it up.

> never explicitly defines it.

Will fix.

>
> I agree with Justin that ACR is not the best example to lead with. If it
> stays there should be a reference to OIDC to explain what this means.
>

Yup.

>
> The paragraph that begins “It also allows clients to push the form encoded
> …” is confusing because the use of “also” suggests this is different from
> the previous paragraph, but it seems to actually be saying the same thing?
>

Yeah, that is rather awkward because it is the same thing. Will fix.

> The “but” should be an “and” in this paragraph. It’s also not clear what is being said here - isn’t JAR what provides JWT-based request objects?
Yeah, JAR defines JWT-based request objects and PAR allows for their use by
sending the 'request' parameter to the PAR endpoint.  Will try and make
that more clear.

> The paragraph beginning “As a further benefit, …” is a little bit of a Columbo moment (“Just one more thing…”) at the end of the introduction. Maybe move this as another bullet point at the start o…
 Agree with the sentiment here and will endeavor to rework things along the
lines of the suggestion.

> presumably this is not a normative requirement that any PAR implementation
> has to also support JAR, or is it? Maybe change the wording to “MAY also
> support …”.
>

Interesting question. PAR has a normative reference to JAR for the
request_uri parameter at the authz endpoint. But does that necessitate that
every PAR implementation also has to support all of JAR? I'm thinking
probably not. I mean, different but related, an AS PAR implementation might
legitimately support the request_uri parameter with URI values that it
creates but not support the more general retrieval of arbitrary URIs. In
the same vein, it seems legit and still useful to support PAR without also
supporting request object JWTs. So yeah, I think changing this to MAY or
something similar would be appropriate.

>
> The first mention of “token_endpoint_auth_method” and client metadata
> should have a reference to RFC 7591 - currently it’s only referenced later
> in section 2.1.
>

Will fix.

2.1:
> I’m a little bit wary of the relaxing of the redirect_uri processing
> rules, because this removes a layer of defence in depth. With the current
> requirement for pre-registered URIs an attacker needs to compromise the
> redirect endpoint *and* the client credentials in order to steal an
> authorization code and use it. With this change, compromising the client
> credentials alone would be enough. If the use-case is specifically in a PKI
> environment, could the redirect_uri be baked into the cert? Maybe this
> use-case could be better addressed in a separate draft.
>

 I agree that the specifics of a PKI type environment use-case would be
better in a separate draft or profile somewhere. And do plan to add some
more considerations around the possibility of relaxed redirect_uri
validation.

2.2:
> The definition of “expires_in” as a "JSON number" suggests that
> fractional/floating-point values are allowed. Presumably this is intended
> to be an integer?
>

Yes and I need to clarify that it's a positive integer.

> Is there a recommended minimum/maximum? Suggested wording:
>
> ===
>
> Those values are fairly arbitrary - 5 seconds seems a reasonable lower
> bound for a client with a bad network connection, and 10 minutes seems more
> than adequate upper bound for the typical uses.
>

Arbitrary, yes. But also pretty reasonable. I'm not too keen on using
arbitrary values with normative language, however, even if reasonable. I
think we could work them in with slightly different language something
like, "for example, at least 5 seconds but at most 600".

> I confirmed that the request JWT verifies with the given JWK using our

Yay for interoperability!  I produced those using a different JWT library
:)

5:
> “require_pushed_authorization_requests” might be better
> named “requires_pushed_authorization_requests” given that it is describing
> the server’s policy rather than telling the client to require them, but
> this is a really pedantic point. Same for the client metadata in section 6.
>

Fair point but I don't think sufficient to warrant a change to a protocol
parameter name at this point.

7:
> An attacker could capture the request URI from one request and then
> substitute it into a different authorization request. For example, in the
> context of OpenID Connect, an attacker could replace a request URI asking
> for a high level of authentication assurance with one that requires a lower
> level of assurance. Clients SHOULD make use of PKCE, a unique “state"
> parameter, or the OIDC “nonce” parameter in the pushed request object to
> prevent this attack.
> ===
>

Thanks. Will incorporate (with added refs and minor tweaks). I'm wondering
if there's a good example that isn't OpenID Connect though? Just thinking
something OAuth only might be more accessible to most readers (similar to
what you and Justin pointed out that the ACR example earlier in the draft
may not be the best).

-- 
_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._

---

From: Neil Madden <[email-masked]>
Date: Fri, 28 Aug 2020 11:54:40 +0100

Thanks for the response Brian, I agree with your comments. I’ve been scratching my head for a non-OIDC example for the URI swapping issue, but can’t think of one that isn’t very contrived at the moment.

— Neil

> On 26 Aug 2020, at 21:04, Brian Campbell <[email-masked]> wrote:
> 
> Thanks Neil. Appreciate the review and feedback. My attempts at responses are inline below. 
> 
> 
> On Thu, Aug 20, 2020 at 5:30 AM Neil Madden <[email-masked] <mailto:[email-masked]>> wrote:
> Yeah, JAR defines JWT-based request objects and PAR allows for their use by sending the 'request' parameter to the PAR endpoint.  Will try and make that more clear.
>  
> 
>  Agree with the sentiment here and will endeavor to rework things along the lines of the suggestion. 
> 
>  
> Section 2:
> Interesting question. PAR has a normative reference to JAR for the request_uri parameter at the authz endpoint. But does that necessitate that every PAR implementation also has to support all of JAR? I'm thinking probably not. I mean, different but related, an AS PAR implementation might legitimately support the request_uri parameter with URI values that it creates but not support the more general retrieval of arbitrary URIs. In the same vein, it seems legit and still useful to support PAR without also supporting request object JWTs. So yeah, I think changing this to MAY or something similar would be appropriate. 
>  
> 
>  I agree that the specifics of a PKI type environment use-case would be better in a separate draft or profile somewhere. And do plan to add some more considerations around the possibility of relaxed redirect_uri validation. 
> 
> 
> 2.2:
> The definition of “expires_in” as a "JSON number" suggests that fractional/floating-point values are allowed. Presumably this is intended to be an integer? 
> 
> Arbitrary, yes. But also pretty reasonable. I'm not too keen on using arbitrary values with normative language, however, even if reasonable. I think we could work them in with slightly different language something like, "for example, at least 5 seconds but at most 600". 
>  
> 
> 3:
> Yay for interoperability!  I produced those using a different JWT library :) 
> 
> 5:
> Fair point but I don't think sufficient to warrant a change to a protocol parameter name at this point. 
>  
> 
> 7:
> Thanks. Will incorporate (with added refs and minor tweaks). I'm wondering if there's a good example that isn't OpenID Connect though? Just thinking something OAuth only might be more accessible to most readers (similar to what you and Justin pointed out that the ACR example earlier in the draft may not be the best).   
> 
>  
> 
> 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.