Re: [OAUTH-WG] WGLC Review of PAR

[Brian Campbell <[e-mail]>]

Thanks for the review and comments Justin. Replies (or attempts thereat)
are inline below.


On Wed, Aug 19, 2020 at 2:06 PM Justin Richer <[e-mail]> wrote:

> I’ve done a full read through of the PAR specification, and here are my
> notes on it.
>
> For additional context, I’ve implemented this specification for both a
> client and a server in a couple of languages. Overall, I think it’s in good
> shape and it makes sense from a developer’s perspective. I’ve got a few
> comments, some small and some that might need more conversation within the
> WG,
>

Always nice to get feedback from implementation experience. Especially when
the overall is that it's "in good shape".


Throughout: Suggest using “credentialed” instead of “confidential” client,
> as introduced in OAuth 2.1 draft.
>

I'm hesitant to use *new* terminology from the 2.1 draft, which was just
recently adopted by the WG, in this document that is written as an
extension of OAuth 2.0 and is further along in the process going through
WGLC. There's a temporal dependency problem including potential risk of
change after the fact.

Perhaps this draft could avoid use of the terms and be more explicit and
wordy with something like "clients having established authentication
credentials with the AS"?



> §1: Suggest the problems list start with changing scopes or swapping
> client IDs as scenarios in the first bullet, ACR is an esoteric use case
> for many and not in OAuth 2 core, either remove it or put it at the end of
> the bullet.
>

Fair suggestion. Will look at reworking it a bit.


   Suggest the second bullet note who the information needs to be protected
> from, at least in passing. It’s not clear from this setup why the
> parameters should be confidential, and this is a major motivation for this
> work.
>

Also a fair suggestion. Although there are some subtleties and complexities
that I think make this a little tricky to write. Will try though.



>    Avoid use of phrase “so-called” and just give the name “Request Object”.
>

Okay, yeah. I think a moment of terminology frustration slipped into the
text there.



>    ¶4: Perhaps overly pedantic but I suggest extending: “in exchange for a
> request_uri value usable at the authorization server”.
>

I like the pedanticness.


>
>    ¶4/5: Alternatively, suggest combining these paragraphs: “This document
> complements JAR by providing an interoperable way for the client to push
> its authorization request parameters to the authorization server in
> exchange for a request_uri usable at the authorization server. The document
> further allows the client to push request objects as specified in JAR in
> exchange for a request_uri usable at the authorization server.”
>

 Yeah, I think that working works better.



>     ¶12: “This is directly utilized” is a little ambiguous into what it’s
> referring to. Would suggest rewording the start as: “This early stage
> client authentication is used by this draft to allow confidential clients…”
> or something of that sort.
>

Makes sense to be less ambiguous there.


>
>     ¶13: Not only is POST much harder to use, it’s also optional for the
> AS to implement so it can’t be counted on by a client to be available
> generally. (To be honest in retrospect we shouldn’t have included it in
> OAuth 2.)
>

Connect says the AS/OP must support both get and post. But your point on
optionality stands with respect to pure OAuth only ASs. Will add something
about that to the paragraph.


>
> §2: Please provide a reference to JWT client assertion auth here (either
> the assertion RFC or OIDC’s definition of the client auth methods
> mentioned). I would also phrase this as direct guidance instead of a
> note/aside.
>

Will do.


>
> §2.1: There’s some potential weirdness about client_id here. Since the
> authz request was designed around not having client authentication, that
> request requires client_id. However, here the client is authenticating, and
> the client_id might be included elsewhere like the Basic header. A
> developer might be curious about whether they need to include them twice.
>

Fair point about the weirdness. Suggestions on some text to preempt
curiosity or confusion? My thinking is that the client_id parameter should
always be sent so as to conform to the syntax of a regular authz request.
But I'll admit that, due to reusing client auth logic, my server
implementation won't strictly require client_id when it's included
elsewhere like the Basic header.


>
>     ¶2: Of necessity, this spec mixes parameters in the authorization
> endpoint and token endpoint registries into a single request. Is there any
> danger of conflict between them? The registry holds them in one list but
> they could possibly have different semantics in both places.
>

I think that technically such danger does exist but that it's highly
unlikely in practice. Especially because the only token endpoint parameters
that are relevant to PAR are those that deal with client authentication
(currently client_secret, client_assertion, and client_assertion_type). I'm
also not sure what can reasonably be done about it given the way the
registries are. I guess PAR could update the registration for those three
(client_secret, client_assertion, and client_assertion_type) to also
indicate authorization request as a usage location with some commentary
that it's only for avoiding name collisions. And offer some guidance about
doing the same for any future client auth methods being defined. But
honestly I'm not sure what, if anything, to do here?

