Skip to content

Require "." boundary in can_be_signed_by#259

Merged
lablans merged 2 commits into
developfrom
signer-suffix-check
May 27, 2026
Merged

Require "." boundary in can_be_signed_by#259
lablans merged 2 commits into
developfrom
signer-suffix-check

Conversation

@lablans
Copy link
Copy Markdown
Member

@lablans lablans commented May 26, 2026

Fixed security bug in authorization logic: Check for a .-delimited label boundary instead of a simple string suffix match, preventing potential impersonation attacks. See unit test for potential attack.

Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR fixes an authorization bug in beam-lib’s ID signing checks by requiring a .-delimited label boundary when allowing a proxy ID to sign on behalf of another ID (itself or its apps), preventing bare-suffix impersonation (e.g., a.broker… incorrectly matching data.broker…).

Changes:

  • Replace str::ends_with signer checks with a boundary-aware signer_matches helper.
  • Add a unit test demonstrating and preventing the bare-suffix impersonation case.
  • Document the security fix in the changelog.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File Description
CHANGELOG.md Adds an “Unreleased” entry documenting the authorization/signing boundary fix.
beam-lib/src/ids.rs Implements boundary-aware signer matching and adds a regression test covering the impersonation scenario.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@lablans lablans force-pushed the signer-suffix-check branch 3 times, most recently from 9bc698d to 2fcfebe Compare May 26, 2026 20:05
@lablans lablans force-pushed the signer-suffix-check branch from 2fcfebe to 5d2f7e0 Compare May 26, 2026 20:40
Copy link
Copy Markdown
Member

@Threated Threated left a comment

Choose a reason for hiding this comment

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

I feel like the security impact here is a bit overstated. can_be_signed_by is only used as a sanity check after the jwt code.
An attacker claiming to be someone else still won't work as the jwt signature verification would still fail as its verified against whichever pubkey they claim to be.

@lablans lablans merged commit d127d1a into develop May 27, 2026
29 of 33 checks passed
@lablans lablans deleted the signer-suffix-check branch May 27, 2026 19:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants