-
Notifications
You must be signed in to change notification settings - Fork 108
feat(objectstore): enable objectstore auth if key is configured #5525
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
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
| @@ -1,4 +1,4 @@ | ||||||
| use std::collections::{BTreeMap, HashMap}; | ||||||
| use std::collections::{BTreeMap, HashMap, HashSet}; | ||||||
| use std::error::Error; | ||||||
| use std::io::Write; | ||||||
| use std::net::{IpAddr, SocketAddr, ToSocketAddrs}; | ||||||
|
|
@@ -1268,6 +1268,25 @@ impl Default for OutcomeAggregatorConfig { | |||||
| } | ||||||
| } | ||||||
|
|
||||||
| /// Configuration values for attachment upload auth. | ||||||
| #[derive(Serialize, Deserialize, Debug, Default)] | ||||||
| #[serde(default)] | ||||||
| pub struct UploadServiceAuthConfig { | ||||||
| /// 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. | ||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
| pub key_file: String, | ||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
Dav1dde marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
|
|
||||||
| /// The number of seconds that an Objectstore JWT should be valid for. | ||||||
| pub expiry_seconds: Option<u64>, | ||||||
|
|
||||||
| /// The permissions a token signed by this client should have. | ||||||
| /// | ||||||
| /// TODO: Expose `Permission` type in `objectstore_client` | ||||||
| pub permissions: Option<HashSet<String>>, | ||||||
matt-codecov marked this conversation as resolved.
Show resolved
Hide resolved
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is there a difference between
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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.
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
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
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes I'm fine with removing the config.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: https://develop.sentry.dev/ingestion/relay/relay-best-practices/#data-structures
Suggested change
|
||||||
| } | ||||||
|
|
||||||
| /// Configuration values for attachment uploads. | ||||||
| #[derive(Serialize, Deserialize, Debug)] | ||||||
| #[serde(default)] | ||||||
|
|
@@ -1283,6 +1302,9 @@ pub struct UploadServiceConfig { | |||||
|
|
||||||
| /// Maximum duration of an attachment upload in seconds. Uploads that take longer are discarded. | ||||||
| pub timeout: u64, | ||||||
|
|
||||||
| /// Configuration values for upload auth. | ||||||
| pub auth: Option<UploadServiceAuthConfig>, | ||||||
| } | ||||||
|
|
||||||
| impl Default for UploadServiceConfig { | ||||||
|
|
@@ -1291,6 +1313,7 @@ impl Default for UploadServiceConfig { | |||||
| objectstore_url: None, | ||||||
| max_concurrent_requests: 10, | ||||||
| timeout: 60, | ||||||
| auth: None, | ||||||
| } | ||||||
| } | ||||||
| } | ||||||
|
|
||||||
Uh oh!
There was an error while loading. Please reload this page.
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.
Is
kidsomething objectstore uses a lot? If not for consistency I'd preferkey_id,key_file,key_{...}.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.
it's the name of the JWT header field. key_id is probably clearer in code, you're right