Lnd: walletunlocker+lnd: add command line flag to allow passing macaroons after wallet creation

[Roasbeef]

In the current state, there's a bit of a gap when initializing a wallet for the first time remotely. The set of RPC's (GenSeed and InitWallet) allow users to programmatically instantiate a wallet for the first time. However, once the seed is created and returned to the user, in order to issue any further commands, they'll need to set of _macaroons_. Atm, the only way to obtain these macaroons is to either use sftp/rsync, etc to grab the macaroon on disk. Instead, we can extend the InnitWallet RPC call to return the serialized binary macaroon over the RPC, and _not_ write it to disk. This bridges the gap and allows the client to provision a new node in a purely remote fashion only using our set of RPC's. In the future, once we add rich macaroon baking capabilities to the RPC interface, the user that created the node (the alleged admin) will be able to instruct lnd to bake macaroons on the fly.

Steps To Completion

• [ ] Extend the InitWalletResponse to also optionally return the binary serialized admin.macaroon within the response.
• [ ] Add a new command line flag (something like --stateless-init) which instructs lnd to not write the macaroons to disk, and instead will only return them to the user over RPC.
• [ ] Implement the new logic in the walletunlocker package and update tests to exercise both operating modes

[guggero]

I'll work on this one.

[neogeno]

@guggero Any idea when this feature will be ready to merge?

[guggero]

The code is ready for review IMHO. So you might be able to speed things up by adding a review yourself. But in the end, at least two of the main developers will need to approve the PR.

[Roasbeef]

@neogeno if you'd like to help to test/review the PR, then that inches it closer to a mergeable state (even just testing an reporting your findings helps a lot)

[neogeno]

@Roasbeef I'm on it ..

[neogeno]

@guggero @Roasbeef I have reviewed this PR/commit and am happy to report that this revised initWallet RPC function does in fact return a result object of admin_macaroon : [Hex Bytes] correctly. However, I found an issue that when I then subsequently call the ChangePassword RPC, the admin macaroon should be encrypted with the new password specified by the user and returned as hex too, but this functionality has not been implemented.

In another scenario, which happens to exist in my app, a pool of cloud LND nodes is first launched by an automated process with no password so it can sync with the chain via btcd and then each node is assigned to a user when it is ready to use who will then change the password and assume it as his own. I am not sure how a new macaroon can be generated without writing to disk with the InitWallet RPC in this case since the wallet has already been created and synched, and there is no secure way to assign it to a user without starting a new wallet from scratch and syncing the node all over again?

Is it possible to modify the InitWallet and ChangePassword RPC functions to accommodate the scenario I described for nodes launched remotely on the cloud??

[Roasbeef]

Thanks for testing! ChangePassword doesn't re-encrypt macaroons. It re-encrypts the macaroon _database_.

[Roasbeef]

who will then change the password and assume it as his own

What if users forget to change the password? What if the service doesn't fully wipe the state? This sounds a bit dangerous and may results in users' funds being swapped.

[Roasbeef]

The more dangerous part is a service running nodes _for_ a user. The service must now take responsibility to ensure proper uptime of the nodes, otherwise users can miss time lock sweeps, etc.

[neogeno]

@roasbeef I do not agree with your opinion that this is unsafe. These are isolated servers running nothing but LND housed in industrial strength facilities with fire suppression, tight physical security, multi site redundancies and monitored 24x7 by a dedicated team. Do you actually think your own server or phone at home is just as safe?

[Roasbeef]

What I'm trying to say is, that you as the operator, are liable for the up time of those nodes. If _something_ happens (as things do), then you're responsible for that.

Yes, to me, my phone/server is safer as it's in my _physical_ possession, and I can audit it to the extent that my skill set allows.

[neogeno]

Yes you are only safe as your ability to keep your own node secure. Alas, in reality, we live in a world where over 90% of the population are not clued in on proper security and would rather use something that makes this effortless.

[Roasbeef]

You need to acknowledge that there are tradeoffs you're making that can potentially compromise security for your users. You need to be aware of these risks, and also educate your users accordingly. If you don't do so, then you do so at your own peril.

[neogeno]

Yes sure of course. I believe in being transparent and giving people a choice. This ultimately will be better for everyone in the community.

[neogeno]

@guggero @Roasbeef I have confirmed that once you launch a node in stateless init mode, your macaroon's are no longer valid after issuing a changepassword request. Calling The ChangePassword RPC will result in new macaroons on DISK which defeats the purpose of stateless init.

In My test:

