-
-
Notifications
You must be signed in to change notification settings - Fork 4
feat(clients): implement JWT bearer auth #228
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
Conversation
c75860f to
d66a284
Compare
a962e6c to
7d0088d
Compare
d66a284 to
4932181
Compare
7d0088d to
c6fefdd
Compare
c6fefdd to
440353c
Compare
lcian
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, when we roll out the auth we should document that this is always required in saas, we can do so in a follow-up
| base_url: str, | ||
| metrics_backend: MetricsBackend | None = None, | ||
| propagate_traces: bool = False, | ||
| auth_token: str | None = None, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This requires the user to know too much about our auth mechanism and do too much manually. The follow-up PR shows this very well: https://github.com/getsentry/objectstore/pull/231/files#diff-2901f12558868d8df8503eb8c709ebe697eaf471057eabeadede4c292f869fbeR29-R59
Compare with the implementation we had initially for this, which just required to configure a secret, and the client did all the rest.
We should change it like so:
- The client is configured with options it needs to create tokens itself
- (mandatory) The private key. If missing, no tokens will be issued.
- (optional) A set of permissions. Builder defaults this to all permissions.
- (optional) An expiry duration (not timestamp!). Defaults to something reasonably short, like 60s.
- The session is responsible to create the token
- References above options from the client or gets them injected
- Together with the session's usecase and scopes it builds the token
- Applies it to all outgoing requests on the session.
- Caching:
- The token can be reused until it is close to expiry, session then regenerates it on-the-fly
- In practice, this means the session initializes w/o a token and the first request creates it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i still disagree for all the reasons i've argued previously, but i will take your direction
|
replacing with: |
client implementations don't care about token content so these PRs are simple plumbing
i would still like to enable authz enforcement in our client e2e tests but this PR currently does not include that
Ref FS-202