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

Brian Campbell bcampbell at pingidentity.com
Thu Mar 19 16:29:49 UTC 2020

[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
<https://tools.ietf.org/html/draft-ietf-oauth-incremental-authz-03> 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).
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.
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.

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

*===> 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:

fapi-grant-management-00.xml(116): Warning: Too long line found (L270), 11
characters longer than 72 characters:

fapi-grant-management-00.xml(167): Warning: Too long line found (L355), 25
characters longer than 72 characters:

fapi-grant-management-00.xml(174): Warning: Too long line found (L362), 7
characters longer than 72 characters:
fapi-grant-management-00.xml(205): Warning: Too long line found (L408), 10
characters longer than 72 characters:
 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.

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

*===> 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). 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...

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

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

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.

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

*===>  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?
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.

Okay, that's all for a first review :)

_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/bdacff7f/attachment.html>

More information about the Openid-specs-fapi mailing list