zitadel
Access token response is not compliant with OAuth 2.0 (RFC 6749)
Preflight Checklist
- [x] I could not find a solution in the existing issues, docs, nor discussions
- [ ] I have joined the ZITADEL chat
Describe your problem
In the RFC 6749 document describing the Access Token scope request parameter it indicates the following:
If the issued access token scope is different from the one requested by the client, the authorization server MUST include the "scope" response parameter to inform the client of the actual scope granted.
I've implemented both authorization_code and client_credentials in my program with the op.Storage interface. However, when I supply the scope parameter in the token endpoint the response does not include the required parameter.
I think I don't need to supply my code because it's easy to see that oidc.AccessTokenResponse does not include the scope parameter: https://github.com/zitadel/oidc/blob/97d7b28fc080f9929e590ef1989880b0762e0098/pkg/oidc/token.go#L232-L239
Describe your ideal solution
Be compliant with OAuth 2.0 in this case. Whenever the scope parameter is sent to the token endpoint, check whether the returned scope matches the request. Or return the scope always if that is easier, which complies also with the rule.
Version
3.30.0
Environment
Self-hosted
Additional Context
I'm not sure if I this filed this issue correctly. In the OpenID spec, in the token endpoint section indicates to follow "Section 3.2" of RFC 6749. Clearly, 3.3 is not mentioned. Should 3.3 be included?
Aside of this, I think is very helpful to obtain the actual scope given in the access token response. I may have missed if there is other way to achieve this, so please forgive me and educate me if this is the case.
Edit: oidc.AccessTokenResponse permalink to show all lines of the struct.
@isegura-eos-eng Yes, you are right it should't take much to add it to the struct.
@livio-a this feels like a Deja Vu. Wasn't there already an issue, fix or discussion somewhere?
Found it: https://github.com/zitadel/zitadel/issues/6609
At the time someone volunteered for a fix, but went silent. @isegura-eos-eng do you want to send a PR for this?
@muhlemmer I could try. I've looked at the code a little bit, already. There are four places where this response must be returned, assuming this is the only struct used for returning the Access Token response. I think this can be very easy or the opposite. Depending on whether the AuthRequest and TokenRequest (or other similar signature structs) return the actual scopes that are granted or just the ones requested by the client. Maybe someone that knows better the implementation could tell me.
If it's the first case, then is pretty straight-forward, as we can just get the actual scopes from there and into the response. As I explained in the issue, we don't really need to know if the scopes granted differ from the ones requested, although is not very elegant, this would comply with the spec (maybe enough for now). The elegant way to do it is compare whether the scopes are different form the ones requested, as it's written by the RFC.
If it's the second case, which I suspect it is. Then we need to get the information there. And looking at the code, I don't see a clear way of accessing this information. Does anyone have an idea how to make this change compatible with the API if this is the case?
@isegura-eos-eng validation of scope is done early in the Authorize handler. The invalid scopes are silently dropped.
https://github.com/zitadel/oidc/blob/5ae555e19136066760d02e10af451464c6a3e3c8/pkg/op/auth_request.go#L268-L286
The token endpoint has no access to the originally requested scope. For this fix I would just always return scope, which is not forbidden by the standard and fulfills the MUST requirement on different scopes.
Then it's pretty straight forward. I've never done a PR in this project, any tips @muhlemmer ?
And another question: To build the scopes I have to do: strings.Join(" ", scopes), nothing fancy. However, this uses the standard package strings. I noticed there is another package strings in this project fighting over the same name. Also, I see the only function that this package implements (and tests) is the same functionality that slices.Contains implements, but just for strings. Since the project uses Go +1.21, I would suggest getting rid of the code in favor of using the standard library. Maybe there is another reason to keep it there, is there? This would be outside the scope of the PR, do you usually work this small refactors in other issues?
Then it's pretty straight forward. I've never done a PR in this project, any tips @muhlemmer ?
Contribution guidelines are here: https://github.com/zitadel/oidc/blob/main/CONTRIBUTING.md
And another question: To build the scopes I have to do: strings.Join(" ", scopes), nothing fancy.
You can use the oidc.SpaceDelimitedArray type in the struct and that should take care of proper marshalling the field in the JSON response.
I noticed there is another package strings in this project fighting over the same name. Also, I see the only function that this package implements (and tests) is the same functionality that slices.Contains implements, but just for strings. Since the project uses Go +1.21, I would suggest getting rid of the code in favor of using the standard library. Maybe there is another reason to keep it there, is there? This would be outside the scope of the PR, do you usually work this small refactors in other issues?
We are always welcoming other improvements! But please open a separate PR. It is good to change custom functions to newer standard functions, but take care we have a strict semver policy that does not allow us to remove exported functions under pkg. Unfortunately there are many functions which never should have been exported and should have been in internal. There is nothing we can do about that now. Every once in a while we do a major release where we can do such breaking changes, for that you can open a PR to the next branch.
Thanks in advance!