Merge remote-tracking branch 'upstream/sigp-audit-fixes' into ssz-update-v2

Author: JasonVranek

api/signer-api.yml

@@ -86,7 +86,7 @@ paths:
           application/json:
             schema:
               type: object
-              required: [pubkey, object_root]
+              required: [pubkey, object_root, nonce]
               properties:
                 pubkey:
                   description: The 48-byte BLS public key, with optional `0x` prefix, of the proposer key that you want to request a signature from.
@@ -234,7 +234,7 @@ paths:
           application/json:
             schema:
               type: object
-              required: [proxy, object_root]
+              required: [proxy, object_root, nonce]
               properties:
                 proxy:
                   description: The 48-byte BLS public key (for `proxy_bls` mode) or the 20-byte Ethereum address (for `proxy_ecdsa` mode), with optional `0x` prefix, of the proxy key that you want to request a signature from.
@@ -382,7 +382,7 @@ paths:
           application/json:
             schema:
               type: object
-              required: [proxy, object_root]
+              required: [proxy, object_root, nonce]
               properties:
                 proxy:
                   description: The 20-byte Ethereum address, with optional `0x` prefix, of the proxy key that you want to request a signature from.
@@ -695,7 +695,12 @@ components:
           $ref: "#/components/schemas/EcdsaSignature"
     Nonce:
       type: integer
-      description: If your module tracks nonces per signature (e.g., to prevent replay attacks), this is the unique nonce to use for the signature. It should be an unsigned 64-bit integer in big-endian format. It must be between 0 and 2^64-2, inclusive. If your module doesn't use nonces, we suggest setting this to 2^64-1 instead of 0 because 0 is a legal nonce and will cause complications with your module if you ever want to use a nonce in the future.
+      description: |
+        Replay-protection nonce, always mixed into the signing root via `PropCommitSigningInfo`. It
+        must be an unsigned 64-bit integer between 0 and 2^64-2 (18446744073709551614), inclusive.
+
+        Modules that track nonces for replay protection should use a monotonically increasing value
+        per key. Modules that do not use replay protection should always send `0`.
       minimum: 0
-      maximum: 18446744073709551614 // 2^64-2
+      maximum: 18446744073709551614
       example: 1

crates/cli/src/docker_init.rs

@@ -497,6 +497,8 @@ fn create_signer_service_dirk(
     let mut envs = IndexMap::from([
         get_env_val(CONFIG_ENV, CONFIG_DEFAULT),
         get_env_same(JWTS_ENV),
+        get_env_same(ADMIN_JWT_ENV),
+        get_env_val(SIGNER_TLS_CERTIFICATES_PATH_ENV, SIGNER_TLS_CERTIFICATES_PATH_DEFAULT),
         get_env_val(DIRK_CERT_ENV, DIRK_CERT_DEFAULT),
         get_env_val(DIRK_KEY_ENV, DIRK_KEY_DEFAULT),
         get_env_val(DIRK_DIR_SECRETS_ENV, DIRK_DIR_SECRETS_DEFAULT),
@@ -548,6 +550,7 @@ fn create_signer_service_dirk(
 
     // write jwts to env
     service_config.envs.insert(JWTS_ENV.into(), format_comma_separated(&service_config.jwts));
+    service_config.envs.insert(ADMIN_JWT_ENV.into(), random_jwt_secret());
 
     // CA cert volume and env
     if let Some(ca_cert_path) = ca_cert_path {
@@ -589,8 +592,8 @@ fn create_signer_service_dirk(
         environment: Environment::KvPair(envs),
         healthcheck: Some(Healthcheck {
             test: Some(HealthcheckTest::Single(format!(
-                "curl -f http://localhost:{}/status",
-                signer_config.port,
+                "curl -k -f {}/status",
+                cb_config.signer_server_url(SIGNER_PORT_DEFAULT),
             ))),
             interval: Some("30s".into()),
             timeout: Some("5s".into()),
@@ -932,6 +935,13 @@ mod tests {
         service.volumes.iter().any(|v| matches!(v, Volumes::Simple(s) if s.contains(substr)))
     }
 
+    fn get_healthcheck_cmd(service: &Service) -> Option<String> {
+        service.healthcheck.as_ref().and_then(|hc| match &hc.test {
+            Some(HealthcheckTest::Single(cmd)) => Some(cmd.clone()),
+            _ => None,
+        })
+    }
+
     fn has_port(service: &Service, substr: &str) -> bool {
         match &service.ports {
             Ports::Short(ports) => ports.iter().any(|p| p.contains(substr)),
@@ -1309,12 +1319,33 @@ mod tests {
         assert!(env_str(&service, DIRK_CERT_ENV).is_some());
         assert!(env_str(&service, DIRK_KEY_ENV).is_some());
         assert!(env_str(&service, DIRK_DIR_SECRETS_ENV).is_some());
+        assert!(has_env_key(&service, ADMIN_JWT_ENV));
+        assert!(has_env_key(&service, SIGNER_TLS_CERTIFICATES_PATH_ENV));
         assert!(has_volume(&service, "client.crt"));
         assert!(has_volume(&service, "client.key"));
         assert!(has_volume(&service, "dirk_secrets"));
         Ok(())
     }
 
+    #[test]
+    fn test_create_signer_service_dirk_generates_admin_jwt() -> eyre::Result<()> {
+        let mut sc = minimal_service_config();
+        let signer_config = dirk_signer_config();
+        create_signer_service_dirk(
+            &mut sc,
+            &signer_config,
+            Path::new("/certs/client.crt"),
+            Path::new("/certs/client.key"),
+            Path::new("/dirk_secrets"),
+            &None,
+            &None,
+        )?;
+
+        let admin_jwt = sc.envs.get(ADMIN_JWT_ENV).expect("ADMIN_JWT_ENV must be set");
+        assert!(!admin_jwt.is_empty(), "admin JWT secret must not be empty");
+        Ok(())
+    }
+
     #[test]
     fn test_create_signer_service_dirk_with_ca_cert() -> eyre::Result<()> {
         let mut sc = minimal_service_config();
@@ -1690,6 +1721,50 @@ mod tests {
         Ok(())
     }
 
+    #[test]
+    fn test_create_signer_service_dirk_healthcheck_uses_https_with_tls() -> eyre::Result<()> {
+        let dir = tempfile::tempdir()?;
+        let certs_path = dir.path().to_path_buf();
+        std::fs::write(certs_path.join(SIGNER_TLS_CERTIFICATE_NAME), b"cert")?;
+        std::fs::write(certs_path.join(SIGNER_TLS_KEY_NAME), b"key")?;
+
+        let mut sc = service_config_with_tls(certs_path);
+        let signer_config = dirk_signer_config();
+        let service = create_signer_service_dirk(
+            &mut sc,
+            &signer_config,
+            Path::new("/certs/client.crt"),
+            Path::new("/certs/client.key"),
+            Path::new("/dirk_secrets"),
+            &None,
+            &None,
+        )?;
+
+        let cmd = get_healthcheck_cmd(&service).expect("healthcheck must be set");
+        assert!(cmd.contains("https://"), "healthcheck must use https with TLS: {cmd}");
+        assert!(cmd.contains("-k"), "healthcheck must use -k flag for self-signed certs: {cmd}");
+        Ok(())
+    }
+
+    #[test]
+    fn test_create_signer_service_dirk_healthcheck_uses_http_without_tls() -> eyre::Result<()> {
+        let mut sc = minimal_service_config();
+        let signer_config = dirk_signer_config();
+        let service = create_signer_service_dirk(
+            &mut sc,
+            &signer_config,
+            Path::new("/certs/client.crt"),
+            Path::new("/certs/client.key"),
+            Path::new("/dirk_secrets"),
+            &None,
+            &None,
+        )?;
+
+        let cmd = get_healthcheck_cmd(&service).expect("healthcheck must be set");
+        assert!(cmd.contains("http://"), "healthcheck must use http without TLS: {cmd}");
+        Ok(())
+    }
+
     // -------------------------------------------------------------------------
     // create_module_service – TLS cert env/volume
     // -------------------------------------------------------------------------

crates/common/src/commit/request.rs

@@ -84,6 +84,10 @@ impl<T: ProxyId> fmt::Display for SignedProxyDelegation<T> {
 pub struct SignConsensusRequest {
     pub pubkey: BlsPublicKey,
     pub object_root: B256,
+    /// Replay-protection nonce mixed into the signing root via
+    /// `PropCommitSigningInfo`. Modules that do not track nonces should
+    /// send `0`. Modules that do track nonces should use a monotonically
+    /// increasing value per key to prevent signature reuse.
     pub nonce: u64,
 }
 
@@ -93,7 +97,7 @@ impl SignConsensusRequest {
     }
 
     pub fn builder(pubkey: BlsPublicKey) -> Self {
-        Self::new(pubkey, B256::ZERO, u64::MAX - 1)
+        Self::new(pubkey, B256::ZERO, 0)
     }
 
     pub fn with_root<R: Into<B256>>(self, object_root: R) -> Self {
@@ -125,6 +129,10 @@ impl Display for SignConsensusRequest {
 pub struct SignProxyRequest<T: ProxyId> {
     pub proxy: T,
     pub object_root: B256,
+    /// Replay-protection nonce mixed into the signing root via
+    /// `PropCommitSigningInfo`. Modules that do not track nonces should
+    /// send `0`. Modules that do track nonces should use a monotonically
+    /// increasing value per key to prevent signature reuse.
     pub nonce: u64,
 }
 
@@ -134,7 +142,7 @@ impl<T: ProxyId> SignProxyRequest<T> {
     }
 
     pub fn builder(proxy: T) -> Self {
-        Self::new(proxy, B256::ZERO, u64::MAX - 1)
+        Self::new(proxy, B256::ZERO, 0)
     }
 
     pub fn with_root<R: Into<B256>>(self, object_root: R) -> Self {

crates/common/src/config/module.rs

@@ -83,7 +83,7 @@ pub fn load_commit_module_config<T: DeserializeOwned>() -> Result<StartCommitMod
     struct StubConfig<U> {
         chain: Chain,
         modules: Vec<ThisModule<U>>,
-        signer: SignerConfig,
+        signer: Option<SignerConfig>,
     }
 
     // load module config including the extra data (if any)
@@ -106,7 +106,7 @@ pub fn load_commit_module_config<T: DeserializeOwned>() -> Result<StartCommitMod
         .find(|m| m.static_config.id == module_id)
         .wrap_err(format!("failed to find module for {module_id}"))?;
 
-    let certs_path = match cb_config.signer.tls_mode {
+    let certs_path = match cb_config.signer.context("No [signer] section in config")?.tls_mode {
         TlsMode::Insecure => None,
         TlsMode::Certificate(path) => Some(
             load_env_var(SIGNER_TLS_CERTIFICATES_PATH_ENV)

crates/common/src/config/pbs.rs

@@ -378,7 +378,7 @@ pub async fn load_pbs_custom_config<T: DeserializeOwned>() -> Result<(PbsModuleC
         chain: Chain,
         relays: Vec<RelayConfig>,
         pbs: CustomPbsConfig<U>,
-        signer: SignerConfig,
+        signer: Option<SignerConfig>,
         muxes: Option<PbsMuxes>,
     }
 
@@ -435,7 +435,11 @@ pub async fn load_pbs_custom_config<T: DeserializeOwned>() -> Result<(PbsModuleC
         // if custom pbs requires a signer client, load jwt
         let module_jwt = Jwt(load_env_var(MODULE_JWT_ENV)?);
         let signer_server_url = load_env_var(SIGNER_URL_ENV)?.parse()?;
-        let certs_path = match cb_config.signer.tls_mode {
+        let certs_path = match cb_config
+            .signer
+            .ok_or_else(|| eyre::eyre!("with_signer = true but no [signer] section in config"))?
+            .tls_mode
+        {
             TlsMode::Insecure => None,
             TlsMode::Certificate(path) => Some(
                 load_env_var(SIGNER_TLS_CERTIFICATES_PATH_ENV)

crates/common/src/config/signer.rs

@@ -92,10 +92,12 @@ impl Display for ReverseProxyHeaderSetup {
                 write!(f, "\"{header} (unique)\"")
             }
             ReverseProxyHeaderSetup::Rightmost { header, trusted_count } => {
-                let suffix = match trusted_count.get() % 10 {
-                    1 => "st",
-                    2 => "nd",
-                    3 => "rd",
+                let n = trusted_count.get();
+                let suffix = match (n % 100, n % 10) {
+                    (11..=13, _) => "th",
+                    (_, 1) => "st",
+                    (_, 2) => "nd",
+                    (_, 3) => "rd",
                     _ => "th",
                 };
                 write!(f, "\"{header} ({trusted_count}{suffix} from the right)\"")

crates/common/src/signature.rs

@@ -165,10 +165,18 @@ pub fn verify_proposer_commitment_signature_ecdsa(
 #[cfg(test)]
 mod tests {
 
-    use alloy::primitives::aliases::B32;
-
-    use super::compute_domain;
-    use crate::{constants::APPLICATION_BUILDER_DOMAIN, types::Chain};
+    use alloy::primitives::{U256, aliases::B32};
+
+    use super::{compute_domain, sign_builder_message, verify_signed_message};
+    use crate::{
+        constants::APPLICATION_BUILDER_DOMAIN,
+        pbs::{
+            BlindedBeaconBlockElectra, BuilderBid, BuilderBidElectra,
+            ExecutionPayloadHeaderElectra, ExecutionRequests,
+        },
+        types::{BlsSecretKey, Chain},
+        utils::TestRandomSeed,
+    };
 
     #[test]
     fn test_builder_domains() {
@@ -178,4 +186,48 @@ mod tests {
         assert_eq!(compute_domain(Chain::Sepolia, domain), Chain::Sepolia.builder_domain());
         assert_eq!(compute_domain(Chain::Hoodi, domain), Chain::Hoodi.builder_domain());
     }
+
+    #[test]
+    fn test_builder_bid_sign_and_verify() {
+        let secret_key = BlsSecretKey::test_random();
+        let pubkey = secret_key.public_key();
+
+        let message = BuilderBid::Electra(BuilderBidElectra {
+            header: ExecutionPayloadHeaderElectra::test_random(),
+            blob_kzg_commitments: Default::default(),
+            execution_requests: ExecutionRequests::default(),
+            value: U256::from(10),
+            pubkey: pubkey.clone().into(),
+        });
+
+        let sig = sign_builder_message(Chain::Mainnet, &secret_key, &message);
+
+        assert!(verify_signed_message(
+            Chain::Mainnet,
+            &pubkey,
+            &message,
+            &sig,
+            None,
+            &B32::from(APPLICATION_BUILDER_DOMAIN),
+        ));
+    }
+
+    #[test]
+    fn test_blinded_block_sign_and_verify() {
+        let secret_key = BlsSecretKey::test_random();
+        let pubkey = secret_key.public_key();
+
+        let block = BlindedBeaconBlockElectra::test_random();
+
+        let sig = sign_builder_message(Chain::Mainnet, &secret_key, &block);
+
+        assert!(verify_signed_message(
+            Chain::Mainnet,
+            &pubkey,
+            &block,
+            &sig,
+            None,
+            &B32::from(APPLICATION_BUILDER_DOMAIN),
+        ));
+    }
 }