multi: bake super macaroon during stateless init mode (V2) #858

ellemouton · 2024-09-30T12:14:42Z

An alternative approach to #844

Looking for approach ACK here first (ie, ACK-ing this approach over #844) after which I'll work on an itest.

Replaces #844

In this commit, we special case the handling of BakeSuperMacaroon so as
to allow a user to make use of litcli bakesupermacaroon while LiT is
running in stateless init mode. The handling is as follows:

if the call to LiT's BakeSuperMacaroon is made while in stateless
init mode then we can assume that the macaroon provided is either:
1) an LND native macaroon which may or may not have the
necessary permissions for the LND
"/lnrpc.Lightning/BakeMacaroon" call.
2) a baked macaroon (possibly a super macaroon) which may or may
not have the permissions to the LiT
"/litrpc.Proxy/BakeSuperMacaroon" call.

For case 1: we check that the provided macaroon has the correct perms.
If it does, then we use LiT's existing connection to LND to bake the
super mac.
For case 2: we have a macaroon that doesnt have LND's bakemac call perms
directly but does have LiT's BakeSuperMac perms. So for this, we treat
the call as normal and verify using LiT's macaroon validator as normal.

Fixes #736

ellemouton · 2024-09-30T12:15:22Z

cc @ViktorTigerstrom for approach ACK here.

Decided to open a new PR instead of pushing to the old PR since the approaches are completely different

ViktorT-11

Definitely prefer this approach compared to #844, as it's much cleaner and easier to use for users. Awesome job 🚀🔥!

There's one issue though that neither this or the other PR addresses unfortunately, that'll require some refactoring (potentially quite big, and I'm not quite sure we'll be able be able to address it easily😞):
Both of these PRs will require that the macaroonService has started. Now the reason why that's problematic is because that only happens in the startInternalSubServers, which is only executed after both lnd has been fully synced, and our internal sub-servers have been started. Now the reason that's problematic is that the feedback we've gotten from our users is that they wouldn't like to wait for lnd to be fully synced before macaroons can be generated.

I think we miiight potentially be able to make a bakesupermacaroon call in stateless_init mode work before lnd has been fully synced, as it only require a Basic Lnd Client (and it's the Full Lnd Client in setUpLNDClients that requires the chain sync). The perms for all sub-servers are also added in initSubServers which is executed way earlier, so the perms manager is aware of all different sub-servers permissions before the sub-servers are started.

If you do think that's a user requirement this PR/functionality shouldn't address, let me know, as I then prefer this approach over the other PR!

ViktorT-11 · 2024-10-04T10:58:21Z

terminal.go

 		// daemons. In all other cases we want default macaroons so we
 		// can use the CLI tools to interact with loop/pool/faraday.
 		macService := g.lndInterceptorChain.MacaroonService()
-		createDefaultMacaroons = !macService.StatelessInit


Noting that this happens after terminal.go setUpLNDClients has been executed.

ellemouton · 2024-10-04T12:01:25Z

Thanks for the feedback @ViktorTigerstrom - although, iiuc, This should not be an issue since we should not be hitting LiT's macaroon service for this call if an LND admin macaroon is passed in. We only hit that if there is an error validating the macaroon via or if the statelessinit boolean is still false. So I think your concern is easily solved by just moving the spot that the statelessinit boolean gets set. I have tested this locally with an LND node that never syncs and am able to bake the macaroon just fine. Let me know if im misunderstanding something

ellemouton · 2024-10-04T12:11:32Z

@ViktorTigerstrom - see latest commit

ViktorT-11 · 2024-10-04T12:15:36Z

Thank for the response and update @ellemouton!

I have tested this locally with an LND node that never syncs and am able to bake the macaroon just fine. Let me know if im misunderstanding something

Oh interesting, I hit the "the macaroon service has not started yet" when testing it locally during the review. I'll test again with your changes though, as I there may have been some issue on my side :)

ellemouton · 2024-10-04T12:29:16Z

as I there may have been some issue on my side

No i dont think there was a specific issue on your side - it makes sense that we would hit that error if the statelessinit boolean is not yet set. But there is no reason we cant set it earlier.

My main point is that it doesnt make sense to need to wait for LiTs macaroon service to be started for this since it is not required for what we are doing here.

ViktorT-11

tACK LGTM 🚀🎉, after fixing the release notes issue!

You were right Elle, I think I mistook that error due to the late assignment that's now fixed 🙂!

Really like that this functionally can handle any of lnd admin macs, previously baked macs, and also normal lit.macaroon:s 🔥

lightninglabs-deploy · 2024-10-14T00:44:29Z

@guggero: review reminder

ellemouton · 2024-10-14T06:40:58Z

will try get round to adding an itest this week.

guggero

tACK, LGTM 🎉

config.go

terminal.go

rpc_proxy.go

guggero

Marking this as "request changes" until we have an itest, which would definitely be nice.

ellemouton · 2024-10-14T13:42:35Z

Alrighty, itest added. pls see from commit litrpc: run make rpc onwards :)

So that it is easy to check elsewhere if we are in this mode.

In this commit, we special case the handling of BakeSuperMacaroon so as to allow a user to make use of `litcli bakesupermacaroon` while LiT is running in stateless init mode. The handling is as follows: - if the call to _LiT's_ BakeSuperMacaroon is made while in stateless init mode then we can assume that the macaroon provided is either: 1) an LND native macaroon which may or may not have the necessary permissions for the _LND_ "/lnrpc.Lightning/BakeMacaroon" call. 2) a baked macaroon (possibly a super macaroon) which may or may not have the permissions to the _LiT_ "/litrpc.Proxy/BakeSuperMacaroon" call. For case 1: we check that the provided macaroon has the correct perms. If it does, then we use LiT's existing connection to LND to bake the super mac. For case 2: we have a macaroon that doesnt have LND's bakemac call perms directly but does have LiT's BakeSuperMac perms. So for this, we treat the call as normal and verify using LiT's macaroon validator as normal.

So that it is set correctly by the time we potentially want to special case the handling of the bakesupermacaroon method. This is required in the case where the full LND connection takes long to create due to it blocking on LND being fully synced.

Use LiT's new(ish) BakeSuperMacaroon endpoint instead of redefining all the logic again.

This is so that we can use this Wait function to check the state of sub-servers even when LiT is running in stateless init mode. As currently, a macaroon is used to make a call to each sub-server which we wont have access to in stateless init mode. This commit also adds a new WaitForLNDWalletReady call which only waits for LND's wallet to be ready. We'll use this during the set-up of a stateless init node in an itest so that we can know when we are able to Create the wallet.

So that a node can be started without unlocking the wallet.

guggero

Very nice 🎉

itest/litd_mode_integrated_test.go

itest/network_harness.go

This will be used in our stateless init tests to initialise a LiT node in stateless init mode. This method handles the creation of the wallet and returns the LND admin macaroon that we can use.

Extract some of the tests used for the integrated and remote mode tests and re-use them in a new test that runs them against a node stareted in state-less init mode.

ViktorT-11

tACK LGTM 🚀! Great work 🔥!

Thanks a lot for the itest + the option to also create a read_only macaroon :)

ViktorT-11 · 2024-10-17T23:13:11Z

itest/litd_stateless_init_test.go

+	charlie, adminMac := net.NewNodeWithSeed(
+		t.t, "Charlie", nil, walletPassword, false, true,
+	)


Should we maybe add test coverage for what the intended behaviour is when attempting to use a remote lnd node + stateless init mode, as we've had users who have attempted that? Else feel free to disable setting both of them to true in the NewNodeWithSeed function.

what would such a test look like though? cause lit would just fail to connect to lnd cause there would be no macaroon to authenticate itself

Else feel free to disable setting both of them to true in the NewNodeWithSeed function.

dont think we need to explicitly guard against the set-up though - the test will fail either way :)

gonna merge this in the mean time & we can continue discussion offline with possible follow up 👍

ellemouton requested a review from ViktorT-11 September 30, 2024 12:14

ellemouton mentioned this pull request Sep 30, 2024

multi: bake super macaroon during stateless init mode #844

Closed

levmi assigned ellemouton Oct 3, 2024

ViktorT-11 reviewed Oct 4, 2024

View reviewed changes

ViktorT-11 approved these changes Oct 4, 2024

View reviewed changes

guggero self-requested a review October 6, 2024 07:52

guggero approved these changes Oct 14, 2024

View reviewed changes

config.go Outdated Show resolved Hide resolved

terminal.go Outdated Show resolved Hide resolved

rpc_proxy.go Outdated Show resolved Hide resolved

rpc_proxy.go Outdated Show resolved Hide resolved

guggero suggested changes Oct 14, 2024

View reviewed changes

ellemouton force-pushed the bakeSuperMacInStatelessV2 branch 3 times, most recently from 1e4bd50 to 7a94f5a Compare October 14, 2024 13:36

ellemouton requested review from ViktorT-11 and guggero October 14, 2024 13:44

ellemouton added 9 commits October 14, 2024 15:48

perms: hold the mutex for any public call

d3133ff

lit: add a statelessInit bool in the config

2fc07ff

So that it is easy to check elsewhere if we are in this mode.

rpc_proxy: give the proxy access to basicLNDClient

d3dc753

litrpc: run make rpc

a3e8c17

litrpc+proto: add ReadOnly param to BakeSuperMacaroon

a59bf27

multi: use ReadOnly param of BakeSuperMacaroon

b4f8224

itest: cleanup bakeSuperMacaroon helper

52cab3b

ellemouton added 3 commits October 14, 2024 15:48

itest: use LiTs BakeSuperMacaroon endpoint for helper

e4e68d0

Use LiT's new(ish) BakeSuperMacaroon endpoint instead of redefining all the logic again.

itest: pass SkipUnlock to BaseNodeConfig

28b8f2a

So that a node can be started without unlocking the wallet.

ellemouton force-pushed the bakeSuperMacInStatelessV2 branch from 7a94f5a to 44a7065 Compare October 14, 2024 13:49

guggero approved these changes Oct 14, 2024

View reviewed changes

itest/litd_mode_integrated_test.go Outdated Show resolved Hide resolved

itest/litd_mode_integrated_test.go Show resolved Hide resolved

itest/network_harness.go Outdated Show resolved Hide resolved

ellemouton added 3 commits October 14, 2024 16:57

itest: add NewNodeWithSeed

70cacf0

This will be used in our stateless init tests to initialise a LiT node in stateless init mode. This method handles the creation of the wallet and returns the LND admin macaroon that we can use.

itest: add a stateless_init test

b76832a

Extract some of the tests used for the integrated and remote mode tests and re-use them in a new test that runs them against a node stareted in state-less init mode.

docs: add release note

a38837d

ellemouton force-pushed the bakeSuperMacInStatelessV2 branch from 44a7065 to a38837d Compare October 14, 2024 14:57

ViktorT-11 approved these changes Oct 17, 2024

View reviewed changes

ellemouton merged commit 9eb11e2 into lightninglabs:master Oct 18, 2024
13 checks passed

ellemouton deleted the bakeSuperMacInStatelessV2 branch October 18, 2024 07:33

Uh oh!

multi: bake super macaroon during stateless init mode (V2) #858

multi: bake super macaroon during stateless init mode (V2) #858

Uh oh!

Conversation

ellemouton commented Sep 30, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ellemouton commented Sep 30, 2024

Uh oh!

ViktorT-11 left a comment

Choose a reason for hiding this comment

Uh oh!

ViktorT-11 Oct 4, 2024

Choose a reason for hiding this comment

Uh oh!

ellemouton commented Oct 4, 2024

Uh oh!

ellemouton commented Oct 4, 2024

Uh oh!

ViktorT-11 commented Oct 4, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ellemouton commented Oct 4, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ViktorT-11 left a comment

Choose a reason for hiding this comment

Uh oh!

lightninglabs-deploy commented Oct 14, 2024

Uh oh!

ellemouton commented Oct 14, 2024

Uh oh!

guggero left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

guggero left a comment

Choose a reason for hiding this comment

Uh oh!

ellemouton commented Oct 14, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

guggero left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ViktorT-11 left a comment

Choose a reason for hiding this comment

Uh oh!

ViktorT-11 Oct 17, 2024

Choose a reason for hiding this comment

Uh oh!

ellemouton Oct 18, 2024

Choose a reason for hiding this comment

Uh oh!

ellemouton Oct 18, 2024

Choose a reason for hiding this comment

Uh oh!

ellemouton Oct 18, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

ellemouton commented Sep 30, 2024 •

edited

Loading

ViktorT-11 commented Oct 4, 2024 •

edited

Loading

ellemouton commented Oct 4, 2024 •

edited

Loading

ellemouton commented Oct 14, 2024 •

edited

Loading