Add Ledger key rotation tutorial #367
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
✅ Deploy Preview for aptos-developer-docs ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
apps/nextra/pages/en/build/cli/trying-things-on-chain/ledger.mdx
Outdated
Show resolved
Hide resolved
apps/nextra/pages/en/build/cli/trying-things-on-chain/ledger.mdx
Outdated
Show resolved
Hide resolved
apps/nextra/pages/en/build/cli/trying-things-on-chain/ledger.mdx
Outdated
Show resolved
Hide resolved
apps/nextra/pages/en/build/cli/trying-things-on-chain/ledger.mdx
Outdated
Show resolved
Hide resolved
apps/nextra/pages/en/build/cli/trying-things-on-chain/ledger.mdx
Outdated
Show resolved
Hide resolved
This comment was marked as resolved.
This comment was marked as resolved.
There was a problem hiding this comment.
@movekevin can you take a look at this? Seems like encouraging people to rotate back and forth between different keys is not a good idea.
|
|
||
| This tutorial will walk you through both scenarios. | ||
|
|
||
| <Callout type="warning" emoji="💀"> |
There was a problem hiding this comment.
Let's use a red exclamation, instead of skulls.
We should also add, this is really only something that should be done if you know what you're doing or something similar.
There was a problem hiding this comment.
Addressed in 543aaa6
Note that the callout additionally specifies not to do it until finishing the base ledger guide, which includes a power user warning
| If you are on a UNIX-like system, the following command can be used to start a | ||
| fresh localnet as a background process: |
There was a problem hiding this comment.
I think this can go over people's heads sometimes.
Let's say macOS or Linux (I know there's BSD and other things, but it should be good enough).
I also, generally try not to suggest people to launch background processes unless they know what they're doing. Pretty easy to do things wrong there, especially since the local testnet will print output from 0x1::debug::print
There was a problem hiding this comment.
Addressed in 543aaa6 with MacOS/Linux distinction and power user callout
| Create a private key corresponding to an authentication key, and thus initial | ||
| account address, that starts with the vanity prefix `0xaaa`: | ||
|
|
||
| ```sh filename="Terminal" | ||
| aptos key generate \ | ||
| --assume-yes \ | ||
| --output-file private-key-a \ | ||
| --vanity-prefix 0xaaa | ||
| ``` |
There was a problem hiding this comment.
Vanity prefixes are nice, but let's keep it to a randomly generated one, so people aren't waiting for a while to churn through prefixes to get to the rest of the tutorial.
There was a problem hiding this comment.
It can actually be kinda hard to follow along if luck of the draw two accounts start with similar hex chars
Vanity address with only 3 chars is negligible time to generate, and the labelling make the tutorial flow way easier
| <Callout type="info" emoji="🧠"> | ||
| As a best practice, this command uses a [BIP44 account index] offset of 1000 to | ||
| indicate that the account is secured by a rotated authentication key on a | ||
| Ledger. | ||
|
|
||
| This practice aids in profile recovery, as shown below. | ||
| </Callout> |
There was a problem hiding this comment.
I'm not sure what that means.
Maybe it's better to say that it's best practice to choose a common large number, such as 1000 to ensure it doesn't conflict with already existing ledger accounts.
| ```sh filename="Terminal" | ||
| aptos account rotate-key \ | ||
| --assume-yes \ | ||
| --new-derivation-index 1000 \ |
There was a problem hiding this comment.
I don't think this is in the CLI yet?
At least not 3.4.1
There was a problem hiding this comment.
As I mentioned in the PR description
Install the CLI from source, as described in aptos-labs/aptos-core#11151
This PR pairs simply because I had to split docs and source into two separate repos aptos-labs/aptos-core#11151
There was a problem hiding this comment.
Noting that I've updated the PR description per the recent set of aptos-core PRs that have merged, to stipulate installing the latest Framework updates
| <Callout type="info" emoji="🧠"> | ||
| If you are using a UNIX-like machine: | ||
|
|
||
| ```shell filename="Terminal" | ||
| rm private-key-* | ||
| ``` | ||
| </Callout> |
There was a problem hiding this comment.
This is really dangerous, please remove it.
Someone who copies and pastes this could wipe out other private keys. Ideally should not be having a rm ...* command
| If you are using a UNIX-like machine: | ||
|
|
||
| ```shell filename="Terminal" | ||
| aptos config delete-profile --profile ledger-wallet-1000 |
There was a problem hiding this comment.
This is also not a command?
There was a problem hiding this comment.
As I mentioned in the PR description
Install the CLI from source, as described in aptos-labs/aptos-core#11151
This PR pairs simply because I had to split docs and source into two separate repos aptos-labs/aptos-core#11151
There was a problem hiding this comment.
Noting that I've updated the PR description per the recent set of aptos-core PRs that have merged, to stipulate installing the latest Framework updates
|
@gregnazario thanks for the above comments. I believe I've addressed all where applicable Just flagging here again that this PR requires aptos-labs/aptos-core#11151, without which there is no support for ledger auth key rotation I'd try and do everything in one PR (docs and CLI source updates), but alas there is no longer a monorepo
Before ledger support became available, plenty of projects launched under hot keys (and many still do, unfortunately, due to the >20kb issue on ledger), and without the CLI updates proposed in the linked PR, there is no way to secure an upgradeable package with anything other than a hot key I agree that key rotation is advanced and should be reserved for specific use cases, but securing Move packages with accounts that don't use hot keys is important for ecosystem OpSec |
| not rotate an account's authentication key to a key that is already in the | ||
| table, as this attack that would prevent lookup of the valid originating address | ||
| that the holder of an authentication key had previously approved. |
There was a problem hiding this comment.
| not rotate an account's authentication key to a key that is already in the | |
| table, as this attack that would prevent lookup of the valid originating address | |
| that the holder of an authentication key had previously approved. | |
| not rotate an account's authentication key to a key that is already in the | |
| table, as this attack would prevent lookup of the valid originating address | |
| that the holder of an authentication key had previously approved. |
There was a problem hiding this comment.
Addressed in 806a984
| mapped to in the table (since the table is only updated upon during rotation, | ||
| not upon standard account generation). |
There was a problem hiding this comment.
| mapped to in the table (since the table is only updated upon during rotation, | |
| not upon standard account generation). | |
| mapped to in the table (since the table is only updated during rotation, | |
| not upon standard account generation). |
There was a problem hiding this comment.
Addressed in 806a984
|
|
||
| <Callout type="info" emoji="🧠"> | ||
| The [`account::rotate_authentication_key_call`] was introduced to support | ||
| non-standard key algorith, like passkeys, which cannot produce proofs of |
There was a problem hiding this comment.
| non-standard key algorith, like passkeys, which cannot produce proofs of | |
| non-standard key algorithms, like passkeys, which cannot produce proofs of |
There was a problem hiding this comment.
Addressed in 806a984
| if so, verifies that the rotating account's address is the one mapped to in the | ||
| table. | ||
|
|
||
| This means that if an arbitary account's authentication key is rotated to |
There was a problem hiding this comment.
| This means that if an arbitary account's authentication key is rotated to | |
| This means that if an arbitrary account's authentication key is rotated to |
There was a problem hiding this comment.
Addressed in 806a984
There was a problem hiding this comment.
Some nits and otherwise it looks good to me. Will let @gregnazario take a final pass
|
|
||
| <Callout type="warning" emoji="❗"> | ||
| Before you start this tutorial make sure you have completed the | ||
| [key rotation guide](../../advanced-guides/key-rotation.mdx). |
There was a problem hiding this comment.
All of the relative links to the key rotation guide do not work for me when testing it in the preview.
There was a problem hiding this comment.
It looks like advanced-guides was renamed to guides
Resolved in c32f593
|
@hariria I believe I've addressed all your recent comments and that I had previously addressed all comments from @gregnazario I've updated the PR description for more clarity on the associated |
|
Discussed with @gregnazario, ok to merge. |
Background
@davidiw @gedigi @hariria @hardsetting @xbtmatt
Per in-person discussions re: authentication key rotation and Ledger.
This relies on the new features from the
aptos-corePR aptos-labs/aptos-core#11151 which was subsequently broken into 3 smaller PRs:Changes
bashtotextto avoid syntax highlighting issues in IDE.Testing
From in
apps/nextra:aptos-coreCLI from source using [Framework] Safe onchain key rotation address mapping for standard accounts aptos-core#14309 (ormain, once it merges)Checklist
pnpm spellcheck? (This is failing due to issues with content I have not modified)pnpm fmt?pnpm lint? (This is failing due to issues with content I have not modified)