Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Framework] Safe onchain key rotation address mapping for standard accounts #14309

Merged
merged 3 commits into from
Nov 6, 2024

Conversation

alnoki
Copy link
Contributor

@alnoki alnoki commented Aug 18, 2024

@banool @gregnazario

Closes #13517

AIP: aptos-foundation/AIPs#487

Background

There are currently several issues with the OriginatingAddress table (which is
supposed to be a one-to-one lookup table) that render the mapping unsafe in
practice:

  1. Per [Framework][Key rotation] Add OriginatingAddress followup reconciliation function for key rotations without a proof challenge #13517, rotate_authentication_key_call does not update the
    OriginatingAddress table for an "unproven" key rotation without a
    RotationProofChallenge (resolved in this PR with new
    set_originating_address private entry function).
  2. During an operation that attempts to map an authentication key (which has
    already been mapped) to a different originating address, the inner function
    update_auth_key_and_originating_address_table overwrites the initial
    mapping, rather than aborting. This oversight can lead to account loss if
    someone accidentally attempts to rotate to the same authentication key twice
    (resolved in this PR with ENEW_AUTH_KEY_ALREADY_MAPPED check), because they
    will not be able to identify their account from private key alone unless they
    keep an external record of the rotated accounts the private key in question
    has been used to secure.
  3. Standard accounts that have not yet had their key rotated are not registered
    in the OriginatingAddress table, such that two accounts can be
    authenticated by the same authentication key: the original account whose
    address is its authentication key, and another account that has had its
    authentication key rotated to the authentication key of the original account.
    Since OriginatingAddress is one-to-one, a dual-account situation can
    inhibit indexing and OpSec (resolved in this PR with
    set_originating_address private entry function).

Contribution history

I originally proposed a large set of changes associated with authentication key
rotation for accounts secured by a Ledger wallet in #11151, which paired with a
docs PR at aptos-labs/developer-docs#367. Due to the key
rotation issues raised during development functionality, I included the features
now isolated in this PR.

However, due to the difficulty of merging large sets of changes as an external
contributor, I'm splitting off into this PR the minimum amount of functionality
required to support safe authentication key rotation mapping, as I did similarly
in #14084 and #14266.

Verifying the changes in this PR

  1. Install the Aptos CLI from source using the changes in this PR.

  2. Make a new test directory called localnet-data, then use it to start a
    localnet with the framework changes in this PR:

    aptos node run-localnet --test-dir localnet-data
  3. Save the localnet shell running off to the side.

  4. In a new shell, create a private key file:

    aptos key generate --output-file keyfile-a
  5. Use it to create a localnet profile:

    aptos init \
        --network local \
        --private-key-file keyfile-a \
        --profile localnet-a
  6. Store the address:

    ADDR_A=<profile-address>
  7. Use the new originating_address view function to observe that the account
    does not have an entry in the OriginatingAddress table:

    aptos move view \
        --args address:$ADDR_A \
        --function-id 0x1::account::originating_address \
        --profile localnet-a
  8. Use the new set_originating_address private entry function to set a mapping
    in the table:

    aptos move run \
        --function-id 0x1::account::set_originating_address \
        --profile localnet-a
  9. Check the originating_address view function again and note the result:

    aptos move view \
        --args address:$ADDR_A \
        --function-id 0x1::account::originating_address \
        --profile localnet-a
  10. Now that you've established a one-to-one mapping for the authentication key,
    the new check for ENEW_AUTH_KEY_ALREADY_MAPPED in
    update_auth_key_and_originating_address_table will prevent another account
    from rotating its authentication key to that of keyfile-a, thus preserving
    a one-to-one mapping. To verify this, create a new profile:

    aptos init \
        --network local \
        --profile localnet-b
  11. Press enter when prompted to generate a new private key for the profile.
    Then observe the new guard against breaking the one-to-one mapping, by
    trying to rotate the authentication key to that of keyfile-a:

    aptos account rotate-key \
        --new-private-key-file keyfile-a \
        --profile localnet-b \
        --save-to-profile localnet-b-secured-by-keyfile-a

