[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,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

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 }

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

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));
         }

aptos-move/e2e-move-tests/src/tests/keyless_feature_gating.rs

@@ -24,89 +24,114 @@ 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<FeatureFlag>,
+    disabled_features: Vec<FeatureFlag>,
+) -> (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`.
 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)

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<u8>` trait implementation.
 #[derive(DeserializeKey, Clone, SerializeKey)]
 #[key_name("Secp256r1EcdsaSignature")]
 pub struct Signature(pub(crate) p256::ecdsa::Signature);

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:

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:

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:

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;
 

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)
     }