CBST2-02: Make proposer commitment signatures unique to modules #329
+1,315
−225
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…he source" This reverts commit 58c6117.
ltitanb
requested changes
Jul 9, 2025
| @@ -239,6 +239,8 @@ proxy_dir = "./proxies" | |||
| [[modules]] | |||
| # Unique ID of the module | |||
| id = "DA_COMMIT" | |||
| # Unique hash that the Signer service will combine with the incoming data in signing requests to generate a signature specific to this module | |||
| signing_id = "0x6a33a23ef26a4836979edff86c493a69b26ccf0b4a16491a815a13787657431b" | |||
There was a problem hiding this comment.
does this need to be 32 bytes? something shorter like we do for chain id could also work
| @@ -37,6 +38,8 @@ pub struct StaticModuleConfig { | |||
| /// Type of the module | |||
| #[serde(rename = "type")] | |||
| pub kind: ModuleKind, | |||
| /// Signing ID for the module to use when requesting signatures | |||
| pub signing_id: Option<B256>, | |||
There was a problem hiding this comment.
also worth creating a type alias for clarity
Comment on lines
+41
to
+46
| 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; |
There was a problem hiding this comment.
is this supposed to be in this PR or does it need to be rebased?
| jwt_secrets: &HashMap<ModuleId, String>, | ||
| ) -> Result<HashMap<ModuleId, ModuleSigningConfig>> { | ||
| let mut mod_signing_configs = HashMap::new(); | ||
| if let Some(modules) = &config.modules { |
There was a problem hiding this comment.
if this is None this function should not be called, we should error if it is, something like
let Some(modules) = &config.modules else { bail!("...") }
Comment on lines
+283
to
+284
| let mut seen_jwt_secrets = HashMap::new(); | ||
| let mut seen_signing_ids = HashMap::new(); |
There was a problem hiding this comment.
if we go through modules one by one, then these could just be hashsets
| #[derive(Default, Debug, TreeHash)] | ||
| pub struct PropCommitSigningInfo { | ||
| pub data: [u8; 32], | ||
| pub module_signing_id: [u8; 32], |
Comment on lines
+55
to
+67
| let signing_root = match module_signing_id { | ||
| Some(id) => compute_signing_root(&types::SigningData { | ||
| object_root: compute_signing_root(&types::PropCommitSigningInfo { | ||
| data: msg.tree_hash_root().0, | ||
| module_signing_id: id.0, | ||
| }), | ||
| signing_domain: domain, | ||
| }), | ||
| None => compute_signing_root(&types::SigningData { | ||
| object_root: msg.tree_hash_root().0, | ||
| signing_domain: domain, | ||
| }), | ||
| }; |
There was a problem hiding this comment.
this logic is repeated a couple of times can we refactor to a separate function?
| @@ -52,14 +48,27 @@ pub fn verify_signed_message<T: TreeHash>( | |||
| pubkey: &BlsPublicKey, | |||
| msg: &T, | |||
| signature: &BlsSignature, | |||
| module_signing_id: Option<&B256>, | |||
There was a problem hiding this comment.
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
|
|
||
| ## The Signing ID | ||
|
|
||
| Your module's signing ID is a 32-byte value that is used as a unique identifier within the signing process. Preconfirmation signatures incorporate this value along with the data being signed as a way to create signatures that are exclusive to your module, so other modules can't maliciously construct signatures that appear to be from your module. Your module must have this ID incorporated into itself ahead of time, and the user must include this same ID within their Commit Boost configuration file section for your module. Commit Boost does not maintain a global registry of signing IDs, so this is a value you should provide to your users in your documentation. |
There was a problem hiding this comment.
nit: not "preconfirmation signatures" but "commitment signatures", preconfs are just one type of commitment
Comment on lines
+138
to
+149
| #### `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. | ||
|
|
||
|
|
There was a problem hiding this comment.
i wonder if this part is actually needed? we do have an OpenAPI schema after all
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.