Housekeeping

The following was run from repository root to ensure CI passes:

cargo build -p aptos-cached-packages
pre-commit run --all-files
scripts/rust_lint.sh
aptos move test --package-dir aptos-move/framework/aptos-framework/ --dev

Copy link

trunk-io bot commented Aug 18, 2024

⏱️ 9s total CI duration on this PR

Job Cumulative Duration Recent Runs
permission-check 3s 🟥
permission-check 2s 🟥
permission-check 2s 🟥
permission-check 2s 🟥

settingsfeedbackdocs ⋅ learn more about trunk.io

@alnoki alnoki changed the title [Framework] Safe authentication key rotation address mapping [Framework] Safe onchain key rotation address mapping for standard accounts Aug 18, 2024
Copy link
Contributor

github-actions bot commented Oct 3, 2024

This issue is stale because it has been open 45 days with no activity. Remove the stale label, comment or push a commit - otherwise this will be closed in 15 days.

@github-actions github-actions bot added the Stale label Oct 3, 2024
@alnoki
Copy link
Contributor Author

alnoki commented Oct 3, 2024

Bumping per stale label and recently merged AIP

aptos-foundation/AIPs#499
aptos-foundation/AIPs#514

@github-actions github-actions bot removed the Stale label Oct 4, 2024
@thepomeranian
Copy link
Contributor

This PR is related to AIP-101 and has been approved/ready to land. aptos-foundation/AIPs#487

@sherry-x sherry-x enabled auto-merge (rebase) November 4, 2024 21:32

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

@alinush
Copy link
Contributor

alinush commented Nov 5, 2024

cc @junkil-park: just a heads up that a new private entry function has been added and I tried taking care of the Move Prover errors myself by using pragma verify=false;. (Let's see if that worked.)

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

Copy link
Contributor

github-actions bot commented Nov 6, 2024

✅ Forge suite realistic_env_max_load success on 93942524619d497fdcb75db8c994418e783e4426

two traffics test: inner traffic : committed: 14535.71 txn/s, latency: 2732.68 ms, (p50: 2700 ms, p70: 2700, p90: 2800 ms, p99: 3100 ms), latency samples: 5526800
two traffics test : committed: 100.05 txn/s, latency: 1546.60 ms, (p50: 1400 ms, p70: 1400, p90: 1600 ms, p99: 8200 ms), latency samples: 1820
Latency breakdown for phase 0: ["MempoolToBlockCreation: max: 2.005, avg: 1.566", "ConsensusProposalToOrdered: max: 0.318, avg: 0.291", "ConsensusOrderedToCommit: max: 0.359, avg: 0.352", "ConsensusProposalToCommit: max: 0.652, avg: 0.643"]
Max non-epoch-change gap was: 0 rounds at version 0 (avg 0.00) [limit 4], 0.80s no progress at version 2813511 (avg 0.20s) [limit 15].
Max epoch-change gap was: 0 rounds at version 0 (avg 0.00) [limit 4], 8.77s no progress at version 2813509 (avg 8.77s) [limit 15].
Test Ok

Copy link
Contributor

github-actions bot commented Nov 6, 2024

✅ Forge suite framework_upgrade success on 1086a5e00d773704731ab84fb4ed3538613b2250 ==> 93942524619d497fdcb75db8c994418e783e4426

Compatibility test results for 1086a5e00d773704731ab84fb4ed3538613b2250 ==> 93942524619d497fdcb75db8c994418e783e4426 (PR)
Upgrade the nodes to version: 93942524619d497fdcb75db8c994418e783e4426
framework_upgrade::framework-upgrade::full-framework-upgrade : committed: 1357.16 txn/s, submitted: 1360.28 txn/s, failed submission: 3.12 txn/s, expired: 3.12 txn/s, latency: 2240.19 ms, (p50: 2100 ms, p70: 2400, p90: 3300 ms, p99: 5300 ms), latency samples: 121840
framework_upgrade::framework-upgrade::full-framework-upgrade : committed: 1304.72 txn/s, submitted: 1307.82 txn/s, failed submission: 3.09 txn/s, expired: 3.09 txn/s, latency: 2282.27 ms, (p50: 2100 ms, p70: 2400, p90: 3200 ms, p99: 4500 ms), latency samples: 118180
5. check swarm health
Compatibility test for 1086a5e00d773704731ab84fb4ed3538613b2250 ==> 93942524619d497fdcb75db8c994418e783e4426 passed
Upgrade the remaining nodes to version: 93942524619d497fdcb75db8c994418e783e4426
framework_upgrade::framework-upgrade::full-framework-upgrade : committed: 1396.78 txn/s, submitted: 1399.27 txn/s, failed submission: 2.49 txn/s, expired: 2.49 txn/s, latency: 2223.85 ms, (p50: 2100 ms, p70: 2400, p90: 3400 ms, p99: 4500 ms), latency samples: 123420
Test Ok

Copy link
Contributor

github-actions bot commented Nov 6, 2024

✅ Forge suite compat success on 1086a5e00d773704731ab84fb4ed3538613b2250 ==> 93942524619d497fdcb75db8c994418e783e4426

Compatibility test results for 1086a5e00d773704731ab84fb4ed3538613b2250 ==> 93942524619d497fdcb75db8c994418e783e4426 (PR)
1. Check liveness of validators at old version: 1086a5e00d773704731ab84fb4ed3538613b2250
compatibility::simple-validator-upgrade::liveness-check : committed: 6945.86 txn/s, submitted: 6957.02 txn/s, failed submission: 7.54 txn/s, expired: 11.16 txn/s, latency: 2350.29 ms, (p50: 1800 ms, p70: 2000, p90: 2900 ms, p99: 12700 ms), latency samples: 460560
2. Upgrading first Validator to new version: 93942524619d497fdcb75db8c994418e783e4426
compatibility::simple-validator-upgrade::single-validator-upgrading : committed: 6371.70 txn/s, latency: 4087.99 ms, (p50: 4600 ms, p70: 4800, p90: 4900 ms, p99: 5000 ms), latency samples: 130320
compatibility::simple-validator-upgrade::single-validator-upgrade : committed: 6710.49 txn/s, latency: 4754.11 ms, (p50: 5000 ms, p70: 5200, p90: 7000 ms, p99: 7200 ms), latency samples: 223440
3. Upgrading rest of first batch to new version: 93942524619d497fdcb75db8c994418e783e4426
compatibility::simple-validator-upgrade::half-validator-upgrading : committed: 5632.63 txn/s, latency: 4970.13 ms, (p50: 5500 ms, p70: 5800, p90: 6500 ms, p99: 6700 ms), latency samples: 113160
compatibility::simple-validator-upgrade::half-validator-upgrade : committed: 5975.60 txn/s, latency: 5427.80 ms, (p50: 5900 ms, p70: 6000, p90: 7200 ms, p99: 7500 ms), latency samples: 204140
4. upgrading second batch to new version: 93942524619d497fdcb75db8c994418e783e4426
compatibility::simple-validator-upgrade::rest-validator-upgrading : committed: 8164.03 txn/s, latency: 3380.52 ms, (p50: 3700 ms, p70: 4000, p90: 4300 ms, p99: 5000 ms), latency samples: 158460
compatibility::simple-validator-upgrade::rest-validator-upgrade : committed: 8580.61 txn/s, latency: 3715.09 ms, (p50: 3700 ms, p70: 4000, p90: 5900 ms, p99: 6300 ms), latency samples: 283300
5. check swarm health
Compatibility test for 1086a5e00d773704731ab84fb4ed3538613b2250 ==> 93942524619d497fdcb75db8c994418e783e4426 passed
Test Ok

@sherry-x sherry-x merged commit 2524895 into aptos-labs:main Nov 6, 2024
48 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
6 participants