[OAUTH-WG] WGLC review of PAR

[Neil Madden <[e-mail]>]

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