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.

Sign up for GitHub

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

Jump to bottom

multi: use lndclient MacaroonService #140

Merged
merged 2 commits into from
Jun 23, 2022
Merged

multi: use lndclient MacaroonService #140

merged 2 commits into from
Jun 23, 2022

Conversation

ellemouton
Copy link
Member

Since the code for creating and using a macaroon service is the same for
multiple projects (pool, loop, faraday & litd), the code has been
unified in lndclient. So this commit removes the macaroon service code
and instead uses the lndclient code.

Depends on lightninglabs/lndclient#86

@ellemouton ellemouton marked this pull request as draft January 14, 2022 13:10
@ellemouton ellemouton mentioned this pull request Jan 14, 2022
Merged
@ellemouton ellemouton force-pushed the macaroonService branch 2 times, most recently from c81b3b9 to 5b1ae63 Compare January 17, 2022 14:39
@ellemouton ellemouton marked this pull request as ready for review January 17, 2022 14:41
@ellemouton ellemouton requested review from carlaKC and guggero January 17, 2022 14:41
carlaKC
carlaKC approved these changes Jan 18, 2022
View reviewed changes
Copy link
Contributor

@carlaKC carlaKC left a comment

Choose a reason for hiding this comment

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

utACK

@guggero
Copy link
Member

guggero commented Jan 18, 2022

Tested this and seems to work well with upgrading/changing the password.
Though in Faraday this has the effect that now the readonly.macaroon isn't enough anymore to connect to lnd, because deriving the shared key requires a write permission.

This also causes the itest to fail:

[faraday-err] Error starting faraday: error starting macaroon service: unable to derive a shared secret with LND: rpc error: code = Unknown desc = permission denied

Not sure if this is a problem, @carlaKC ? If it is, we could say that since Faraday does read only stuff only (currently), encrypting the macaroon DB isn't as important as in the other daemons?

@carlaKC
Copy link
Contributor

carlaKC commented Jan 24, 2022

Though in Faraday this has the effect that now the readonly.macaroon isn't enough anymore to connect to lnd, because deriving the shared key requires a write permission.

I think it's ok to just switch over to needing write permissions for this purpose? If we update the docs accordingly, shouldn't be an issue.

guggero
guggero requested changes Jan 28, 2022
View reviewed changes
Copy link
Member

@guggero guggero left a comment

Choose a reason for hiding this comment

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

I think it's ok to just switch over to needing write permissions for this purpose? If we update the docs accordingly, shouldn't be an issue.

Okay, cool. Just wanted to let you know in case there are complaints later on...

@ellemouton seems like this requires a small change in the itest to no longer use the readonlym.macaroon. And we should also update the docs where necessary.

@ellemouton ellemouton force-pushed the macaroonService branch 2 times, most recently from e99db80 to 273594b Compare January 31, 2022 10:23
@ellemouton
Copy link
Member Author

updated 👍 thanks for catching!

@ellemouton ellemouton requested a review from guggero January 31, 2022 10:39
@guggero
Copy link
Member

guggero commented Jun 23, 2022

Argh, another PR that fell off the radar, sorry. Can you rebase please? Then this can get in before #147.

@ellemouton ellemouton force-pushed the macaroonService branch 3 times, most recently from 714aa15 to 7fe36ec Compare June 23, 2022 15:23
@ellemouton
multi: use lndclient MacaroonService
8a59081 
Since the code for creating and using a macaroon service is the same for
multiple projects (pool, loop, faraday & litd), the code has been
unified in lndclient. So this commit removes the macaroon service code
and instead uses the lndclient code.

Since the default key for the macaroon db is now a shared secret with
LND, faraday requires LND's admin macaroon so that this secret can be
derived. So this commit also updates all references of LND's `readme`
macaroon to the `admin` macaroon.
@ellemouton
frdrpc/rpcserver: rename createDefaultMacaroonFile
00a8a39 
Rename createDefaultMacaroonFile to withMacaroonService since this is
now a more appropriate name.
@ellemouton
Copy link
Member Author

rebased :)

@guggero guggero merged commit 2b33087 into lightninglabs:master Jun 23, 2022
guggero added a commit that referenced this pull request Jun 24, 2022
@guggero
frdrpcserver: also create macaroon service in subserver mode
f780b14 
This fixes a small oversight in #140 where we forgot to also _create_
the macaroon service in the StartAsSubserver() method.
guggero added a commit that referenced this pull request Jun 24, 2022
@guggero
frdrpcserver: also create macaroon service in subserver mode
42329df 
This fixes a small oversight in #140 where we forgot to also _create_
the macaroon service in the StartAsSubserver() method.
@guggero guggero mentioned this pull request Jun 24, 2022
Merged
1 task
@junderw
Copy link

junderw commented Apr 20, 2023 •
edited
Loading

This was unfortunate...

Edit: when bumping the newest master, I discovered that now admin.macaroon is needed.

This issue reverts #84 (not technically, but in spirit)

@junderw junderw mentioned this pull request Apr 20, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@carlaKC carlaKC carlaKC approved these changes

@guggero guggero guggero approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@ellemouton @guggero @carlaKC @junderw