Skip to content

add joserfc==1.0.4 #1384

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

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

add joserfc==1.0.4 #1384

wants to merge 2 commits into from

Conversation

matt-codecov
Copy link

https://pypi.org/project/joserfc/
https://jose.authlib.org/en/

this is a relatively active, popular Python library with better JWE support than PyJWT (which has none) or python-jose (in which encryption is completely disconnected from actual JWTs). we are interested in using it to secure and authenticate requests made between Sentry and Codecov

@asottile-sentry
Copy link
Member

shouldn't we use something already in use for api authentication at sentry rather than introducing yet-another-way to do it?

@matt-codecov
Copy link
Author

@asottile-sentry i am also not eager to add a third JWT library to our deps list. JWEs were recommended for this project by @mdtro, but the two JWT libraries we are already using don't have functional JWE implementations, so here we are

taking a step back: we are bridging Codecov features into Sentry. Codecov frequently needs an OAuth token for the user's git provider, and we want a secure way for Sentry to provide that token in each request it makes to Codecov. that way, every Sentry user doesn't have to create a Codecov account, link their GitHub account to their Codecov account, and then link their Sentry account to their Codecov account to get Codecov data to show up in Sentry.

doing that with JWEs is fairly clean, but if you can point us to an approach sentry already uses we can look into it

@asottile-sentry
Copy link
Member

there are many many other auth solutions already in play that are not JWE -- I would prefer we pick any of them just to avoid yet-another

@mdtro is there any particular reason to prefer a new one?

@mdtro
Copy link
Member

mdtro commented Apr 24, 2025

@asottile-sentry -- In this case, a sensitive value + identity information was needed to be passed via the service-to-service authentication. I'm not the biggest fan of JWTs, but using JWE felt like the natural choice to avoid dealing with some bespoke encryption on a specific claim within the JWT.

In terms of what already exists on service-to-service communication, we really only see HMAC-signatures on the payload being used. That doesn't provide the privacy guarantees I'd like to see here.

That said, after thinking over this some more, this feels like a smell. @matt-codecov -- Let's go back to the drawing board on this. We may have other options we should consider.

@matt-codecov
Copy link
Author

matt-codecov commented Apr 29, 2025

sorry for the silence here, some other things came up

@asottile-sentry i haven't worked in sentry's codebase before and don't really have an on-ramp, so it'd be helpful if you could point me towards these other auth systems or relevant teams. otherwise i have to guess and grep for popular libraries

the property we want which led us to JWEs is confidentiality beyond what's provided by HTTPS. Sentry needs to send Codecov the current user's GitHub OAuth token on every request, and we want application-layer encryption because this is happening over public infrastructure and we want to cross our Ts. JWT fits the use case, but PyJWT only supports the JWS part of the JOSE specs which doesn't provide confidentiality. enter joserfc, which supports both JWS and JWE

*python-jose, which Sentry also uses, advertises JWE support, but it only supports "Nested JWTs" which are both signed and encrypted and require different keys for each. and the JWE support is completely disconnected from claims validation. it's not a good option here. also python-jose is pretty dead, is only used in Snuba, and mdtro tried to remove it in getsentry/snuba#6739 because of (now fixed) security issues but it was reverted

from the common user's perspective the difference between JWE and JWS is mostly whether you pass in HS256/RS256 or A256GCM as your algorithm. unless src/sentry/utils/jwt.py leaks PyJWT's exceptions/types, it would be pretty easy to replace PyJWT with joserfc under the hood and get support for JWE without affecting anyone. i could tackle that if it's what we want to do

the real potential problem to me is that joserfc only has one maintainer (which was called out by @mdtro). while it's active today, i don't know whether taking it on as a dependency would create problems long-term.

if we are not okay with taking joserfc as a dependency, our options are probably:

  • give up on additional confidentiality
  • make infra set up peered VPC and DNS backchannels to take the traffic off public infrastructure
  • something bespoke

and we would talk about them elsewhere

@matt-codecov matt-codecov requested a review from a team May 2, 2025 20:57
@matt-codecov
Copy link
Author

matt-codecov commented May 2, 2025

adding @getsentry/owners-python-build since, separate from this JWE discussion, i would like to know whether joserfc would be considered safe to add as a dependency in https://github.com/getsentry/sentry

it is a JWT library that, unlike PyJWT, implements support for the JWE parts of the standard as well. it is currently fairly active and looks robust, but it only has one real maintainer right now. github: https://github.com/authlib/joserfc

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