feat(upload): Use dedicated secret to sign location URLs

CHANGELOG.md

@@ -26,6 +26,7 @@
 - Prefix upload location query params for forward compatibility. ([#6076](https://github.com/getsentry/relay/pull/6076))
 - Add config option to bypass the kafka fallback for objectstore uploads. ([#6127](https://github.com/getsentry/relay/pull/6127))
 - Use upstream descriptor in upload requests. ([#6128](https://github.com/getsentry/relay/pull/6128))
+- Use dedicated secret to sign upload URLs. ([#6132](https://github.com/getsentry/relay/pull/6132))
 
 ## 26.6.0
 

relay-config/src/config.rs

@@ -281,10 +281,10 @@ impl Credentials {
     /// Generates new random credentials.
     pub fn generate() -> Self {
         relay_log::info!("generating new relay credentials");
-        let (sk, pk) = generate_key_pair();
+        let (secret_key, public_key) = generate_key_pair();
         Self {
-            secret_key: sk,
-            public_key: pk,
+            secret_key,
+            public_key,
             id: generate_relay_id(),
         }
     }
@@ -1635,7 +1635,7 @@ impl Default for Cogs {
 }
 
 /// Configuration for the upload service.
-#[derive(Debug, Clone, Copy, Serialize, Deserialize)]
+#[derive(Debug, Clone, Serialize, Deserialize)]
 #[serde(default)]
 pub struct Upload {
     /// Maximum number of uploads that the service accepts.
@@ -1648,6 +1648,11 @@ pub struct Upload {
     ///
     /// In seconds.
     pub max_age: i64,
+
+    /// Credentials used for signing & verifying upload locations.
+    ///
+    /// If omitted, relay's default [`Credentials`] are used.
+    pub credentials: Option<UploadCredentials>,
 }
 
 impl Default for Upload {
@@ -1656,10 +1661,36 @@ impl Default for Upload {
             max_concurrent_requests: 100,
             timeout: 5 * 60,  // five minutes
             max_age: 60 * 60, // 1h
+            credentials: None,
         }
     }
 }
 
+/// Credentials used for signing & verifying upload locations.
+#[derive(Clone, Serialize, Deserialize)]
+pub struct UploadCredentials {
+    /// Key used to sign upload locations.
+    #[cfg(feature = "processing")]
+    pub signing_key: SecretKey,
+
+    /// Key used to verify upload locations.
+    pub verification_key: PublicKey,
+}
+
+impl fmt::Debug for UploadCredentials {
+    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
+        let Self {
+            #[cfg(feature = "processing")]
+                signing_key: _,
+            verification_key,
+        } = self;
+        let mut b = f.debug_struct("UploadCredentials");
+        #[cfg(feature = "processing")]
+        b.field("signing_key", &"[redacted]");
+        b.field("verification_key", verification_key).finish()
+    }
+}
+
 /// All configuration values that can be deserialized from `config.yml`.
 #[derive(Serialize, Deserialize, Debug, Default)]
 #[serde(default)]
@@ -2599,6 +2630,16 @@ impl Config {
         &self.values.upload
     }
 
+    /// Returns the key used to sign upload locations.
+    #[cfg(feature = "processing")]
+    pub fn upload_signing_key(&self) -> Option<&SecretKey> {
+        self.upload()
+            .credentials
+            .as_ref()
+            .map(|c| &c.signing_key)
+            .or(self.credentials().map(|c| &c.secret_key))
+    }
+
     /// Redis servers to connect to for project configs, cardinality limits,
     /// rate limiting, and metrics metadata.
     pub fn redis(&self) -> Option<RedisConfigsRef<'_>> {
@@ -2737,7 +2778,6 @@ impl Default for Config {
 
 #[cfg(test)]
 mod tests {
-
     use super::*;
 
     /// Regression test for renaming the envelope buffer flags.
@@ -2754,6 +2794,37 @@ cache:
         assert_eq!(values.cache.envelope_expiry, 1800);
     }
 
+    #[cfg(feature = "processing")]
+    #[test]
+    fn test_upload_secret_key_from_file() {
+        let path = env::temp_dir().join(Uuid::new_v4().to_string());
+        fs::create_dir(&path).unwrap();
+        fs::write(
+            path.join("my_secret.txt"),
+            "U3LSQM5NorvgnoYHW_aZpc_43nuuh3lhs3zjjcBwaks",
+        )
+        .unwrap();
+        fs::write(
+            ConfigValues::path(&path),
+            r#"
+upload:
+    credentials:
+        signing_key: ${file:my_secret.txt}
+        verification_key: "VNS8haF0VTnuMMDR2t-f7AgnmUcXmcdzV3SVksSk34s""#,
+        )
+        .unwrap();
+
+        let config = Config::from_path(&path).unwrap();
+
+        fs::remove_dir_all(path).unwrap();
+
+        let signing_key = &config.upload().credentials.as_ref().unwrap().signing_key;
+        assert_eq!(
+            signing_key.to_string(),
+            "U3LSQM5NorvgnoYHW_aZpc_43nuuh3lhs3zjjcBwaks"
+        );
+    }
+
     #[test]
     fn test_emit_outcomes() {
         for (serialized, deserialized) in &[

relay-server/src/services/upload.rs

@@ -456,17 +456,14 @@ impl<L: UploadLength> Location<L> {
     #[cfg(feature = "processing")]
     fn try_sign(self, config: &Config) -> Result<SignedLocation<L>, Error> {
         let uri = self.try_to_uri()?;
-        let signature = config
-            .credentials()
-            .ok_or(Error::SigningFailed)?
-            .secret_key
-            .sign_with_header(
-                uri.as_bytes(),
-                &SignatureHeader {
-                    timestamp: Utc::now(),
-                    signature_algorithm: None,
-                },
-            );
+        let secret_key = config.upload_signing_key().ok_or(Error::SigningFailed)?;
+        let signature = secret_key.sign_with_header(
+            uri.as_bytes(),
+            &SignatureHeader {
+                timestamp: Utc::now(),
+                signature_algorithm: None,
+            },
+        );
 
         Ok(SignedLocation {
             location: self,
@@ -585,14 +582,33 @@ impl<L: UploadLength> SignedLocation<L> {
     /// Fails if the signature is outdated or incorrect.
     #[cfg(feature = "processing")]
     pub fn verify(self, received: DateTime<Utc>, config: &Config) -> Result<Location<L>, Error> {
-        let public_key = config.public_key().ok_or(Error::SigningFailed)?;
+        let mut result = Err(SignatureError::Unverifiable);
+        let location = self.location.try_to_uri()?;
+        let max_age = chrono::Duration::seconds(config.upload().max_age);
+
+        if let Some(public_key) = &config
+            .upload()
+            .credentials
+            .as_ref()
+            .map(|c| &c.verification_key)
+        {
+            result = self
+                .signature
+                .verify(location.as_bytes(), public_key, received, max_age);
+        }
 
-        self.signature.verify(
-            self.location.try_to_uri()?.as_bytes(),
-            public_key,
-            received,
-            chrono::Duration::seconds(config.upload().max_age),
-        )?;
+        // For the transition phase, check the general purpose signature even when there is a
+        // special-purpose upload key, because the URL may have been signed by an old instance.
+        // This can be simplified after the rollout.
+        if matches!(&result, Err(SignatureError::Unverifiable))
+            && let Some(public_key) = config.credentials().map(|c| &c.public_key)
+        {
+            result = self
+                .signature
+                .verify(location.as_bytes(), public_key, received, max_age);
+        }
+
+        result?;
 
         Ok(self.location)
     }
@@ -839,6 +855,109 @@ where
 mod tests {
     use super::*;
 
+    #[cfg(feature = "processing")]
+    mod with_processing {
+        use chrono::Utc;
+        use relay_auth::SignatureError;
+        use relay_base_schema::project::ProjectId;
+        use relay_config::{Config, Credentials, OverridableConfig, UploadCredentials};
+
+        use super::*;
+
+        fn location() -> Location<Provisional> {
+            Location {
+                project_id: ProjectId::new(42),
+                key: "upload-key".to_owned(),
+                length: Provisional(Some(123)),
+                other: UploadParams::default(),
+            }
+        }
+
+        fn config(
+            relay_credentials: Credentials,
+            credentials: Option<UploadCredentials>,
+        ) -> Config {
+            let mut config = Config::from_json_value(serde_json::json!({
+                "upload": {
+                    "credentials": credentials,
+                },
+            }))
+            .unwrap();
+            config
+                .apply_override(OverridableConfig {
+                    id: Some(relay_credentials.id.to_string()),
+                    secret_key: Some(relay_credentials.secret_key.to_string()),
+                    public_key: Some(relay_credentials.public_key.to_string()),
+                    ..Default::default()
+                })
+                .unwrap();
+            config
+        }
+
+        #[test]
+        fn verify_legacy() {
+            let relay_credentials = Credentials::generate();
+            let (signing_key, verification_key) = relay_auth::generate_key_pair();
+
+            let signing_config = config(relay_credentials.clone(), None);
+            let verification_config = config(
+                relay_credentials,
+                Some(UploadCredentials {
+                    signing_key,
+                    verification_key,
+                }),
+            );
+
+            let signed_location = location().try_sign(&signing_config).unwrap();
+
+            assert!(
+                signed_location
+                    .verify(Utc::now(), &verification_config)
+                    .is_ok()
+            );
+        }
+
+        #[test]
+        fn verify_new() {
+            let relay_credentials = Credentials::generate();
+            let (signing_key, verification_key) = relay_auth::generate_key_pair();
+            let config = config(
+                relay_credentials,
+                Some(UploadCredentials {
+                    signing_key,
+                    verification_key,
+                }),
+            );
+
+            let signed_location = location().try_sign(&config).unwrap();
+
+            assert!(signed_location.verify(Utc::now(), &config).is_ok());
+        }
+
+        #[test]
+        fn verify_inverse() {
+            let relay_credentials = Credentials::generate();
+            let (signing_key, verification_key) = relay_auth::generate_key_pair();
+
+            let signing_config = config(
+                relay_credentials.clone(),
+                Some(UploadCredentials {
+                    signing_key,
+                    verification_key,
+                }),
+            );
+
+            let verification_config = config(relay_credentials, None);
+
+            let signed_location = location().try_sign(&signing_config).unwrap();
+
+            assert!(matches!(
+                signed_location.verify(Utc::now(), &verification_config),
+                Err(Error::InvalidSignature(SignatureError::Unverifiable))
+            ));
+        }
+    }
+
     #[test]
     fn parse_location_incomplete() {
         let url = "signature=foo";