Re: [OAUTH-WG] WGLC review of PAR

[Neil Madden <[e-mail]>]

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 <[e-mail]> 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 <[e-mail] <mailto:[e-mail]>> 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. 
>  
> 
> The end of the introduction uses the acronym “PAR” for the first time, but 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. 
>  
> 
> “[…] 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? 
> 
> 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 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. […]”
> 
>  Agree with the sentiment here and will endeavor to rework things along the lines of the suggestion. 
> 
>  
> 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 …”.
> 
> 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:
> 
> ===
>    *  "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.
> 
> 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:
> I confirmed that the request JWT verifies with the given JWK using our internal JWT library :-)
> 
> 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:
> 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.
> ===
> 
> 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.