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

feat: Generate JWT tokens for logged in users, and add middleware to accept them for auth #79489

Closed
wants to merge 2 commits into from

Conversation

ryan953
Copy link
Member

@ryan953 ryan953 commented Oct 22, 2024

This adds a new class, UserJWTToken for minting and checking JWT tokens that relate to a user.

You can mess with JWT tokens at https://jwt.io/ and read more about claims at https://datatracker.ietf.org/doc/html/rfc7519#section-4

The token contains a few claims:

  • exp expiration date, from_request() sets this to 30 days. it's kind of arbitrary right now though for our use-case (see also middleware below)
  • iss issuer, this is set to system.base-hostname and exists to help fingerprint that tokens are coming from our system, specifically which instance of sentry. in prod this will be something like sentry.io
  • iat issued at date, this is for tracking purposes, we record the timestamp the token was issued. could be dropped later.
  • sub the subject, set to the user id who owns the ticket, the token will get access to the system as this user
  • org and proj organization and project id's, these are a convenience, by putting these claims in the token we know some more information about the context from which the token was created.

As with cookies, tokens are minted after the user has already logged in, and they provide continuing access to sentry. so tokens must be protected in transit and at rest on systems. (see the login-success template below).

With the UserJWTToken we've got two callsites:

  1. LoginSuccessView
    This is an essentially static page that is called by the dev toolbar sdk. it's purpose is to shuttle the jwt into the toolbar so that it can be used for api requests. Look at the html template and it's possible to see how we're using window.location.origin to push the token only to a page which is on the same domain as where is hosted.
  2. UserJWTTokenAuthentication middleware
    This is the middleware that checks the token. The token is signed with the same options.get("system.secret-key") that we use for cookies, so provided that the token signature is valid we can trust the claims inside of it. decode_verified_user handles all this; it can emit a number of different types of exceptions (token expired, invalid formats, bad signature, etc) and they all get converted into AuthenticationFailed which is part of our framework.

@ryan953 ryan953 requested review from a team as code owners October 22, 2024 15:22
@ryan953 ryan953 requested a review from BYK October 22, 2024 15:22
@github-actions github-actions bot added the Scope: Backend Automatically applied to PRs that change backend components label Oct 22, 2024
@ryan953 ryan953 requested a review from cmanallen October 22, 2024 16:34
@ryan953 ryan953 marked this pull request as draft October 22, 2024 21:57
@ryan953 ryan953 marked this pull request as ready for review October 23, 2024 18:22
@ryan953 ryan953 requested a review from mdtro October 23, 2024 18:23
@ryan953 ryan953 marked this pull request as draft October 23, 2024 18:27
@mdtro
Copy link
Member

mdtro commented Oct 23, 2024

I'd suggest a few changes to the implementation:

  1. Add a jti claim or user-bound nonce of some sort in the JWT. This will allow us to revoke specific JWTs and/or all of a user's JWTs without having to rotate the signing key.
  2. Add an aud claim. These are typically used by the receiving server to verify the JWT is intended for them.
  3. Use the RS256 asymmetric algorithm for key signing and add a kid (key id) to the JWT. This has a few advantages:
    • the Sentry Dev Toolbar can verify the JWT via the public key from a JWKS url
    • if a key is compromised, we can easily deny all JWTs signed by the particular key while other keys are unaffected
    • key rotation will be easier in the event of a compromised private key
    • key rotation can be automated while allowing JWTs signed with previous keys to still be valid

All that said, key rotation can still be implemented with HS256, so if I need to reduce the scope of my suggested changes -- I'd scrap RS256 and ask for easy key rotation support. :)

@BYK
Copy link
Member

BYK commented Oct 23, 2024

@ryan953 do we still need this after our discussion?

@ryan953
Copy link
Member Author

ryan953 commented Oct 23, 2024

For context! I throbbed the button but have learned new things since typing this all out...

@BYK put together a PoC of using a client app to authenticate with sentry which is really great:

This solves the main problem of "get a secure token to identify the user, that can be revoked, so the user can make api requests from js".

