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

alnoki · 2024-09-17T19:38:08Z

Add an AIP for safe onchain key rotation address mapping for standard accounts

gregnazario

I think it's overall fine, just a little more in the problem description, and we're good to go

gregnazario · 2024-09-17T19:54:24Z

aips/aip-x-safe-auth-key-mapping.md

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


I think it needs to be clearer on this problem statement.

I think it assumes a lot about the knowledge of the reader.

As a reader I have these questions:

What is authentication key mapping, what does it map?

Why is it unsafe? What issues?

They can be lost why? Why is purely from the CLI a driver? What about web wallets, or any other wallets?

Addressed in 0f9c486

gregnazario · 2024-09-19T21:49:25Z

aips/aip-x-safe-auth-key-mapping.md

+This impacts projects or users who have used key rotation functionality,
+in particular the unproven rotation used for exotic purposes like passkeys.


This specifically affects users who have run the non-proof driven call:

0x1::account::rotate_authentication_key_call

https://github.com/aptos-labs/aptos-core/blob/main/aptos-move/framework/aptos-framework/sources/account.move#L289-L296

Addressed in 0f9c486

alinush · 2024-09-20T22:24:10Z

aips/aip-x-safe-auth-key-mapping.md

+   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


By "an operation," do you mean a key rotation?

Addressed in 4e5fb6a

alinush · 2024-09-20T22:29:57Z

aips/aip-x-safe-auth-key-mapping.md

+
+# Alternative solutions
+
+Per @davidiw in a separate chat:


For an AIP, we would need to be more precise about what the alternative solutions are.

I would remove the informal quote here and just precisely describe the alternative solution(s) considered.

Addressed in 4e5fb6a

alinush · 2024-09-20T22:30:44Z

aips/aip-x-safe-auth-key-mapping.md

+
+# Risks and drawbacks
+
+Enforces a one-to-one mapping of private key to account address in the general


[This proposal] enforces?

Addressed in 4e5fb6a

alinush · 2024-09-20T22:32:16Z

aips/aip-x-safe-auth-key-mapping.md

+
+# Multisig considerations
+
+In a separate chat, @davidiw asked about how the changes in the reference


I would remove the reference to David's question and just focus on the multisig consideration itself.

Addressed in 4e5fb6a

alinush · 2024-09-20T22:37:15Z

aips/aip-x-safe-auth-key-mapping.md

+
+# Future potentials
+
+Potentially in a separate update, logic to eradicate the existing multisig v2


Pun intended? 😆

Addressed in 4e5fb6a

alinush · 2024-09-20T22:38:03Z

aips/aip-x-safe-auth-key-mapping.md

+
+# Verifying changes in reference implementation
+
+As requested by @thepom on 2024-09-17:


Once again, I would remove such references; the AIP should stand on its own.

Addressed in 4e5fb6a

alinush · 2024-09-20T22:41:04Z

aips/aip-x-safe-auth-key-mapping.md

+   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


Upon a first read, and then a second one, I am not sure I understand this.

Are you saying something goes wrong when someone rotates two different accounts a1 and a2 to the same authentication key ak? Because the OriginatingAddress table t will be updated from t[ak] = a1 to t[ak] = a2, breaking the (efficient) a1 account recovery flow?

(I think at least you are giving this example in the 3rd bullet point below?)

And in the 2nd bulletpoint you are trying to argue that OriginatingAddress should not be overwritten?

If so, I think there should be a much more concise way to explain this.

Addressed in 4e5fb6a

thepomeranian · 2024-09-25T17:03:05Z

aips/aip-x-safe-auth-key-mapping.md

