-
Notifications
You must be signed in to change notification settings - Fork 952
Add support for mnemonics in hsm_secret #8400
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
base: master
Are you sure you want to change the base?
Conversation
common/hsm_secret.c
Outdated
#include <ccan/tal/str/str.h> | ||
#include <ccan/tal/tal.h> | ||
#include <common/errcode.h> | ||
#include <common/utils.h> | ||
#include <common/hsm_secret.h> | ||
#include <errno.h> | ||
#include <termios.h> | ||
#include <unistd.h> | ||
#include <ccan/crypto/sha256/sha256.h> | ||
#include <ccan/mem/mem.h> | ||
#include <sodium.h> | ||
#include <wally_bip39.h> | ||
#include <sys/stat.h> | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These need to be in alphabetical order!
common/hsm_secret.c
Outdated
case HSM_SECRET_INVALID: | ||
return false; | ||
} | ||
return false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
abort()
common/hsm_secret.c
Outdated
return HSM_SECRET_ENCRYPTED; | ||
|
||
/* Check if it starts with our type bytes (mnemonic formats) */ | ||
if (len > 32) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if (len < 32)
return HSM_SECRET_INVALID;
common/hsm_secret.c
Outdated
static void hash_passphrase(const char *passphrase, u8 hash[PASSPHRASE_HASH_LEN]) | ||
{ | ||
struct sha256 sha; | ||
sha256(&sha, passphrase, strlen(passphrase)); | ||
memcpy(hash, sha.u.u8, PASSPHRASE_HASH_LEN); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have a sha256 type for a reason, let's use it, not convert to u8[]
common/hsm_secret.c
Outdated
/* First 32 bytes are the stored passphrase hash */ | ||
const u8 *stored_hash = hsm_secret; | ||
u8 computed_hash[32]; | ||
|
||
hash_passphrase(passphrase, computed_hash); | ||
return memcmp(stored_hash, computed_hash, 32) == 0; | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
struct sha256 actual, expected;
/* First 32 bytes are the stored passphrase hash */
memcpy(&expected, hsm_secret, sizeof(struct sha256));
sha256(&actual, passphrase, strlen(passphrase));
return sha256_eq(&expected, &actual);
if (strlen(passphrase) == 0) { | ||
passphrase = NULL; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mildly prefer if (streq(passphrase, "")) ?
if (bip39_mnemonic_to_seed(mnemonic, passphrase, bip32_seed, sizeof(bip32_seed), &bip32_seed_len) != WALLY_OK) | ||
errx(ERROR_LIBWALLY, "Unable to derive BIP32 seed from BIP39 mnemonic"); | ||
|
||
/* Write to file using your new mnemonic format */ | ||
int fd = open(hsm_secret_path, O_CREAT|O_EXCL|O_WRONLY, 0400); | ||
if (fd < 0) { | ||
errx(ERROR_USAGE, "Unable to create hsm_secret file"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
err() not errx()
tools/hsmtool.c
Outdated
sha256(&sha, passphrase, strlen(passphrase)); | ||
|
||
if (!write_all(fd, sha.u.u8, PASSPHRASE_HASH_LEN)) | ||
errx(ERROR_USAGE, "Error writing passphrase hash to hsm_secret file"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
err...
tools/hsmtool.c
Outdated
if (passphrase) { | ||
/* Write passphrase hash (32 bytes) + mnemonic for protected format */ | ||
struct sha256 sha; | ||
sha256(&sha, passphrase, strlen(passphrase)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be hash of the final seed. This is a bit trickier: we need hsm_secret to expose that "calculate seed from mnemonic and passphrase" function (or opencode it with libwally?)
u8 bip32_seed[BIP39_SEED_LEN_512]; | ||
size_t bip32_seed_len; | ||
|
||
if (bip39_mnemonic_to_seed(mnemonic, passphrase, bip32_seed, sizeof(bip32_seed), &bip32_seed_len) != WALLY_OK) | ||
errx(ERROR_LIBWALLY, "Unable to derive BIP32 seed from BIP39 mnemonic"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We still need this for below.,
5de3e77
to
1ae8dc0
Compare
This is a newer API to replace hsm_encryption.c and hsm_encryption.c, this tidies up the API to be used and also cleans things up to support our new formts. Our hsm_secret formats now include: - Legacy 32-byte plain format - Legacy 73-byte encrypted format - New mnemonic format without passphrase (32 zero bytes + mnemonic) - New mnemonic format with passphrase (32-byte hash + mnemonic) This commit includes support to detect the format based on the file size and content structure. The hsm will store mnemonics in the hsm_secret file as: `passphraseHash`mnemonic`
Changelog-Added: `hsmtool` now supports hsm_secret files using a 12-word mnemonic.
In preparation for BIP-39, we need to hand the passphrase (if any) to HSMD. So we extend the hsmd wire protocol to allow that.
Changelog-Changed: hsmd: New nodes will now be created with a BIP-39 12-word phrase as their root secret.
Update the exposesecret plugin to work with the new unified HSM secret format that supports BIP39 mnemonics.
67a6b9b
to
e8c0864
Compare
BIP86 derivation requires the full 64-byte seed that comes from the BIP39 mnemonic. The first 32 bytes goes towards to master seed material and the nest 32 bytes go towards the chaincode, so we need the entire 64 bytes for deterministic derivations. I've kept the old secret struct in for now for backwards compatibility and also added some accessors which will eventually die in this branch's git multiverse but that's a spoiler, they're on the ride for the next few commits at least to help us migrate to this length aware API throughout the rest of the code without making a lot of breaking changes.
hsmd: plumb length-aware secret into hsmd_init; keep 32B mirror BIP86 (from BIP39) wants the full 64-byte BIP32 seed. This commit plumbs a variable-length (32/64B) secret into hsmd and uses the accessors from the previous commit. We keep the old 32B hsm_secret mirror and, for now, only use the first 32 bytes so legacy paths keep working. Spoiler: HKDFs will keep using the 32B seed; only wallet address derivation will switch to the full 64B in a follow-up.
Here's some *foreshadowing* for what's to come. Here's what we're aiming for with our derivation flow: Derivation split (hardened vs unhardened) ======================================== ┌───────────────┐ │ HSM │ (secrets live here) │ │ │ BIP39 → seed (64B) │ ↓ │ m/86'/0'/0' ← derive hardened base (private) │ ↓ (neuter) │ BIP86 base xpub ← public-only + chain code │ ↓ │ [send once over wire] └───────────────┘ │ ▼ ┌───────────────────────┐ │ lightningd / wallet │ │ │ │ local (unhardened) derivations: │ /0/i → external │ /1/i → change │ │ │ P2TR(BIP86) from pubkey_i │ (optionally: CHECK with HSM) └───────────────────────┘ We want to do part of the derivation inside hsmd and then send this base "pubkey" over the wire so our wallet can do the remaining derivation based on the address type and index. This lays the foundation for the base key wire message.
Add HSM_PERM_DERIVE_BIP86_KEY to gate future requests that derive under m/86'/0'/0' (BIP86 base). No behaviour change yet—handlers are not enforcing this permission in this commit.
BIP86 wants the full 64-byte BIP32 seed (from BIP39). This wires up BIP86 support so the HSM derives the hardened base m/86'/0'/0' inside the box, and exposes helpers: • derive_bip86_base_key() // m/86'/0'/0' • bip86_key(index) // m/86'/0'/0'/0/index Spoiler: derive_bip86_base_key() and bip86_key() now live in libhsmd.c as they will later be used to check the derived wallet address against hsmd's derivation, this is just to sanity check that we haven't had an accidental bit flip while we have generated this address.
RIP to this commit there's a good chance a lot of this code doesn't even make this into the final PR. Pour one out for the fallen lines of code. This commit is doing the rest of the derivation. There was a significant overlap between the bip32_pubkey derivation and the bip86_pubkey derivation so that has been refactored in one place.
Add the UTXO_P2TR_BIP86 in preparation to add BIP86 wallet functions such as newaddr, listaddr etc. We also add a new index in the database for BIP86 as this is using a completely different derivation path and hsm_secret.
We should now be able to get BIP86 Taproot addresses through lightning-cli! For now we're just adding taproot addresses.
In the case where we receive a taproot utxo we want to be able to tell if it was derived using a BIP32 seed or a BIP86 seed. Considering we will only be supporting BI86 type wallet addresses for mnemonics we can check if the out secret is 64 bytes long and if it is we can use our BIP86 for the withdrawal.
This commit will be rebased for its crimes. This commit is updating hsmtool and exposesecrets to use the new pattern for storing the secret, which is the secret_data and secret_len, to support both 64 byte and 32 byte seeds. However, it also introduces some anti patterns to handle memleak because we provide a NULL context to tal because want the secret_data and to hang around for longer. We also get rid of the accessors we introduced at the start in this commit.
} | ||
|
||
/* Helper function to validate a mnemonic string */ | ||
static bool validate_mnemonic(const char *mnemonic, enum hsm_secret_error *err) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: this would be better returning the error? Or maybe just clearer.
enum hsm_secret_type type = detect_hsm_secret_type(hsm_secret, len); | ||
|
||
switch (type) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe skip the tmp var?
return HSM_SECRET_ENCRYPTED; | ||
|
||
/* Check if it starts with our type bytes (mnemonic formats) */ | ||
if (memeqzero(hsm_secret, 32)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Implied assumption: len >= 32.
/* We would have exited above if we didn't have 32 bytes! */
BUILD_ASSERT(HSM_SECRET_PLAIN_SIZE > 32);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe sizeof(struct sha256)?
size_t len, | ||
enum hsm_secret_error *err) | ||
{ | ||
struct hsm_secret *hsms = tal(ctx, struct hsm_secret); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
assert len == sizeof(hsms->secret) just before memcpy?
*err = HSM_SECRET_OK; | ||
return hsms; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This err is unused!
tal_resize(&contents, tal_bytelen(contents) - 1); | ||
enum hsm_secret_type type = detect_hsm_secret_type(contents, tal_bytelen(contents)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Again, you don't need to resize, you can just hand tal_bytelen(contents)-1 to detect_hsm_secret_type...
return 0; | ||
} | ||
|
||
static int getemergencyrecover(const char *emer_rec_path) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ditto. print_emergencyrecover, returns void,
}; | ||
|
||
static bool check_lang(const char *abbr) | ||
static int generate_hsm(const char *hsm_secret_path) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can be void, does this ever return anything but 1?
@@ -651,53 +513,58 @@ static int dumponchaindescriptors(const char *hsm_secret_path, | |||
return 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
here too...
tests/test_plugin.py
Outdated
@@ -4141,6 +4141,7 @@ def test_important_plugin_shutdown(node_factory): | |||
l1.rpc.plugin_start(os.path.join(os.getcwd(), 'plugins/pay')) | |||
|
|||
|
|||
@pytest.mark.skip(reason="Temporarily disabled expose secrets test") | |||
@unittest.skipIf(VALGRIND, "It does not play well with prompt and key derivation.") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changelog-Removed: hsmtool
support for mnemonics in non-english languages removed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Excellent work! Minor tweaks and some reshuffling...
status_debug("HSM: Starting create_hsm with passphrase=%s", passphrase ? "provided" : "none"); | ||
|
||
/* Initialize wally tal context for libwally operations */ | ||
|
||
status_debug("HSM: Initialized wally tal context"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
too chatty!
hsm_secret_len = PASSPHRASE_HASH_LEN + strlen(mnemonic); | ||
hsm_secret_data = tal_arr(tmpctx, u8, hsm_secret_len); | ||
|
||
/* Copy seed hash first */ | ||
memcpy(hsm_secret_data, &seed_hash, PASSPHRASE_HASH_LEN); | ||
/* Copy mnemonic after seed hash */ | ||
memcpy(hsm_secret_data + PASSPHRASE_HASH_LEN, mnemonic, strlen(mnemonic)); | ||
status_debug("HSM: Created hsm_secret data structure"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is v efficient. However, it's slightly safer to use an extend pattern, and in fact we have towire() which does this:
hsm_secret_data = tal_arr(tmpctx, u8, 0);
towire_sha256(&hsm_secret_data, &seed_hash);
towire(&hsm_secret_data, mnemonic, strlen(mnemonic))
exit(1); | ||
} | ||
/* Remove the NUL terminator that grab_file adds */ | ||
tal_resize(&hsm_secret_contents, tal_bytelen(hsm_secret_contents) - 1); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Still unsure about this tal_resize() pattern, but it's not wrong.
* the TLV, and left the old "hsm_encryption_key" field in place (and lightningd | ||
* never sets that anymore), and we use the TLV instead. */ | ||
if (tlvs->hsm_passphrase) | ||
hsm_passphrase = (const char *)tlvs->hsm_passphrase; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you need this cast?
|
||
/* Define the minimum common max version for the hsmd one */ | ||
hsmd_mutual_version = maxversion < our_maxversion ? maxversion : our_maxversion; | ||
return req_reply(conn, c, hsmd_init(hsm_secret, hsmd_mutual_version, | ||
|
||
/* This was tallocated off NULL, and memleak complains if we don't free it */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, really, it's because not freeing it is wrong! Don't complain about your tools!
/* BIP86 key derivation functions moved from hsmd.c */ | ||
void derive_bip86_base_key(struct ext_key *bip86_base) | ||
{ | ||
/* Check if we have the full BIP32 seed available */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Move back to reduce diff!
/* Forward declaration for secretstuff access */ | ||
u8 *get_secretstuff_bip32_seed(void); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Kill it
"It specifies the type of address wanted; currently *bech32* (e.g. `tb1qu9j4lg5f9rgjyfhvfd905vw46eg39czmktxqgg` on bitcoin testnet or `bc1qwqdg6squsna38e46795at95yu9atm8azzmyvckulcc7kytlcckxswvvzej` on bitcoin mainnet), or *p2tr* taproot addresses. The special value *all* generates all known address types for the same underlying key." | ||
"It specifies the type of address wanted; currently *bech32* (e.g. `tb1qu9j4lg5f9rgjyfhvfd905vw46eg39czmktxqgg` on bitcoin testnet or `bc1qwqdg6squsna38e46795at95yu9atm8azzmyvckulcc7kytlcckxswvvzej` on bitcoin mainnet), *p2tr* taproot addresses, or *bip86* for BIP86-derived taproot addresses. The special value *all* generates all known address types for the same underlying key." |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Obv drop API change
@unittest.skipIf(TEST_NETWORK != 'regtest', "BIP86 tests are regtest-specific") | ||
def test_bip86_deterministic_addresses(node_factory): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can remove this with more precise checks above...
} | ||
|
||
u8 *towire_hsmd_derive_bip86_key(const tal_t *ctx UNNEEDED, u32 index UNNEEDED, bool is_change UNNEEDED) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
?
Important
25.09 FREEZE July 28TH: Non-bugfix PRs not ready by this date will wait for 25.12.
RC1 is scheduled on August 11th
The final release is scheduled for September 1st.
Checklist
Before submitting the PR, ensure the following tasks are completed. If an item is not applicable to your PR, please mark it as checked: