From 2f357bef4eab17d8d2f68db5cf8517693123db06 Mon Sep 17 00:00:00 2001 From: Alin Tomescu Date: Thu, 14 Mar 2024 17:18:14 -0700 Subject: [PATCH] [keyless] update feature gating & match alg field (#12521) * keyless feature gating should allow zkless mode on its own * keyless txn verification should match the JWK alg with the JWT header alg --- CODEOWNERS | 10 +- Cargo.lock | 1 + aptos-move/aptos-vm/Cargo.toml | 1 + aptos-move/aptos-vm/src/aptos_vm.rs | 11 +- aptos-move/aptos-vm/src/keyless_validation.rs | 32 +++- .../src/tests/keyless_feature_gating.rs | 149 ++++++++++-------- .../secp256r1_ecdsa/secp256r1_ecdsa_sigs.rs | 3 +- .../generate-format/tests/staged/api.yaml | 5 + .../generate-format/tests/staged/aptos.yaml | 5 + .../tests/staged/consensus.yaml | 5 + types/src/keyless/mod.rs | 2 +- types/src/on_chain_config/aptos_features.rs | 4 +- types/src/transaction/authenticator.rs | 10 +- 13 files changed, 158 insertions(+), 80 deletions(-) diff --git a/CODEOWNERS b/CODEOWNERS index 563eedb0f1073a..eb383d5103da92 100644 --- a/CODEOWNERS +++ b/CODEOWNERS @@ -10,9 +10,12 @@ /aptos-move/aptos-aggregator/ @georgemitenkov @gelash @zekun000 /aptos-move/aptos-gas/ @vgao1996 /aptos-move/aptos-vm/ @davidiw @wrwg @zekun000 @vgao1996 @georgemitenkov +/aptos-move/aptos-vm/src/keyless_validation.rs @alinush @heliuchuan /aptos-move/aptos-vm-types/ @georgemitenkov @gelash @vgao1996 /aptos-move/framework/ @davidiw @movekevin @wrwg /aptos-move/framework/aptos-framework/sources/account.move @alinush +/aptos-move/framework/aptos-framework/sources/jwks.move @zjma +/aptos-move/framework/aptos-framework/sources/keyless_account.move @alinush /aptos-move/framework/aptos-stdlib/sources/cryptography/ @alinush @zjma @mstraka100 /aptos-move/framework/**/*.spec.move @junkil-park /aptos-move/framework/aptos-stdlib/sources/hash.move @alinush @@ -39,7 +42,7 @@ /consensus/src/quorum_store/ @bchocho @sasha8 @gelash # Owners for the `/crates/aptos` directory and all its subdirectories. -/crates/aptos @gregnazario @0xjinn @banool +/crates/aptos @gregnazario @banool # Owners for the `/crates/aptos-crypto*` directories. /crates/aptos-crypto-derive/ @alinush @zjma @mstraka100 @rex1fernando @@ -86,8 +89,8 @@ /docker/ @aptos-labs/prod-eng # Owners for the `SDKs`. -/ecosystem/python/sdk @davidiw @gregnazario @banool @0xmaayan @0xjinn -/ecosystem/typescript/sdk @gregnazario @banool @0xmaayan @0xjinn +/ecosystem/python/sdk @davidiw @gregnazario @banool @0xmaayan +/ecosystem/typescript/sdk @gregnazario @banool @0xmaayan # Owners for execution and storage. /execution/ @msmouse @lightmark @grao1991 @@ -110,4 +113,5 @@ # Owners for the `aptos-dkg` crate. /crates/aptos-dkg @alinush @rex1fernando +/types/src/keyless/ @alinush @heliuchuan /types/src/transaction/authenticator.rs @alinush @mstraka100 diff --git a/Cargo.lock b/Cargo.lock index fd78fc9470078e..21af7270772359 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -4282,6 +4282,7 @@ dependencies = [ "dashmap", "fail 0.5.1", "futures", + "hex", "jsonwebtoken 8.3.0", "move-binary-format", "move-bytecode-utils", diff --git a/aptos-move/aptos-vm/Cargo.toml b/aptos-move/aptos-vm/Cargo.toml index 79ff63345615d3..dd712d962da4e2 100644 --- a/aptos-move/aptos-vm/Cargo.toml +++ b/aptos-move/aptos-vm/Cargo.toml @@ -45,6 +45,7 @@ crossbeam-channel = { workspace = true } dashmap = { workspace = true } fail = { workspace = true } futures = { workspace = true } +hex = { workspace = true } jsonwebtoken = { workspace = true } move-binary-format = { workspace = true } move-bytecode-utils = { workspace = true } diff --git a/aptos-move/aptos-vm/src/aptos_vm.rs b/aptos-move/aptos-vm/src/aptos_vm.rs index 74c6439608399a..26030031952dfa 100644 --- a/aptos-move/aptos-vm/src/aptos_vm.rs +++ b/aptos-move/aptos-vm/src/aptos_vm.rs @@ -1425,12 +1425,11 @@ impl AptosVM { // If there are keyless TXN authenticators, validate them all. if !authenticators.is_empty() { - // Feature-gating keyless TXNs: if they are *not* enabled, return `FEATURE_UNDER_GATING`, - // which will discard the TXN from being put on-chain. - if !self.features.is_keyless_enabled() { - return Err(VMStatus::error(StatusCode::FEATURE_UNDER_GATING, None)); - } - keyless_validation::validate_authenticators(&authenticators, &self.features, resolver)?; + keyless_validation::validate_authenticators( + &authenticators, + self.features, + resolver, + )?; } // The prologue MUST be run AFTER any validation. Otherwise you may run prologue and hit diff --git a/aptos-move/aptos-vm/src/keyless_validation.rs b/aptos-move/aptos-vm/src/keyless_validation.rs index 81f8349e0c8411..51d5626f23738a 100644 --- a/aptos-move/aptos-vm/src/keyless_validation.rs +++ b/aptos-move/aptos-vm/src/keyless_validation.rs @@ -95,20 +95,44 @@ fn get_jwk_for_authenticator( let jwk = JWK::try_from(jwk_move_struct) .map_err(|_| invalid_signature!("Could not unpack Any in JWK Move struct"))?; + + match &jwk { + JWK::RSA(rsa_jwk) => { + if rsa_jwk.alg != jwt_header.alg { + return Err(invalid_signature!(format!( + "JWK alg ({}) does not match JWT header's alg ({})", + rsa_jwk.alg, jwt_header.alg + ))); + } + }, + JWK::Unsupported(jwk) => { + return Err(invalid_signature!(format!( + "JWK with KID {} and hex-encoded payload {} is not supported", + jwt_header.kid, + hex::encode(&jwk.payload) + ))) + }, + } + Ok(jwk) } +/// Ensures that **all** keyless authenticators in the transaction are valid. pub(crate) fn validate_authenticators( authenticators: &Vec<(KeylessPublicKey, KeylessSignature)>, features: &Features, resolver: &impl AptosMoveResolver, ) -> Result<(), VMStatus> { for (_, sig) in authenticators { - // Feature-gating for keyless-but-zkless TXNs: If keyless TXNs *are* enabled, and (1) this - // is a ZKless transaction but (2) ZKless TXNs are not yet enabled, discard the TXN from - // being put on-chain. + // Feature-gating for keyless TXNs (whether ZK or ZKless, whether passkey-based or not) + if matches!(sig.cert, EphemeralCertificate::ZeroKnowledgeSig { .. }) + && !features.is_zk_keyless_enabled() + { + return Err(VMStatus::error(StatusCode::FEATURE_UNDER_GATING, None)); + } + if matches!(sig.cert, EphemeralCertificate::OpenIdSig { .. }) - && !features.is_keyless_zkless_enabled() + && !features.is_zkless_keyless_enabled() { return Err(VMStatus::error(StatusCode::FEATURE_UNDER_GATING, None)); } diff --git a/aptos-move/e2e-move-tests/src/tests/keyless_feature_gating.rs b/aptos-move/e2e-move-tests/src/tests/keyless_feature_gating.rs index cd83e53decb2c7..2c5da51df9540b 100644 --- a/aptos-move/e2e-move-tests/src/tests/keyless_feature_gating.rs +++ b/aptos-move/e2e-move-tests/src/tests/keyless_feature_gating.rs @@ -24,81 +24,106 @@ use move_core_types::{ vm_status::StatusCode::FEATURE_UNDER_GATING, }; -#[test] -fn test_keyless_disabled() { - let mut h = MoveHarness::new_with_features(vec![], vec![FeatureFlag::KEYLESS_ACCOUNTS]); +fn init_feature_gating( + enabled_features: Vec, + disabled_features: Vec, +) -> (MoveHarness, Account) { + let mut h = MoveHarness::new_with_features(enabled_features, disabled_features); - let (sig, pk) = get_sample_groth16_sig_and_pk(); - let bob = h.new_account_at(AccountAddress::from_hex_literal("0xb0b").unwrap()); + let recipient = h.new_account_at(AccountAddress::from_hex_literal("0xb0b").unwrap()); - let transaction = get_keyless_txn(&mut h, sig, pk, bob); + // initialize JWKs + run_setup_script(&mut h); - let output = h.run_raw(transaction); - match output.status() { - TransactionStatus::Discard(status) => { - assert_eq!(*status, FEATURE_UNDER_GATING) - }, - _ => { - panic!("Expected to get FEATURE_UNDER_GATING DiscardedVMStatus") - }, - } + (h, recipient) } -#[test] -fn test_keyless_enabled() { - let mut h = MoveHarness::new_with_features(vec![FeatureFlag::KEYLESS_ACCOUNTS], vec![]); - - let (sig, pk) = get_sample_groth16_sig_and_pk(); - let bob = h.new_account_at(AccountAddress::from_hex_literal("0xb0b").unwrap()); - - // initialize JWK - run_setup_script(&mut h); - - let transaction = get_keyless_txn(&mut h, sig, pk, bob); +fn test_feature_gating( + h: &mut MoveHarness, + recipient: &Account, + get_sig_and_pk: fn() -> (KeylessSignature, KeylessPublicKey), + should_succeed: bool, +) { + let (sig, pk) = get_sig_and_pk(); + let transaction = get_keyless_txn(h, sig, pk, *recipient.address()); let output = h.run_raw(transaction); - assert_success!(output.status().clone()); + + if !should_succeed { + match output.status() { + TransactionStatus::Discard(status) => { + assert_eq!( + *status, FEATURE_UNDER_GATING, + "Expected TransactionStatus::Discard to be FEATURE_UNDER_GATING, but got: {:?}", + status + ) + }, + _ => { + panic!( + "Expected to get a TransactionStatus::Discard, but got: {:?}", + output.status() + ) + }, + } + } else { + assert_success!( + output.status().clone(), + "Expected TransactionStatus::Keep(ExecutionStatus::Success), but got: {:?}", + output.status() + ); + } } #[test] -fn test_keyless_enabled_but_zkless_disabled() { - let mut h = MoveHarness::new_with_features(vec![FeatureFlag::KEYLESS_ACCOUNTS], vec![ +fn test_feature_gating_with_zk_on() { + // + // ZK & ZKless + let (mut h, recipient) = init_feature_gating( + vec![ + FeatureFlag::KEYLESS_ACCOUNTS, + FeatureFlag::KEYLESS_BUT_ZKLESS_ACCOUNTS, + ], + vec![], + ); + // Groth16-based sig => success + test_feature_gating(&mut h, &recipient, get_sample_groth16_sig_and_pk, true); + // OIDC-based sig => success + test_feature_gating(&mut h, &recipient, get_sample_openid_sig_and_pk, true); + + // + // ZK & !ZKless + let (mut h, recipient) = init_feature_gating(vec![FeatureFlag::KEYLESS_ACCOUNTS], vec![ FeatureFlag::KEYLESS_BUT_ZKLESS_ACCOUNTS, ]); - - let (sig, pk) = get_sample_openid_sig_and_pk(); - let bob = h.new_account_at(AccountAddress::from_hex_literal("0xb0b").unwrap()); - - // initialize JWK - run_setup_script(&mut h); - - let transaction = get_keyless_txn(&mut h, sig, pk, bob); - - let output = h.run_raw(transaction); - match output.status() { - TransactionStatus::Discard(status) => { - assert_eq!(*status, FEATURE_UNDER_GATING) - }, - _ => { - panic!("Expected to get FEATURE_UNDER_GATING DiscardedVMStatus") - }, - } + // Groth16-based sig => success + test_feature_gating(&mut h, &recipient, get_sample_groth16_sig_and_pk, true); + // OIDC-based sig => discard + test_feature_gating(&mut h, &recipient, get_sample_openid_sig_and_pk, false); } #[test] -fn test_keyless_enabled_but_zkless_enabled() { - let mut h = MoveHarness::new_with_features(vec![FeatureFlag::KEYLESS_ACCOUNTS], vec![]); - - let (sig, pk) = get_sample_openid_sig_and_pk(); - let bob = h.new_account_at(AccountAddress::from_hex_literal("0xb0b").unwrap()); - - // initialize JWK - run_setup_script(&mut h); - - let transaction = get_keyless_txn(&mut h, sig, pk, bob); - - let output = h.run_raw(transaction); - assert_success!(output.status().clone()); +fn test_feature_gating_with_zk_off() { + // + // !ZK & ZKless + let (mut h, recipient) = + init_feature_gating(vec![FeatureFlag::KEYLESS_BUT_ZKLESS_ACCOUNTS], vec![ + FeatureFlag::KEYLESS_ACCOUNTS, + ]); + // Groth16-based sig => discard + test_feature_gating(&mut h, &recipient, get_sample_groth16_sig_and_pk, false); + // OIDC-based sig => success + test_feature_gating(&mut h, &recipient, get_sample_openid_sig_and_pk, true); + + // + // !ZK & !ZKless + let (mut h, recipient) = init_feature_gating(vec![], vec![ + FeatureFlag::KEYLESS_ACCOUNTS, + FeatureFlag::KEYLESS_BUT_ZKLESS_ACCOUNTS, + ]); + // Groth16-based sig => discard + test_feature_gating(&mut h, &recipient, get_sample_groth16_sig_and_pk, false); + // OIDC-based sig => discard + test_feature_gating(&mut h, &recipient, get_sample_openid_sig_and_pk, false); } /// Creates and funds a new account at `pk` and sends coins to `recipient`. @@ -106,7 +131,7 @@ fn get_keyless_txn( h: &mut MoveHarness, mut sig: KeylessSignature, pk: KeylessPublicKey, - recipient: Account, + recipient: AccountAddress, ) -> SignedTransaction { let apk = AnyPublicKey::keyless(pk.clone()); let addr = AuthenticationKey::any_key(apk.clone()).account_address(); @@ -119,7 +144,7 @@ fn get_keyless_txn( println!("Actual address: {}", addr.to_hex()); println!("Account address: {}", account.address().to_hex()); - let payload = aptos_stdlib::aptos_coin_transfer(*recipient.address(), 1); + let payload = aptos_stdlib::aptos_coin_transfer(recipient, 1); //println!("Payload: {:?}", payload); let raw_txn = TransactionBuilder::new(account.clone()) .payload(payload) diff --git a/crates/aptos-crypto/src/secp256r1_ecdsa/secp256r1_ecdsa_sigs.rs b/crates/aptos-crypto/src/secp256r1_ecdsa/secp256r1_ecdsa_sigs.rs index 6bf5d80f3a415d..28cfb761051700 100644 --- a/crates/aptos-crypto/src/secp256r1_ecdsa/secp256r1_ecdsa_sigs.rs +++ b/crates/aptos-crypto/src/secp256r1_ecdsa/secp256r1_ecdsa_sigs.rs @@ -17,7 +17,8 @@ use serde::Serialize; use signature::Verifier; use std::{cmp::Ordering, fmt}; -/// A Secp256r1 ECDSA signature +/// A secp256r1 ECDSA signature. +/// NOTE: The max size on this struct is enforced in its `TryFrom` trait implementation. #[derive(DeserializeKey, Clone, SerializeKey)] #[key_name("Secp256r1EcdsaSignature")] pub struct Signature(pub(crate) p256::ecdsa::Signature); diff --git a/testsuite/generate-format/tests/staged/api.yaml b/testsuite/generate-format/tests/staged/api.yaml index 224ed4788f26f9..350cdd4314c387 100644 --- a/testsuite/generate-format/tests/staged/api.yaml +++ b/testsuite/generate-format/tests/staged/api.yaml @@ -237,6 +237,11 @@ EphemeralPublicKey: STRUCT: - public_key: TYPENAME: Ed25519PublicKey + 1: + Secp256r1Ecdsa: + STRUCT: + - public_key: + TYPENAME: Secp256r1EcdsaPublicKey EphemeralSignature: ENUM: 0: diff --git a/testsuite/generate-format/tests/staged/aptos.yaml b/testsuite/generate-format/tests/staged/aptos.yaml index 209dfd54dfbec2..b3790517f5636a 100644 --- a/testsuite/generate-format/tests/staged/aptos.yaml +++ b/testsuite/generate-format/tests/staged/aptos.yaml @@ -214,6 +214,11 @@ EphemeralPublicKey: STRUCT: - public_key: TYPENAME: Ed25519PublicKey + 1: + Secp256r1Ecdsa: + STRUCT: + - public_key: + TYPENAME: Secp256r1EcdsaPublicKey EphemeralSignature: ENUM: 0: diff --git a/testsuite/generate-format/tests/staged/consensus.yaml b/testsuite/generate-format/tests/staged/consensus.yaml index a57e463d15b40b..17bc09bc8763c5 100644 --- a/testsuite/generate-format/tests/staged/consensus.yaml +++ b/testsuite/generate-format/tests/staged/consensus.yaml @@ -459,6 +459,11 @@ EphemeralPublicKey: STRUCT: - public_key: TYPENAME: Ed25519PublicKey + 1: + Secp256r1Ecdsa: + STRUCT: + - public_key: + TYPENAME: Secp256r1EcdsaPublicKey EphemeralSignature: ENUM: 0: diff --git a/types/src/keyless/mod.rs b/types/src/keyless/mod.rs index 0a3ed51c32d52a..a42c94d142a34b 100644 --- a/types/src/keyless/mod.rs +++ b/types/src/keyless/mod.rs @@ -132,7 +132,7 @@ pub struct JWTHeader { } impl KeylessSignature { - /// A reasonable upper bound for the number of bytes we expect in a keyless public key. This is + /// A reasonable upper bound for the number of bytes we expect in a keyless signature. This is /// enforced by our full nodes when they receive TXNs. pub const MAX_LEN: usize = 4000; diff --git a/types/src/on_chain_config/aptos_features.rs b/types/src/on_chain_config/aptos_features.rs index fa39c70e60fe86..b732eedd02d44e 100644 --- a/types/src/on_chain_config/aptos_features.rs +++ b/types/src/on_chain_config/aptos_features.rs @@ -223,7 +223,7 @@ impl Features { /// Whether the keyless accounts feature is enabled, specifically the ZK path with ZKP-based signatures. /// The ZK-less path is controlled via a different `FeatureFlag::KEYLESS_BUT_ZKLESS_ACCOUNTS` flag. - pub fn is_keyless_enabled(&self) -> bool { + pub fn is_zk_keyless_enabled(&self) -> bool { self.is_enabled(FeatureFlag::KEYLESS_ACCOUNTS) } @@ -233,7 +233,7 @@ impl Features { /// safety precaution in case of emergency (e.g., if the ZK-based signatures must be temporarily /// turned off due to a zeroday exploit, the ZK-less path will still allow users to transact, /// but without privacy). - pub fn is_keyless_zkless_enabled(&self) -> bool { + pub fn is_zkless_keyless_enabled(&self) -> bool { self.is_enabled(FeatureFlag::KEYLESS_BUT_ZKLESS_ACCOUNTS) } diff --git a/types/src/transaction/authenticator.rs b/types/src/transaction/authenticator.rs index 67efa5370c0359..6af3cd64fb6eec 100644 --- a/types/src/transaction/authenticator.rs +++ b/types/src/transaction/authenticator.rs @@ -1164,12 +1164,20 @@ impl<'de> Deserialize<'de> for EphemeralPublicKey { #[derive(::serde::Deserialize)] #[serde(rename = "EphemeralPublicKey")] enum Value { - Ed25519 { public_key: Ed25519PublicKey }, + Ed25519 { + public_key: Ed25519PublicKey, + }, + Secp256r1Ecdsa { + public_key: secp256r1_ecdsa::PublicKey, + }, } let value = Value::deserialize(deserializer)?; Ok(match value { Value::Ed25519 { public_key } => EphemeralPublicKey::Ed25519 { public_key }, + Value::Secp256r1Ecdsa { public_key } => { + EphemeralPublicKey::Secp256r1Ecdsa { public_key } + }, }) } }