Re: [OAUTH-WG] WGLC review of PAR

[Brian Campbell <[e-mail]>]

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