But i'm having problems with it, mainly around user workflows. therefore i want to keep this alive and see if it's workable for a 1st party tool that we're building.
So first, the token is sentry-wide, meaning the user can make requests about any org that they have access to. In the context of the toolbar the user is expected to make requests against a specific org. I don't really have a problem with the scope of the token itself, what I do want though is for the user to have a workflow where they can request an invite to the specific org if they don't already have it.
Also, I'm not excited about asking for specific scopes for the user, ideally the token we get would inherit scopes that the user already has within the org that that the toolbar instance is related to. I could hardcode all the possible scopes grants into the toolbar codebase, which sounds like a less secure idea, or i could track what we're actually using, which sounds like a surprise roadblock for sentry engineers as more things get built up... neither of which sound super fun.
Also, this PoC linked above is like inside someone's user account, so if we wanted to do maintenance on the clientID or something idk how the security team would manage that kind of process. But actually the clientID would need to be unique for each sentry project because the list of redirect uri's is checked during the process... and this has an impact for onboarding. I want to continue having a list of allowed uris, but copy+pasting the clientid is an extra step which a 1st party tool could skip, esp since we know the org/project pair

What I'm thinking about now however is that what i really am focused on is:

  • I need a bearer token that can be used in auth header. i don't have strong opinions on how that is minted, only that using a bearer token helps a lot with making the ajax requests.
  • I don't really want to expose the token to the 3rd party site where it could be exfiltrated. I like the idea of the token living only on a sentry.io domain & I already have code written to proxy ajax requests from a 3rd party side through a page on sentry.io; so we're not saving time anymore by avoiding this type of thing.
  • the existing login flow solves a lot of the problems listed above in relation to: checking that user can access the specific org before a token is minted, once a user is logged in their scopes within the org are available, checks that the 3rd party domain in question is a valid origin for this toolbar app to be running on.

So i was thinking about a jti claim, or actually i was thinking to salt the signing secret, per user, with something that behaves like an entry in the session-cookie database (literally put an entry into that db), so that if that salt is revoked the signature will be invalid and the token can't be used. Adding an aud claim is easy @mdtro , i was kind of treating the iss claim in the same way, but probably iss could have a value like sentry.io and aud a value like acme.sentry.io which helps limit the scope of the token to only the org that's targeted.

Thoughts on that?

BYK
BYK previously requested changes Oct 24, 2024
Copy link
Member

@BYK BYK left a comment

Choose a reason for hiding this comment

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

I'll respond in place with quotes as this is a long text. Might be terse but only due to efficiency:

it's workable for a 1st party tool that we're building.

It is a first-party tool but it is operating on other web sites and as far as I'm aware, not necessarily in a read-only manner. It would probably make sense to treat and develop this as a third-party tool and see the gaps in our integration endpoints and extend them.

So first, the token is sentry-wide, meaning the user can make requests about any org that they have access to. In the context of the toolbar the user is expected to make requests against a specific org.

This was intentional. If we want to keep this per-org then we should be looking into integrations which requires "installing" the integration per project or org. We may find a way to have "integrations enabled by default" to bypass the explicit installation requirement by project owners or use that mechanism as a lead for installing the toolbar for their project. That said I disagree with the notion of per-org or per-project authentication as, as a Sentry end-user I expect to be recognized by Sentry regardless of the project.

I don't really have a problem with the scope of the token itself, what I do want though is for the user to have a workflow where they can request an invite to the specific org if they don't already have it.
Also, I'm not excited about asking for specific scopes for the user, ideally the token we get would inherit scopes that the user already has within the org that that the toolbar instance is related to. I could hardcode all the possible scopes grants into the toolbar codebase, which sounds like a less secure idea, or i could track what we're actually using, which sounds like a surprise roadblock for sentry engineers as more things get built up... neither of which sound super fun.

I'm having a hard time dissecting this bit. As the tool developer, I understand you have a specific model in your head but as an external person and a potential user of the toolbar/spotlight all I care about is "I'm connected with my Sentry account and now Sentry Toolbar/Spotlight knows me on the pages the toolbar applies to". From that, as a user I also expect Sentry Toolbar to have the same access as myself. For better scoping, we can include all scopes from SENTRY_SCOPES or have a "scope set" like we have here:

DEFAULT_SCOPES = ["project:read", "event:read", "team:read", "org:read", "member:read"]

Also, this PoC linked above is like inside someone's user account, so if we wanted to do maintenance on the clientID or something idk how the security team would manage that kind of process.

Having it under my account was the quickest way, hence why it is tied to me. I see no reason for us to not to have a service account where we issue OAuth API Applications from there. This is an easily solvable problem and I'd say it isn't even a problem at this point.

But actually the clientID would need to be unique for each sentry project because the list of redirect uri's is checked during the process... and this has an impact for onboarding. I want to continue having a list of allowed uris, but copy+pasting the clientid is an extra step which a 1st party tool could skip, esp since we know the org/project pair

