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

Add AIP for safe onchain key rotation address mapping for standard accounts #499

Merged
merged 7 commits into from
Oct 3, 2024
Merged
Changes from 6 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
133 changes: 133 additions & 0 deletions aips/aip-x-safe-auth-key-mapping.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,133 @@
---
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 ([email protected])
discussions-to (*optional): https://github.com/aptos-foundation/AIPs/issues/487
Status: Draft
last-call-end-date (*optional): <mm/dd/yyyy the last date to leave feedbacks and reviews>
type: Standard Framework
created: 09/17/2024
updated (*optional): <mm/dd/yyyy>
requires (*optional): <AIP number(s)>
---

# 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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This feels out of place, probably a meta note for the template. Compare this to the summary and the summary inadequately addresses the goal, which is to make it possible to perform updates to the address mapping without requiring a complex proof that is prohibitively expensive for multisig.

Whereas out of scope would be sufficient to state that an account that has already performed rotation followed by an upgrade to mutlisig would have an incorrect entry in the table (notice the concise description).

Copy link
Contributor Author

@alnoki alnoki Oct 1, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This feels out of place

@davidiw This section is included because of your previous inquiry about how how the reference implementation will affect multisig v2, which I answered in the AIP issue that I was first instructed to file, under the section "Multisig considerations": #487

I also included it here so that this PR matches the issue, then revised it as @alinush requested at #499 (comment)

I hesitate to remove this section because then I will invalidate the prior request for modification


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,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would state the more ... positive form, that the process for managing the table as is is expensive and may preclude leveraging on-chain state management for maintaining keys to accounts

Copy link
Contributor Author

@alnoki alnoki Oct 1, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@davidiw the current language and layout is the result of multiple rounds of review as requested by @alinush and @gregnazario, and changes pushed to this branch by @thepomeranian :

  1. Add AIP for safe onchain key rotation address mapping for standard accounts #499 (comment)
  2. Add AIP for safe onchain key rotation address mapping for standard accounts #499 (comment)
  3. Add AIP for safe onchain key rotation address mapping for standard accounts #499 (comment)
  4. c038f98

... positive form

So I hesitate to modify the language because I don't want to invalidate all of requested changes I applied based on the existing sequence of feedback

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

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.

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

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](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](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).
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.
(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).

## Reference Implementation

Assorted checks and extra function logic in [`aptos-core` #14309](https://github.com/aptos-labs/aptos-core/pull/14309)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

reminder to add the two new functions here and describe what they do

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed in 0d82010


## Testing

> - 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)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@alnoki can you add the testing plan as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed in 0d82010

> - When can we expect the results?
> - What are the test results and are they what we expected? If not, explain the gap.

## 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

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
key rotation attacks.

## Future Potential
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My take is that this is a kludge to get a little more longevity out of this model. Ultimately we need a more practical approach that solves all the issues -- in particular maps individual public keys to all accounts managed with that key and describes how those keys are used.

Copy link
Contributor Author

@alnoki alnoki Oct 1, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

more longevity out of this model

maps individual public keys to all accounts managed with that key

@davidiw Regarding your idea of a one-to-many mapping, I addressed this consideration in the AIP discussion issue under the section "Alternative solutions" (#487)

However as @alinush requested at #499 (comment), I removed the description, so for your reference again I'll post the original suggestion as well as my response:

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.


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

## Timeline

Ideally during next release