hsm_secret encryption #3129

darosior · 2019-10-03T21:58:02Z

This adds a new startup option which allows one to encrypt its hsm_secret, bearer of all its secrets. This uses the already-included (and amazing!) libsodium with defaults (i.e. Argon2id for deriving a key out of the provided password, and XChacha20_Poly1305 for stream encryption).

This and #2925 put, I think, the basis to have wallet locking. A plugin using the rpc_command hook to restrict the use of some RPC commands against a password, which can easily be bypassed if [it / lightningd] is killed, but not with an encrypted hsm_secret.

darosior · 2019-10-07T11:55:09Z

Rebased and added handling for non-encrypted to encrypted hsm. Still need to polish the tests and add the doc.

darosior · 2019-10-07T21:51:39Z

Rebased, added tests and doc, edited OP. Removing WIP.

darosior · 2019-10-08T09:05:29Z

Rebased, increase sleep()ing time for the test to pass on Travis.

rustyrussell

Like the idea, but minor changes needed before we can get this into -rc1 under @niftynei 's nose :)

lightningd/lightningd.c

rustyrussell · 2019-10-08T10:01:20Z

hsmd/hsmd.c

+			 * an error message. */
+			exit(1);
+		}
+	}


... else?

If it's encrypted, and they don't provide the key? [Edit: ah, fixed in next commit!]

Or if it's not, and they do? Or if it's been truncated to 31 bytes?

... else?

Otherwise we are done, close the fd and populate the secretstuff.

[Edit: ah, fixed in next commit!]

I could have squashed this one ^^ (but the diff was becoming too big and sparse)

Or if it's not, and they do?

Then we load the seed, delete the hsm_secret and call create_encrypted_hsm_secret().

Or if it's been truncated to 31 bytes?

Could it be ? Anyway the encrypted seed is always > 32 so I can make the condition a <=

If it's encrypted, and they don't provide the key? [Edit: ah, fixed in next commit!]

As a side note I handle this in hsm_control because I was not able to get satus_failed (with a new failure type) to send an error before replying to init. The msg type would always be zero. Hence also the hacky "wrong password" case handling.

hsmd/hsmd.c

darosior · 2019-10-08T11:40:25Z

Amended f4d65f4 to init libsodium in setup_subdaemons()
Correct the sizeof argument when unlocking memory (and correct a wrong freeing)
Use <= for the condition on the hsm_secret size
Add a skipping condition if VALGRIND to the test: the key generation consumes too much, and the prompt doesn't play well with it.

Thanks for the review!

darosior · 2019-10-08T14:32:53Z

Hmm looks like there is a travis cache problem: the check-source is obviously not up to date

niftynei · 2019-10-08T19:25:33Z

Hmm looks like there is a travis cache problem: the check-source is obviously not up to date

afaict there's no cache for branch 3129 on Travis

darosior · 2019-10-08T19:32:17Z

Hmm looks like there is a travis cache problem: the check-source is obviously not up to date

afaict there's no cache for branch 3129 on Travis

Ok, thanks for checking.
~~That's weird because the file version is not the same on Travis. I'm trying something else now [edit] it didnt work.~~
Nevermind I messed up my git tree and blamed the little guy with the hard hat..

trueptolemy

Looks great!
And you can also add two more functions in the future:

Change the passwaord:
Ask user to enter the old password, if it is valid then ask user to enter a new password.
Unencrypt:
Ask user to enter the old password, if it is valid then decrypt the secret and don't encrypt it again.

trueptolemy · 2019-10-09T13:22:05Z

common/daemon.c

+	 * not start if it cannot do its job correctly. */
+	if (sodium_init() == -1)
+		errx(1, "Could not initialize libsodium. Maybe not enough entropy"
+		         " available ?");


Only a question: What does this("not enough entropy") mean? It means the machine can't run clightning forever?

No it means that, in this rare case (maybe just after booting), the machine does not have enough source of randomness. https://en.wikipedia.org/wiki/Entropy_(computing)

trueptolemy · 2019-10-09T13:34:26Z

lightningd/options.c

+	printf("\n");
+
+	/* Derive the key from the password. */
+	if (strlen(passwd) < crypto_pwhash_argon2id_PASSWD_MIN)


