[Openid-specs-fapi] A token review of the grant management draft

[Brian Campbell]

Thanks Torsetn, a few follow ups inline below

On Thu, Mar 19, 2020 at 11:38 AM Torsten Lodderstedt <
torsten at lodderstedt.net> wrote:

> Hi Brian,
>
> > On 19. Mar 2020, at 17:29, Brian Campbell via Openid-specs-fapi <
> openid-specs-fapi at lists.openid.net> wrote:
> >
> > [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]
> >
> >
> > ===>1. Introduction
> > 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.
> > 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).
>
> Sorry from my side. I did not follow draft-ietf-oauth-incremental-authz
> close enough. And thanks for pointing out the overlap.
>
> 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.
>
> I just created a PR trying to align both.
> https://bitbucket.org/openid/fapi/pull-requests/156/modified-draft-to-align-with-draft-ietf/diff


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?



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

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.



> > 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.
>
> There are two features that require extensions in the authorization
> process:
> - forcing the creation and maintenance of parallel/concurrent grants
> - forcing interaction with the same user (without the need to reveal her
> identity)
>
> >
> > ===>2. Overview
> > I don't quite get the use of the word "disclosure" how it's used here
> alongside create and update in the first sentence.
>
> seems “disclosure" is gone
>
> >
> > ===> 3.1. Authorization Request
> > 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.
> >
> > 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:
> >
> > fapi-grant-management-00.xml(87): Warning: Too long line found (L214),
> 10 characters longer than 72 characters:
> >
>  &grant_id=4d276a8ab980c436b4ffe0c1ff56c049b27e535b6f1266e734d9bca992509c25
> > fapi-grant-management-00.xml(116): Warning: Too long line found (L270),
> 11 characters longer than 72 characters:
> >
>  "grant_id":"4d276a8ab980c436b4ffe0c1ff56c049b27e535b6f1266e734d9bca992509c25"
> > fapi-grant-management-00.xml(167): Warning: Too long line found (L355),
> 25 characters longer than 72 characters:
> >
> https://as.example.com/grants/4d276a8ab980c436b4ffe0c1ff56c049b27e535b6f1266e734d9bca992509c25
> > fapi-grant-management-00.xml(174): Warning: Too long line found (L362),
> 7 characters longer than 72 characters:
> >    GET
> /grants/4d276a8ab980c436b4ffe0c1ff56c049b27e535b6f1266e734d9bca992509c25
> > fapi-grant-management-00.xml(205): Warning: Too long line found (L408),
> 10 characters longer than 72 characters:
> >    DELETE
> /grants/4d276a8ab980c436b4ffe0c1ff56c049b27e535b6f1266e734d9bca992509c25
> >  Created file fapi-grant-management-00.txt
> >
> > 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.
>
> Changed the examples to TSdqirmAxDa0_-DB_1bASQ
>
> >
> > ===> 3.2.1. Error Response
> > 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.
>
> 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.
>

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.


> ===> 3.3. Token Request
> > "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).
>
> Took that text from RAR :-)
>

We should look to fix that text up in PAR then too :)

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.


>
> > 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.
> >
> > Using the grant_mode and grant_id parameters at the CIBA auth endpoint
> is something to think about though...
>
> Yep, makes sense. Added a note.
>
> >
> > ===> 3.4. Token Response
> > 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.
> > 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.
> > I think perhaps saying less here would be better. Like what's in the
> prior paragraph and similar in 3.1. is maybe sufficient.
>
> I removed the sentence.
>
> >
> > "OPEN: Should the AS always return the grant_id if the client once asked
> for it?" - no
> >
> > "OPEN: Should the AS rotate grant ids for privacy reasons?" - no
> >
> >  ===>  4.1. API authorization
> > Is the granularity of query and revoke really needed? Maybe just using
> one scope of grant_management would be okay? Sometimes less is more.
>
> Depends on whether we want to support least privileges.
>

I don't think that's a fair assessment of the question at hand.



> > 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.
>
> I kind of assume the text “to query the status of its grants” would imply
> this without prescribing how the AS actually implements it.
>

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.



> > ===>  4.2. Endpoint
> > 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.
>
> Do we have examples of endpoints used with GET requests we could use as
> template?
>

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


> >
> >
> > ===>  4.5. Revoke Grant
> > 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?
>
> 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).
>
> I think this is a clear separation between grant (legal/policy aspects)
> and credential (security). We implemented this philosophy back at DT.
>

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
query and delete-the-whole-grant offer enough useful utility. I don't know.
But do think it's a legit question.

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

Okay, seems like an important detail to the overall utility of the
functionality proposed by the draft.



> > Okay, that's all for a first review :)
> >
>
> Thanks a lot! I incorporated some of your comments in the PR
> https://bitbucket.org/openid/fapi/pull-requests/156/modified-draft-to-align-with-draft-ietf/diff
> .
>

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.

-- 
_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._
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.openid.net/pipermail/openid-specs-fapi/attachments/20200319/884f0bc5/attachment-0001.html>