And yes it is super unfortunate that client auth and protocol parameters
got mixed together in the HTTP body. I didn't cause that situation but I've
certainly contributed to it and for that I apologize.


>     ¶6: Does the AS really have "the ability to authenticate and
> *authorize* clients”? I think what we mean here is "the ability to
> authenticate clients and validate client requests”, but I’m not positive of
> the intent.
>

I think the intent is that the AS can check whether a client is authorized
to make a particular authorization request (specific scopes, response type,
etc.). But checking authorization to request authorization is confusing
wording. I think your working is less confusing and still allows for the
intent.

I'll let Torsten interject if he feels differently as I think he originally
wrote the text in question.



>
>     ¶7: I’m not sure I buy this example. Even if the clientID is managed
> externally, the association with a set or pattern of allowed redirect URIs
> is still important, and the AS will need to know what that is. I think this
> example could lead an AS developer to (erroneously and dangerously)
> conclude that they don’t have to check any other values in a request,
> including scope and redirect URI. It’s important that DynReg doesn’t
> alleviate that issue, but removal of DynReg doesn’t really change things in
> that regard. Suggest removing example or reworking paragraph.
>

I'm going to have to defer to Torsten on this because, to be honest, I'm
not too sure about it myself. I tend to lean towards thinking the draft
would be better off without it.



>
> §2.2: Is “expires_in” required? If so, can an AS decide that a request URI
> doesn’t expire after a certain amount of time? Related, what does a “0” or
> negative value mean, if anything?
>

Yes, no, and that it's already expired, respectively. I guess anyway. But
maybe better to say that it has to be there and is a positive integer
value. Will clarify all that.


>
>     I don’t see why a request URI with unguessable values isn’t a MUST for
> one-time-use, is there a reason?
>

The reason AFAIK was to not be overly prescriptive and allow for eventually
consistent or not atomic storage of the data by not strictly requiring the
AS to enforce one-time-use. Do you think that's too loose or could be
worded/explained differently or better?


>
> §2.3: Are the HTTP status codes a normative MAY or just an informative
> “can” as written?
>

It's meant to be more of an informative “can” or even just a "does". It's
supposed to just say that the error response looks the same as a token
endpoint error response - same JSON and same status code, which is a 400
unless otherwise stated. There have been other similar WGLC comments on
this particular text so it's already on the list to reword for clarity.


>
> §3: This bit should be normative as follows: "he authorization server MUST
> take the following steps beyond the processing rules…"
>
>     Clean up the tenses and voice of the numbered list, which are
> currently inconsistent.
>

Yep and yep, respectively.


>
> §7.2: The AS should also make sure that the new redirect URI is applicable
> within that client’s domain or policies. So you wouldn’t want a
> legit-but-rogue client registering for someone else’s redirect URI in order
> to start an attack, for example. I think overall we might need better
> guidance around the redirect URI variability feature (which, to be clear,
> I’m hugely in favor of — but OAuth’s assumptions in the client model make
> this trickier to talk about and implement safely, so we need to be extra
> careful).
>

Yeah, I think I follow and agree with you here. I'll try and add some more
coverage/guidance on the topic. But if you have any concrete suggestions or
text, I'd welcome it.


>
> §7.3: As above, is there a reason that this isn’t a MUST? It’s a temporary
> credential representing a request, much like an authorization code in many
> ways. I don’t see why a legitimate client would use it more than once, or
> why a server would want to allow it for AS-generated values. For
> client-supplied request URIs, sure, but not here.
>

The intent wasn't to allow it to be used more than once. But rather to
avoid putting a strict requirement on the AS to enforce one-time-use (and
the eventual certification tests that send the same URI twice within
milliseconds). If that distinction makes any sense. It is much like an
authorization code in many ways including the one-time-use text on code in
RFC 6749 being subject to different interpretations :/ I would like to
avoid different interpretations here.

For better or worse, that's some of the reasoning behind what's there now.
But I'm open to changing the normative strength of the statement, if you
and other folks in the WG feel it should be more MUSTy. And I'm certainly
open to suggestions on better wording/explanation.


>
> §X: Missing privacy considerations section. If anything this spec helps
> alleviate some privacy concerns by trading values for a reference, but the
> section is still needed.
>

Because of that and because it's already discussed to some extent in the
body of the document, a separate privacy considerations section didn't seem
needed. But we can look at adding a short section that says as much.

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