Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bearer token prefix may lead to issues with JWT tooling #338

Open
aljacob opened this issue Oct 21, 2020 · 9 comments
Open

Bearer token prefix may lead to issues with JWT tooling #338

aljacob opened this issue Oct 21, 2020 · 9 comments
Labels
breaking Breaking changes, requires a major-version (2.0.0 for example) user management incl. authorization, account management and billing
Milestone

Comments

@aljacob
Copy link
Member

aljacob commented Oct 21, 2020

Today while implementing authorization I realized that our way of defining the token we send as access token is not JWT conform and can create all kinds of issues when relying on external libraries or frameworks for parsing the token.
Can we remove the /oidc/provider/ before the token, so that we just send Bearer token instead of Bearer /oidc/provider/token. If we need for some reason the information on which auth method and which provider has been chosen I would rather just add this as additional custom header fields instead of encoding them in the Bearer String.

@aljacob aljacob added the user management incl. authorization, account management and billing label Oct 21, 2020
@m-mohr
Copy link
Member

m-mohr commented Oct 22, 2020

The Bearer Token has the actual token after the prefix. The API doesn't require JWT to be used due to different authentication procedures, which may not even allow JWT.

See also the API description for the Bearer Token format, which was discussed in a broader round of OIDC back-ends (I think including you):

The Bearer Token MUST consist of the authentication method, a provider ID (if available) and the token itself. All separated by a forward slash /. Examples (replace TOKEN with the actual access token): (1) Basic authentication (no provider ID available): basic//TOKEN (2) OpenID Connect (provider ID is ms): oidc/ms/TOKEN. For OpenID Connect, the provider ID corresponds to the value specified for id for each provider in GET /credentials/oidc.

The TOKEN above is your JWT, so you need to split the Bearer token at the / char and the third part is your JWT you can then parse.

We can't change this easily, it would be a breaking change. Also, we need to know the auth procedure and the provider, otherwise we can't make sense of the Token. Closing for now as I hope this helps you to solve the issue and I don't see anything we can do directly.

@m-mohr m-mohr closed this as completed Oct 22, 2020
@soxofaan
Copy link
Member

I agree with @m-mohr , the bearer token that is sent from openEO client ot openEO backend should be handled as a opaque blob. I don't think there is a standard that requires it to be a JWT token.

Also, I noticed lately that JWT gets some bad press from time to time, so we shouldn't bet too big on it. It is not unlikely that some providers switch to another solution in the future.

@aljacob
Copy link
Member Author

aljacob commented Oct 28, 2020

maybe I expressed myself poorly. The token must not be expected to be a JWT token in general. I agree with this. But if somebody relies on tools that are using JWT tokens, then the validators in those tools will fail by default with the current definition, as you are basically manipulating the token that has been send to you, so when you send it back for validation to the backend from which you got it, it does not get the same thing as it expects and hence validation fails.

@aljacob aljacob reopened this Oct 28, 2020
@m-mohr
Copy link
Member

m-mohr commented Oct 28, 2020

No, I understood the issue, but what do you expect us to do now? If you want a change here you need to write a proposal to the PSC and wait for API 2.0 as a change would be breaking. I still think you can solve this in implementation though.

@m-mohr m-mohr added the breaking Breaking changes, requires a major-version (2.0.0 for example) label Oct 28, 2020
@soxofaan
Copy link
Member

you are basically manipulating the token that has been send to you, so when you send it back for validation to the backend from which you got it, it does not get the same thing as it expects and hence validation fails.

This is not completely correct I think. There are in general 3 parties in this dance: the OIDC provider, the client and the backend. The OIDC provider sends an access token to the client, the client indeed manipulates this by adding a prefix (which creates the bearer token) and sends it to the backend, and the backend strips the prefix again to get the access token to communicate with the OIDC provider (to get user info). So it is only between the client and backend (which both are fully openEO API aware) that we use this custom bearer token format. Interaction (from client or backend) to other parties should use the original access token.

But maybe I'm still misunderstanding the issue. Do you have a concrete example where it goes wrong @aljacob ?

@lforesta
Copy link

The backend always needs to have the following check for each request sent with a token:

  • is it basic/oidc?
  • if oidc, which provider is it?
    This information can be in the token (like it is now), or in the request's header, but we still need to have a small wrapper around every request sent to the backend, no?

Changing this info to be in the header instead in the token itself seems more work than it's worth? (given that we would need to go through a proposal to the PSC and then all backends/clients would need to update this part)

@m-mohr m-mohr changed the title Bearer token not JWT conform Bearer token prefix may lead to issues with JWT tooling Oct 28, 2020
@m-mohr m-mohr added this to the future milestone Oct 29, 2020
@m-mohr
Copy link
Member

m-mohr commented Oct 29, 2020

So after the discussion today, I'd propose to re-consider this whenever the next breaking version will be released (i.e. API 2.0, milestone 'future') and then discuss the switch to additional HTTP headers. Until then workaround need to be implemented.

I'd guess that "normal" tooling still has issues with additional headers as with multiple IPs and auth methods you still have to have a step in-between to not mix up tokens from different IPs etc (as Luca also points out). You don't want to send a Microsoft token to Google for validation, but tooling might not expect different IPs involved. Then there would be no real gain compared to the current specification, but that likely depends on the actual libraries used and should be checked with a couple of popular libraries once this comes up again.

@soxofaan
Copy link
Member

I'd guess that "normal" tooling still has issues with additional headers

Indeed, any solution or alternative will probably cause some issue somewhere because this multi-provider feature is non-standard in the first place I guess

@aljacob
Copy link
Member Author

aljacob commented Oct 29, 2020

Additional headers, won't be an issue. The toolchains that I have looked at, just evaluate the headers interesting for them. So other "unknown" headers will just be ignored. The authorization header will only be evaluated once by the corresponding tool chain. If you have additional headers for selecting the right auth chain, you can just filter based on their presence and select the right security protocol and provider. This way especially for the more simple case of having just an individual identity provider you don't need to change anything.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking Breaking changes, requires a major-version (2.0.0 for example) user management incl. authorization, account management and billing
Projects
None yet
Development

No branches or pull requests

4 participants