From 8db46c786c7dbe7f65d049bd030033fd8f57d483 Mon Sep 17 00:00:00 2001 From: Martin Lablans Date: Tue, 26 May 2026 23:28:55 +0300 Subject: [PATCH 1/2] Fix CI/CD: Reuse existing proxy key when re-enrolling in dev PKI --- dev/pki/pki | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/dev/pki/pki b/dev/pki/pki index f19dabfd..d301291e 100755 --- a/dev/pki/pki +++ b/dev/pki/pki @@ -95,7 +95,15 @@ function request() { application=$1 cn=$2 ttl=$3 - openssl req -new -newkey rsa:2048 -nodes -keyout ${application}.priv.pem -out ${application}.csr -subj "/CN=${cn}" 2>/dev/null + # Beam only supports multiple certificates per proxy when they share one key. + # devsetup enrolls some proxies repeatedly to create duplicate certs, so reuse + # an existing key on re-enrollment and generate a fresh one only the first time; + # otherwise senders may encrypt to a key the proxy no longer holds. + if [ -s "${application}.priv.pem" ] && openssl pkey -in "${application}.priv.pem" -noout 2>/dev/null; then + openssl req -new -key "${application}.priv.pem" -out ${application}.csr -subj "/CN=${cn}" 2>/dev/null + else + openssl req -new -newkey rsa:2048 -nodes -keyout ${application}.priv.pem -out ${application}.csr -subj "/CN=${cn}" 2>/dev/null + fi data=$(jq -Rs '{common_name: "'$cn'", ttl: "'$ttl'", csr: .}' < ${application}.csr) echo "Creating Certificate for domain $cn" curl --header "X-Vault-Token: $VAULT_TOKEN" \ From 5d2f7e019482df6c2b58b95b87d386111fc95c9c Mon Sep 17 00:00:00 2001 From: Martin Lablans Date: Tue, 26 May 2026 22:12:49 +0300 Subject: [PATCH 2/2] Require "." boundary in can_be_signed_by --- CHANGELOG.md | 6 ++++++ beam-lib/src/ids.rs | 35 +++++++++++++++++++++++++++++++++-- 2 files changed, 39 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 900c3dbf..e72b5d4a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,3 +1,9 @@ +# Unreleased + +## Bugfixes + +* Fixed an authorization flaw where the signer check matched bare id suffixes, allowing a proxy to sign on behalf of another proxy whose id ended with its own (e.g. `ulm.broker` for `neu-ulm.broker`). The check now requires a `.`-delimited label boundary. + # Samply.Beam 0.10.0 - 2025-05-26 ## Breaking changes diff --git a/beam-lib/src/ids.rs b/beam-lib/src/ids.rs index 60614691..3c883b2e 100644 --- a/beam-lib/src/ids.rs +++ b/beam-lib/src/ids.rs @@ -68,7 +68,7 @@ impl AppOrProxyId { } pub fn can_be_signed_by(&self, other: &impl AsRef) -> bool { - self.as_ref().ends_with(other.as_ref()) + signer_matches(self.as_ref(), other.as_ref()) } pub fn hide_broker(&self) -> &str { @@ -150,7 +150,7 @@ macro_rules! impl_id { #[cfg(feature = "strict-ids")] pub fn can_be_signed_by(&self, other: &impl AsRef) -> bool { - self.as_ref().ends_with(other.as_ref()) + signer_matches(self.as_ref(), other.as_ref()) } } @@ -271,6 +271,18 @@ impl Display for BeamIdError { } } +/// Returns true if `signer` is allowed to sign on behalf of `id`, i.e. `id` +/// equals `signer` or `signer` is a suffix of `id` at a `.`-delimited label +/// boundary. A bare `str::ends_with` is insufficient here: it would let a proxy +/// `ulm.broker` sign for the unrelated `neu-ulm.broker`. +#[cfg(feature = "strict-ids")] +fn signer_matches(id: &str, signer: &str) -> bool { + id == signer + || id + .strip_suffix(signer) + .is_some_and(|prefix| prefix.ends_with('.')) +} + #[cfg(feature = "strict-ids")] fn check_valid_id_part(id: &str) -> Result<(), BeamIdError> { for char in id.chars() { @@ -335,6 +347,25 @@ mod tests { ); } + #[test] + fn test_can_be_signed_by_label_boundary() { + set_broker_id("broker.samply.de".to_string()); + let victim_proxy = ProxyId::new("neu-ulm.broker.samply.de").unwrap(); + let victim_app: AppOrProxyId = + AppId::new("app.neu-ulm.broker.samply.de").unwrap().into(); + let attacker = ProxyId::new("ulm.broker.samply.de").unwrap(); + + // Legitimate: a proxy signs for itself and its own apps. + assert!(victim_proxy.can_be_signed_by(&victim_proxy)); + assert!(victim_app.can_be_signed_by(&victim_proxy)); + + // Attack: `ulm.broker.samply.de` is a bare-suffix of the unrelated + // `neu-ulm.broker.samply.de`, but the preceding char is `-`, not a label + // boundary -> must be rejected. + assert!(!victim_proxy.can_be_signed_by(&attacker)); + assert!(!victim_app.can_be_signed_by(&attacker)); + } + #[test] fn test_app_or_proxy_id() { let app_id_str = "app.proxy1.broker.samply.de";