+
+# Proposed solution
+
+Assorted checks and extra function logic in [`aptos-core` #14309]


@alnoki Can you explain what the checks are looking for and what the new function does?

Function originating_address

Function set_originating_address

Addressed in 0d82010

aips/aip-x-safe-auth-key-mapping.md

formatting change

thepomeranian · 2024-09-25T17:41:21Z

aips/aip-x-safe-auth-key-mapping.md

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


@alnoki can you add the testing plan as well?

Addressed in 0d82010

thepomeranian · 2024-09-25T17:44:15Z

aips/aip-x-safe-auth-key-mapping.md

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


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

Addressed in 0d82010

davidiw

I think we could land as is and iterate or we can modify before landing. Up to you, Alex, let me know your preference.

davidiw · 2024-10-01T05:44:40Z

aips/aip-x-safe-auth-key-mapping.md

+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


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

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

davidiw · 2024-10-01T05:46:20Z

aips/aip-x-safe-auth-key-mapping.md

+
+## Impact
+
+1. Without the changes proposed in this AIP's reference implementation,


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

@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 :

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

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

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

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

davidiw · 2024-10-01T05:52:09Z

aips/aip-x-safe-auth-key-mapping.md

+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 below


This actually feels like it is out of scope... it isn't really supported for multiple keys.

it isn't really supported for multiple keys.

@davidiw I agree that the intended design does not include support for multiple keys, which is exactly the issue: special situations are possible which render the mapping unsafe in practice, hence the guardrails I've added in this AIP

This actually feels like it is out of scope...

The language in this section is the result of these rounds of feedback from @alinush and @gregnazario:

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

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

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

So seeing as the language specifically addressed these comments I hesitate to remove anything because then I will have invalidated prior rounds of review

davidiw · 2024-10-01T05:53:54Z

aips/aip-x-safe-auth-key-mapping.md

+[`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


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.

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.

alnoki · 2024-10-01T13:12:14Z

I think we could land as is and iterate or we can modify before landing. Up to you, Alex, let me know your preference.

@davidiw Land as is please

…counts (aptos-foundation#499) * Add safe auth key mapping * Revise problem statement, impact * Address comments * Update aip-x-safe-auth-key-mapping.md * Update aip-x-safe-auth-key-mapping.md formatting change * Update aip-x-safe-auth-key-mapping.md * Address comments --------- Co-authored-by: Frances Liu <[email protected]>

Add safe auth key mapping

dfd0123

gregnazario reviewed Sep 19, 2024

View reviewed changes

Revise problem statement, impact

0f9c486

thepomeranian requested review from davidiw and movekevin September 20, 2024 04:51

alinush reviewed Sep 20, 2024

View reviewed changes

alnoki and others added 2 commits September 23, 2024 16:09

Address comments

4e5fb6a

Update aip-x-safe-auth-key-mapping.md

a4e016d

thepomeranian reviewed Sep 25, 2024

View reviewed changes

thepomeranian added 2 commits September 25, 2024 10:37

Update aip-x-safe-auth-key-mapping.md

c038f98

formatting change

Update aip-x-safe-auth-key-mapping.md

00124c8

thepomeranian reviewed Sep 25, 2024

View reviewed changes

Address comments

0d82010

davidiw reviewed Oct 1, 2024

View reviewed changes

thepomeranian merged commit 067c676 into aptos-foundation:main Oct 3, 2024

alnoki mentioned this pull request Oct 3, 2024

[Framework] Safe onchain key rotation address mapping for standard accounts aptos-labs/aptos-core#14309

Merged

		This impacts projects or users who have used key rotation functionality,
		in particular the unproven rotation used for exotic purposes like passkeys.


		# Risks and drawbacks

		Enforces a one-to-one mapping of private key to account address in the general


		# Multisig considerations

		In a separate chat, @davidiw asked about how the changes in the reference


		# Future potentials

		Potentially in a separate update, logic to eradicate the existing multisig v2


		# Verifying changes in reference implementation

		As requested by @thepom on 2024-09-17:


		# Proposed solution

		Assorted checks and extra function logic in [`aptos-core` #14309]


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


		## Reference Implementation

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


		## Impact

		1. Without the changes proposed in this AIP's reference implementation,

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

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

Conversation

alnoki commented Sep 17, 2024

gregnazario left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alinush Sep 20, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alinush Sep 20, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Function originating_address

Function set_originating_address

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidiw left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alnoki Oct 1, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alnoki Oct 1, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alnoki Oct 1, 2024 • edited Loading

Choose a reason for hiding this comment

alnoki commented Oct 1, 2024

alinush Sep 20, 2024 •

edited

Loading

alinush Sep 20, 2024 •

edited

Loading

Function `originating_address`

Function `set_originating_address`

alnoki Oct 1, 2024 •

edited

Loading

alnoki Oct 1, 2024 •

edited

Loading

alnoki Oct 1, 2024 •

edited

Loading