From 561ae2aa743acc3db592f4d69fc9f6f105c00f72 Mon Sep 17 00:00:00 2001 From: Hannes Karppila Date: Tue, 27 Aug 2024 16:01:33 +0300 Subject: [PATCH 1/7] Perform recovery id correction for KMS signatures as well --- bin/fuel-core/src/cli/run.rs | 22 ++++---- .../consensus_module/poa/src/signer.rs | 54 +++++++++++++++++-- tests/tests/poa.rs | 8 +-- 3 files changed, 64 insertions(+), 20 deletions(-) diff --git a/bin/fuel-core/src/cli/run.rs b/bin/fuel-core/src/cli/run.rs index e620a0f9f78..4c32075056d 100644 --- a/bin/fuel-core/src/cli/run.rs +++ b/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) { diff --git a/crates/services/consensus_module/poa/src/signer.rs b/crates/services/consensus_module/poa/src/signer.rs index 264a5dfbc3a..abb6665f023 100644 --- a/crates/services/consensus_module/poa/src/signer.rs +++ b/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, }, } @@ -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 { + 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)] diff --git a/tests/tests/poa.rs b/tests/tests/poa.rs index 6f8c2b93dc6..8398fbf714c 100644 --- a/tests/tests/poa.rs +++ b/tests/tests/poa.rs @@ -114,12 +114,8 @@ 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 = 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 From b99a526f6ae2c2ffe6341b80cf75953b7078e717 Mon Sep 17 00:00:00 2001 From: Hannes Karppila Date: Tue, 27 Aug 2024 16:07:15 +0300 Subject: [PATCH 2/7] Add changelog entry --- CHANGELOG.md | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 5ee9c5d2a1c..5eec5f12350 100644 --- a/CHANGELOG.md +++ b/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] From e617afdf1799cb298981194db77a56ac6e12c246 Mon Sep 17 00:00:00 2001 From: Hannes Karppila Date: Tue, 27 Aug 2024 16:11:34 +0300 Subject: [PATCH 3/7] Ensure the unchangedness of kms public key recovery with a test --- tests/tests/poa.rs | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/tests/tests/poa.rs b/tests/tests/poa.rs index 8398fbf714c..7ef954dcee6 100644 --- a/tests/tests/poa.rs +++ b/tests/tests/poa.rs @@ -95,6 +95,7 @@ async fn can_get_sealed_block_from_poa_produced_block() { #[tokio::test] #[cfg(feature = "aws-kms")] async fn can_get_sealed_block_from_poa_produced_block_when_signing_with_kms() { + use fuel_core::producer::block_producer; use fuel_core_types::fuel_crypto::PublicKey; // This test is only enabled if the environment variable is set @@ -141,7 +142,8 @@ 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()) @@ -155,6 +157,12 @@ async fn can_get_sealed_block_from_poa_produced_block_when_signing_with_kms() { signature .verify(&poa_public, &block_id.into_message()) .expect("failed to verify signature"); + let this_bp = signature.block_producer(); + if let Some(bp) = block_producer { + assert_eq!(bp, this_bp, "Block producer changed"); + } else { + block_producer = Some(this_bp); + } } } From 3eb76eedb9dfdf7261325d0cd13b92d7ae7a7dd4 Mon Sep 17 00:00:00 2001 From: Hannes Karppila Date: Tue, 27 Aug 2024 16:12:14 +0300 Subject: [PATCH 4/7] Clippy --- crates/services/consensus_module/poa/src/signer.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/services/consensus_module/poa/src/signer.rs b/crates/services/consensus_module/poa/src/signer.rs index abb6665f023..cd4e65fbdf8 100644 --- a/crates/services/consensus_module/poa/src/signer.rs +++ b/crates/services/consensus_module/poa/src/signer.rs @@ -115,7 +115,7 @@ async fn sign_with_kms( 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) + let correct_public_key = k256::PublicKey::from_public_key_der(public_key_bytes) .map_err(|_| anyhow!("invalid DER public key from AWS KMS"))? .into(); From 0691f5131fe69c3a392d3b01fb442e394010df54 Mon Sep 17 00:00:00 2001 From: Hannes Karppila Date: Tue, 27 Aug 2024 16:18:10 +0300 Subject: [PATCH 5/7] Clippy --- crates/services/consensus_module/poa/src/signer.rs | 1 + tests/tests/poa.rs | 11 +++++++---- 2 files changed, 8 insertions(+), 4 deletions(-) diff --git a/crates/services/consensus_module/poa/src/signer.rs b/crates/services/consensus_module/poa/src/signer.rs index cd4e65fbdf8..d34a18d2530 100644 --- a/crates/services/consensus_module/poa/src/signer.rs +++ b/crates/services/consensus_module/poa/src/signer.rs @@ -168,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()) }; diff --git a/tests/tests/poa.rs b/tests/tests/poa.rs index 7ef954dcee6..c64085e15e7 100644 --- a/tests/tests/poa.rs +++ b/tests/tests/poa.rs @@ -95,8 +95,8 @@ async fn can_get_sealed_block_from_poa_produced_block() { #[tokio::test] #[cfg(feature = "aws-kms")] async fn can_get_sealed_block_from_poa_produced_block_when_signing_with_kms() { - use fuel_core::producer::block_producer; 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 { @@ -117,7 +117,7 @@ async fn can_get_sealed_block_from_poa_produced_block_when_signing_with_kms() { .into_inner(); 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); + let poa_public = PublicKey::from(poa_public); // start node with the kms enabled and produce some blocks let num_blocks = 100; @@ -151,13 +151,16 @@ async fn can_get_sealed_block_from_poa_produced_block_when_signing_with_kms() { .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 = signature.block_producer(); + 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 { From 8ed3df7eaee1b438021e313b6cf1aa4f2f876521 Mon Sep 17 00:00:00 2001 From: Hannes Karppila Date: Wed, 28 Aug 2024 11:56:50 +0300 Subject: [PATCH 6/7] Change panic to an error in case of reduced-x coordinate --- crates/services/consensus_module/poa/src/signer.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/crates/services/consensus_module/poa/src/signer.rs b/crates/services/consensus_module/poa/src/signer.rs index d34a18d2530..efd82edeb0b 100644 --- a/crates/services/consensus_module/poa/src/signer.rs +++ b/crates/services/consensus_module/poa/src/signer.rs @@ -124,13 +124,13 @@ async fn sign_with_kms( } else if rec2.map(|r| r == correct_public_key).unwrap_or(false) { recid2 } else { - unreachable!("Invalid signature generated"); + anyhow::bail!("Invalid signature generated (reduced-x form coordinate)"); }; // Insert the recovery id into the signature - assert!( + debug_assert!( !recovery_id.is_x_reduced(), - "reduced-x form coordinates are never generated by this" + "reduced-x form coordinates are cought by the if-else chain above" ); let v = recovery_id.is_y_odd() as u8; let mut signature = <[u8; 64]>::from(sig.to_bytes()); From 34c5e39718ca5179858b3ba538a1b1844800dd34 Mon Sep 17 00:00:00 2001 From: Hannes Karppila Date: Wed, 28 Aug 2024 11:57:51 +0300 Subject: [PATCH 7/7] Fix typo --- crates/services/consensus_module/poa/src/signer.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/services/consensus_module/poa/src/signer.rs b/crates/services/consensus_module/poa/src/signer.rs index efd82edeb0b..f9ba98d3599 100644 --- a/crates/services/consensus_module/poa/src/signer.rs +++ b/crates/services/consensus_module/poa/src/signer.rs @@ -130,7 +130,7 @@ async fn sign_with_kms( // Insert the recovery id into the signature debug_assert!( !recovery_id.is_x_reduced(), - "reduced-x form coordinates are cought by the if-else chain above" + "reduced-x form coordinates are caught by the if-else chain above" ); let v = recovery_id.is_y_odd() as u8; let mut signature = <[u8; 64]>::from(sig.to_bytes());