Skip to content

ingest-router: Verify incoming project configs requests #138

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.

Sign up for GitHub

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

Merged
lynnagara merged 4 commits into from
Jun 18, 2026
Merged

ingest-router: Verify incoming project configs requests #138

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
4db31aa
ingest-router: Verify incoming project configs requests
lynnagara Jun 18, 2026
5ec417f
.
lynnagara Jun 18, 2026
ed4db72
.
lynnagara Jun 18, 2026
5f35c5b
.
lynnagara Jun 18, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion example_config_ingest_router.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ ingest_router:
port: 3001


# relay_keys:
relay_keys:
# Verified downstream Relays (POPs) can be configured here, as a map of relay id to relay
# info.
# The relay id (the map key) must be a valid UUIDv4 string. This is the same ID that the
Expand Down
4 changes: 4 additions & 0 deletions ingest-router/src/api/project_config.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -402,6 +402,10 @@ impl Handler for ProjectConfigsHandler {
fn execution_mode(&self) -> ExecutionMode {
ExecutionMode::Parallel
}

fn requires_relay_auth(&self) -> bool {
true
}
async fn split_request(
&self,
request: Request<Bytes>,
Expand Down
11 changes: 7 additions & 4 deletions ingest-router/src/auth.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ use hyper::header::{HeaderMap, HeaderName, HeaderValue};
use serde::{Deserialize, Serialize};
use std::collections::HashMap;
use std::path::Path;
use std::sync::Arc;

/// Signature freshness window, matching Sentry
/// https://github.com/getsentry/sentry/blob/c9138b328e9aad58f95f087c0f8a8843a06dbbe9/src/sentry/api/authentication.py#L260
Expand Down Expand Up @@ -184,7 +185,7 @@ pub enum VerifyError {

/// Configuration for a single trusted downstream relay, matching the upstream's
/// `static_relays` entry shape.
#[derive(Debug, Clone, Deserialize)]
#[derive(Debug, Clone, Deserialize, PartialEq)]
pub struct RelayInfo {
/// base64url-nopad encoding of the relay's 32-byte ed25519 public key.
pub public_key: String,
Expand All @@ -202,7 +203,7 @@ pub struct RelayInfo {
#[derive(Clone, Default)]
pub struct RelayVerifier {
/// Trusted downstream relays, keyed by relay id (a UUID).
trusted_relays: HashMap<String, VerifyingKey>,
trusted_relays: Arc<HashMap<String, VerifyingKey>>,
}

impl RelayVerifier {
Expand All @@ -212,8 +213,10 @@ impl RelayVerifier {
let trusted_relays = relays
.into_iter()
.map(|(id, info)| Ok((id.clone(), parse_public_key(&info.public_key, &id)?)))
.collect::<Result<_, VerifyError>>()?;
Ok(Self { trusted_relays })
.collect::<Result<HashMap<_, _>, VerifyError>>()?;
Ok(Self {
trusted_relays: Arc::new(trusted_relays),
})
}

/// Verifies the `X-Sentry-Relay-Id` / `X-Sentry-Relay-Signature` headers against `body`.
Expand Down
5 changes: 5 additions & 0 deletions ingest-router/src/config.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
use crate::auth::RelayInfo;
use locator::client::{LocatorConfig as ClientLocatorConfig, LocatorType as ClientLocatorType};
use locator::config::{BackupRouteStore, ControlPlane, LocatorDataType};
use serde::Deserialize;
Expand Down Expand Up @@ -187,6 +188,8 @@ pub struct Config {
/// Timeout configuration for relay handlers
#[serde(default)]
pub relay_timeouts: RelayTimeouts,
/// Trusted downstream relay public keys, keyed by relay id
pub relay_keys: HashMap<String, RelayInfo>,

@sentry sentry Bot Jun 18, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bug: The new relay_keys config field is missing #[serde(default)], which will cause service startup to fail with existing configurations that don't have this field.
Severity: CRITICAL

Suggested Fix

Add the #[serde(default)] attribute to the relay_keys field in config.rs. This will make the field optional during deserialization, allowing the service to start with older configuration files.

Prompt for AI Agent 
Review the code at the location below. A potential bug has been identified by an AI
agent. Verify if this is a real issue. If it is, propose a fix; if not, explain why it's
not valid.

Location: ingest-router/src/config.rs#L192

Potential issue: The `relay_keys` field in the configuration struct is missing the
`#[serde(default)]` attribute. This makes the field mandatory during deserialization.
Consequently, deploying the service with an existing configuration file that lacks this
new field will cause a deserialization error, leading to a service startup failure. This
breaks backward compatibility for existing deployments.

Did we get this right? 👍 / 👎 to inform future reviews.

@lynnagara lynnagara Jun 18, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

there aren't any existing deployments we need to maintain compatibility with

@sentry sentry Bot Jun 18, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bug: The relay_keys field in the config struct is a HashMap without #[serde(default)]. Deserialization will fail if the YAML key is present but has a null value.
Severity: CRITICAL

Suggested Fix

Add the #[serde(default)] attribute to the relay_keys field in the Config struct. This will instruct serde to create a default, empty HashMap when the key is missing or its value is null, preventing the deserialization from failing.

Prompt for AI Agent 
Review the code at the location below. A potential bug has been identified by an AI
agent. Verify if this is a real issue. If it is, propose a fix; if not, explain why it's
not valid.

Location: ingest-router/src/config.rs#L192

Potential issue: The `relay_keys` field in the `Config` struct is a `HashMap` but lacks
the `#[serde(default)]` attribute. When `serde_yaml` attempts to deserialize a
configuration where the `relay_keys` key is present but has no value (e.g., it's null or
all sub-keys are commented out), the deserialization will fail with an "invalid type:
unit value, expected a map" error. This will cause a panic at service startup. This
scenario is present in both the unit test `test_parse_valid_config` and the
`example_config_ingest_router.yaml`, making startup failure highly likely for users
following the example.

Also affects:

  • example_config_ingest_router.yaml:10~10
  • ingest-router/src/config.rs:355~355

}

impl Config {
Expand Down Expand Up @@ -349,6 +352,7 @@ routes:
action:
handler: health
locality: us
relay_keys:
"#;

let config: Config = serde_yaml::from_str(yaml).unwrap();
Expand Down Expand Up @@ -386,6 +390,7 @@ routes:
}],
)]),
relay_timeouts: RelayTimeouts::default(),
relay_keys: HashMap::new(),
Comment thread
cursor[bot] marked this conversation as resolved.
routes: vec![Route {
r#match: Match {
path: Some("/api/".to_string()),
Expand Down
3 changes: 3 additions & 0 deletions ingest-router/src/errors.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,4 +51,7 @@ pub enum IngestRouterError {

#[error("Serde error: {0}")]
SerdeError(#[from] serde_json::Error),

#[error("Relay verifier configuration error: {0}")]
RelayVerifierError(#[from] crate::auth::VerifyError),
}
6 changes: 6 additions & 0 deletions ingest-router/src/handler.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,12 @@ pub trait Handler: Send + Sync {

fn execution_mode(&self) -> ExecutionMode;

/// Whether this handler participates in synapse's relay auth: verifying the inbound
/// signature and re-signing the outbound request with synapse's own credentials.
fn requires_relay_auth(&self) -> bool {
false
}

/// Split one request into multiple per-cell requests
///
/// This method routes the request data to appropriate cells and builds
Expand Down
51 changes: 41 additions & 10 deletions ingest-router/src/ingest_router_service.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
use crate::auth;
use crate::config;
use crate::errors::IngestRouterError;
use crate::executor;
Expand All @@ -22,12 +23,21 @@ static INFLIGHT: AtomicU64 = AtomicU64::new(0);
pub struct IngestRouterService {
router: router::Router,
executor: executor::Executor,
verifier: auth::RelayVerifier,
}

impl IngestRouterService {
pub fn new(router: router::Router, timeouts: config::RelayTimeouts) -> Self {
pub fn new(
router: router::Router,
timeouts: config::RelayTimeouts,
verifier: auth::RelayVerifier,
) -> Self {
let executor = executor::Executor::new(timeouts);
Self { router, executor }
Self {
router,
executor,
verifier,
}
}
}

Expand All @@ -49,16 +59,32 @@ where
let resolved = self.router.resolve(&req);
let (parts, body) = req.into_parts();
let executor = self.executor.clone();
// Clone verifier only for requests that require it
let maybe_verifier = match &resolved {
Some((handler, _)) if handler.requires_relay_auth() => Some(self.verifier.clone()),
_ => None,
};

Box::pin(async move {
let (response, handler_name): (Response<Full<Bytes>>, &str) = match resolved {
Some((handler, cells)) => {
let handler_name = handler.name();
match body.collect().await {
Ok(c) => {
let request = Request::from_parts(parts, c.to_bytes());
let response = executor.execute(handler, request, cells).await;
(response.map(Full::new), handler_name)
let body_bytes = c.to_bytes();
if let Some(verifier) = &maybe_verifier
&& let Err(err) =
verifier.verify_request(&parts.headers, &body_bytes)
{
tracing::warn!(error = %err, handler = handler_name, "relay signature verification failed");
let response =
make_error_response(StatusCode::UNAUTHORIZED).map(Full::new);
(response, handler_name)
} else {
let request = Request::from_parts(parts, body_bytes);
let response = executor.execute(handler, request, cells).await;
(response.map(Full::new), handler_name)
}
}
Err(_) => {
let response =
Expand Down Expand Up @@ -111,6 +137,8 @@ mod tests {
use std::time::Duration;
use url::Url;

use crate::testutils::make_signing_keypair;

struct TestServer {
child: Child,
}
Expand Down Expand Up @@ -181,24 +209,27 @@ mod tests {
)]))
.await;

let (signer, verifier) = make_signing_keypair();

let service = IngestRouterService::new(
router::Router::new(routes_config, localities, locator),
config::RelayTimeouts {
http_timeout_secs: 5000,
task_initial_timeout_secs: 10000,
task_subsequent_timeout_secs: 10000,
},
verifier,
);

// Project configs request
let request = Request::builder()
// Project configs request — must be signed by a trusted relay
let body = r#"{"publicKeys": ["aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa"], "global": 1}"#;
let mut request = Request::builder()
.method(Method::POST)
.uri("/api/0/relays/projectconfigs/")
.header(HOST, "us.sentry.io")
.body(Full::new(Bytes::from(
r#"{"publicKeys": ["aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa"], "global": 1}"#,
)))
.body(Full::new(Bytes::from(body)))
.unwrap();
signer.sign_request(request.headers_mut(), body.as_bytes());

let response = service.call(request).await.unwrap();

Expand Down
4 changes: 4 additions & 0 deletions ingest-router/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ pub mod router;
mod testutils;

use crate::errors::IngestRouterError;
use auth::RelayVerifier;
use locator::client::Locator;
use shared::http::run_http_service;

Expand All @@ -22,9 +23,12 @@ use shared::admin_service::AdminService;
pub async fn run(config: config::Config) -> Result<(), IngestRouterError> {
let locator = Locator::new(config.locator.to_client_config()).await?;

let verifier = RelayVerifier::from_relays(config.relay_keys)?;

let ingest_router_service = ingest_router_service::IngestRouterService::new(
router::Router::new(config.routes, config.localities, locator.clone()),
config.relay_timeouts,
verifier,
);
let admin_service = AdminService::new({
let locator = locator.clone();
Expand Down
20 changes: 20 additions & 0 deletions ingest-router/src/testutils.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,10 @@
use crate::auth::{RelayInfo, RelaySigner, RelayVerifier, generate_credentials_json};
use locator::backup_routes::{BackupRouteProvider, FilesystemRouteProvider};
use locator::client::Locator;
use locator::config::Compression;
use locator::types::RouteData;
use std::collections::HashMap;
use std::io::Write;
use std::sync::Arc;

pub async fn get_mock_provider() -> (tempfile::TempDir, FilesystemRouteProvider) {
Expand All @@ -26,6 +28,24 @@ pub async fn get_mock_provider() -> (tempfile::TempDir, FilesystemRouteProvider)
(dir, provider)
}

/// A signer plus a verifier that trusts that signer's freshly generated credentials, so signed
/// requests verify end-to-end.
pub fn make_signing_keypair() -> (RelaySigner, RelayVerifier) {
let json = generate_credentials_json();
let parsed: serde_json::Value = serde_json::from_str(&json).unwrap();
let id = parsed["id"].as_str().unwrap().to_string();
let public_key = parsed["public_key"].as_str().unwrap().to_string();

let mut tmp = tempfile::NamedTempFile::new().unwrap();
tmp.write_all(json.as_bytes()).unwrap();
let signer = RelaySigner::from_file(tmp.path()).unwrap();

let verifier =
RelayVerifier::from_relays(HashMap::from([(id, RelayInfo { public_key })])).unwrap();

(signer, verifier)
}

// Mock backup provider for testing
struct MockBackupProvider {
data: RouteData,
Expand Down
Loading