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

JWT token mananger now returns the hash of the token #1935

Closed
wants to merge 3 commits into from

Conversation

ishank011
Copy link
Contributor

No description provided.

@ishank011 ishank011 requested a review from labkode as a code owner July 28, 2021 14:17
@ishank011 ishank011 marked this pull request as draft July 28, 2021 15:41
@ishank011 ishank011 force-pushed the jwt-token-hash branch 4 times, most recently from 5a03d01 to b1a333e Compare July 28, 2021 17:13
if err != nil {
return nil, nil, err
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// TODO fetch token from .. somewhere ?

This PR would make the tokenmanager only work when all services are running in the same process. Do you plan to make this more flexble? The x-access-token JWT is an internal token that should not leave the boundaries of the microservices. For extarnal access there is the openid connect auth, refresh and id token.

You should never have to encode the x-access-token in a url. We should consider proper API keys for that, IMO.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried with a persistent kv store as well (https://github.com/xujiajun/nutsdb), the problem was that the data wasn't synced until the connection was closed, so I faced the same issue of access from a different process. I'll try to benchmark how much of a drop in performance we face because of opening and closing a new file descriptor every time.

Agreed that the access token shouldn't be present in the URL, currently it's required by wopiserver though. We embed it in an iframe so the user doesn't see it but it can be easily extracted. But the issue is not just that; even adding it as a header might hit the upper limits (in our deployment, my token was 2.9k characters long and the nginx limit is 4-8k)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For external access OpenID Connect should be used. I'm not sure kow the wopiserver works. Could we treat it like an openid connect client? We could generate a secret key for it ...

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Currently, the wopiserver needs a token with full access rights to create/rename/delete files and folders on behalf of the user. If a different openID connect based token could be used, that's fine a priori. But I understand the issue is to carry over wherever needed the Reva token as it can be too large even for headers...

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The internal access token is used to authenticate requests between micreservices and carry the some user data and the roles of a user. In OCIS we do it that way and the token remains small enough to cause problems. The services can then verify the tokens and fetch the permissions that are attached to the roles the user has. The permissions rarely change, so they can be cached. I haven't had the time to fully digest the roles and scopes concept in reva, but if it causes the token to become so large I think we need a different solution. It might depend on what we actually add to the token. maybe using abbreviations in the json keys and values will shrink it to a reasonable size?

Furthermore, I don't think the internal access token should ever leave the boundaries of the microservices. OpenID Connect has an id token that carries the user data, a refresh token that is longer lived and can be used to retrieve a new access token (which should then be a hash, not a jwt). access tokens should never be part of urls as they will be logged in intermediate proxies.

@ishank011
Copy link
Contributor Author

Superseded by #2085

@ishank011 ishank011 closed this Sep 29, 2021
@ishank011 ishank011 deleted the jwt-token-hash branch October 27, 2021 11:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants