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.

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

[mobile] mobile macaroons support #3783

Merged
merged 6 commits into from
Dec 18, 2019

Conversation

halseth
Copy link
Contributor

@halseth halseth commented Dec 2, 2019

This adds initial macaroon support for mobile lnd.

Currently the admin macaroon is used, in the future we can be more granular, even let the applications supply macaroons themselves.

Builds on #3775

Depends on

Copy link
Collaborator

@bhandras bhandras left a comment

Choose a reason for hiding this comment

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

nice, only nits.

lnd.go Outdated
@@ -59,24 +60,72 @@ var (
networkDir string
)

// Creds is a list of DialOptions that can be used to authenticate with
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think the comment may be off.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed: 35b2de7

lnd.go Outdated
func Authenticate() ([]grpc.DialOption, error) {
creds, err := credentials.NewClientTLSFromFile(cfg.TLSCertPath, "")
if err != nil {
return nil, fmt.Errorf("unable to read TLC cert: %v", err)
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: s/TLC/TLS

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed: d245eb5

}

// Get the admin macaroon if macaroons are active.
if !cfg.NoMacaroons {
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: I know this is not part of this PR just double negation looks a bit weird :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, it is a result of macaroons being default and have to be turned off 😂

"path (check the network setting!): %v", err)
}

mac := &macaroon.Macaroon{}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is there any downside for having mac on the stack instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think the compiler mostly takes care of this: https://golang.org/doc/faq#stack_or_heap

mac := &macaroon.Macaroon{}
if err = mac.UnmarshalBinary(macBytes); err != nil {
return nil, fmt.Errorf("unable to decode macaroon: %v",
err)
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: you could probably move this to one line.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Line a tad bit too long

lnd.go Outdated

// RPCListener can be set to the listener to use for the RPC server. If
// nil a regular network listener will be created.
RPCListener net.Listener
RPCListener *ListenerWithSignal
}

// rpcListeners is a function type used for closures that fetches a set of RPC
// listeners for the current configuration, and the GRPC server options to use
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this godoc needs an update (grpc.ServerOption removed).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed: 7574b05

lnd.go Outdated Show resolved Hide resolved
lnd.go Outdated
//
// NOTE: This should only be called after the RPCListener has signaled it is
// ready.
func Authenticate() ([]grpc.DialOption, error) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should we maybe call this AdminAuthOptions? To me Authenticate sounds like it's performing an action or doing something other than just returning something.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed! 35b2de7

@halseth halseth force-pushed the mobile-macaroons branch 2 times, most recently from 2c1e7f6 to 1a69156 Compare December 17, 2019 13:22
Copy link
Collaborator

@bhandras bhandras left a comment

Choose a reason for hiding this comment

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

Ready to go 🚀

Copy link
Collaborator

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

Earlier we used emtpy grpc server options if custom listeners were set.
This was done to disable TLS. Now, we reuse the same server options as
for the regular listeners, in a move towards enabling TLS also here.
Adds a global Authenticate method that can be used to get the
atuhentication options needed to call the grpc server.

Currently meant only for used with the mobile bindings, so we use the
admin macaroon.
This makes the mobile bindings work with TLS and macaroons enabled,
which is supported from falafel 0.6.
Now that macaroons are supported, encourage usage.
@halseth halseth merged commit 773212b into lightningnetwork:master Dec 18, 2019
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.

None yet

3 participants