diff --git a/CHANGELOG.md b/CHANGELOG.md index a6046b2dc73..fe238ce3465 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -19,6 +19,7 @@ - Restore top-level _performance_issues_spans. ([#6045](https://github.com/getsentry/relay/pull/6045)) - Update sentry-conventions to 0.11.0, migrating deprecated `gen_ai` attribute constants. ([#6068](https://github.com/getsentry/relay/pull/6068)) +- Prefix upload location query params for forward compatibility. ([#6076](https://github.com/getsentry/relay/pull/6076)) ## 26.5.2 diff --git a/relay-server/src/endpoints/upload.rs b/relay-server/src/endpoints/upload.rs index 8e3640d9438..22330d53c1f 100644 --- a/relay-server/src/endpoints/upload.rs +++ b/relay-server/src/endpoints/upload.rs @@ -102,6 +102,7 @@ impl IntoResponse for Error { upload::Error::InvalidLocation(_) | upload::Error::SigningFailed => { StatusCode::INTERNAL_SERVER_ERROR } + upload::Error::SerializeFailed(_) => StatusCode::INTERNAL_SERVER_ERROR, upload::Error::InvalidSignature => StatusCode::BAD_REQUEST, upload::Error::ObjectstoreServiceUnavailable(_) => StatusCode::SERVICE_UNAVAILABLE, #[cfg(feature = "processing")] @@ -200,7 +201,11 @@ async fn handle_patch( meta: RequestMeta, headers: HeaderMap, Path(upload::LocationPath { project_id, key }): Path, - Query(LocationQueryParams { length, signature }): Query>, + Query(LocationQueryParams { + upload_length, + upload_signature, + other, + }): Query>, body: Body, ) -> axum::response::Result { check_kill_switch(&state)?; @@ -208,7 +213,8 @@ async fn handle_patch( relay_log::trace!("Validating headers"); tus::validate_patch_headers(&headers).map_err(Error::from)?; - let location = SignedLocation::from_parts(project_id, key, length, signature); + let location = + SignedLocation::from_parts(project_id, key, upload_length, upload_signature, other); let config = state.config(); @@ -233,7 +239,7 @@ async fn handle_patch( .boxed(); let stream = MeteredStream::new(stream, "upload"); - let (lower_bound, upper_bound) = match length.value() { + let (lower_bound, upper_bound) = match upload_length.value() { None => (1, config.max_upload_size()), Some(u) => (u, u), }; diff --git a/relay-server/src/services/upload.rs b/relay-server/src/services/upload.rs index 0b39761cdaf..d915c151482 100644 --- a/relay-server/src/services/upload.rs +++ b/relay-server/src/services/upload.rs @@ -1,5 +1,6 @@ //! Utilities for uploading large files. +use std::collections::BTreeMap; use std::fmt; use std::sync::Arc; use std::time::Duration; @@ -22,7 +23,7 @@ use relay_system::{ Addr, AsyncResponse, ConcurrentService, FromMessage, Interface, LoadShed, SendError, Sender, SimpleService, }; -use serde::Deserialize; +use serde::{Deserialize, Serialize}; use tokio::io::BufReader; use tokio::sync::oneshot; use tokio::sync::oneshot::error::RecvError; @@ -58,6 +59,8 @@ pub enum Error { Upstream(#[source] reqwest::Error), #[error("upstream provided invalid location: {0:?}")] InvalidLocation(Option), + #[error("serializing location failed: {0}")] + SerializeFailed(#[from] serde_urlencoded::ser::Error), #[error("failed to sign location")] SigningFailed, #[error("invalid signature")] @@ -82,6 +85,7 @@ impl Error { Error::Upstream(_) => "upstream_response", Error::InvalidLocation(_) => "invalid_location", Error::SigningFailed => "signing_failed", + Error::SerializeFailed(_) => "serialize_failed", Error::InvalidSignature => "invalid_signature", Error::ObjectstoreServiceUnavailable(_) => "service_unavailable", #[cfg(feature = "processing")] @@ -265,6 +269,7 @@ impl Service { project_id: scoping.project_id, key, length: Provisional(length), + other: Default::default(), } .try_sign(config) } @@ -291,6 +296,7 @@ impl Service { project_id, key, length, + other, } = location.verify(received, config)?; debug_assert_eq!(scoping.project_id, project_id); @@ -313,6 +319,7 @@ impl Service { project_id, key, length, + other, } .try_sign(config) } @@ -396,7 +403,7 @@ impl UploadLength for Final { /// The location can be converted into a URI to be put in the `Location` HTTP header /// used by the TUS protocol. /// -/// Calling [`Self::try_sign`] appends a `&signature=` query parameter that can later be used +/// Calling [`Self::try_sign`] appends an `&upload_signature=` query parameter that can later be used /// to validate whether the URI (especially the length) has been tempered with. #[derive(Debug)] pub struct Location { @@ -406,24 +413,60 @@ pub struct Location { pub key: String, /// Value of the `Upload-Length` header. `None` if `Upload-Defer-Length: 1`. pub length: L, + pub other: UploadParams, } impl Location { - fn as_uri(&self) -> String { + fn try_to_uri(&self) -> Result { let Location { project_id, key, length, + other, } = self; - match length.value() { - Some(length) => format!("/api/{project_id}/upload/{key}/?length={length}"), - None => format!("/api/{project_id}/upload/{key}/"), + #[derive(Debug, Serialize)] + struct QueryParams<'a> { + pub upload_length: Option, + #[serde(flatten)] + pub other: &'a UploadParams, + } + let params = QueryParams { + upload_length: length.value(), + other, + }; + let query = serde_urlencoded::to_string(params)?; + match query.as_str() { + "" => Ok(format!("/api/{project_id}/upload/{key}/")), + _ => Ok(format!("/api/{project_id}/upload/{key}/?{query}")), + } + } + + /// Temporary function used to verify legacy signatures. + #[cfg(feature = "processing")] + fn try_to_legacy_uri(&self) -> Result { + let Location { + project_id, + key, + length, + other: _, + } = self; + #[derive(Debug, Serialize)] + struct QueryParams { + pub length: Option, + } + let params = QueryParams { + length: length.value(), + }; + let query = serde_urlencoded::to_string(params)?; + match query.as_str() { + "" => Ok(format!("/api/{project_id}/upload/{key}/")), + _ => Ok(format!("/api/{project_id}/upload/{key}/?{query}")), } } #[cfg(feature = "processing")] fn try_sign(self, config: &Config) -> Result, Error> { - let uri = self.as_uri(); + let uri = self.try_to_uri()?; let signature = config .credentials() .ok_or(Error::SigningFailed)? @@ -454,8 +497,51 @@ pub struct LocationPath { #[derive(Debug, Deserialize)] #[serde(bound = "L: UploadLength")] pub struct LocationQueryParams { - pub length: L, - pub signature: String, + #[serde(alias = "length")] + pub upload_length: L, + #[serde(alias = "signature")] + pub upload_signature: String, + #[serde(flatten)] + pub other: UploadParams, +} + +#[derive(Debug, Default, Serialize)] +pub struct UploadParams(BTreeMap); + +impl<'de> Deserialize<'de> for UploadParams { + fn deserialize(deserializer: D) -> Result + where + D: serde::Deserializer<'de>, + { + struct UploadParamsVisitor; + + impl<'de> serde::de::Visitor<'de> for UploadParamsVisitor { + type Value = UploadParams; + + fn expecting(&self, formatter: &mut fmt::Formatter<'_>) -> fmt::Result { + formatter.write_str("upload query parameters") + } + + fn visit_map(self, mut map: A) -> Result + where + A: serde::de::MapAccess<'de>, + { + let mut upload_params = BTreeMap::new(); + + while let Some(key) = map.next_key::<&str>()? { + if key.starts_with("upload_") { + upload_params.insert(key.to_owned(), map.next_value()?); + } else { + map.next_value::()?; + } + } + + Ok(UploadParams(upload_params)) + } + } + + deserializer.deserialize_map(UploadParamsVisitor) + } } /// A verifiable [`Location`] signed by this Relay or an upstream Relay. @@ -469,13 +555,19 @@ impl SignedLocation { /// Creates an unverified location from path and query params. /// /// Call `verify` to make sure the signature is correct. - pub fn from_parts(project_id: ProjectId, key: String, length: L, signature: String) -> Self { - // TODO: forward compat: allow other query params? + pub fn from_parts( + project_id: ProjectId, + key: String, + length: L, + signature: String, + other: UploadParams, + ) -> Self { Self { location: Location { project_id, key, length, + other, }, signature: Signature(signature), } @@ -483,23 +575,20 @@ impl SignedLocation { /// Converts the location into an URI for future reference. pub fn into_header_value(self) -> Result { - HeaderValue::from_str(&self.as_uri()).map_err(Error::Internal) + HeaderValue::from_str(&self.try_to_uri()?).map_err(Error::Internal) } - fn as_uri(&self) -> String { + fn try_to_uri(&self) -> Result { let Self { location, signature, } = self; - let mut uri = location.as_uri(); - uri.push(if location.length.value().is_some() { - '&' - } else { - '?' - }); // TODO: brittle. - uri.push_str("signature="); + + let mut uri = location.try_to_uri()?; + uri.push(if uri.ends_with('/') { '?' } else { '&' }); + uri.push_str("upload_signature="); uri.push_str(&signature.to_string()); - uri + Ok(uri) } /// Converts the signed location into a location object. @@ -508,12 +597,24 @@ impl SignedLocation { #[cfg(feature = "processing")] pub fn verify(self, received: DateTime, config: &Config) -> Result, Error> { let public_key = config.public_key().ok_or(Error::SigningFailed)?; - let is_valid = self.signature.verify( - self.location.as_uri().as_bytes(), + + let mut is_valid = self.signature.verify( + self.location.try_to_uri()?.as_bytes(), public_key, received, chrono::Duration::seconds(config.upload().max_age), ); + + // NOTE: This code path can be removed once all legacy URIs are invalid (~1h after rollout) + if !is_valid { + is_valid = self.signature.verify( + self.location.try_to_legacy_uri()?.as_bytes(), + public_key, + received, + chrono::Duration::seconds(config.upload().max_age), + ) + } + match is_valid { true => Ok(self.location), false => Err(Error::InvalidSignature), @@ -559,9 +660,19 @@ where }; // Parse query parameters. - let LocationQueryParams { length, signature } = serde_urlencoded::from_str(query).ok()?; + let LocationQueryParams { + upload_length, + upload_signature, + other, + } = serde_urlencoded::from_str(query).ok()?; - Some(Self::from_parts(project_id, key, length, signature)) + Some(Self::from_parts( + project_id, + key, + upload_length, + upload_signature, + other, + )) } } @@ -657,7 +768,9 @@ impl UpstreamRequest for UploadRequest { let project_id = self.scoping.project_id; match &self.kind { RequestKind::Create { .. } => format!("/api/{project_id}/upload/"), - RequestKind::Upload { location, .. } => location.as_uri(), + RequestKind::Upload { location, .. } => location + .try_to_uri() + .expect("upload location should be serializable"), } .into() } @@ -756,7 +869,7 @@ mod tests { // Can only parse provisional: let provisional: LocationQueryParams = serde_urlencoded::from_str(url).unwrap(); - assert!(provisional.length.0.is_none()); + assert!(provisional.upload_length.0.is_none()); assert!(serde_urlencoded::from_str::>(url).is_err()); } @@ -764,11 +877,70 @@ mod tests { fn parse_location_complete() { let json = r#"signature=foo&length=123"#; - // Can only parse provisional: let provisional: LocationQueryParams = serde_urlencoded::from_str(json).unwrap(); - assert_eq!(provisional.length.0, Some(123)); + assert_eq!(provisional.upload_length.0, Some(123)); + let full: LocationQueryParams = serde_urlencoded::from_str(json).unwrap(); + assert_eq!(full.upload_length.0, 123); + } + + #[test] + fn parse_location_with_other() { + let json = + r#"upload_signature=foo&upload_length=123¬_an_upload_param=123&upload_type=bar"#; + + let provisional: LocationQueryParams = + serde_urlencoded::from_str(json).unwrap(); + insta::assert_debug_snapshot!(provisional, @r#" + LocationQueryParams { + upload_length: Provisional( + Some( + 123, + ), + ), + upload_signature: "foo", + other: UploadParams( + { + "upload_type": "bar", + }, + ), + } + "#); let full: LocationQueryParams = serde_urlencoded::from_str(json).unwrap(); - assert_eq!(full.length.0, 123); + insta::assert_debug_snapshot!(full, @r#" + LocationQueryParams { + upload_length: Final( + 123, + ), + upload_signature: "foo", + other: UploadParams( + { + "upload_type": "bar", + }, + ), + } + "#); + } + + #[cfg(feature = "processing")] + #[test] + fn verify_rejects_location_signed_with_legacy_length_param() { + let mut config = Config::default(); + config.regenerate_credentials(false).unwrap(); + + let legacy_uri = "/api/42/upload/my_objectstore_key/?length=123"; + let signature = config.credentials().unwrap().secret_key.sign_with_header( + legacy_uri.as_bytes(), + &SignatureHeader { + timestamp: Some(Utc::now()), + signature_algorithm: None, + }, + ); + let signed_location = SignedLocation::::try_from_str(&format!( + "{legacy_uri}&upload_signature={signature}" + )) + .unwrap(); + + assert!(signed_location.verify(Utc::now(), &config).is_ok()); } } diff --git a/tests/integration/consts.py b/tests/integration/consts.py index 5846cbb5081..91d60f244da 100644 --- a/tests/integration/consts.py +++ b/tests/integration/consts.py @@ -2,6 +2,6 @@ METRICS_EXTRACTION_MIN_SUPPORTED_VERSION = 4 DUMMY_UPLOAD_PATH = "/api/42/upload/019cdc82ed6c7761ba21fd34b86481c2/" -DUMMY_UPLOAD_LOCATION = f"{DUMMY_UPLOAD_PATH}?length=11&signature=z_fUMhT0EZqJz6OQtwGHqTlOOLPpTVpvPa-rYTg18FVWZM1OGny-LeVJB5H-sSR_5e--I1xt-FlCmRG2bsmcAQ.eyJ0IjoiMjAyNi0wMy0xMVQxMDo0ODoxMy45NDM1ODNaIn0" +DUMMY_UPLOAD_LOCATION = f"{DUMMY_UPLOAD_PATH}?upload_length=11&upload_signature=z_fUMhT0EZqJz6OQtwGHqTlOOLPpTVpvPa-rYTg18FVWZM1OGny-LeVJB5H-sSR_5e--I1xt-FlCmRG2bsmcAQ.eyJ0IjoiMjAyNi0wMy0xMVQxMDo0ODoxMy45NDM1ODNaIn0" ZSTD_MAGIC_HEADER = b"\x28\xb5\x2f\xfd"