Require "." boundary in can_be_signed_by

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

beam-lib/src/ids.rs

@@ -68,7 +68,7 @@ impl AppOrProxyId {
     }
 
     pub fn can_be_signed_by(&self, other: &impl AsRef<str>) -> 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<str>) -> 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";

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" \