Will the length of the password and the complexity of the password affect the encryption?
Should we limit minimum length?

It is limited actually in lightningd/options.c, but the limit is set to 0 by the library.

trueptolemy · 2019-10-09T13:34:33Z

lightningd/options.c

+		return "Could not disable password echoing.";
+	printf("Enter hsm_secret password : ");
+	if (getline(&passwd, &passwd_size, stdin) < 0)
+		return "Could not read password from stdin.";


I think we'd better ask users to enter the password twice and check the consistency.

If two passwords are different, users must repeat this setting process.

And if user enter enter or null, this process will be cancelled?

The decryption is done in hsmd, and I think it's better to keep it that way so that only one daemon interacts with hsm_secret. In case of a wrong password, an user can still arrow_up then enter :-).

And if user enter enter or null, this process will be cancelled?

If null is not its password then yes..:

trueptolemy · 2019-10-09T13:43:29Z

README.md

@@ -102,6 +102,8 @@ Once you've started for the first time, there's a script called
 `contrib/bootstrap-node.sh` which will connect you to other nodes on
 the lightning network.

+You can encrypt the BIP32 root seed (what is stored in `hsm_secret`) by passing the `--encrypted-hsm` startup argument. You can start `lightningd` with `--encrypted-hsm` on an already existing `lightning-dir` (with a not encrypted `hsm_secret`). If you pass that option, you __will not__ be able to start `lightningd` (with the same wallet) again without the password, so please beware with your password management. Also beware of not feeling too safe with an encrypted `hsm_secret`: unlike for `bitcoind` where the wallet encryption can restrict the usage of some RPC command, `lightningd` always need to access keys from the wallet which is thus __not locked__ (yet), even with an encrypted BIP32 master seed.
+


I think here we can also remind users like the sentence "Note if the password or hsm_secret losts, you WILL lost all your funds forever. MUST take good care of your hsm_secret file and its password."

darosior · 2019-10-09T14:33:39Z

Change the passwaord:
Ask user to enter the old password, if it is valid then ask user to enter a new password.

Unencrypt:
Ask user to enter the old password, if it is valid then decrypt the secret and don't encrypt it again.

I thought about making a tool to unencrypt hsm_secret, I'm not sure of making a JSONRPC request to hsmd (that's also why I made this a startup option in the first place).

