Skip to content

Conversation

@guggero
Copy link
Contributor

@guggero guggero commented Aug 28, 2020

Fixes #194.

Depends on lightningnetwork/lnd#4464 (and indirectly on lightningnetwork/lnd#4463).

With this PR we first secure the loop daemon's RPC connection by encrypting the transmitted data with TLS.
Next we add macaroon authentication to the RPC to be able to restrict access to authorized users only.
With these two features enabled by default, the loop RPC port can be opened to the internet because unauthorized access is no longer possible.

By default only one macaroon is created for loop and stored in a file called ~/.loop/<network>/loop.macaroon.To give users the ability to fully customize their access with custom macaroons, the two functionalities BakeMacaroon and ListPermissions are copied from lnd and added to both the RPC and command line. --> Split this part off into its own PR: #285

@guggero guggero requested review from carlaKC and joostjager August 28, 2020 15:02
@carlaKC
Copy link
Contributor

carlaKC commented Aug 31, 2020

Depends on lightningnetwork/lnd#4464 (and indirectly on lightningnetwork/lnd#4463).

Just a procedural question about this dependency, is this PR now going to wait until we tag 0.12 to go in? Or are the lnd PRs going into lnd v0.11.1?

Seems to be the case as per 3ad4cd9.

@guggero
Copy link
Contributor Author

guggero commented Aug 31, 2020

Yes, the dependent PRs are both candidates for lnd v0.11.1, so this PR has to wait for that to be released.
But it's a compile-time dependency only, not an RPC dependency.

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.

Awesome to have thins functionality added to loop 🎉
No major questions, since a lot of this is copied from lnd, just some nits from my first pass. Will do some testing for this as well.

Perhaps also update the README since we're going to enable by default?

Copy link
Contributor

@joostjager joostjager left a comment

Choose a reason for hiding this comment

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

Just quickly skimmed through the PR. Is it worth to split this PR before continuing into the review cycle? For example in three parts:

  • Add TLS
  • Add (single) macaroon
  • Add macaroon baking via rpc

@guggero guggero changed the title loopd: add TLS and macaroon authentication to the loop RPC server [1/2] loopd: add TLS and macaroon authentication to the loop RPC server Aug 31, 2020
Copy link
Contributor

@joostjager joostjager left a comment

Choose a reason for hiding this comment

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

Tested and works. Was just wondering how I can verify that it is now actually using TLS?

@guggero
Copy link
Contributor Author

guggero commented Sep 2, 2020

I addressed all comments and also added a short section about TLS and macaroons to the README.

@joostjager you can verify if TLS is enabled by using openssl s_client -connect <host>:<port>. If you get presented a ceritifcate then TLS is turned on.

@guggero guggero force-pushed the macaroons branch 2 times, most recently from 9b99779 to 9d78ec2 Compare September 3, 2020 12:03
@guggero guggero requested a review from joostjager September 3, 2020 12:03
loopd/daemon.go Outdated
// Next, start the gRPC server listening for HTTP/2 connections.
log.Infof("Starting gRPC listener")
d.grpcListener, err = d.listenerCfg.grpcListener()
d.grpcListener, err = d.listenerCfg.grpcListener(nil)
Copy link
Contributor

Choose a reason for hiding this comment

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

If it crashes with nil, this isn't a working commit and possibly a sign that it should be squashed with another one. But non-blocking.

@joostjager
Copy link
Contributor

But it's a compile-time dependency only, not an RPC dependency.

With grub, a comment like this isn't relevant anymore, because even for compile-time deps we need to wait for a release?

@guggero guggero changed the title [1/2] loopd: add TLS and macaroon authentication to the loop RPC server [2/3] loopd: add TLS and macaroon authentication to the loop RPC server Sep 3, 2020
@guggero guggero changed the title [2/3] loopd: add TLS and macaroon authentication to the loop RPC server [2/3] loopd: add macaroon authentication to the loop RPC server Sep 3, 2020
@guggero guggero mentioned this pull request Sep 3, 2020
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.

LGTM

@guggero guggero force-pushed the macaroons branch 3 times, most recently from 404420c to e365c4a Compare September 3, 2020 12:43
@guggero
Copy link
Contributor Author

guggero commented Sep 3, 2020

With grub, a comment like this isn't relevant anymore, because even for compile-time deps we need to wait for a release?

We have to wait for a release (of lnd and loop that is) in LiT. But we can merge this PR as soon as the dependent PRs are merged to master. That's the main difference between an RPC and compile time dep IMO.

@joostjager
Copy link
Contributor

joostjager commented Sep 3, 2020

But won't your compile-time required code disappear when loop is embedded in grub and the dependency is overriden with the last release 0.11.0? This PR would block other features from being included in LiT that are merged after this PR but before the 0.11.1 release?

We update to the newest version of lnd so we can use the updated
macaroon service.
NOTE: This is a compile time dependency update only, no RPC level update
is required.
To secure access to loop's RPC server, we add a macaroon authentication
service and its gRPC interceptors to the daemon's server connection.
When loopd runs in the same process as lnd (in LiT), it hooks itself
into lnd's RPC server as an external subserver. But because the user
should still be able to use the default loop macaroon, the loop daemon
must be able to validate its own macaroons as lnd's macaroon service
doesn't know the root key for it.
@guggero guggero merged commit d638d07 into lightninglabs:master Sep 11, 2020
@guggero guggero deleted the macaroons branch September 11, 2020 10:44
@Kixunil
Copy link
Contributor

Kixunil commented Sep 16, 2020

Awesome! @guggero do you have a LN donation page? Would like to send you a tip for doing this. :)

@guggero
Copy link
Contributor Author

guggero commented Sep 16, 2020

Thanks for the feedback, I'm happy to hear this is useful! No need for a tip though, I am getting paid to work on these projects ;-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

rpc: add tls and macaroon auth for loopd clients

4 participants