CBST2-02: Make proposer commitment signatures unique to modules #329

jclapis · 2025-07-01T21:29:21Z

This is a large change that modifies the ways proposer commitment signatures are generated. Currently, the requesting module isn't embedded into the data being signed so any authorized module can request the signature of any data - including data that might be used or expected by other modules. This PR precludes that possibility by embedding a new special per-module signing ID into the data being signed.

The signing ID is a 32-byte hex string provided in the Commit Boost configuration's [[modules]] section, per module. Module authors can make it whatever they want, and should publish it within their own documentation so users know which unique value to configure for that module. Since Commit Boost doesn't (and won't) maintain a global registry of such IDs, the onus is on the user to ensure the correct ID is entered for each module. Module authors can then directly bake this into their module code, in their signature validation routines, to confirm that any proposer commitments returned by the signer used the correct ID.

Since different modules will have different signing IDs (a condition enforced by the loader logic), this prevents one module from "forging" a signature destined for another one.

Using a discrete signing ID means module authors can change their module ID at any time (such as with a version number bump or a vendor rename) without invalidating previous signatures. Conversely, if they do want to invalidate previous signatures while retaining the module ID, they can do so by simply changing their signing ID and informing their clients to update with the new configuration accordingly.

This also adds new round-trip integration tests that confirm signing works as expected with the new paradigm and cross-module signatures are no longer possible.

Finally, it starts some documentation on how to request proposer commitments from the signer service, including how to request a signature and verify it. See docs/docs/developing/prop-commit-signing.md.

⚠️ This is a breaking change that will invalidate any existing proposer commitment signatures. Once this is added, clients using the signer module will need to be informed of the breaking change and adapt their signature verification routines with the new logic.

…s. static

…he source" This reverts commit 58c6117.

config.example.toml

ltitanb · 2025-07-09T12:27:33Z

crates/common/src/config/module.rs

    #[serde(rename = "type")]
    pub kind: ModuleKind,
+    /// Signing ID for the module to use when requesting signatures
+    pub signing_id: Option<B256>,


why make it optional?

also worth creating a type alias for clarity

why make it optional?

So this was a concession I had to make in order to support the other use cases where the signature generation pipeline is used with COMMIT_BOOST_DOMAIN, but not tied to a specific module. For example, proxy key generation will do this. Proxy keys are already stored by module ID and don't have any sense of signing ID, so coupling them to the signing ID as well would either involve storing that as part of their path or adding a lookup from module ID to signing ID to the signer. Either way, any time the signing ID changes (e.g, when previous signatures need to be invalidated), you'd have to regenerate all of the proxy keys. It seemed easier to make this field optional to support the existing calls instead of refactoring all of that.

Even if we want to still make it optional internally, it should not be optional in the module configuration. After this PR, modules need to have this set

Yep, that makes sense. Fixed in 509dba8.

I agree with making it required for commit modules, but I think for PBS Events modules it's useless as they won't sign anything. With that said, it's not a tedious config to set, so I wouldn't mind to leave it required in favor of a cleaner codebase

#338 is going to remove PBS events modules anyway, so assuming that goes in soon-ish then this may end up being OBE?

ltitanb · 2025-07-09T12:28:12Z

crates/common/src/config/constants.rs

+pub const SIGNER_JWT_AUTH_FAIL_LIMIT_DEFAULT: u32 = 3;
+
+/// How long to rate limit the client after auth failures
 pub const SIGNER_JWT_AUTH_FAIL_TIMEOUT_SECONDS_ENV: &str =
    "CB_SIGNER_JWT_AUTH_FAIL_TIMEOUT_SECONDS";
+pub const SIGNER_JWT_AUTH_FAIL_TIMEOUT_SECONDS_DEFAULT: u32 = 5 * 60;


is this supposed to be in this PR or does it need to be rebased?

This was some incidental cleanup of #310 that landed in here after it was merged. There are a few little non-functional changes related to it. For cleanliness I can pull all of those out and make a separate PR for them if you'd like, no problem.

Yep let's do that to keep the PR clean

Done in PR #346 which has been merged into this now.

crates/common/src/config/signer.rs

ltitanb · 2025-07-09T12:37:33Z

crates/common/src/config/signer.rs

+        let mut seen_jwt_secrets = HashMap::new();
+        let mut seen_signing_ids = HashMap::new();


if we go through modules one by one, then these could just be hashsets

The value (the module_id) is used in the error messages that get thrown during duplication detection so the user knows which module entries are conflicting. We can swap to HashSets, just print the offending duplicated value itself, and let the user find it in the config if that's preferable, but listing the modules directly seems like good UX.

ltitanb · 2025-07-09T14:35:36Z

crates/common/src/types.rs

+#[derive(Default, Debug, TreeHash)]
+pub struct PropCommitSigningInfo {
+    pub data: [u8; 32],
+    pub module_signing_id: [u8; 32],


B256 implements TreeHash

Yeah, I left these as [u8; 32] because the original SigningData above it used those. Happy to convert everything to B256 though, in fact I'd prefer it if that's ok.

yep let's do that

Ok, this is a large enough change that I made it a PR for this PR, see #347.

crates/common/src/signature.rs

ltitanb · 2025-07-09T14:38:15Z

crates/common/src/signature.rs

    pubkey: &BlsPublicKey,
    msg: &T,
    signature: &BlsSignature,
+    module_signing_id: Option<&B256>,


Option here and below can be removed, we only do signatures in the signer module and modules will always need to have the id set after this PR

See the earlier response - the proxy key generator uses it too, and it (currently) doesn't include signing IDs which is why it's an Option in here. Validating the signature in e.g. SignedProxyDelegationBls won't work without it.

docs/docs/developing/prop-commit-signing.md

ltitanb · 2025-07-09T14:49:05Z

docs/docs/developing/prop-commit-signing.md

+#### `200 OK`
+
+A successful signing request, with the signature provided as a plaintext quoted hex-encoded string, with a `0x` prefix. For example, the response body would look like:
+```
+"0xa43e623f009e615faa3987368f64d6286a4103de70e9a81d82562c50c91eae2d5d6fb9db9fe943aa8ee42fd92d8210c1149f25ed6aa72a557d74a0ed5646fdd0e8255ec58e3e2931695fe913863ba0cdf90d29f651bce0a34169a6f6ce5b3115"
+```
+
+#### `401 Unauthorized`
+
+Your module did not provide a JWT string in the request's authorization header, or the JWT string was not configured in the signer service's configuration file as belonging to your module.
+
+


i wonder if this part is actually needed? we do have an OpenAPI schema after all

That would be lovely, because maintaining it manually is a chore. I'll ping you about it and add the results to this PR once we have them.

Done in ff716a0.

ltitanb · 2025-07-27T21:27:01Z

crates/common/src/signature.rs

-    }
-
-    let signing_data = SigningData { object_root, signing_domain };
+pub fn compute_signing_root<T: TreeHash>(signing_data: &T) -> [u8; 32] {


this function should be either renamed or removed, as technically it's just the tree hash and not a signing root

Also included in #347.

Co-authored-by: Manuel Iñaki Bilbao <[email protected]> Co-authored-by: ltitanb <[email protected]>

crates/common/src/config/signer.rs

crates/common/src/signature.rs

crates/signer/src/manager/local.rs

docs/docs/developing/prop-commit-signing.md

docs/docs/get_started/configuration.md

ltitanb and others added 30 commits May 13, 2025 17:17

bump version

c68125d

Successful cross-compilation, but runtime has memory allocation issues

d9979a2

Working with OpenSSL static-linked

97ef653

Got dynamic linking working, added a feature flag to toggle dynamic v…

91eefe2

…s. static

Fixed the vendored build arg

de09415

Reintroduced the cargo chef setup

3aee63d

Ported the cross-compilation stuff into PBS

c07c717

Split the dockerfiles into separate builder / image definitions

699b7ec

Added a build guide

7165f12

Refactored the Github release action to use the Docker builder

9438dae

Fixed the Docker image binary filenames

12c020a

Cleaned up the Darwin artifact step

53cafc0

Made the CI workflow and justfile use the same toolchain as the source

58c6117

Revert "Made the CI workflow and justfile use the same toolchain as t…

45e581b

…he source" This reverts commit 58c6117.

Testing removal of OpenSSL vendored option

24a10c5

Updating just in the CI workflow

e36da54

Merge branch 'main' into cross-compile

843b110

Refactored the signer to support host and port config settings

e7c6d19

Updated docs

6117219

Fixing Clippy in CI workflow

c0f591d

Removed obviated CI setup

adbd34a

Minor dedup of RwLock guard acquisition

e3488b3

Added rate limiting for signer clients with repeated JWT auth failures

c3d7ec4

Added Signer config validation

9ddad64

Started unit test setup for the Signer

c62185e

Finished a basic signer module unit test

dc73c62

Added a JWT failure unit test

6c3d967

Added a rate limit test and cleaned up a bit

6464638

Added unique ports to unit tests for parallel execution

0313f18

Cleaned up the build Dockerfile and removed an extra dependency layer

346eea4

Added prop commit signature verification helpers for modules to use

d5641df

ltitanb requested changes Jul 9, 2025

View reviewed changes

jclapis added 5 commits July 10, 2025 02:49

Fixed some params in da_commit

5da31bf

Merge branch 'main' into prevent-cross-module-sigs

bfd72c5

Cleaned load_module_signing_configs a bit

1a0efec

Fixed some docs language

ee282da

Refactored into compute_prop_commit_signing_root

133447d

ltitanb reviewed Jul 27, 2025

View reviewed changes

jclapis added 3 commits July 28, 2025 12:40

Merge branch 'main' into prevent-cross-module-sigs

9f72aff

Signing IDs are no longer optional in the config

509dba8

Refactored some of the signer consts for consistency

2c507d7

jclapis mentioned this pull request Jul 28, 2025

Chore: cleanup some signer consts #346

Merged

Merge branch 'refactor-signer-consts' into prevent-cross-module-sigs

d1ad9c9

jclapis mentioned this pull request Jul 28, 2025

Move from [u8; 32] to B256 everywhere #347

Merged

Updated the Signer API docs

ff716a0

jclapis mentioned this pull request Jul 29, 2025

Merge sigp-audit-fixes #348

Merged

jclapis and others added 4 commits July 30, 2025 14:42

Merge sigp-audit-fixes (#348)

de61066

Co-authored-by: Manuel Iñaki Bilbao <[email protected]> Co-authored-by: ltitanb <[email protected]>

Merge branch 'sigp-audit-fixes' into prevent-cross-module-sigs

db08d77

Move from [u8; 32] to B256 everywhere (#347)

ce29c3a

Merge branch 'sigp-audit-fixes' into prevent-cross-module-sigs

b837e44

ManuelBilbao reviewed Jul 31, 2025

View reviewed changes

jclapis added 4 commits August 4, 2025 15:28

Cleaned up some hashmap usage

874e07d

Removed compute_tree_hash_root()

3ad11a8

Some minor cleanup

a75605e

Fixed some docs

d091a90

ManuelBilbao approved these changes Aug 4, 2025

View reviewed changes

jclapis added 2 commits August 12, 2025 13:34

Merge branch 'sigp-audit-fixes' into prevent-cross-module-sigs

0313cc2

Merge branch 'sigp-audit-fixes' into prevent-cross-module-sigs

358b3cb

ltitanb approved these changes Aug 18, 2025

View reviewed changes

jclapis merged commit 498eed9 into sigp-audit-fixes Aug 19, 2025
1 check passed

		let mut seen_jwt_secrets = HashMap::new();
		let mut seen_signing_ids = HashMap::new();

CBST2-02: Make proposer commitment signatures unique to modules #329

CBST2-02: Make proposer commitment signatures unique to modules #329

Uh oh!

Conversation

jclapis commented Jul 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jclapis Jul 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ManuelBilbao Jul 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

jclapis commented Jul 1, 2025 •

edited

Loading

jclapis Jul 14, 2025 •

edited

Loading

ManuelBilbao Jul 29, 2025 •

edited

Loading