According to the doc (https://download.libsodium.org/doc): "sodium_init() initializes the library and should be called before any other function provided by Sodium. [...] the function ensures that the system's random number generator has been properly seeded.".

Add a new startup option which will, if set, prompt the user for a password to derive a key from. This key will later be used to encrypt and/or decrypt `hsm_secret`. This was made a noarg option even if it would have been preferable to let the user the choice of how to specify the password. Since we have to chose, better to not let the password in the commands history.

This splits maybe_create_hsm_secret() in two parts (either encrypted or in clear) for clarity, and adds an encryption detection in load_hsm(). There are actually three cases if an encryption key is passed: - There is no hsm_secret => just create it and store the encrypted seed - There is an encrypted hsm_secret => the provided key should be able to decrypt the seed, if the wrong key is passed libsodium will nicely error and hsmd will exit() to not throw a backtrace (using status_failed() as for other errors) at the face of an user who mistyped its password. - There is a non-encrypted hsm_secret => load the seed, delete the hsm_secret, create the hsm_secret, store the encrypted seed.

…as not encrypted

And also allow to not wait for it to be started. Passing stderr=subprocess.STDOUT can be useful to wait_for_log() also on stderr messages.

rustyrussell · 2019-10-10T01:32:21Z

Trivial rebase.

rustyrussell · 2019-10-10T01:33:24Z

Ack e1af450

Once Travis is happy, I'll merge.

darosior force-pushed the hsm_encryption branch 2 times, most recently from f9731da to 5dc55aa Compare October 7, 2019 11:54

darosior requested a review from cdecker as a code owner October 7, 2019 11:54

darosior force-pushed the hsm_encryption branch 3 times, most recently from a1509a3 to 4a06029 Compare October 7, 2019 21:45

darosior changed the title ~~[WIP] hsm_secret encryption~~ hsm_secret encryption Oct 7, 2019

darosior force-pushed the hsm_encryption branch 2 times, most recently from 854a1d1 to 4e3db08 Compare October 8, 2019 09:03

rustyrussell requested changes Oct 8, 2019

View reviewed changes

darosior force-pushed the hsm_encryption branch from 4e3db08 to e48692f Compare October 8, 2019 11:38

darosior force-pushed the hsm_encryption branch from e48692f to 95f3be4 Compare October 8, 2019 12:09

darosior force-pushed the hsm_encryption branch 2 times, most recently from 419aa7f to 7fcbf3d Compare October 9, 2019 08:57

trueptolemy reviewed Oct 9, 2019

View reviewed changes

darosior added 7 commits October 10, 2019 12:00

lightningd: pass the hsm_secret encryption key in hsm_init

6e51ef4

lightningd: prevent hsmd from interpretating an encrypted hsm_secret …

5cbf091

…as not encrypted

pytest: allow to set stdin, stdout and stderr at lightningd startup

8d04ba1

And also allow to not wait for it to be started. Passing stderr=subprocess.STDOUT can be useful to wait_for_log() also on stderr messages.

pytest: test hsm_secret encryption

9c576cb

Document hsm_secret encryption

e1af450

rustyrussell force-pushed the hsm_encryption branch from 7fcbf3d to e1af450 Compare October 10, 2019 01:32

rustyrussell added this to the 0.7.3 milestone Oct 10, 2019

rustyrussell approved these changes Oct 10, 2019

View reviewed changes

niftynei merged commit fac5faa into ElementsProject:master Oct 10, 2019

darosior deleted the hsm_encryption branch October 17, 2019 13:19

darosior mentioned this pull request Nov 28, 2019

RPC locking #3303

Closed

jb55 mentioned this pull request Feb 17, 2020

Decrypt hsm_secret jb55/clightning-dumpkeys#2

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

hsm_secret encryption #3129

hsm_secret encryption #3129

darosior commented Oct 3, 2019 •

edited

Loading

darosior commented Oct 7, 2019

darosior commented Oct 7, 2019

darosior commented Oct 8, 2019

rustyrussell left a comment

rustyrussell Oct 8, 2019

darosior Oct 8, 2019 •

edited

Loading

darosior Oct 8, 2019 •

edited

Loading

darosior commented Oct 8, 2019 •

edited

Loading

darosior commented Oct 8, 2019

niftynei commented Oct 8, 2019

darosior commented Oct 8, 2019 •

edited

Loading

trueptolemy left a comment

trueptolemy Oct 9, 2019

darosior Oct 9, 2019

trueptolemy Oct 9, 2019

darosior Oct 9, 2019

trueptolemy Oct 9, 2019

trueptolemy Oct 9, 2019

trueptolemy Oct 9, 2019

darosior Oct 9, 2019 •

edited

Loading

trueptolemy Oct 9, 2019

darosior commented Oct 9, 2019 •

edited

Loading

rustyrussell commented Oct 10, 2019

rustyrussell commented Oct 10, 2019

hsm_secret encryption #3129

hsm_secret encryption #3129

Conversation

darosior commented Oct 3, 2019 • edited Loading

darosior commented Oct 7, 2019

darosior commented Oct 7, 2019

darosior commented Oct 8, 2019

rustyrussell left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

darosior Oct 8, 2019 • edited Loading

Choose a reason for hiding this comment

darosior Oct 8, 2019 • edited Loading

Choose a reason for hiding this comment

darosior commented Oct 8, 2019 • edited Loading

darosior commented Oct 8, 2019

niftynei commented Oct 8, 2019

darosior commented Oct 8, 2019 • edited Loading

trueptolemy 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

darosior Oct 9, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

darosior commented Oct 9, 2019 • edited Loading

rustyrussell commented Oct 10, 2019

rustyrussell commented Oct 10, 2019

darosior commented Oct 3, 2019 •

edited

Loading

darosior Oct 8, 2019 •

edited

Loading

darosior Oct 8, 2019 •

edited

Loading

darosior commented Oct 8, 2019 •

edited

Loading

darosior commented Oct 8, 2019 •

edited

Loading

darosior Oct 9, 2019 •

edited

Loading

darosior commented Oct 9, 2019 •

edited

Loading