Sigp audit fixes by JasonVranek · Pull Request #438 · Commit-Boost/commit-boost-client

JasonVranek · 2026-03-19T18:22:31Z

Implements the recommended changes to address issues from the Sigma Prime audit report.

Co-authored-by: eltitanb <lorenzo@gattaca.com> Co-authored-by: ltitanb <163874448+ltitanb@users.noreply.github.com> Co-authored-by: Manuel Iñaki Bilbao <manuel.bilbao@lambdaclass.com>

Co-authored-by: ltitanb <163874448+ltitanb@users.noreply.github.com>

…st-client into sigp-audit-fixes

Co-authored-by: eltitanb <lorenzo@gattaca.com> Co-authored-by: ltitanb <163874448+ltitanb@users.noreply.github.com> Co-authored-by: Manuel Iñaki Bilbao <manuel.bilbao@lambdaclass.com>

Co-authored-by: ltitanb <163874448+ltitanb@users.noreply.github.com>

Co-authored-by: Manuel Iñaki Bilbao <manuel.bilbao@lambdaclass.com>

…st-client into sigp-audit-fixes

Co-authored-by: Joe Clapis <jclapis@outlook.com>

… rightmost setup (#410)

crates/common/src/commit/request.rs

examples/da_commit/src/main.rs

tests/src/signer_service.rs

tests/tests/signer_request_sig.rs

crates/cli/src/docker_init.rs

crates/common/src/commit/request.rs

crates/common/src/config/pbs.rs

crates/common/src/config/utils.rs

…t.rs

… 1.91 as required. Add audit.toml to ignore irrevelent audit error in CI.

ninaiiad · 2026-03-24T10:40:00Z

crates/common/src/config/signer.rs

+
+    fn make_local_signer_config(tls_mode: TlsMode) -> SignerConfig {
+        SignerConfig {
+            host: Ipv4Addr::new(127, 0, 0, 1),


can use Ipv4Addr::LOCALHOST

ninaiiad · 2026-03-24T10:59:19Z

crates/signer/src/service.rs

+    let chain_id: U256;
+    match &*state.manager.read().await {
+        SigningManager::Local(local_manager) => {
+            chain_id = local_manager.get_chain().id();
+            if is_proxy {


I think it would help readaility to refactor the match to return (chain_id, Result) which are later handled in the following map, rather than chaining these together directly

ninaiiad · 2026-03-24T11:00:47Z

crates/signer/src/service.rs

+            ?req_id,
+            "Module signing ID not found"
+        );
+        return Err(SignerModuleError::RequestError("Module signing ID not found".to_string()));


maybe we could return the request id too? otherwise there is no reason to construct it beforehand just to print it once

the same pattern was used in handle_request_signature_bls(). Don't think req_id provides any value since it's only generated server-side, and you can't easily correlate it back to a client-side request. I'm opting towards removing req_id from both bls/ecdsa signing paths and instead adding the pubkey + object_root + nonce to the error messages which are unambiguous and already public info.

ninaiiad · 2026-03-24T11:01:36Z

crates/signer/src/service.rs

+    let chain_id: U256;
+    match &*state.manager.read().await {


same as above

ninaiiad · 2026-03-24T11:12:09Z

crates/common/src/config/mod.rs

+            })
+    }
+
+    /// Helper to return if signer uses TLS


nit: I'm not sure these comments are super helpful?

ninaiiad · 2026-03-24T11:13:32Z

crates/common/src/config/signer.rs

-        let jwts = load_jwt_secrets()?;
+        let (admin_secret, jwt_secrets) = load_jwt_secrets()?;
+
+        // Load the module signing configs


ninaiiad · 2026-03-24T11:14:43Z

crates/common/src/config/signer.rs

+    let mut seen_jwt_secrets = HashMap::new();
+    let mut seen_signing_ids = HashMap::new();
+    for module in modules {
+        // Validate the module ID


nit: same as above. most other comments in this function are doing much either

ninaiiad · 2026-03-24T11:14:59Z

crates/common/src/config/signer.rs

+            pbs: StaticPbsConfig {
+                docker_image: String::from("cb-fake-repo/fake-cb:latest"),
+                pbs_config: PbsConfig {
+                    host: Ipv4Addr::new(127, 0, 0, 1),


also LOCALHOST

ninaiiad · 2026-03-24T11:18:33Z

crates/common/src/utils.rs

+    Ok(claims)
 }

 /// Validate a JWT with the given secret


nit: would remove?

ninaiiad · 2026-03-24T11:18:57Z

crates/common/src/utils.rs

+    Ok(())
+}
+
+/// Validate an admin JWT with the given secret


nit: and this? given that it says the same thing as the function name

ltitanb · 2026-03-25T11:21:07Z

crates/common/src/signature.rs

should we add some tests here for singing and verifying signatures from a real world block? esp for the builder api messages

- add missing ADMIN_JWT_ENV and SIGNER_TLS_CERTIFICATES_PATH_ENV - support https healthchecks

…eserialization error message

…which conflicted with deserializing `SignConsensusRequest`

crates/common/src/commit/request.rs

…fault to readlock

ninaiiad · 2026-04-07T09:44:37Z

crates/signer/src/service.rs

+                    "Module {module_id} signing ID not found in commit-boost config, cannot reload"
+                );
+                error!(event = "reload", module_id = %module_id, error = %error_message);
+                return Err(SignerModuleError::RequestError(error_message));


if we're able to return midway through the loop it's strange that we're also updating the configs in place because we may exit and return an error to the client having half-updated the configs
It doesn't look like an actual vulnerability, but it's not clean and it's not obvious to the client I think

if you think it's better to be able to do this, then I think we should be able to communicate to the client which modules have been updated, also maybe save the error and continue updating other modules afterwards
though i would prefer to check first, update later only if all are ok

ninaiiad · 2026-04-07T09:46:59Z

crates/signer/src/service.rs

+    if elapsed > state.jwt_auth_fail_timeout {
+        drop(failures);
+        debug!("Removing {client_ip} from JWT auth failure list");
+        state.jwt_auth_failures.write().remove(client_ip);


this is a concurrency issue - we can't expect the state to have stayed the same after we've dropped the read lock above
there could've been another failure we missed in between the dropping of the read lock and the acquiring of the write lock where a new failure was added
not likely a big vulnerability either, but a bug nevertheless

crates/common/Cargo.toml

docs/docs/get_started/configuration.md

crates/common/src/signer/schemes/ecdsa.rs

crates/signer/src/manager/dirk.rs

ManuelBilbao · 2026-04-07T16:49:34Z

crates/signer/src/service.rs

        SigningService::init_metrics(config.chain)?;

-        let app = axum::Router::new()
-            .route(REQUEST_SIGNATURE_PATH, post(handle_request_signature))


Should we keep the path as deprecated to avoid sudden breaking changes?

…estart

ManuelBilbao and others added 30 commits July 27, 2025 22:13

CBST2-04: Update JWT secrets on reload and revoke module endpoint (#295)

5935659

Merge branch 'main' into sigp-audit-fixes

91f2cb4

Merge branch 'main' into sigp-audit-fixes

46e6f94

Merge branch 'main' into sigp-audit-fixes

b802fec

Merge branch 'main' into sigp-audit-fixes

151e811

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

498eed9

Co-authored-by: eltitanb <lorenzo@gattaca.com> Co-authored-by: ltitanb <163874448+ltitanb@users.noreply.github.com> Co-authored-by: Manuel Iñaki Bilbao <manuel.bilbao@lambdaclass.com>

Split request_signature into separate paths that return JSON (#350)

af13089

Co-authored-by: eltitanb <lorenzo@gattaca.com> Co-authored-by: ltitanb <163874448+ltitanb@users.noreply.github.com> Co-authored-by: Manuel Iñaki Bilbao <manuel.bilbao@lambdaclass.com>

Add nonce and chain ID to signature requests (#354)

0ef8787

Co-authored-by: eltitanb <lorenzo@gattaca.com> Co-authored-by: ltitanb <163874448+ltitanb@users.noreply.github.com> Co-authored-by: Manuel Iñaki Bilbao <manuel.bilbao@lambdaclass.com>

Merge branch 'main' into sigp-audit-fixes

ee18aa1

Merge main to sigp-audit-fixes (#361)

6988444

Co-authored-by: ltitanb <163874448+ltitanb@users.noreply.github.com>

Merge branch 'sigp-audit-fixes' of github.com:Commit-Boost/commit-boo…

25bd5a6

…st-client into sigp-audit-fixes

Add payload hash to signer JWT claims (#356)

52aec57

Co-authored-by: eltitanb <lorenzo@gattaca.com> Co-authored-by: ltitanb <163874448+ltitanb@users.noreply.github.com> Co-authored-by: Manuel Iñaki Bilbao <manuel.bilbao@lambdaclass.com>

Merge branch 'main' into sigp-audit-fixes

f2a275b

Add TLS to signer (#357)

6b14d77

Co-authored-by: eltitanb <lorenzo@gattaca.com> Co-authored-by: ltitanb <163874448+ltitanb@users.noreply.github.com> Co-authored-by: Manuel Iñaki Bilbao <manuel.bilbao@lambdaclass.com>

Removed old dependency (#370)

eb61667

Merge main to sigp audit fixes (#371)

2dfe96b

Co-authored-by: ltitanb <163874448+ltitanb@users.noreply.github.com>

Fix misc findings from reaudit (#376)

d566dea

Co-authored-by: Manuel Iñaki Bilbao <manuel.bilbao@lambdaclass.com>

Added make to the build file

8ddb055

Merge branch 'main' into sigp-audit-fixes

9103b51

fix(signer): Mark a JWT failure if payload conversion fail (#387)

a3f8d9d

Merge branch 'sigp-audit-fixes' of github.com:Commit-Boost/commit-boo…

7bf2589

…st-client into sigp-audit-fixes

Removed a test that was deleted in main

b66f411

Merge branch 'main' into sigp-audit-fixes

b541ffa

Merge branch 'main' into sigp-audit-fixes

58b4eb9

refactor(signer): Use config-based header to extract IP from (#388)

e58d67e

Co-authored-by: Joe Clapis <jclapis@outlook.com>

fix(signer): introduce config to set the amount of trusted proxies in…

85382a5

… rightmost setup (#410)

fix(signer): rightmost header fixes (#420)

19121f3

Merge branch 'main' into sigp-audit-fixes

d4d5260

Merge branch 'main' into sigp-audit-fixes

02364f3

Merge branch 'main' into sigp-audit-fixes

533e4c0

JasonVranek added 2 commits March 17, 2026 13:27

Merge remote-tracking branch 'upstream/main' into sigp-audit-fixes

4681103

improve tls/cert config testing

9782d22

JasonVranek requested review from ManuelBilbao and ltitanb March 19, 2026 18:22

github-advanced-security bot found potential problems Mar 19, 2026

View reviewed changes

JasonVranek commented Mar 20, 2026

View reviewed changes

crates/cli/src/docker_init.rs Show resolved Hide resolved

crates/common/src/commit/request.rs Show resolved Hide resolved

crates/common/src/config/pbs.rs Outdated Show resolved Hide resolved

crates/common/src/config/utils.rs Outdated Show resolved Hide resolved

more util test coverage and remove duplicate env read from docker_ini…

bb28eb0

…t.rs

JasonVranek requested a review from a team March 20, 2026 21:57

JasonVranek added 2 commits March 22, 2026 10:50

Bump lh from v8.0.0-rc.0 to stable v8.0.0, and bump rust from 1.89 to…

511d07d

… 1.91 as required. Add audit.toml to ignore irrevelent audit error in CI.

bump rust toolchain to nightly-2026-01-01

4afbf35

ninaiiad approved these changes Mar 24, 2026

View reviewed changes

address review comments

717390e

ninaiiad approved these changes Mar 24, 2026

View reviewed changes

ltitanb previously approved these changes Mar 25, 2026

View reviewed changes

JasonVranek added 2 commits March 25, 2026 12:10

address Dirk issues in docker_init.rs:

04ae8b7

- add missing ADMIN_JWT_ENV and SIGNER_TLS_CERTIFICATES_PATH_ENV - support https healthchecks

cleaner error message if TLS CryptoProvider fails

6ec3782

JasonVranek dismissed ltitanb’s stale review via 6ec3782 March 25, 2026 19:32

JasonVranek added 3 commits March 25, 2026 13:03

users get error message on missing [signer] section instead of toml d…

58b4b22

…eserialization error message

support partial jwt reloads

001b312

require the nonce in signer-api.yml - previously was marked optional …

76905de

…which conflicted with deserializing `SignConsensusRequest`

github-advanced-security bot found potential problems Mar 25, 2026

View reviewed changes

crates/common/src/commit/request.rs Dismissed Show dismissed Hide dismissed

crates/common/src/commit/request.rs Dismissed Show dismissed Hide dismissed

JasonVranek added 5 commits March 25, 2026 15:43

fix suffix when displaying X-Forwaded-For

8268572

only take the jwt_auth_failures writelock if strictly necessary, de…

7c8cce6

…fault to readlock

remove mark_jwt_failure() calls from failures unrelated to jwts

4a9aff7

add round-trip sign->verify unit tests

d834242

Merge remote-tracking branch 'upstream/main' into sigp-audit-fixes

a9a5c11

ninaiiad suggested changes Apr 7, 2026

View reviewed changes

ManuelBilbao reviewed Apr 7, 2026

View reviewed changes

address comments. /reload now allows new module jwts without a full r…

2c3efde

…estart

Conversation

JasonVranek commented Mar 19, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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!

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!

Uh oh!

ninaiiad Apr 7, 2026 • 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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

JasonVranek commented Mar 19, 2026 •

edited

Loading

ninaiiad Apr 7, 2026 •

edited

Loading