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

Operational certificate + pool cold key derivation #48

Closed
wants to merge 14 commits into from

Conversation

refi93
Copy link
Collaborator

@refi93 refi93 commented Dec 9, 2020

Motivation: Add instructions for derivation of pool cold keys and operational certificate signing.

Changes:

  • introduced getExtendedPoolColdPublicKey(coldKeyPath) call - the expected derivation path structure is m/1853'/1815'/0/<cold key index>'
  • introduced signOperationalCertificate(kesPublicKeyHex, kesPeriod, counter, coldKeyPath) which assembles the operational certificate and returns its signature - as the OpCert structure has no variable length fields and is well under 255 bytes, it's done in a single APDU
  • the security policy both for key derivation and operational certificate signing validates the coldKey derivation path
  • refactored messageSigning.c to have a common function for signing a message with a key derived from a path and purpose-specific functions for tx witnesses and operational certificate signatures with corresponding message structure and derivation path validation, to ensure pool cold keys are not used for tx witnesses and vice-versa to prevent even hypothetical collisions between the different types of messages (txs vs operational certificates) being signed by those keys
  • refactored bip44_hasValidCardanoPrefix() into bip44_hasValidCardanoWalletPrefix() to introduce equivalent function for pool cold keys named bip44_hasValidCardanoPoolColdKeyPrefix() - which is technically not required to have but I see it as good for readability and consistency, making it clear to a casual reader of the codebase that pool cold keys and wallet keys are logically separate
  • added str_formatUint64() helper function which prints uint64 integers in decimal representation
  • added TRACE_UINT64 debugging macro to trace uint64 values like issueCounter or kesPeriod

Testing:

yarn test-integration --grep signOperationalCertificate
yarn test-integration --grep getExtendedPoolColdPublicKey

TODO: add docs (to be added once high-level idea is validated)
TODO2: missing flag in makefile to separate operator functionality (IMHO worth doing after we merge the with the rest of functionality)

@refi93 refi93 force-pushed the pool-operator-app-refi93 branch 3 times, most recently from e1af750 to bc4bd40 Compare December 14, 2020 12:16
@refi93 refi93 changed the title WIP operational certificate + pool cold key derivation Operational certificate + pool cold key derivation Dec 14, 2020
@refi93 refi93 requested a review from janmazak December 14, 2020 12:27
@refi93 refi93 force-pushed the pool-operator-app-refi93 branch 2 times, most recently from 6e06ad8 to 99bb0fd Compare December 14, 2020 12:48
Copy link
Collaborator

@janmazak janmazak left a comment

Choose a reason for hiding this comment

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

Overall, I quite like it.

The only major problem is displaying uint64_t, again.

Perhaps we should push ledger to fix it and support it properly in snprintf.

@@ -29,7 +29,7 @@ PIN = 5555
APPNAME = "Cardano ADA"
APPVERSION = "2.1.0"

APP_LOAD_PARAMS =--appFlags 0x240 --curve ed25519 --path "44'/1815'" --path "1852'/1815'"
APP_LOAD_PARAMS =--appFlags 0x240 --curve ed25519 --path "44'/1815'" --path "1852'/1815'" --path "1853'/1815'"
Copy link
Collaborator

Choose a reason for hiding this comment

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

This should be allowed only for the operator app.

See 8ff1652 on branch operatorapp. All the code should be included only under the flag, too. (That is not a definitive solution, just a first attempt, so feel free to improve it.)

Copy link
Collaborator Author

@refi93 refi93 Dec 22, 2020

Choose a reason for hiding this comment

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

I'd rather not introduce the flagging of features here as the solution is not final yet, so it is likely to result in unnecessary conflicts (and once finalized, should be easy to apply). Maybe I would call the flag HAVE_OPERATOR_FEATURES to be consistent with similar flags for U2F (HAVE_U2F) and WebUSB (HAVE_WEBUSB)

src/bip44.c Outdated Show resolved Hide resolved
src/bip44.c Show resolved Hide resolved
src/bip44.h Show resolved Hide resolved
src/cardano.h Show resolved Hide resolved
src/signOpCert.c Outdated Show resolved Hide resolved
src/signOpCert.c Outdated Show resolved Hide resolved
src/signOpCert.c Outdated Show resolved Hide resolved
src/signOpCert.c Outdated Show resolved Hide resolved
src/signOpCert.c Outdated Show resolved Hide resolved
@refi93
Copy link
Collaborator Author

refi93 commented Dec 16, 2020

the current version of the getPoolColdPublicKey call is returning the non-extended public key - I realized that even though it should technically be enough, it's likely to cause "pain" with the hw cli which internally deals with extended public key (especially in the hw signing file) and existing wallet implementations also likely use extended public keys in their internal logic, so this would only force them to introduce special code for cold keys.

TLDR - TODO rework getPoolColdPublicKey call to return the extended public key

@refi93 refi93 changed the base branch from develop to operatorapp December 22, 2020 23:33
@refi93 refi93 changed the base branch from operatorapp to develop December 22, 2020 23:33
@refi93
Copy link
Collaborator Author

refi93 commented Jan 19, 2021

TODO - update cold key derivation to comply with CIP-1853: cardano-foundation/CIPs#56 (i.e. change cold key usecase index to hardened)

@janmazak janmazak mentioned this pull request Jan 20, 2021
@janmazak
Copy link
Collaborator

Will be included as part of another PR.

@janmazak janmazak closed this Feb 15, 2021
@janmazak janmazak deleted the pool-operator-app-refi93 branch March 5, 2021 09:57
janmazak pushed a commit that referenced this pull request Aug 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants