Hi Brian, just a couple responses inline where it seemed fitting. Thanks for going through everything! — Justin
> On Aug 25, 2020, at 6:01 PM, Brian Campbell <bcampb...@pingidentity.com> > wrote: > > 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 <jric...@mit.edu > <mailto:jric...@mit.edu>> 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"? Fair point about the terminology, and while it’s verbose I think the more precise wording might be warranted here so as not to extend the problems and confusion with “confidential” clients as a term. > > > §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. I think the only perfect solution is to go back in time and fix the registries with based on the last decade of knowledge in using them. :P For this, I think maybe being very prescriptive about the fact that the only parameters from the token endpoint that are allowed here are those used for client authentication and that when they show up, they’re interpreted as in the token endpoint request not the authorization endpoint request. Does that work? > > > ¶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? I do think it’s too loose and it should be a MUST, and the methods for enforcing that “MUST” are going to vary based on the deployments and implementations out there. > > > §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. I’ll see if I can suggest something to help with this. > > > §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. Same comment as above. > > > §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. I think having a specific section for privacy is important and should be added, even if it’s short. In this case I don’t think it would actually be all that short since you’re now hiding a lot of potentially privacy-leaking parameters from the browser. — Justin > > 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
