feat(objectstore): enable objectstore auth if key is configured #5525
Conversation
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
| /// The permissions a token signed by this client should have. | ||
| /// | ||
| /// TODO: Expose `Permission` type in `objectstore_client` | ||
| pub permissions: Option<HashSet<String>>, |
There was a problem hiding this comment.
Is there a difference between None and the empty set here? I.e. None means all permissions, empty set means no permissions at all? If not, we could remove the option.
There was a problem hiding this comment.
Come to think of it, should the permissions of this client really be configurable? I assume that Relay would always need the same set of permissions to function correctly, so I would hard-code them instead of putting them in the config.
There was a problem hiding this comment.
None is taken to mean "we don't set it, use the client default" whereas empty set means "create tokens with no permissions"
you are right that relay will generally use a single set of permissions, and if that changes it might be something that is overridden when creating a session. either way i need to expose the Permission type for y'all to reference, but if y'all are fine with hardcoding i can remove this permission config
There was a problem hiding this comment.
Yes I'm fine with removing the config.
| /// The permissions a token signed by this client should have. | ||
| /// | ||
| /// TODO: Expose `Permission` type in `objectstore_client` | ||
| pub permissions: Option<HashSet<String>>, |
There was a problem hiding this comment.
nit: https://develop.sentry.dev/ingestion/relay/relay-best-practices/#data-structures
| pub permissions: Option<HashSet<String>>, | |
| pub permissions: Option<BTreeSet<String>>, |
| if let Some(auth) = auth { | ||
| let secret_key = ObjectstoreKey { | ||
| kid: auth.kid.clone(), | ||
| secret_key: std::fs::read_to_string(auth.key_file.clone())?, |
There was a problem hiding this comment.
Didn't test this but it must be possible to not clone here:
| secret_key: std::fs::read_to_string(auth.key_file.clone())?, | |
| secret_key: std::fs::read_to_string(&auth.key_file)?, |
| /// JWT private key ID that Objectstore must use to load a corresponding public key. | ||
| pub kid: String, | ||
|
|
||
| /// A file where the JWT private key content is located, in EdDSA PEM form. |
There was a problem hiding this comment.
| /// A file where the JWT private key content is located, in EdDSA PEM form. | |
| /// Path of the JWT private key file, in EdDSA PEM form. |
| pub kid: String, | ||
|
|
||
| /// A file where the JWT private key content is located, in EdDSA PEM form. | ||
| pub key_file: String, |
There was a problem hiding this comment.
| pub key_file: String, | |
| pub key_file: PathBuf, |
Dav1dde
left a comment
Dav1dde
left a comment
There was a problem hiding this comment.
14 additional dependencies, but I guess that's not avoidable.
| #[serde(default)] | ||
| pub struct UploadServiceAuthConfig { | ||
| /// JWT private key ID that Objectstore must use to load a corresponding public key. | ||
| pub kid: String, |
There was a problem hiding this comment.
Is kid something objectstore uses a lot? If not for consistency I'd prefer key_id, key_file, key_{...}.
There was a problem hiding this comment.
it's the name of the JWT header field. key_id is probably clearer in code, you're right
|
@matt-codecov With #5531 Relay's config can transparently load values from files, meaning you can just expect the config variable to contain the key as a String. Example relay/tests/integration/test_config.py Lines 55 to 68 in e98cd49 |
objectstore_clientto 0.0.15UploadServiceAuthConfigstruct which can configure Objectstore signing key/token creation optionsTokenGeneratorif config is providedtwo things to note:
Legal Boilerplate
Look, I get it. The entity doing business as "Sentry" was incorporated in the State of Delaware in 2015 as Functional Software, Inc. and is gonna need some rights from me in order to utilize my contributions in this here PR. So here's the deal: I retain all rights, title and interest in and to my contributions, and by keeping this boilerplate intact I confirm that Sentry can use, modify, copy, and redistribute my contributions, under Sentry's choice of terms.