From dfd0123a9aea9dab639e46db906b156d4c5c22e3 Mon Sep 17 00:00:00 2001 From: alnoki <43892045+alnoki@users.noreply.github.com> Date: Tue, 17 Sep 2024 12:37:10 -0700 Subject: [PATCH 1/7] Add safe auth key mapping --- aips/aip-x-safe-auth-key-mapping.md | 212 ++++++++++++++++++++++++++++ 1 file changed, 212 insertions(+) create mode 100644 aips/aip-x-safe-auth-key-mapping.md diff --git a/aips/aip-x-safe-auth-key-mapping.md b/aips/aip-x-safe-auth-key-mapping.md new file mode 100644 index 00000000..97090246 --- /dev/null +++ b/aips/aip-x-safe-auth-key-mapping.md @@ -0,0 +1,212 @@ +--- +aip: (this is determined by the AIP Manager, leave it empty when drafting) +title: Safe onchain key rotation address mapping for standard accounts +author: Alex Kahn (43892045+alnoki@users.noreply.github.com) +Status: Draft +type: Standard (Framework) +created: 2024-09-17 +--- + +# Problem statement + +Authentication key mapping has functional issues that render +originating address mapping unsafe. + +Generally this means that address mappings can be lost, leading to +account loss and ultimately inability to access funds purely from CLI. + +# Impact + +This impacts projects or users who have used key rotation functionality, +in particular the unproven rotation used for exotic purposes like passkeys. + +For more, see: + +https://aptos.dev/en/build/guides/key-rotation +https://aptos.dev/en/build/cli/trying-things-on-chain/ledger#authentication-key-rotation + +# Summary + +The onchain key rotation address mapping has functional issues which inhibit +safe mapping of authentication key to originating address for standard accounts. +I propose resolving these issues with the reference implementation. + +# Motivation + +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 https://github.com/aptos-labs/aptos-core/issues/13517, + `rotate_authentication_key_call` does not update the `OriginatingAddress` + table for an "unproven" key rotation without a `RotationProofChallenge`. +1. 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, + because they will not be able to identify their old account from private key + alone unless they keep an external record of the rotated accounts it has been + used to secure. +1. 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. + +# Proposed solution + +Assorted checks and extra function logic in +https://github.com/aptos-labs/aptos-core/pull/14309 + +# Alternative solutions + +Per @davidiw in a separate chat: + +> something where we update the rotation command to take in new and removed keys +> as well as any additional metadata that might be useful such as parameters for +> the authkey type. Then we add to the indexer a set of all keys that point to +> potential addresses. + +As I understand, this approach would require breaking changes and would +introduce offchain indexing as an additional dependency in the authentication +key mapping paradigm. + +My solution, captured in the proposed reference implementation, offers a +purely onchain solution to existing issues and does not require altering the +existing design space or introducing an offchain dependency. + +# Specification + +N/A + +# Reference implementations + +https://github.com/aptos-labs/aptos-core/pull/14309 + +# Risks and drawbacks + +Enforces a one-to-one mapping of private key to account address in the general +case of following best practices, which extreme users (wishing to use one +private key to authenticate all their accounts) may find restrictive. + +# Security considerations + +Note that the function `account::set_originating_address` proposed in +https://github.com/aptos-labs/aptos-core/pull/14309 must remain a private entry +function to prevent unproven key rotation attacks. + +# Multisig considerations + +In a separate chat, @davidiw asked about how the changes in the reference +implementation will interact with multisig v2. Note that even without the +changes in this PR, it is already possible for a multisig to have an entry in +the `OriginatingAddress` table: + +1. Rotate account `A` to have a new authentication key, thus generating an entry + in the `OriginatingAddress` table. +2. Convert account `A` to a multisig via + `multisig_account::create_with_existing_account_and_revoke_auth_key`, which + will set the account's authentication key to `0x0`, but which will *not* + mutate the `OriginatingAddress` table, since it makes an inner call to + `account::rotate_authentication_key_internal`. +3. The `OriginatingAddress` table then (incorrectly) reports that a mapping from + the authentication key (from before multisig conversion) to the multisig + address. + +# Timelines + +Ideally during next release + +# Future potentials + +Potentially in a separate update, logic to eradicate the existing multisig v2 +indexing issues mentioned above (which is outside the scope of what the +reference implementation intends to resolve). + +# Verifying changes in reference implementation + +As requested by @thepom on 2024-09-17: + +1. Install the Aptos CLI [from source] using the changes in this PR. +1. Make a new test directory called `localnet-data`, then use it to start a + localnet with the framework changes in this PR: + + ```sh + aptos node run-localnet --test-dir localnet-data + ``` + +1. Save the localnet shell running off to the side. +1. In a new shell, create a private key file: + + ```sh + aptos key generate --output-file keyfile-a + ``` + +1. Use it to create a localnet profile: + + ```sh + aptos init \ + --network local \ + --private-key-file keyfile-a \ + --profile localnet-a + ``` + +1. Store the address: + + ```sh + ADDR_A= + ``` + +1. Use the new `originating_address` view function to observe that the account + does *not* have an entry in the `OriginatingAddress` table: + + ```sh + aptos move view \ + --args address:$ADDR_A \ + --function-id 0x1::account::originating_address \ + --profile localnet-a + ``` + +1. Use the new `set_originating_address` private entry function to set a mapping + in the table: + + ```sh + aptos move run \ + --function-id 0x1::account::set_originating_address \ + --profile localnet-a + ``` + +1. Check the `originating_address` view function again and note the result: + + ```sh + aptos move view \ + --args address:$ADDR_A \ + --function-id 0x1::account::originating_address \ + --profile localnet-a + ``` + +1. 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: + + ```sh + aptos init \ + --network local \ + --profile localnet-b + ``` + +1. 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`: + + ```sh + aptos account rotate-key \ + --new-private-key-file keyfile-a \ + --profile localnet-b \ + --save-to-profile localnet-b-secured-by-keyfile-a + ``` \ No newline at end of file From 0f9c486cf670bfddf34ca4d25ecf1641de76b77c Mon Sep 17 00:00:00 2001 From: alnoki <43892045+alnoki@users.noreply.github.com> Date: Thu, 19 Sep 2024 16:42:14 -0700 Subject: [PATCH 2/7] Revise problem statement, impact --- aips/aip-x-safe-auth-key-mapping.md | 85 ++++++++++++++++------------- 1 file changed, 47 insertions(+), 38 deletions(-) diff --git a/aips/aip-x-safe-auth-key-mapping.md b/aips/aip-x-safe-auth-key-mapping.md index 97090246..8bbc0e95 100644 --- a/aips/aip-x-safe-auth-key-mapping.md +++ b/aips/aip-x-safe-auth-key-mapping.md @@ -9,57 +9,61 @@ created: 2024-09-17 # Problem statement -Authentication key mapping has functional issues that render -originating address mapping unsafe. - -Generally this means that address mappings can be lost, leading to -account loss and ultimately inability to access funds purely from CLI. - -# Impact - -This impacts projects or users who have used key rotation functionality, -in particular the unproven rotation used for exotic purposes like passkeys. - -For more, see: - -https://aptos.dev/en/build/guides/key-rotation -https://aptos.dev/en/build/cli/trying-things-on-chain/ledger#authentication-key-rotation - -# Summary - -The onchain key rotation address mapping has functional issues which inhibit -safe mapping of authentication key to originating address for standard accounts. -I propose resolving these issues with the reference implementation. - -# Motivation +Aptos authentication key rotation is accompanied by a global mapping from an +authentication key to the address that it authenticates, the +`OriginatingAddress` table. For more background see the [key rotation docs] and +the [Ledger key rotation docs]. 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 https://github.com/aptos-labs/aptos-core/issues/13517, - `rotate_authentication_key_call` does not update the `OriginatingAddress` - table for an "unproven" key rotation without a `RotationProofChallenge`. +1. Per [`aptos-core` #13517], `rotate_authentication_key_call` does not update + the `OriginatingAddress` table for an "unproven" key rotation without a + `RotationProofChallenge` (resolved in this AIP's reference implementation + with a new `set_originating_address` private entry function). 1. 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, - because they will not be able to identify their old account from private key - alone unless they keep an external record of the rotated accounts it has been - used to secure. + 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. 1. 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. + inhibit indexing and OpSec (resolved in this PR with + `set_originating_address` private entry function). + +# Impact + +1. Without the changes proposed in this AIP's reference implementation, + unproven authentications (specifically those relying on + `rotate_authentication_key_call`) will result in an unidentifiable mapping, + such that users will be unable to identify accounts secured by their private + key unless they have maintained their own offchain mapping. This applies to + exotic wallets like passkeys. +1. The overwrite behavior (described above) for + `update_auth_key_and_originating_address_table` can similarly result in an + inability to identify an account based on the private key. +1. A user who authenticates two accounts with the same private key per the above + schema will experience undefined behavior during indexing and OpSec due to + the original one-to-one mapping assumption. + +# Summary + +The onchain key rotation address mapping has functional issues which inhibit +safe mapping of authentication key to originating address for standard accounts. +I propose resolving these issues with the reference implementation. # Proposed solution -Assorted checks and extra function logic in -https://github.com/aptos-labs/aptos-core/pull/14309 +Assorted checks and extra function logic in [`aptos-core` #14309] # Alternative solutions @@ -84,7 +88,7 @@ N/A # Reference implementations -https://github.com/aptos-labs/aptos-core/pull/14309 +[`aptos-core` #14309] # Risks and drawbacks @@ -95,8 +99,8 @@ private key to authenticate all their accounts) may find restrictive. # Security considerations Note that the function `account::set_originating_address` proposed in -https://github.com/aptos-labs/aptos-core/pull/14309 must remain a private entry -function to prevent unproven key rotation attacks. +[`aptos-core` #14309] must remain a private entry function to prevent unproven +key rotation attacks. # Multisig considerations @@ -130,7 +134,7 @@ reference implementation intends to resolve). As requested by @thepom on 2024-09-17: -1. Install the Aptos CLI [from source] using the changes in this PR. +1. Install the Aptos CLI from source using the changes in [`aptos-core` #14309]. 1. Make a new test directory called `localnet-data`, then use it to start a localnet with the framework changes in this PR: @@ -209,4 +213,9 @@ As requested by @thepom on 2024-09-17: --new-private-key-file keyfile-a \ --profile localnet-b \ --save-to-profile localnet-b-secured-by-keyfile-a - ``` \ No newline at end of file + ``` + +[`aptos-core` #13517]: https://github.com/aptos-labs/aptos-core/pull/13517 +[`aptos-core` #14309]: https://github.com/aptos-labs/aptos-core/pull/14309 +[key rotation docs]: https://aptos.dev/en/build/guides/key-rotation +[Ledger key rotation docs]: https://aptos.dev/en/build/cli/trying-things-on-chain/ledger#authentication-key-rotation \ No newline at end of file From 4e5fb6a558e6d3ee7d437878c50345f464160e47 Mon Sep 17 00:00:00 2001 From: alnoki <43892045+alnoki@users.noreply.github.com> Date: Mon, 23 Sep 2024 16:09:07 -0700 Subject: [PATCH 3/7] Address comments --- aips/aip-x-safe-auth-key-mapping.md | 149 +++++++--------------------- 1 file changed, 34 insertions(+), 115 deletions(-) diff --git a/aips/aip-x-safe-auth-key-mapping.md b/aips/aip-x-safe-auth-key-mapping.md index 8bbc0e95..26465481 100644 --- a/aips/aip-x-safe-auth-key-mapping.md +++ b/aips/aip-x-safe-auth-key-mapping.md @@ -22,23 +22,29 @@ practice: the `OriginatingAddress` table for an "unproven" key rotation without a `RotationProofChallenge` (resolved in this AIP's reference implementation with a new `set_originating_address` private entry function). -1. 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. +1. When a given authentication key already has an entry in the + `OriginatingAddress` table (`t[ak] = a1`) and a key rotation operation + attempts to establish a mapping to a new account address, (`t[ak] = a2`), the + inner function `update_auth_key_and_originating_address_table` silently + overwrites the existing mapping rather than aborting, such that the owner of + authentication key `ak` is unable to identify account address `a1` purely + onchain from authentication key `ak`. Hence account loss may ensure if + someone accidentally maps the same authentication key twice but does not keep + an offchain record of all authenticated accounts (resolved in reference + implementation with `ENEW_AUTH_KEY_ALREADY_MAPPED` check). 1. 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). + (This situation is possible even with proposed `ENEW_AUTH_KEY_ALREADY_MAPPED` + since account initialization logic does not create an `OriginatingAddress` + entry for a standard account when it is first initialized). Hence since + `OriginatingAddress` is intended to be one-to-one, a dual-account situation + can inhibit indexing and OpSec (resolved in reference implementation with + `set_originating_address` private entry function, which allows setting a + mapping for the original account address). + # Impact @@ -67,16 +73,12 @@ Assorted checks and extra function logic in [`aptos-core` #14309] # Alternative solutions -Per @davidiw in a separate chat: - -> something where we update the rotation command to take in new and removed keys -> as well as any additional metadata that might be useful such as parameters for -> the authkey type. Then we add to the indexer a set of all keys that point to -> potential addresses. +Separately, @davidiw has proposed a primarily offchain and indexing-based +approach to mapping authentication keys (see [AIP issue 487] more more detail). -As I understand, this approach would require breaking changes and would -introduce offchain indexing as an additional dependency in the authentication -key mapping paradigm. +However, such an approach would require breaking changes and would introduce +offchain indexing as an additional dependency in the authentication key mapping +paradigm. My solution, captured in the proposed reference implementation, offers a purely onchain solution to existing issues and does not require altering the @@ -92,9 +94,9 @@ N/A # Risks and drawbacks -Enforces a one-to-one mapping of private key to account address in the general -case of following best practices, which extreme users (wishing to use one -private key to authenticate all their accounts) may find restrictive. +This proposal enforces a one-to-one mapping of private key to account address in +the general case of following best practices, which extreme users (wishing to +use one private key to authenticate all their accounts) may find restrictive. # Security considerations @@ -104,10 +106,9 @@ key rotation attacks. # Multisig considerations -In a separate chat, @davidiw asked about how the changes in the reference -implementation will interact with multisig v2. Note that even without the -changes in this PR, it is already possible for a multisig to have an entry in -the `OriginatingAddress` table: +Note that this AIP does not attempt to address multisig v2 effects, because even +without the changes in this AIP, it is already possible for a multisig to +(misleadingly) generate an entry in the `OriginatingAddress` table: 1. Rotate account `A` to have a new authentication key, thus generating an entry in the `OriginatingAddress` table. @@ -126,96 +127,14 @@ Ideally during next release # Future potentials -Potentially in a separate update, logic to eradicate the existing multisig v2 -indexing issues mentioned above (which is outside the scope of what the -reference implementation intends to resolve). +In a separate update, logic to eradicate the existing multisig v2 indexing +issues mentioned above (which is outside the scope of what the reference +implementation intends to resolve). # Verifying changes in reference implementation -As requested by @thepom on 2024-09-17: - -1. Install the Aptos CLI from source using the changes in [`aptos-core` #14309]. -1. Make a new test directory called `localnet-data`, then use it to start a - localnet with the framework changes in this PR: - - ```sh - aptos node run-localnet --test-dir localnet-data - ``` - -1. Save the localnet shell running off to the side. -1. In a new shell, create a private key file: - - ```sh - aptos key generate --output-file keyfile-a - ``` - -1. Use it to create a localnet profile: - - ```sh - aptos init \ - --network local \ - --private-key-file keyfile-a \ - --profile localnet-a - ``` - -1. Store the address: - - ```sh - ADDR_A= - ``` - -1. Use the new `originating_address` view function to observe that the account - does *not* have an entry in the `OriginatingAddress` table: - - ```sh - aptos move view \ - --args address:$ADDR_A \ - --function-id 0x1::account::originating_address \ - --profile localnet-a - ``` - -1. Use the new `set_originating_address` private entry function to set a mapping - in the table: - - ```sh - aptos move run \ - --function-id 0x1::account::set_originating_address \ - --profile localnet-a - ``` - -1. Check the `originating_address` view function again and note the result: - - ```sh - aptos move view \ - --args address:$ADDR_A \ - --function-id 0x1::account::originating_address \ - --profile localnet-a - ``` - -1. 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: - - ```sh - aptos init \ - --network local \ - --profile localnet-b - ``` - -1. 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`: - - ```sh - aptos account rotate-key \ - --new-private-key-file keyfile-a \ - --profile localnet-b \ - --save-to-profile localnet-b-secured-by-keyfile-a - ``` - [`aptos-core` #13517]: https://github.com/aptos-labs/aptos-core/pull/13517 [`aptos-core` #14309]: https://github.com/aptos-labs/aptos-core/pull/14309 [key rotation docs]: https://aptos.dev/en/build/guides/key-rotation -[Ledger key rotation docs]: https://aptos.dev/en/build/cli/trying-things-on-chain/ledger#authentication-key-rotation \ No newline at end of file +[Ledger key rotation docs]: https://aptos.dev/en/build/cli/trying-things-on-chain/ledger#authentication-key-rotation +[AIP issue 487]: https://github.com/aptos-foundation/AIPs/issues/487 \ No newline at end of file From a4e016d57cd746d2da3f9dc775051825a9bb4d94 Mon Sep 17 00:00:00 2001 From: Frances Liu Date: Wed, 25 Sep 2024 09:58:13 -0700 Subject: [PATCH 4/7] Update aip-x-safe-auth-key-mapping.md --- aips/aip-x-safe-auth-key-mapping.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/aips/aip-x-safe-auth-key-mapping.md b/aips/aip-x-safe-auth-key-mapping.md index 26465481..b3f19578 100644 --- a/aips/aip-x-safe-auth-key-mapping.md +++ b/aips/aip-x-safe-auth-key-mapping.md @@ -65,7 +65,7 @@ practice: The onchain key rotation address mapping has functional issues which inhibit safe mapping of authentication key to originating address for standard accounts. -I propose resolving these issues with the reference implementation. +This proposal resolves these issues by adding assorted checks and extra function logic. # Proposed solution @@ -137,4 +137,4 @@ implementation intends to resolve). [`aptos-core` #14309]: https://github.com/aptos-labs/aptos-core/pull/14309 [key rotation docs]: https://aptos.dev/en/build/guides/key-rotation [Ledger key rotation docs]: https://aptos.dev/en/build/cli/trying-things-on-chain/ledger#authentication-key-rotation -[AIP issue 487]: https://github.com/aptos-foundation/AIPs/issues/487 \ No newline at end of file +[AIP issue 487]: https://github.com/aptos-foundation/AIPs/issues/487 From c038f98a1c4e866b2f7524c49f39757bfd565470 Mon Sep 17 00:00:00 2001 From: Frances Liu Date: Wed, 25 Sep 2024 10:37:30 -0700 Subject: [PATCH 5/7] Update aip-x-safe-auth-key-mapping.md formatting change --- aips/aip-x-safe-auth-key-mapping.md | 155 ++++++++++++++-------------- 1 file changed, 75 insertions(+), 80 deletions(-) diff --git a/aips/aip-x-safe-auth-key-mapping.md b/aips/aip-x-safe-auth-key-mapping.md index b3f19578..b3eba070 100644 --- a/aips/aip-x-safe-auth-key-mapping.md +++ b/aips/aip-x-safe-auth-key-mapping.md @@ -2,23 +2,80 @@ aip: (this is determined by the AIP Manager, leave it empty when drafting) title: Safe onchain key rotation address mapping for standard accounts author: Alex Kahn (43892045+alnoki@users.noreply.github.com) +discussions-to (*optional): https://github.com/aptos-foundation/AIPs/issues/487 Status: Draft -type: Standard (Framework) -created: 2024-09-17 +last-call-end-date (*optional): +type: Standard Framework +created: 09/17/2024 +updated (*optional): +requires (*optional): --- -# Problem statement +# AIP-X - Safe onchain key rotation address mapping for standard accounts + +## Summary + +The onchain key rotation address mapping has functional issues which inhibit +safe mapping of authentication key to originating address for standard accounts. +This proposal resolves these issues by adding assorted checks and extra function logic. + +### Out of Scope + +Note that this AIP does not attempt to address multisig v2 effects, because even +without the changes in this AIP, it is already possible for a multisig to +(misleadingly) generate an entry in the `OriginatingAddress` table: + +1. Rotate account `A` to have a new authentication key, thus generating an entry + in the `OriginatingAddress` table. +2. Convert account `A` to a multisig via + `multisig_account::create_with_existing_account_and_revoke_auth_key`, which + will set the account's authentication key to `0x0`, but which will *not* + mutate the `OriginatingAddress` table, since it makes an inner call to + `account::rotate_authentication_key_internal`. +3. The `OriginatingAddress` table then (incorrectly) reports that a mapping from + the authentication key (from before multisig conversion) to the multisig + address. + +## Impact + +1. Without the changes proposed in this AIP's reference implementation, + unproven authentications (specifically those relying on + `rotate_authentication_key_call`) will result in an unidentifiable mapping, + such that users will be unable to identify accounts secured by their private + key unless they have maintained their own offchain mapping. This applies to + exotic wallets like passkeys. +1. The overwrite behavior (described above) for + `update_auth_key_and_originating_address_table` can similarly result in an + inability to identify an account based on the private key. +1. A user who authenticates two accounts with the same private key per the above + schema will experience undefined behavior during indexing and OpSec due to + the original one-to-one mapping assumption. + +## Alternative solutions + +Separately, @davidiw has proposed a primarily offchain and indexing-based +approach to mapping authentication keys (see [AIP issue 487] more more detail). + +However, such an approach would require breaking changes and would introduce +offchain indexing as an additional dependency in the authentication key mapping +paradigm. + +My solution, captured in the proposed reference implementation, offers a +purely onchain solution to existing issues and does not require altering the +existing design space or introducing an offchain dependency. + +## Specification and Implementation Details Aptos authentication key rotation is accompanied by a global mapping from an authentication key to the address that it authenticates, the -`OriginatingAddress` table. For more background see the [key rotation docs] and -the [Ledger key rotation docs]. +`OriginatingAddress` table. For more background see the [key rotation docs](https://aptos.dev/en/build/guides/key-rotation) and +the [Ledger key rotation docs](https://aptos.dev/en/build/cli/trying-things-on-chain/ledger#authentication-key-rotation). 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 [`aptos-core` #13517], `rotate_authentication_key_call` does not update +1. Per [`aptos-core` #13517](https://github.com/aptos-labs/aptos-core/pull/13517), `rotate_authentication_key_call` does not update the `OriginatingAddress` table for an "unproven" key rotation without a `RotationProofChallenge` (resolved in this AIP's reference implementation with a new `set_originating_address` private entry function). @@ -45,96 +102,34 @@ practice: `set_originating_address` private entry function, which allows setting a mapping for the original account address). +## Reference Implementation -# Impact - -1. Without the changes proposed in this AIP's reference implementation, - unproven authentications (specifically those relying on - `rotate_authentication_key_call`) will result in an unidentifiable mapping, - such that users will be unable to identify accounts secured by their private - key unless they have maintained their own offchain mapping. This applies to - exotic wallets like passkeys. -1. The overwrite behavior (described above) for - `update_auth_key_and_originating_address_table` can similarly result in an - inability to identify an account based on the private key. -1. A user who authenticates two accounts with the same private key per the above - schema will experience undefined behavior during indexing and OpSec due to - the original one-to-one mapping assumption. - -# Summary - -The onchain key rotation address mapping has functional issues which inhibit -safe mapping of authentication key to originating address for standard accounts. -This proposal resolves these issues by adding assorted checks and extra function logic. - -# Proposed solution +Assorted checks and extra function logic in [`aptos-core` #14309](https://github.com/aptos-labs/aptos-core/pull/14309) -Assorted checks and extra function logic in [`aptos-core` #14309] +## Testing -# Alternative solutions + > - What is the testing plan? (other than load testing, all tests should be part of the implementation details and won’t need to be called out. Some examples include user stories, network health metrics, system metrics, E2E tests, unit tests, etc) + > - When can we expect the results? + > - What are the test results and are they what we expected? If not, explain the gap. -Separately, @davidiw has proposed a primarily offchain and indexing-based -approach to mapping authentication keys (see [AIP issue 487] more more detail). - -However, such an approach would require breaking changes and would introduce -offchain indexing as an additional dependency in the authentication key mapping -paradigm. - -My solution, captured in the proposed reference implementation, offers a -purely onchain solution to existing issues and does not require altering the -existing design space or introducing an offchain dependency. - -# Specification - -N/A - -# Reference implementations - -[`aptos-core` #14309] - -# Risks and drawbacks +## Risks and drawbacks This proposal enforces a one-to-one mapping of private key to account address in the general case of following best practices, which extreme users (wishing to use one private key to authenticate all their accounts) may find restrictive. -# Security considerations +## Security considerations Note that the function `account::set_originating_address` proposed in -[`aptos-core` #14309] must remain a private entry function to prevent unproven +[`aptos-core` #14309])(https://github.com/aptos-labs/aptos-core/pull/14309) must remain a private entry function to prevent unproven key rotation attacks. -# Multisig considerations - -Note that this AIP does not attempt to address multisig v2 effects, because even -without the changes in this AIP, it is already possible for a multisig to -(misleadingly) generate an entry in the `OriginatingAddress` table: - -1. Rotate account `A` to have a new authentication key, thus generating an entry - in the `OriginatingAddress` table. -2. Convert account `A` to a multisig via - `multisig_account::create_with_existing_account_and_revoke_auth_key`, which - will set the account's authentication key to `0x0`, but which will *not* - mutate the `OriginatingAddress` table, since it makes an inner call to - `account::rotate_authentication_key_internal`. -3. The `OriginatingAddress` table then (incorrectly) reports that a mapping from - the authentication key (from before multisig conversion) to the multisig - address. - -# Timelines - -Ideally during next release - -# Future potentials +## Future Potential In a separate update, logic to eradicate the existing multisig v2 indexing issues mentioned above (which is outside the scope of what the reference implementation intends to resolve). -# Verifying changes in reference implementation +## Timeline -[`aptos-core` #13517]: https://github.com/aptos-labs/aptos-core/pull/13517 -[`aptos-core` #14309]: https://github.com/aptos-labs/aptos-core/pull/14309 -[key rotation docs]: https://aptos.dev/en/build/guides/key-rotation -[Ledger key rotation docs]: https://aptos.dev/en/build/cli/trying-things-on-chain/ledger#authentication-key-rotation -[AIP issue 487]: https://github.com/aptos-foundation/AIPs/issues/487 +Ideally during next release From 00124c8a852d8865983c22f8637da9b14b6a4dd4 Mon Sep 17 00:00:00 2001 From: Frances Liu Date: Wed, 25 Sep 2024 10:39:13 -0700 Subject: [PATCH 6/7] Update aip-x-safe-auth-key-mapping.md --- aips/aip-x-safe-auth-key-mapping.md | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/aips/aip-x-safe-auth-key-mapping.md b/aips/aip-x-safe-auth-key-mapping.md index b3eba070..a883d618 100644 --- a/aips/aip-x-safe-auth-key-mapping.md +++ b/aips/aip-x-safe-auth-key-mapping.md @@ -53,16 +53,14 @@ without the changes in this AIP, it is already possible for a multisig to ## Alternative solutions -Separately, @davidiw has proposed a primarily offchain and indexing-based -approach to mapping authentication keys (see [AIP issue 487] more more detail). +One alternative would be a primarily offchain and indexing-based +approach to mapping authentication keys. However, such an approach would require breaking changes and would introduce offchain indexing as an additional dependency in the authentication key mapping paradigm. -My solution, captured in the proposed reference implementation, offers a -purely onchain solution to existing issues and does not require altering the -existing design space or introducing an offchain dependency. +The proposed solution offers a purely onchain solution to existing issues and does not require altering the existing design space or introducing an offchain dependency. ## Specification and Implementation Details From 0d820107300c8f22ecb0c0bba421390ef52bb289 Mon Sep 17 00:00:00 2001 From: alnoki <43892045+alnoki@users.noreply.github.com> Date: Wed, 25 Sep 2024 15:58:09 -0700 Subject: [PATCH 7/7] Address comments --- aips/aip-x-safe-auth-key-mapping.md | 28 +++++++++++++++++++--------- 1 file changed, 19 insertions(+), 9 deletions(-) diff --git a/aips/aip-x-safe-auth-key-mapping.md b/aips/aip-x-safe-auth-key-mapping.md index a883d618..cf1954b4 100644 --- a/aips/aip-x-safe-auth-key-mapping.md +++ b/aips/aip-x-safe-auth-key-mapping.md @@ -17,7 +17,7 @@ requires (*optional): The onchain key rotation address mapping has functional issues which inhibit safe mapping of authentication key to originating address for standard accounts. -This proposal resolves these issues by adding assorted checks and extra function logic. +This proposal resolves these issues by adding assorted checks and extra function logic. ### Out of Scope @@ -44,10 +44,10 @@ without the changes in this AIP, it is already possible for a multisig to such that users will be unable to identify accounts secured by their private key unless they have maintained their own offchain mapping. This applies to exotic wallets like passkeys. -1. The overwrite behavior (described above) for +1. The overwrite behavior (described below) for `update_auth_key_and_originating_address_table` can similarly result in an inability to identify an account based on the private key. -1. A user who authenticates two accounts with the same private key per the above +1. A user who authenticates two accounts with the same private key per the below schema will experience undefined behavior during indexing and OpSec due to the original one-to-one mapping assumption. @@ -102,13 +102,23 @@ practice: ## Reference Implementation -Assorted checks and extra function logic in [`aptos-core` #14309](https://github.com/aptos-labs/aptos-core/pull/14309) +Assorted checks and extra function logic in [`aptos-core` #14309](https://github.com/aptos-labs/aptos-core/pull/14309): -## Testing +1. Function `set_originating_address()`: Private entry function to establish an + `OriginatingAddress` mapping for account reconciliation after an unproven + rotation or for an account that has just beencreated. +1. Function `originating_address()`: View function to return the address mapped + to an authentication key +1. Abort `ENEW_AUTH_KEY_SAME_AS_CURRENT` to prevent no-op rotations that can + introduce indexing issues. +1. Abort `ENEW_AUTH_KEY_ALREADY_MAPPED` to prevent onchain account loss + from silent overwriting of existing mappings. - > - What is the testing plan? (other than load testing, all tests should be part of the implementation details and won’t need to be called out. Some examples include user stories, network health metrics, system metrics, E2E tests, unit tests, etc) - > - When can we expect the results? - > - What are the test results and are they what we expected? If not, explain the gap. +## Testing + +The reference implementation pairs with a +[docs site walkthrough that I authored](https://aptos.dev/en/build/guides/key-rotation#key-rotation-with-the-aptos-cli), which demonstrates, documents, +and verifies the functionality from the reference implementation. ## Risks and drawbacks @@ -119,7 +129,7 @@ use one private key to authenticate all their accounts) may find restrictive. ## Security considerations Note that the function `account::set_originating_address` proposed in -[`aptos-core` #14309])(https://github.com/aptos-labs/aptos-core/pull/14309) must remain a private entry function to prevent unproven +[`aptos-core` #14309](https://github.com/aptos-labs/aptos-core/pull/14309) must remain a private entry function to prevent unproven key rotation attacks. ## Future Potential