fix(standards): use initial storage state in auth components#2740
fix(standards): use initial storage state in auth components#2740partylikeits1983 wants to merge 7 commits intonextfrom
Conversation
partylikeits1983
force-pushed
the
ajl-init-pub-key-auth-fix
branch
from
af47f5a to
9b6a3d5
Compare
partylikeits1983
added
standards
PhilippGackstatter
left a comment
PhilippGackstatter
left a comment
There was a problem hiding this comment.
Looks good to me!
I think using get_initial* on scheme ID, public key and proc thresholds should be everything we need.
I think it's important to assert the exact error code in the tests, the other comments are optional.
| let (account, mock_chain, note) = setup_singlesig_with_mock_component(auth_scheme)?; | ||
|
|
||
| // Build the authenticator separately (same seed as Auth::BasicAuth uses) | ||
| let (_, authenticator) = Auth::BasicAuth { auth_scheme }.build_component(); |
There was a problem hiding this comment.
To make maintenance easier, can we return the authenticator from the setup function so these cannot get out of sync?
| .await | ||
| .expect("singlesig auth should use initial public key, not the rotated one"); | ||
|
|
||
| prove_and_verify_transaction(executed_tx).await?; |
There was a problem hiding this comment.
I'd only execute the transaction but not prove and verify, because our tests are already long-running and the additional proving and verification probably doesn't validate anything essential about this test.
| // This should FAIL because the auth procedure reads the initial key (A) but the | ||
| // authenticator signs with key B. The signature verification will not match. | ||
| let result = tx_context.execute().await; | ||
| assert!( | ||
| result.is_err(), | ||
| "transaction should fail when signed with the rotated key instead of the initial key" | ||
| ); |
There was a problem hiding this comment.
Can we assert the exact error code we expect using assert_transaction_executor_error!? Transactions can fail for many different reasons and in the current state, it doesn't serve as an actual regression test because signature verification could succeed, but the tx could still fail for some other benign reason, but the test won't alert us to that.
| // This should FAIL because the auth procedure reads the initial key (A) but the | ||
| // authenticator signs with key B. The signature verification will not match. | ||
| let result = tx_context.execute().await; | ||
| assert!( | ||
| result.is_err(), | ||
| "transaction should fail when signed with the rotated key instead of the initial key" | ||
| ); |
bobbinth
left a comment
bobbinth
left a comment
There was a problem hiding this comment.
Looks good! Thank you! (though, I only focused on the non-test code)
4 participants
Closes #2677
Auth components (
singlesig,singlesig_acl,multisig) were usingget_item/get_map_itemto read public keys, scheme IDs, and configuration during authentication. These procedures return the current storage state, which may have been modified during the transaction. During a key rotation, the old key must sign off on the change; usingget_itemwould incorrectly authenticate against the new (rotated) key.Changes:
Replaced all auth-time storage reads with their initial-state counterparts:
singlesig.masm:get_item->get_initial_itemfor public key and scheme ID slotssinglesig_acl.masm:get_item->get_initial_itemfor public key, scheme ID, and config slots; get_map_item → get_initial_map_item for trigger procedure rootsmultisig.masm: get_map_item ->
get_initial_map_iteminassert_proc_thresholds_lte_num_approversRegression Tests
Added key rotation regression tests for
singlesigandsinglesig_aclthat overwrite the public key slot mid-transaction and verify the auth procedure still succeeds using the initial key. If the bug were reintroduced, these tests would fail because signature verification would use the new (bogus) key.