-
-
Notifications
You must be signed in to change notification settings - Fork 7
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
Simplified Upload Tokens for CI Usage #91
Conversation
15e6269
to
8d49e5a
Compare
text/0091-ci-upload-tokens.md
Outdated
The proposed token format is undecided so far. The goals of the token align generally with | ||
both [Macaroons](http:https://macaroons.io/) and [Biscuit](https://www.biscuitsec.org). Unfortunately | ||
the former standard has never seen much attention, and the latter is pretty new and not | ||
particularly proven. Either system however permits adding additional restrictions to the | ||
token which make them a perfect choice for the use in our pipeline. Biscuits however seem | ||
quite promising. The idea of biscuits is that the token itself holds _facts_ which and | ||
can be further constraints via _checks_ and they are checked against a datalog inspired | ||
_policies_. | ||
|
||
One of the benefits of having the tokens carry this data is that the token alone has enough | ||
information available to route to a Sentry installation. This means that `sentry-cli` or | ||
any other tool _just_ needs the token to even determine the host that the token should be | ||
sent against. This benefit also applies to JWT or PASETO tokens which can be considered | ||
for this as well. The RFC here thus proposes two potential options: A **Biscuit** token | ||
format and a **JWT** token format. |
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.
Couldn't the scoping we need be achieved with Paseto or JWT? Those seems like safer bets than biscuit/macroons.
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’m not particularly sold on Biscuit but it’s interesting. Based on my experience with Macaroons with PyPI I don’t think that format has a future. What is nice about Biscuit is that a token can be restricted by the token holder, something that JWT/PASETO cannot do.
text/0091-ci-upload-tokens.md
Outdated
|
||
* `site`: references the target API URL that should be used. A token will always have a | ||
site in it and clients are not supposed to provide a fallback. For instance this | ||
would be `https://myorg.sentry.io/`. |
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.
For hybrid cloud we won't be able to have API endpoints use orgslug.sentry.io
without proxying through control silo. However, we could have structured tokens contain regional domains eg. us.sentry.io
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.
whys this different than org? why do we need this site component?
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.
For hybrid cloud the benefit is that you can (without a preflight) hit the right instance. Particularly for debug file uploads we do not want them to go via the central proxy at all times. For single tenant they all require different URLs anyways and today the docs are quite blind on what you have to reconfigure to point it to the right single tenant instance.
Co-authored-by: Mark Story <[email protected]>
For future readers: it's unlikely we will go down the path of using Biscuits. They are too complex, so don't be put off by the part of that in the RFC. |
@mitsuhiko Might be a dumb question, but can we expose these tokens in the docs UI? It would be amazing if we could just automatically generate a token and put it in the docs link like we do for the user/org |
text/0091-ci-upload-tokens.md
Outdated
* `org`: a token is uniquely bound to an org, so the slug of that org is also always | ||
contained. Note that the slug is used rather than an org ID as the clients typically | ||
need these slugs to create API requests. | ||
* `projects`: a token can be valid for more than one project. For operations such as |
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.
we dont need project for the use case we have - so do we really need that as part of this spec?
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.
Having the project as part of the token would make it possible that just the token is all that is needed to upload debug files or source maps and automatically associate it with the right project without further configuration needed. Otherwise it's yet again an extra piece of information that needs to be configured separately.
A more broad question: why dont we just make an org level token that can be provisioned? why do we need an entire new token format/etc to solve such a simplistic problem?
|
I had similar thoughts. If I'm not mistaken, the underlying problem is that users have a hard time understanding which token to create or use for uploading symbols. We could either solve this by what @dcramer suggested and/or improve the UI. There are several ways how we could solve this. We could add a new some kind of symbols integration. Furthermore, if the user doesn't have the right permissions, we currently show the warning |
To upload correctly the following pieces of information are needed:
By encoding most of this information into the token it means that most configuration is no longer necessary. |
@scefali the goal is for sure that the docs can directly get such a token. |
@scefali think of an experience similar to this |
devtool: "source-map", | ||
plugins: [ | ||
new SentryWebpackPlugin({ | ||
authToken: "AUTO GENERATED TOKEN HERE", |
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.
Should this still be something like process.env.SNTRY_STRUCTURED_TOKEN
for this example? This seems to imply that it is not a secret
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.
@evanpurkhiser the idea is that the docs directly inject the token same as they do for the slugs.
text/0091-ci-upload-tokens.md
Outdated
|
||
We want to encode certain information into the tokens. The following attributes are defined: | ||
|
||
* `sentry_site`: references the target API URL that should be used. A token will always have a |
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.
Maybe specify here that this is options['system.url-prefix']
, for reference?
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.
referencing this comment from @markstory here: getsentry/sentry#50409 (comment)
In the future, organizations will have a couple URLs:
sentryUrl which is our root domain and is the same as system.url-prefix. ex. sentry.io
organizationUrl which is domain for the organization's UI. ex acme.sentry.io
regionUrl Which is the domain that the organization's API endpoints are available on. ex us.sentry.io
Any API requests sent to sentryUrl will be routed to the correct region, but will suffer a latency penalty (because we're proxying the request to the region) and the request will go to the US (this can matter for EU customers).
For this scenario, I think you'll want both the sentryUrl and regionUrl available in the JWT. There are a small number of API endpoints that are only available on sentryUrl (users, integrations, sentry apps).
So based on this, I would say we should add sentry_url
and sentry_region_url
fields instead of sentry_site
?
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.
So based on this, I would say we should add sentry_url and sentry_region_url fields instead of sentry_site?
Yes, having the sentry_url
and sentry_region_url
will work much better in the multi-region future.
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.
why is anything other than the api url needed?
|
||
## Token Issuance | ||
|
||
The purpose of this change is to allow any organization member to issue tokens with little |
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 wonder if we should restrict revocation to managers/owners? 🤔 this has the downside that a user cannot revoke a token they just created, but may be a reasonable safety net (even to disallow accidental deletion)?
text/0091-ci-upload-tokens.md
Outdated
```json | ||
{ | ||
"iss": "sentry.io", | ||
"iat": 1684154626, | ||
"sentry_site": "https://myorg.sentry.io/", | ||
"sentry_org": "myorg" | ||
} |
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.
We should add a nonce
field here so these are less predictable/guessable. A random string will suffice, the longer, the better. With this current format, someone would just need to brute force the iat
to find a value token.
We should also store the tokens as hashed values in the database.
Then when it comes to verifying an inbound request, it's a matter of:
- hash the provided token
- check the database for a hash of equal value
- if one is found, decode the provided token
- perform requested operation (pending authorization)
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.
We would like to at least show the last characters of the token in the UI after creation (e.g. "ends with ABCD"). How important is it that we hash the token in the DB? If it is a definite requirement, then we'll need to store the trailing characters separately as well.
Regards to the none, definitely! Very good point to add to the spec.
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.
Are the last characters to help identify the token? If so, Could we instead expand the model to have a name
or description
or both? Then the user can provide a meaningful value to them.
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.
yes, the idea is to identify the token. We will also have a name
, but since part of this will be about auto-generating tokens from other parts of the UI (e.g. in docs), where we do not want to prompt the user to enter a name, I think there is still value in seeing the last characters of the token in order to identify which of the tokens in the overview is the one I actually ended up using, etc.
So what I would propose is:
- store the token as hash
- store the last 4 characters of the token in plain text in a separate field
- Users can also provide a name (but since we expect to auto-generate the tokens in many cases from other places of the UI, we cannot expect to always have a great/unique name there)
Does that sound OK?
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.
Yeah, that is okay. 👍
Co-authored-by: Mark Story <[email protected]>
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 merged in the changes from #102, which should hopefully address/implement feedback brought up in comments here.
ref #50144 based on RFC getsentry/rfcs#91
This RFC proposes adding org level tokens that contain meta information which simplifies user flows and lets clients forgo of necessary other information.
Rendered RFC
Refs the following issues: