Secp256k1 RecoveryID correction for KMS-genrated signatures

CHANGELOG.md

@@ -8,6 +8,8 @@ and this project adheres to [Semantic Versioning](http://semver.org/).
 
 ### Added
 - [2122](https://github.com/FuelLabs/fuel-core/pull/2122): Changed the relayer URI address to be a vector and use a quorum provider. The `relayer` argument now supports multiple URLs to fetch information from different sources.
+- [2119](https://github.com/FuelLabs/fuel-core/pull/2119): GraphQL query fields for retrieving information about upgrades.
+- [2116](https://github.com/FuelLabs/fuel-core/pull/2116): Replace `H160` in config and cli options of relayer by `Bytes20` of `fuel-types`
 
 ### Changed
 - [2113](https://github.com/FuelLabs/fuel-core/pull/2113): Modify the way the gas price service and shared algo is initialized to have some default value based on best guess instead of `None`, and initialize service before graphql.
@@ -16,9 +18,8 @@ and this project adheres to [Semantic Versioning](http://semver.org/).
 - [2115](https://github.com/FuelLabs/fuel-core/pull/2115): Add test for `SignMode` `is_available` method.
 - [2124](https://github.com/FuelLabs/fuel-core/pull/2124): Generalize the way p2p req/res protocol handles requests.
 
-### Added
-- [2119](https://github.com/FuelLabs/fuel-core/pull/2119): GraphQL query fields for retrieving information about upgrades.
-- [2116](https://github.com/FuelLabs/fuel-core/pull/2116): Replace `H160` in config and cli options of relayer by `Bytes20` of `fuel-types`
+### Fixed
+- [2134](https://github.com/FuelLabs/fuel-core/pull/2134): Perform RecoveryID normalization for AWS KMS -generated signatures.
 
 ## [Version 0.34.0]
 

bin/fuel-core/src/cli/run.rs

@@ -323,17 +323,21 @@ impl Command {
         if let Some(key_id) = consensus_aws_kms {
             let config = aws_config::load_from_env().await;
             let client = aws_sdk_kms::Client::new(&config);
-            // Ensure that the key is accessible and has the correct type
-            let key = client
-                .get_public_key()
-                .key_id(&key_id)
-                .send()
-                .await?
-                .key_spec;
-            if key != Some(aws_sdk_kms::types::KeySpec::EccSecgP256K1) {
+            // Get the public key, and ensure that the signing key
+            // is accessible and has the correct type.
+            let key = client.get_public_key().key_id(&key_id).send().await?;
+            if key.key_spec != Some(aws_sdk_kms::types::KeySpec::EccSecgP256K1) {
                 anyhow::bail!("The key is not of the correct type, got {:?}", key);
             }
-            consensus_signer = SignMode::Kms { key_id, client };
+            let Some(cached_public_key) = key.public_key() else {
+                anyhow::bail!("AWS KMS did not return a public key when requested");
+            };
+            let cached_public_key_bytes = cached_public_key.clone().into_inner();
+            consensus_signer = SignMode::Kms {
+                key_id,
+                client,
+                cached_public_key_bytes,
+            };
         }
 
         if matches!(consensus_signer, SignMode::Unavailable) {

crates/services/consensus_module/poa/src/signer.rs

@@ -38,6 +38,7 @@ pub enum SignMode {
     Kms {
         key_id: String,
         client: aws_sdk_kms::Client,
+        cached_public_key_bytes: Vec<u8>,
     },
 }
 
@@ -59,9 +60,11 @@ impl SignMode {
                 Signature::sign(signing_key, &message)
             }
             #[cfg(feature = "aws-kms")]
-            SignMode::Kms { key_id, client } => {
-                sign_with_kms(client, key_id, message).await?
-            }
+            SignMode::Kms {
+                key_id,
+                client,
+                cached_public_key_bytes,
+            } => sign_with_kms(client, key_id, cached_public_key_bytes, message).await?,
         };
         Ok(Consensus::PoA(PoAConsensus::new(poa_signature)))
     }
@@ -71,8 +74,17 @@ impl SignMode {
 async fn sign_with_kms(
     client: &aws_sdk_kms::Client,
     key_id: &str,
+    public_key_bytes: &[u8],
     message: Message,
 ) -> anyhow::Result<Signature> {
+    use k256::{
+        ecdsa::{
+            RecoveryId,
+            VerifyingKey,
+        },
+        pkcs8::DecodePublicKey,
+    };
+
     let reply = client
         .sign()
         .key_id(key_id)
@@ -90,8 +102,40 @@ async fn sign_with_kms(
     let sig = k256::ecdsa::Signature::from_der(&signature_der)
         .map_err(|_| anyhow!("invalid DER signature from AWS KMS"))?;
     let sig = sig.normalize_s().unwrap_or(sig);
-    let sig_bytes = <[u8; 64]>::from(sig.to_bytes());
-    Ok(Signature::from_bytes(sig_bytes))
+
+    // This is a hack to get the recovery id. The signature should be normalized
+    // before computing the recovery id, but aws kms doesn't support this, and
+    // instead always computes the recovery id from non-normalized signature.
+    // So instead the recovery id is determined by checking which variant matches
+    // the original public key.
+
+    let recid1 = RecoveryId::new(false, false);
+    let recid2 = RecoveryId::new(true, false);
+
+    let rec1 = VerifyingKey::recover_from_prehash(&*message, &sig, recid1);
+    let rec2 = VerifyingKey::recover_from_prehash(&*message, &sig, recid2);
+
+    let correct_public_key = k256::PublicKey::from_public_key_der(public_key_bytes)
+        .map_err(|_| anyhow!("invalid DER public key from AWS KMS"))?
+        .into();
+
+    let recovery_id = if rec1.map(|r| r == correct_public_key).unwrap_or(false) {
+        recid1
+    } else if rec2.map(|r| r == correct_public_key).unwrap_or(false) {
+        recid2
+    } else {
+        unreachable!("Invalid signature generated");
+    };
+
+    // Insert the recovery id into the signature
+    assert!(
+        !recovery_id.is_x_reduced(),
+        "reduced-x form coordinates are never generated by this"
+    );
+    let v = recovery_id.is_y_odd() as u8;
+    let mut signature = <[u8; 64]>::from(sig.to_bytes());
+    signature[32] = (v << 7) | (signature[32] & 0x7f);
+    Ok(Signature::from_bytes(signature))
 }
 
 #[cfg(test)]
@@ -124,6 +168,7 @@ mod tests {
             assert!(SignMode::Kms {
                 key_id: kms_arn.to_string(),
                 client: kms_client,
+                cached_public_key_bytes: vec![], // Dummy value is ok for this test
             }
             .is_available())
         };

tests/tests/poa.rs

@@ -96,6 +96,7 @@ async fn can_get_sealed_block_from_poa_produced_block() {
 #[cfg(feature = "aws-kms")]
 async fn can_get_sealed_block_from_poa_produced_block_when_signing_with_kms() {
     use fuel_core_types::fuel_crypto::PublicKey;
+    use k256::pkcs8::DecodePublicKey;
 
     // This test is only enabled if the environment variable is set
     let Some(kms_arn) = option_env!("FUEL_CORE_TEST_AWS_KMS_ARN") else {
@@ -114,13 +115,9 @@ async fn can_get_sealed_block_from_poa_produced_block_when_signing_with_kms() {
         .public_key
         .unwrap()
         .into_inner();
-    let poa_public_bytes = spki::SubjectPublicKeyInfoRef::try_from(&*poa_public_der)
-        .expect("invalid DER signature from AWS KMS")
-        .subject_public_key
-        .raw_bytes();
-    let poa_public = k256::ecdsa::VerifyingKey::from_sec1_bytes(poa_public_bytes)
-        .expect("invalid public key");
-    let poa_public = PublicKey::from(&poa_public);
+    let poa_public = k256::PublicKey::from_public_key_der(&poa_public_der)
+        .expect("invalid DER public key from AWS KMS");
+    let poa_public = PublicKey::from(poa_public);
 
     // start node with the kms enabled and produce some blocks
     let num_blocks = 100;
@@ -145,20 +142,30 @@ async fn can_get_sealed_block_from_poa_produced_block_when_signing_with_kms() {
 
     let view = db.on_chain().latest_view().unwrap();
 
-    // verify signatures
+    // verify signatures and ensure that the block producer wont change
+    let mut block_producer = None;
     for height in 1..=num_blocks {
         let sealed_block = view
             .get_sealed_block_by_height(&height.into())
             .unwrap()
             .expect("expected sealed block to be available");
         let block_id = sealed_block.entity.id();
         let signature = match sealed_block.consensus {
-            Consensus::PoA(poa) => poa.signature,
+            Consensus::PoA(ref poa) => poa.signature,
             _ => panic!("Not expected consensus"),
         };
         signature
             .verify(&poa_public, &block_id.into_message())
             .expect("failed to verify signature");
+        let this_bp = sealed_block
+            .consensus
+            .block_producer(&block_id)
+            .expect("Block should have a block producer");
+        if let Some(bp) = block_producer {
+            assert_eq!(bp, this_bp, "Block producer changed");
+        } else {
+            block_producer = Some(this_bp);
+        }
     }
 }