<div dir="ltr"><div>Thanks Torsetn, a few follow ups inline below <br></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Thu, Mar 19, 2020 at 11:38 AM Torsten Lodderstedt <<a href="mailto:torsten@lodderstedt.net" target="_blank">torsten@lodderstedt.net</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Hi Brian,<br>
<br>
> On 19. Mar 2020, at 17:29, Brian Campbell via Openid-specs-fapi <<a href="mailto:openid-specs-fapi@lists.openid.net" target="_blank">openid-specs-fapi@lists.openid.net</a>> wrote:<br>
> <br>
> [most of this review is against a revision of the draft that I got from git yesterday and "built" from source but some changes have been committed while reviewing so some comments might be out of date but I've also tried to kinda look at the updates so my thoughts and comments might be more confused or confusing than normal] <br>
> <br>
> <br>
> ===>1. Introduction <br>
> I sorta get the idea (I think) but also struggle to really follow it all. I know that's not actionable feedback and I apologize for that but it maybe gives me an opportunity to pivot into some more meta comments.<br>
> I brought up the OAuth 2.0 Incremental Authorization work on the Wednesday call not because I think it's better necessarily but there does seem to be a fair amount of overlap in what it does and what the goals appear to be. And it always raises questions/concerns in my mind to see what seem to be disparate spec solutions for the same problem. Incremental Authz did go through WGLC but progress on it seems to have stalled. So it may be moot but supporting both standards at the same time would be super awkward (or worse). <br>
<br>
Sorry from my side. I did not follow draft-ietf-oauth-incremental-authz close enough. And thanks for pointing out the overlap. <br>
<br>
The overlap essentially was in the way grant_management_mode and the include_granted_scopes parameter were used to control the behaviour re existing scopes (privileges) for a grant. <br>
<br>
I just created a PR trying to align both. <a href="https://bitbucket.org/openid/fapi/pull-requests/156/modified-draft-to-align-with-draft-ietf/diff" rel="noreferrer" target="_blank">https://bitbucket.org/openid/fapi/pull-requests/156/modified-draft-to-align-with-draft-ietf/diff</a></blockquote><div><br></div><div>I appreciate the effort but I don't know that that improves things (the new text in the 'use' mode). Also, was there a use-case for the 'replace' mode that that removed?</div><div><br></div><div> <br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
> There's also content in the introduction of the grant management draft (4th & 5th paragraphs) that appears to outright contradict Incremental Authz, which gives me pause. Incremental Authz also differentiates behaviour quite a bit between public and confidential clients for privacy and security reasons while there's no such distinction in the grant management draft. I haven't wrapped my head around it enough to say if it's a deficiency for grant management but it did stand out as a notable difference to me that likely needs more consideration and analysis. <br>
<br>
We haven’t considered public clients (yet) since FAPI is focused on confidential clients. I think the considerations in draft-ietf-oauth-incremental-authz apply to grant management as well use the refresh token (in the backchannel!) to proof grant ownership.   <br></blockquote><div><br></div><div>I do think it should be considered and written down at some point. Even if that just means saying public clients can't do grant_*. Or doing something like incremental-authz does. <br></div><div> <br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
> The public vs confidential client thing gets me thinking too that most/all of FAPI has public clients out of scope or disallowed. And I think that, particularly if public clients were not in scope, something like the grant management API could be added on as a resource without changes to the OAuth authz endpoint and token endpoint flow. Maybe that wouldn't meet all the requirements behind this, I don't know, but it would definitely be a less invasive change and that usually means it'd be easier and thus more likely to get added into products and OSS projects. <br>
<br>
There are two features that require extensions in the authorization process:<br>
- forcing the creation and maintenance of parallel/concurrent grants<br>
- forcing interaction with the same user (without the need to reveal her identity) <br>
<br>
> <br>
> ===>2. Overview<br>
> I don't quite get the use of the word "disclosure" how it's used here alongside create and update in the first sentence. <br>
<br>
seems “disclosure" is gone <br>
<br>
> <br>
> ===> 3.1. Authorization Request<br>
> I think the values and behaviours of the different grant_modes needs some more fleshing out. But looks like there've been some changes already recently here so I'm apparently not alone in that thinking. <br>
> <br>
> I know it's only the example but could the value of the grant_id in the examples be something more concise than what looks to be a hex encoding of 256 bits. It offends my delicate sensibilities and looks to be the reason behind all the warnings from xml2rfc:<br>
> <br>
> fapi-grant-management-00.xml(87): Warning: Too long line found (L214), 10 characters longer than 72 characters: <br>
>         &grant_id=4d276a8ab980c436b4ffe0c1ff56c049b27e535b6f1266e734d9bca992509c25<br>
> fapi-grant-management-00.xml(116): Warning: Too long line found (L270), 11 characters longer than 72 characters: <br>
>       "grant_id":"4d276a8ab980c436b4ffe0c1ff56c049b27e535b6f1266e734d9bca992509c25"<br>
> fapi-grant-management-00.xml(167): Warning: Too long line found (L355), 25 characters longer than 72 characters: <br>
>    <a href="https://as.example.com/grants/4d276a8ab980c436b4ffe0c1ff56c049b27e535b6f1266e734d9bca992509c25" rel="noreferrer" target="_blank">https://as.example.com/grants/4d276a8ab980c436b4ffe0c1ff56c049b27e535b6f1266e734d9bca992509c25</a><br>
> fapi-grant-management-00.xml(174): Warning: Too long line found (L362), 7 characters longer than 72 characters: <br>
>    GET /grants/4d276a8ab980c436b4ffe0c1ff56c049b27e535b6f1266e734d9bca992509c25<br>
> fapi-grant-management-00.xml(205): Warning: Too long line found (L408), 10 characters longer than 72 characters: <br>
>    DELETE /grants/4d276a8ab980c436b4ffe0c1ff56c049b27e535b6f1266e734d9bca992509c25<br>
>  Created file fapi-grant-management-00.txt<br>
> <br>
> Even base64url encoding the same underlying value would be a little shorter with TSdqirmAxDa0_-DB_1bASbJ- U1tvEmbnNNm8qZJQnCU but still more than seems needed (~340,000,000,000,000,000,000,000,000,000,000,000,000 times as much entropy as the security tokens in the example which sends a strange message).  Subsets of that value like TSdqirmAxDa0_-DB_1bASbJ-U1tvEmbn or even TSdqirmAxDa0_-DB_1bASQ would make the warnings go away and still look to meet the uniqueness requirements.<br>
<br>
Changed the examples to TSdqirmAxDa0_-DB_1bASQ<br>
<br>
> <br>
> ===> 3.2.1. Error Response <br>
> Not sure it necessarily belongs here as an error code but, with other text about a grant being bound to a user, I do think there needs to be more about the non-happy path cases of what happens when a grant_id is provided but the AS identifies a different user. It's probably just an error condition(?) but could use some more fleshing out me thinks. <br>
<br>
When could that happen? Create requires creation of a new grant_id and use requires use of the grant id as determined by the client. <br></blockquote><div><br></div><div>Client sends the grant_id with a use/update and the AS has a session established in that browser context for a different user. Don't think that kind of thing will be uncommon. <br></div><div><br></div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
> ===> 3.3. Token Request <br>
> "The grant_mode and grant_id parameters MAY be added to the token request in case this request is also an authorization request." - after reading this several times I think I know what is meant but "authorization request" is kinda overloaded and results in this text being super confusing (to me anyway).<br>
<br>
Took that text from RAR :-)<br></blockquote><div><br></div><div>We should look to fix that text up in PAR then too :) <br></div><div><br></div><div>But I also think the context of use is different enough that it might make more sense to not try and define the grant_* parameters at the token endpoint.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
> It maybe better to just not support the grant_* parameters at the token endpoint. I struggle to see a real use case for them there. But maybe I'm not creative enough. Seems to just complicate things though. <br>
> <br>
> Using the grant_mode and grant_id parameters at the CIBA auth endpoint is something to think about though... <br>
<br>
Yep, makes sense. Added a note. <br>
<br>
> <br>
> ===> 3.4. Token Response<br>
> I think Filip said something here already but the RECOMMENDED part here is pretty awkward. The "code verifier" is presumably a mistake? And the value in the example doesn't align with the recommendation.<br>
> Oh, wait, I see there's new text for that paragraph now "The grant_id value MUST be generated by the authorization server and constructed from a cryptographically strong random or pseudo-random number sequence (see [@RFC4086]] for best current practice) of at least 64 bits." - 64 bits is actually probably too few to reliably ensure uniqueness. And if bytes was intended there, 64 is kinda absurd for a MUST. <br>
> I think perhaps saying less here would be better. Like what's in the prior paragraph and similar in 3.1. is maybe sufficient.<br>
<br>
I removed the sentence. <br>
<br>
> <br>
> "OPEN: Should the AS always return the grant_id if the client once asked for it?" - no<br>
> <br>
> "OPEN: Should the AS rotate grant ids for privacy reasons?" - no <br>
> <br>
>  ===>  4.1. API authorization <br>
> Is the granularity of query and revoke really needed? Maybe just using one scope of grant_management would be okay? Sometimes less is more. <br>
<br>
Depends on whether we want to support least privileges. <br></blockquote><div><br></div><div>I don't think that's a fair assessment of the question at hand. <br></div><div><br></div><div> <br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
> Are there other aspects of authorization that need to be called out here? Like it seems like the access token should also identify the same client as the grant_id. And maybe the user too? I guess that kinda depends on how one expects the access token for this will be acquired. <br>
<br>
I kind of assume the text “to query the status of its grants” would imply this without prescribing how the AS actually implements it. <br></blockquote><div><br></div><div>Not looking to prescribe how the AS actually implements it (although I can't really think of a different way) but it felt to me as I read this that some more explicit guidance could be helpful here. <br></div><div> <br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
> ===>  4.2. Endpoint<br>
> Calling this a base URL/endpoint or something might be more clear about how this is used a bit different than other endpoints in metadata. Here this value is used with the grant_id to build the full endpoint, whereas most other endpoints are used as is and parameterized. <br>
<br>
Do we have examples of endpoints used with GET requests we could use as template?<br></blockquote><div><br></div><div>Not quite like this one, that I'm aware of. But that's okay. I think what you've got defined (mostly in Grant Resource URL) is fine. I was just thinking a little tweak to the name might help understanding. Maybe. But maybe I'm bikeshedding too. I was just thinking perhaps "grant_management_endpoint" could be something like "grant_management_base_endpoint" or "base_grant_management_endpoint".<br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
> <br>
> <br>
> ===>  4.5. Revoke Grant <br>
> I really don't want to make things more complicated than need be but I'm questioning the value of having just an all or nothing revoke function. Seems the same is possible via RFC 7009 revocation of a refresh token? <br>
<br>
The difference is token revocation does not necessarily revoke a grant. You can retain the grant even if a refresh token referring to it dies and the client can re-connect. The grant revocation gives you the ability to delete the underlying grant (something today typically done in a UI at the AS).<br>
<br>
I think this is a clear separation between grant (legal/policy aspects) and credential (security). We implemented this philosophy back at DT.<br></blockquote><div><br></div><div>tomato tomahto :) But irrespective of the implementation choice about the treatment of an implied data element on revocation of a refresh token, I'm still wondering if <br></div>query and delete-the-whole-grant offer enough useful utility. I don't know. But do think it's a legit question.<br><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
> How would we expect a user to delete one part (a scope value or authz deets value) from a grant? That seems like something that would be wanted. Maybe with an authz request and update grant mode with the expectation that the AS allow for prior stuff to be un-selected? Or a replace with re-requesting everything and letting the user manage that way? I dunno, maybe I'm not seeing the bigger picture here or something. <br>
<br>
TBD<br></blockquote><div><br></div><div>Okay, seems like an important detail to the overall utility of the functionality proposed by the draft. <br></div><div><br></div><div> <br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
> Okay, that's all for a first review :) <br>
> <br>
<br>
Thanks a lot! I incorporated some of your comments in the PR <a href="https://bitbucket.org/openid/fapi/pull-requests/156/modified-draft-to-align-with-draft-ietf/diff" rel="noreferrer" target="_blank">https://bitbucket.org/openid/fapi/pull-requests/156/modified-draft-to-align-with-draft-ietf/diff</a>.<br></blockquote><div><br></div><div>Thanks! I do think maybe the use vs update/replace bit should maybe be backed out for now. But otherwise thanks for the (prospective) updates, which I do think improve the draft.  <br></div></div></div>

<br>
<i style="margin:0px;padding:0px;border:0px;outline:0px;vertical-align:baseline;background:rgb(255,255,255);font-family:proxima-nova-zendesk,system-ui,-apple-system,system-ui,"Segoe UI",Roboto,Oxygen-Sans,Ubuntu,Cantarell,"Helvetica Neue",Arial,sans-serif;color:rgb(85,85,85)"><span style="margin:0px;padding:0px;border:0px;outline:0px;vertical-align:baseline;background:transparent;font-family:proxima-nova-zendesk,system-ui,-apple-system,BlinkMacSystemFont,"Segoe UI",Roboto,Oxygen-Sans,Ubuntu,Cantarell,"Helvetica Neue",Arial,sans-serif;font-weight:600"><font size="2">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.</font></span></i>