admin.macaroon Received from stateless init RPC:
0201036c6e6402bb01030a103293fe5ebf9ea3c3e66022151e0f71881201301a160a0761646472657373120472656164120577726974651a130a04696e666f120472656164120577726974651a170a08696e766f69636573120472656164120577726974651a160a076d657373616765120472656164120577726974651a170a086f6666636861696e120472656164120577726974651a160a076f6e636861696e120472656164120577726974651a140a05706565727312047265616412057772697465000006202372649a6d0c8e57a8a5cc5e027cae58f84947815ff22226a689f3996974d835

New admin.macaroon written on disk after changing password
0201036c6e6402bb01030a10d290770ee06ef216c7f8821ad5fb947e1201301a160a0761646472657373120472656164120577726974651a130a04696e666f120472656164120577726974651a170a08696e766f69636573120472656164120577726974651a160a076d657373616765120472656164120577726974651a170a086f6666636861696e120472656164120577726974651a160a076f6e636861696e120472656164120577726974651a140a05706565727312047265616412057772697465000006203f102b57dcdff8b284628cf50141c5f54d11234a7364027a6548cfa387c0b7ce

Calling Getinfo straight after changing the password unlocks the wallet results in the following error:
details: 'verification failed: signature mismatch after caveat verification'

Need to amend the ChangePassword RPC code so it returns the new macaroon in binary serial format similar to InitWallet RPC

[guggero]

@neogeno: Thanks for your tests!
I have analyzed the problem in detail and came to the following conclusion:

When I created the PR for this feature, there was no wallet password change (PR #618 was merged a few days after I created #1288). I did see the password change PR because I had to fix merge conflicts. But I did not investigate the changes in details.
So as you say, the stateless initialization does currently not work at all if you change the password.

There are two distinct problems in the change password functionality:

• It is attempted to remove all *.macaroon files prior to changing the password. This fails in case lnd was initialized stateless because there are no macaroon files. The whole request fails and the passwords are not changed.
• The macaroon.db file is also just deleted instead of encrypted. So during the next startup, a new DB and therefore a new root key is being generated. This invalidates the initially created admin macaroon.

I can fix both problems. But the question is: How should the change password functionality behave?
In my opinion, there are two possibilities:

• The macaroon.db is not deleted but encrypted with the new password. This leaves the initial macaroon valid so no new macaroon has to be returned in the ChangePassword RPC. But this might go against the expectation of the user. Because he changes the password he might want to get a new macaroon.
• The macaroon.db is deleted and re-created. The new admin macaroon is returned in the ChangePassword RPC response. This poses the question if the stateless mode should be persisted or detected (by noticing the missing macaroon files) or if there should be the same --stateless_init flag in the changepassword command.

My personal recommendation is going with option 2 and auto detect stateless mode by the absence of macaroon files.

[neogeno]

@guggero I second that opinion and humbly request that you implement option 2 so that we get a new macaroon in the ChangePassword RPC response for better security. You may also add a flag for stateless mode if you prefer instead of detecting absence as the path for macaroon files may change.

I am happy to help testing it as many times as it takes so please let me know once the update is ready.

[guggero]

I have implemented option 2 as requested. It was a bigger rework than expected...
It was not possible to do it without adding the --stateless_init flag to all three startup commands create, unlock and changepassword.
Because for example during the unlock phase, the daemon does not know if the macaroon files are missing because we removed a previously set --no-macaroons flag or because it was initialized stateless. Therefore you also need to add the --stateless_init flag to the unlock command to avoid macaroon files to be created in the file system of the daemon!

To make these facts a bit clearer, I also added some documentation to the macaroons.db file.

@neogeno: You can go ahead and test, if you like. Thanks!

[Roasbeef]

There's no need to _delete_ the existing macaroon db, in the future this
will grow to hold more than just the root macaroon key. If the macaroon
itself is changing due to the changepassword rpc, rather than the db being
re-encrypted, then that's a bug and should be fixed.

On Sun, Jul 22, 2018, 9:35 AM Oliver Gugger [e-mail]
wrote:

I have implemented option 2 as requested.
It was not possible to do it without adding the --stateless_init flag to
all three startup commands create, unlock and changepassword.
Because for example during the unlock phase, the daemon does not know if
the macaroon files are missing because we removed a previously set
--no-macaroons flag or because it was initialized stateless. Therefore
you also need to add the --stateless_init flag to the unlock command to
avoid macaroon files to be created in the file system of the daemon!

To make these facts a bit clearer, I also added some documentation to the
macaroons.db http://docs/macaroons.db file.

@neogeno https://github.com/neogeno: You can go ahead and test, if you
like. Thanks!

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/lightningnetwork/lnd/issues/1236#issuecomment-406879900,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AA87LnTt2DWmNIypRAzW8I4fANx4x1yjks5uJKnOgaJpZM4T60Q9
.

[guggero]

Ah, that was kind of my question in the longer comment from a week ago: Does the user expect the macaroon root key to change as well when changing the password? Or should it stay the same?

The deletion of the macaroon db file was implemented in the change password PR, so I assumed that was correct. But I agree that there probably will be more information stored in there that should not be deleted.

So should I fix that problem in this PR or create a new one?

[Roasbeef]

Ah, that was kind of my question in the longer comment from a week ago: Does the user expect the macaroon root key to change as well when changing the password? Or should it stay the same?

It should stay the same, but be re-encrypted using the new passphrase. Apologies if review cycles can be a bit long atm, as there're 90+ PR's open, but <5 contributors actually doing review. As a result, things can pile up a bit in the short term.

The deletion of the macaroon db file was implemented in the change password PR, so I assumed that was correct.
Yeah that was an oversight, and is very incorrect if you consider that we'll soon start to store additional information aside from just the root macaroon in the database.

[neogeno]

I tested the latest code but was unable to get back any admin_macaroon when calling ChangePassword RPC with stateless init flag. I am using nodeJS and here is the JSON of the response {"admin_macaroon":{"type":"Buffer","data":[]}}

Also, I think for security reasons, if the user is changing the password, he doesn't necessarily want to keep the same macaroon file as if the user suspects his node is compromised, he would want to change BOTH password and macaroon in that case. SSH'ing into the remote node and deleting the macaroon files will not always be an option. So I think the ChangePassword RPC needs to return a new macaroon (with new macaroon root key) by default unless specified by the user.

[guggero]

@neogeno I'll have a look why it's not working on the weekend.

I'll implement an additional flag, something like --new_mac_root_key that changes the macaroon root key as well as the password. So the default would be to not change the key.

[neogeno]

@Roasbeef I can confirm I have tested the latest commit 1e6e526c49a9eef87e116031616fdf90d07d8fd1 from @guggero and the initwallet, unlock and changepassword RPC now work as expected and return the macaroons via TLS. I urge a swift approval/merge of this as it fixes the bug in the ChangePassword RPC that deletes the macaroon root key.

[MasterNeuron]

Same here:

• Found bundle for host 127.0.0.1: 0x55bea3889090 [can pipeline] * Re-using existing connection! (#0) with host 127.0.0.1 * Connected to 127.0.0.1 (127.0.0.1) port 9707 (#0) > POST /v1/invoices HTTP/1.1 Host: 127.0.0.1:9707 Grpc-Metadata-macaroon: 0201036c6e6402bb01030a10131b7c71e7347eca792604fef13b92ce1201301a160a0761646472657373120472656164120577726974651a130a04696e666f120472656164120577726974651a170a08696e766f69636573120472656164120577726974651a160a076d657373616765120472656164120577726974651a170a086f6666636861696e120472656164120577726974651a160a076f6e636861696e120472656164120577726974651a140a05706565727312047265616412057772697465000006200c041e64bc45ba27da7bf1cd7dc217c5a4a3ec1a8632c088c98858a108f894b4 User-Agent: Swagger-Codegen/0.4.1/php Accept: application/json Content-Type: application/json Content-Length: 63 * upload completely sent off: 63 out of 63 bytes < HTTP/1.1 500 Internal Server Error < Content-Type: application/json < Date: Sat, 01 Sep 2018 21:34:48 GMT < Content-Length: 86 < * Connection #0 to host 127.0.0.1 left intact Exception when calling LightningApi->addInvoice: [500] Server error: POST https://127.0.0.1:9707/v1/invoices resulted in a 500 Internal Server Error response: {"error":"verification failed: signature mismatch after caveat verification","code":2}

[madppiper]

Any chance this can be merged quickly? We are also waiting for this feature.

[gsmadi]

Any help needed on this issue to get it into an upcoming release? Seems like a re-init of conversation to finalize some details.

[halseth]

@guggero What are the remaining decisions that must be made before finalizing this one?

[guggero]

@halseth I added a comment in the PR, recapping the current implementation and open discussions: https://github.com/lightningnetwork/lnd/pull/1288#issuecomment-535068878