I strongly disagree with this one. The idea was to have the toolbar or spotlight to have a single entrypoint as the redirect URL inside a popup window where the parent is the page we want to pass the scope to. We can then get the projects' valid URLs list and check that as the origin for the window.parent.postMessage() call and we are good to go. We should, under no circumstances, create a new client id per installation. If we want or need that, that means we go to the "integrations" concept I mentioned above where the installation IDs would serve as this. As I stated earlier, I don't think this is the right approach as it creates extra complexity for no visible gain from my end.

I don't really want to expose the token to the 3rd party site where it could be exfiltrated. I like the idea of the token living only on a sentry.io domain & I already have code written to proxy ajax requests from a 3rd party side through a page on sentry.io; so we're not saving time anymore by avoiding this type of thing.

This is an orthogonal argument as having JWT tokens doesn't change anything in this discussion. Let's discuss this separately.

The rest seems specific to JWT and comments on that so skipping them for now.

@BYK
Copy link
Member

BYK commented Oct 24, 2024

The core of my argument is: adding JWT or a new authentication and authorization mechanism does not necessarily solve our issues but it creates more surface area to cover both in terms of code maintenance and security. So let's make sure we definitely cannot operate with the existing auth mechanisms before creating a new thing.

@ryan953
Copy link
Member Author

ryan953 commented Oct 25, 2024

@BYK We explored oauth approaches and scope changes earlier back in july with docs and code but found the changes introduced more surface area without addressing all the concerns and workflows; meaning we would need even more code and surface area to have something sufficient. These approaches were scrapped (todo: find the notes why) and we looked at problem with fresh eyes.

Today we have an implementation that's workable, and I'm revisiting the problems that a jwt is meant to solve.
In the end I expect what we have today is sufficient, or this PR will be updated with the claims and algorithm changes mentioned above.

@BYK
Copy link
Member

BYK commented Oct 29, 2024

@BYK We explored oauth approaches and scope changes earlier back in july with docs and #74598 but found the changes introduced more surface area without addressing all the concerns and workflows; meaning we would need even more code and surface area to have something sufficient. These approaches were scrapped (todo: find the notes why) and we looked at problem with fresh eyes.

Checked the code and the flow doc. They are very similar to what I had in mind with one big caveat: the PR introduces a whole new concept around org-bound API applications which seems to be the root of the complexity/surface area problem. This issue can very easily be solved with org/project-bound scopes. I've checked the permission code that uses the scopes etc and the code is quite lenient for this kind of modification. So if we decide to go down that path it would be ~50 lines of changes or so instead of inventing a whole new app class and app creation flow. This also means org/project owners don't create apps themselves as we handle this with our Toolbar app through scopes based on where the flow was initiated.

Today we have an implementation that's workable, and I'm revisiting the problems that a jwt is meant to solve.
In the end I expect what we have today is sufficient, or this PR will be updated with the claims and algorithm changes mentioned above.

I agree that it is workable right now but I don't think it is secure, or scalable (in terms of maintenance) so I'm quite certain that we'll need to revisit this, especially in the light of Chrome doubling down on removing localStorage or cookie access on iframes, making the current approach completely nonviable. My stance regarding JWTs not solving these issues succinctly still holds.

@getsantry
Copy link
Contributor

getsantry bot commented Nov 20, 2024

This issue has gone three weeks without activity. In another week, I will close it.

But! If you comment or otherwise update it, I will reset the clock, and if you remove the label Waiting for: Community, I will leave it alone ... forever!


"A weed is but an unloved flower." ― Ella Wheeler Wilcox 🥀

@getsantry getsantry bot added Stale and removed Stale labels Nov 20, 2024
@getsantry
Copy link
Contributor

getsantry bot commented Dec 13, 2024

This issue has gone three weeks without activity. In another week, I will close it.

But! If you comment or otherwise update it, I will reset the clock, and if you remove the label Waiting for: Community, I will leave it alone ... forever!


"A weed is but an unloved flower." ― Ella Wheeler Wilcox 🥀

@getsantry getsantry bot added Stale and removed Stale labels Dec 13, 2024
@getsantry
Copy link
Contributor

getsantry bot commented Jan 5, 2025

This issue has gone three weeks without activity. In another week, I will close it.

But! If you comment or otherwise update it, I will reset the clock, and if you remove the label Waiting for: Community, I will leave it alone ... forever!


"A weed is but an unloved flower." ― Ella Wheeler Wilcox 🥀

@getsantry getsantry bot added Stale and removed Stale labels Jan 5, 2025
@ryan953 ryan953 closed this Jan 7, 2025
@github-actions github-actions bot locked and limited conversation to collaborators Jan 23, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Scope: Backend Automatically applied to PRs that change backend components
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants