Skip to content

multi: allow LND and subserver whitelisted calls #617

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
Merged
merged 7 commits into from
Sep 8, 2023
Merged

multi: allow LND and subserver whitelisted calls #617

merged 7 commits into from
Sep 8, 2023

Conversation

@ellemouton
Copy link
Member

@ellemouton ellemouton commented Aug 7, 2023 •
edited
Loading

This PR does a few things:

  1. Ensures that Lit register's LND's State server to it's REST server
  2. Allow LND's & Lit's and all Lit's subserver's whitelisted endpoints to pass through without requiring a macaroon

@ellemouton ellemouton force-pushed the allowLNDWhitelistCalls branch from 8dab4e4 to d6eb106 Compare August 7, 2023 14:23
@ellemouton ellemouton mentioned this pull request Aug 7, 2023
Merged
@ellemouton ellemouton force-pushed the allowLNDWhitelistCalls branch from d6eb106 to 7387482 Compare August 8, 2023 09:36
@ellemouton ellemouton requested a review from guggero August 8, 2023 09:40
guggero
guggero approved these changes Aug 8, 2023
View reviewed changes
Copy link
Contributor

@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.

LGTM 🎉

@ellemouton ellemouton requested a review from ViktorT-11 August 8, 2023 11:32
@jamaljsr
Copy link
Member

jamaljsr commented Aug 8, 2023

When integrating with tapd's universe stats HTTP endpoints, I ran into an issue with litd requiring a macaroon for whitelisted RPCs.

Can this be addressed in this PR or should I open a separate issue?

@ellemouton
Copy link
Member Author

ah! great catch @jamaljsr ! Yeah I think it defs makes sense to add that in this PR. Will do asap 👍

@ellemouton ellemouton force-pushed the allowLNDWhitelistCalls branch from 7387482 to e2708fa Compare August 11, 2023 08:59
ellemouton
ellemouton commented Aug 11, 2023
View reviewed changes
@ellemouton ellemouton requested a review from guggero August 11, 2023 09:11
@ellemouton
Copy link
Member Author

@guggero - apologies, im re-requesting your review cause quite a bit has changed now so that whitelisted calls from other sub-servers are also taken into account

@ellemouton ellemouton changed the title multi: allow lnd whitelist calls multi: allow LND and subserver whitelisted calls Aug 11, 2023
guggero
guggero reviewed Aug 15, 2023
View reviewed changes
Copy link
Contributor

@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.

Very nice, just one small change then we're good to go.

@ellemouton ellemouton force-pushed the allowLNDWhitelistCalls branch from e2708fa to 14918de Compare August 15, 2023 09:31
@ellemouton
Copy link
Member Author

Thanks @guggero 🎉 updated!

guggero
guggero approved these changes Aug 15, 2023
View reviewed changes
@ellemouton ellemouton force-pushed the allowLNDWhitelistCalls branch from 14918de to 2222f5a Compare August 15, 2023 12:34
@ellemouton ellemouton force-pushed the allowLNDWhitelistCalls branch from 2222f5a to 22bdf79 Compare August 30, 2023 18:13
@lightninglabs-deploy
Copy link

@ViktorTigerstrom: review reminder

ViktorT-11
ViktorT-11 approved these changes Sep 6, 2023
View reviewed changes
Copy link
Contributor

@ViktorT-11 ViktorT-11 left a comment

Choose a reason for hiding this comment

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

LGTM, very clean PR 🔥🚀!

Just for clarification, I just want to verify as well that it's intentional that the LightningTerminal.Permissions function won't include any whitelisted urls, as will happen with this PR. That's intended behaviour, correct? I'm asking because I'm not 100% what "permissions for which the external validator of the terminal is responsible." really entails.

@ellemouton
terminal: register to the LND State REST server
089e718 
Before this commit, LND's State server could not be accessed via Lit's
REST server.
@ellemouton
rpc_proxy+itest: return uniform error for unhandled URI
d856616 
Currently `basicAuthToMacaroon` returns a different error for an
un-handled URI than is returned for other funcions which first check the
permissions manager to see if a URI is handled. With this commit, we
ensure that the error returned is the same so that the error we assert
on in tests can just be one error.
@ellemouton
perms: add IsWhiteListURL method to the Manager
705bf12 
This commit adds a new `IsWhiteListURL` to the permissions manager. This
can then be used by LiT to check if it should perform macaroon
validation on a query or not.
@ellemouton
terminal: don't verify macaroon for whitelisted calls
84df1bf
@ellemouton
subservers: add WhiteListedURLs to the SubServer interface
1332106 
Add a new `WhiteListedURLs` method to the `SubServer` interface
so that Lit can easily collect the set of permissions from each
sub-server that does not require a macaroon.
@ellemouton
perms+subservers: register subserver whitelisted urls with perms manager
15a69e0
@ellemouton
itest: Add tests for whitelisted LND and Taproot-assets URIs
e766e9a
@ellemouton
Copy link
Member Author

Just for clarification, I just want to verify as well that it's intentional that the LightningTerminal.Permissions function won't include any whitelisted urls, as will happen with this PR. That's intended behaviour, correct? I'm asking because I'm not 100% what "permissions for which the external validator of the terminal is responsible." really entails.

As far as I can see, LightningTerminal.Permissions() calls perms.Manager.GetLitPerms() which returns everything in fixedPerms. If a Lit itself has whitelist perms, these will be added to fixedPerms in the NewManager function. Then, if any subserver registers whitelist URLs via RegisterSubServer, then these will also be added to fixedPerms. So Lightning.Permissions right now will include the whiltelist calls. Or am I misunderstanding?

@ellemouton ellemouton force-pushed the allowLNDWhitelistCalls branch from 22bdf79 to e766e9a Compare September 7, 2023 09:29
@ViktorT-11
Copy link
Contributor

ViktorT-11 commented Sep 7, 2023 •
edited
Loading

As far as I can see, LightningTerminal.Permissions() calls perms.Manager.GetLitPerms() which returns everything in fixedPerms.

Ah wait, I realised that I was confused sorry. I interpreted yesterday that GetLitPerms looped over the specific []bakery.Op list for an url to add it to result, which would have resulted in whitelisted urls not getting added as a url is whitelisted by having an empty []bakery.Op list.
But I see now that GetLitPerms loops over the outer map[string][]bakery.Op, which should mean that the specific url does indeed get added with an empty []bakery.Op as the result.

Sorry for the confusion :).

@ellemouton ellemouton merged commit edab9a4 into lightninglabs:master Sep 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ViktorT-11 ViktorT-11 ViktorT-11 approved these changes

+1 more reviewer

@guggero guggero guggero approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@ellemouton @jamaljsr @lightninglabs-deploy @ViktorT-11 @guggero