aptos-core icon indicating copy to clipboard operation
aptos-core copied to clipboard

[CLI][Ledger] Add ledger hardware wallet key rotation support

Open alnoki opened this issue 2 years ago • 7 comments

@0xjinn @banool @gregnazario @hardsetting @larry-aptos

I'm updating the CLI key rotation functionality to support rotations to/from a ledger, but unable to sign the rotation proof message with a ledger. I can't tell if this is an issue with the APDU or not?

When I follow the new tutorial I'm adding on the docs site, the final step (rotate-key command) fails with:

{
  "Error": "Unexpected error: Error - Failed to parse raw transaction (0xb005)"
}

Additional findings from testing:

  • I am able to use aptos_ledger::sign_message() to sign the first 71 bytes of the rotation proof challenge, but not the first 72 bytes
  • If I cap the message to 72 bytes and switch the final byte to either a 0 or a 1 (it is 170 during my testing), then I can sign a 72-byte rotation challenge message
  • I am unable to sign a message 73 bytes or more
  • I am able to sign a message hard-coded to &[0, 0]
  • Using Ledger Nano S

What could be the issue preventing signing the rotation proof challenge message?

From offline discussion with @gregnazario 2023-12-04:

I'm not running into a limit on memory, it is moreso parsing the incoming payload - my ledger Nano S has no problem signing larger payloads for txns with lots of type arguments on testnet, but when I try to sign a simple key rotation message it errors out

I notice that when signing testnet txns it includes plain text about the txn, which contrasts with the key rotation challenge im trying to change, as the latter is just a bytes vector

Could it be that the aptos ledger sign API actually expects a different format (e.g. txn metadata, not just raw bytes) for signign the rotation proof challenge?

alnoki avatar Dec 01 '23 07:12 alnoki

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[bot] avatar Jan 19 '24 01:01 github-actions[bot]

Bumping per summit discussion with @movekevin @lightmark @banool @davidiw

@borispovod

alnoki avatar Jan 23 '24 23:01 alnoki

Hi @alnoki, just looked into this. It seems that the current implementation of the ledger nano app only supports UTF-8 encoded messages.

https://github.com/aptos-labs/ledger-app-aptos/blob/d364fcb16cc43fc1cd6bca33e1ce034489a2e2c0/src/transaction/utils.c#L24

Not sure why this limitation was set. cc @borispovod

I am able to use aptos_ledger::sign_message() to sign the first 71 bytes of the rotation proof challenge, but not the first 72 bytes

maybe the 72th byte is >= 0x7F and that's why it fails?

EDIT: yup, seems to be the case

Screenshot 2024-01-31 at 5 22 27 PM

hardsetting avatar Feb 01 '24 01:02 hardsetting

cc @gregnazario @banool

the ledger nano app only supports UTF-8 encoded messages

Got it, thanks for clarifying. Is there perhaps somewhere in the existing Aptos Ledger SDK that performs UTF-8 encoding in order to sign an arbitrary vector of bytes?

If so, that functionality could be used to sign the rotation proof challenge vector (the step I'm getting hung up on here)

EDIT: from offline discussion with @hardsetting that I didn't see until after I replied here:

the Ledger Nano App would need to support arbitrary messages and not just UTF-8 encoded ones

we’d need a change in the source code of the nano app + a release

the key rotation message signing message needs to be signed as-is, if you sign the encoded version the entry function would fail to verify the signature

So it turns out that the ledger can only sign messages in a BCS public entry format? Is there a more general ledger API, perhaps outside the Aptos specific one, that can sign arbitrary bytes? It seems like a basic cryptographic operation...

Another point I'm confused about: if the ledger can only sign utf-8, then how does it sign the public entry functions? Those are just bytes vectors too once in BCS

alnoki avatar Feb 01 '24 18:02 alnoki

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[bot] avatar Apr 05 '24 01:04 github-actions[bot]

@hardsetting @banool is there any movement on the Ledger integration that would enable the blocked auth key rotation functionality in this PR?

alnoki avatar Apr 07 '24 03:04 alnoki

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[bot] avatar May 23 '24 01:05 github-actions[bot]

Bumping stale label per recent dev discussion in #13515 that can be implemented here

alnoki avatar Jun 01 '24 18:06 alnoki

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[bot] avatar Sep 06 '24 01:09 github-actions[bot]