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 <bcampb...@pingidentity.com> 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 <neil.mad...@forgerock.com > <mailto:neil.mad...@forgerock.com>> 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.
_______________________________________________ OAuth mailing list OAuth@ietf.org https://www.ietf.org/mailman/listinfo/oauth
