Initial macaroon implementation #250

aakselrod · 2017-08-09T00:59:12Z

This is an initial macaroon implementation. It partially fixes issue #20.

lncli already includes the client-side macaroon usage with basic replay protection (an additional caveat that makes the request good for only 1 minute by default). To authenticate your request to lnd with your own framework, put the hex-encoded macaroon in the GRPC request's metadata under the key macaroon. To authenticate your request to lnd via the REST proxy, put the hex-encoded macaroon in an HTTP header called Grpc-metadata-macaroon like so:

$ curl -k --header "Grpc-metadata-macaroon: 303031316C6F636174696F6E206C6E640A303033326964656E74696669657220302D31306563323637613262306566633137613235376662363561616463656161610A3031326363696420616C6C6F77207665726966796D65737361676520676574696E666F206C69737470656572732077616C6C657462616C616E6365206368616E6E656C62616C616E63652070656E64696E676368616E6E656C73206C6973746368616E6E656C73206C6F6F6B7570696E766F696365206C697374696E766F6963657320737562736372696265696E766F69636573207375627363726962657472616E73616374696F6E73206765747472616E73616374696F6E732064657363726962656772617068206765746368616E696E666F206765746E6F6465696E666F207175657279726F75746573206765746E6574776F726B696E666F207375627363726962656368616E6E656C6772617068206C6973747061796D656E7473206465636F64657061797265710A303032667369676E61747572652021A83E1B1EEDC26B8AC36DEF17DE0A06F1B11C253212A270EBA30A8E552634760A" https://localhost:8080/v1/getinfo
{"identity_pubkey":"02c5f2d1e9aa3acc59e385d8c5a45daac39314d930d8a3837be4fbb0f611401c0c","block_height":1156969,"block_hash":"0000000000000409567a8c97e501ef2c76e3df07ad287b016fdfca5659ca5e96","synced_to_chain":true,"testnet":true,"chains":["bitcoin"]}

Macaroons can be turned off with the --no-macaroons option to both lnd and lncli. If lnd is run with --no-macaroons and the client passes a macaroon, the macaroon will be ignored. If lnd is run with --no-macaroons before it generates macaroon files, lncli must be run with --no-macaroons or it will still look for a macaroon file and fail.

This PR also removes lncli's dependency on lnd.conf for the TLS certificate, as that was causing issues for users. Now, the TLS certificate specification behaves like macaroon file specification: only a command-line switch with a default settings.

Additional future improvements over the next weeks/months:

Add macaroon-specific tests
Add command-line tool to create/delegate macaroons
Encrypt the macaroon database with the wallet password
Add support for root key rotation and possibly macaroon rotation
Add support for accounting-based macaroons
Add better request replay protection
Add support for additional restrictions, such as requester's source IP
Add third-party caveat support (long-term, requires further analysis and design)

mention-bot · 2017-08-09T00:59:14Z

@aakselrod, thanks for your PR! By analyzing the history of the files in this pull request, we identified @Roasbeef, @AndrewSamokhvalov and @bryanvu to be potential reviewers.

rodasmith

Thanks for coding it up. I am not yet up to speed so I cannot confidently ack but it looks like an excellent step forward.

rodasmith · 2017-08-11T05:48:03Z

cmd/lncli/main.go

@@ -52,18 +64,22 @@ func getClientConn(ctx *cli.Context) *grpc.ClientConn {
 	// * https://github.com/go-macaroon/macaroon
 	cfg := config{
 		TLSCertPath: defaultTLSCertPath,
+		AdmMacPath:  defaultAdmMacPath,


(trivial) Consider expanding these spellings to AdminMacPath and defaultAdminMacPath throughout for clarity.

I've now made this change.

rodasmith · 2017-08-11T06:08:58Z

cmd/lncli/main.go

+	// the server clock is behind the client clock, or shortened by the time
+	// the server clock is ahead of the client clock (or invalid altogether
+	// if, in the latter case, this time is more than 60 seconds).
+	timeCaveat := checkers.TimeBeforeCaveat(time.Now().Add(time.Minute))


Should this expiration be configurable?

This expiration is now configurable with --macaroontimeout.

rodasmith · 2017-08-11T06:24:18Z

cmd/lncli/main.go

+	// lets us try to get the highest possible permissions by default when
+	// the command line doesn't specify a macaroon file to use for auth.
+	macPath := cleanAndExpandPath(ctx.GlobalString("macaroonpath"))
+	if !fileExists(macPath) {


If the command line specifies a macaroon file, but that file doesn't exist due to a typo, could this admin fallback approach unintentionally escalate privilege?

I've taken out all dependency on lnd.conf, so only the command-line argument is used, which defaults to ~/.lnd/admin.macaroon.

halseth · 2017-08-11T08:27:31Z

lnd.go

+	}
+
+	// Create macaroon files for lncli to use if they don't exist.
+	if !fileExists(cfg.AdmMacPath) && !fileExists(cfg.ReadMacPath) {


What if only one of them exists?

It assumes that this is intentional (specifically, you can remove admin.macaroon and only have a read-only macaroon available, which will be really nice for autopilot to collect fees).

rodasmith · 2017-08-12T06:32:14Z

utACK but needs rebased

aakselrod · 2017-08-12T17:04:45Z

It's already rebased.

rodasmith · 2017-08-13T22:02:43Z

Oh, right. It was rebased. CI is just having troubles with simultaneous test runners, I guess.

* Running tests with creating coverage profile
Can't run the lnd tests:
some program is using the test lnd ports (19555 | 19556)
The command "./gotest.sh" exited with 1.

rodasmith · 2017-08-13T22:04:32Z

Does this work here?

(Talking to CI bot) test this please

Roasbeef · 2017-08-09T01:23:42Z

macaroons/auth.go

+
+// MacaroonSource wraps a macaroon to implement the
+// credentials.PerRPCCredentials interface.
+type MacaroonSource struct {


IMO something along the lines of MacaroonCredential would be a more descriptive name for this struct.

Great idea, I'll change this.

Roasbeef · 2017-08-14T23:10:12Z

cmd/lncli/main.go

+	"time"
+
+	"gopkg.in/macaroon-bakery.v1/bakery/checkers"
+	"gopkg.in/macaroon.v1"


We shouldn't be using gopkg.in here when importing the packages, and also when specifying the new dependancies in the glide.yaml file. gopkg.in was initially created in a time before vendoring support was added to Go. It's a way to create a repo that pins a project against a particular version. But since we use glide for dependency management, it isn't needed IMO.

Instead we can pin directly against the two repositories directly:

https://github.com/go-macaroon/macaroon

https://github.com/go-macaroon-bakery/macaroon-bakery

Unfortunately, macaroon-bakery actually imports itself, the macaroon package, and other libraries using gopkg.in. See, for example, https://github.com/go-macaroon-bakery/macaroon-bakery/blob/v1/bakery/service.go#L14

We can fork these libraries or submit upstream PRs to remove the dependency on gopkg.in, but I haven't been able to get it to work without that.

Would like some feedback here: should I submit a PR upstream to fix this issue by helping the original author use glide? If so, should I modify lnd's glide configuration to use my fork unless/until the upstream PR is merged?

Unfortunately, macaroon-bakery actually imports itself, the macaroon package, and other libraries using gopkg.in

Le sigh...

Alright, for now let's do this: we'll merge this as is now, but then we'll fork the repo's and modify them to not use gopkg.in in a follow up PR.

Roasbeef · 2017-08-14T23:14:52Z

cmd/lncli/main.go

+		// altogether if, in the latter case, this time is more than 60
+		// seconds).
+		// TODO(aakselrod): add better anti-replay protection.
+		timeCaveat := checkers.TimeBeforeCaveat(


Here's a suggested partial re-ordering in order to increase readability a bit:

macaroonTimeout := time.Duration(ctx.GlobalInt64("macaroontimeout")) requestTimeout := time.Now().Add(time.Second * macaroonTimeout) timeCaveat := checkers.TimeBeforeCaveat(requestTimeout)

I will update these lines to improve readability.

Roasbeef · 2017-08-14T23:18:25Z

lnd.go

@@ -108,7 +136,7 @@ func lndMain() error {
 		net.JoinHostPort("", strconv.Itoa(cfg.PeerPort)),
 	}
 	server, err := newServer(defaultListenAddrs, chanDB, activeChainControl,
-		idPrivKey)
+		idPrivKey, macaroonService)


Only part of the way through this PR but, at first glance, it seems that the rpcServer should be tracking the macaroonService, rather than the server itself.

This makes more sense. I'll change it.

Roasbeef · 2017-08-14T23:21:26Z

lnd.go

+		}
+
+		// Create macaroon files for lncli to use if they don't exist.
+		if !fileExists(cfg.AdminMacPath) &&


If the genMacaroons function writes the admin macaroon, but then lnd fails for some reason (crash, unplug, etc), we'll end up in a partial state that current isn't addressed. In this case, only the admin macaroon will have ben created meaning that the body of this conditional won't be executed.

In order to address this the & should be changed to an ||.

The original logic behind this was that you could delete the admin macaroon and leave a read-only one without an admin macaroon being regenerated. If you still prefer I change this to a ||, I will do so.

Would like some feedback here: based on my comment above, should I change this to || or leave as &&?

Nope, upon further reflection this makes sense!

Roasbeef · 2017-08-14T23:34:06Z

macaroons/store.go

+// RootKey implements the RootKey method for the bakery.RootKeyStorage
+// interface.
+// TODO(aakselrod): Add support for key rotation.
+func (r *RootKeyStorage) RootKey() (rootKey []byte, id string, err error) {


We typically avoid using named returns to reduction in readability they typically cause.

I will fix this; I was using the same convention used by the interface being implemented.

Roasbeef · 2017-08-14T23:35:09Z

macaroons/store.go

+		if len(rootKey) == 0 {
+			// Create a 24-byte root key to emulate the memory
+			// storage that comes with bakery.
+			rootKey = make([]byte, 24)


Is the value 24 included anywhere within the macaroon package as a constant? If so, then it should be used here in place of the magic number.

Also I'm wondering why the library opts to go with 24-byte root keys rather than 32-byte root keys...

The value 24 isn't included in the package as a relevant constant as far as I can tell, and is specified as a literal in every call to randomBytes. There's a NonceLen constant that specifies 24 bytes, but it's not used for this purpose.

Increasing the root key size to 32 bytes shouldn't cause any problems; there is a semi-relevant constant called KeyLen that specifies 32 bytes which is used for Ed25519 keys. I can use that constant if desired, or create a new constant, or just use a literal.

Roasbeef · 2017-08-14T23:37:35Z

macaroons/store.go

+			// Create a 24-byte root key to emulate the memory
+			// storage that comes with bakery.
+			rootKey = make([]byte, 24)
+			length, err := rand.Read(rootKey)


We can remove the check to the number of bytes returned by the call to rand.Read by instead using io.ReadFull its place. This would look something like:

rootKey := make([]byte, 24) if err := io.ReadFull(rand.Read, rootKey[:]); err != nil { return err } return ns.Put([]byte(id), rootKey)

This looks more readable and I'll change the code to use it instead.

Roasbeef · 2017-08-14T23:38:05Z

macaroons/store.go

+		ns := tx.Bucket(rootKeyBucketName)
+		rootKey = ns.Get([]byte(id))
+		// If there's no root key stored in the bucket yet, create one.
+		if len(rootKey) == 0 {


This bit of indentation can be avoided by returning early if len(rootKey) != 0.

Good point; will change this.

Roasbeef · 2017-08-14T23:44:27Z

rpcserver.go

+
+	// roMethods is a slice of method names that are considered "read-only"
+	// for authorization purposes, all lowercase.
+	roMethods = []string{


The following methods I consider to be read-only are missing from this list:

SubscribeTransactions

SubscribeInvoices

SubscribeChannelGraph

If you recall our conversation, these subscribe methods are now using the same permissions as list or get methods as they're accessing the same data. Perhaps renaming the list from roMethods to something like roPermissions would make that more clear.

Roasbeef

Very happy to see the oldest open issue in the tracker finally being addressed!

This PR is a great start to the usage of macaroons as the primary authentication mechanism for lnd's RPC interfaces. I'm very excited to explore more advanced functionality in the future beyond the current admin -> readOnly attenuation.

I've tested this locally over the past day or so on my local node and haven't ran into any major issues. None of my review comments are fundamental, so this should be able to be included in short order as I'd like this to land in our upcoming release.

aakselrod · 2017-08-15T00:38:47Z

macaroons/store.go

+		ns := tx.Bucket(rootKeyBucketName)
+		rootKey = ns.Get([]byte(id))
+		// If there's no root key stored in the bucket yet, create one.
+		if len(rootKey) == 0 {


Good point; will change this.

aakselrod · 2017-08-15T00:40:32Z

lnd.go

+		}
+
+		// Create macaroon files for lncli to use if they don't exist.
+		if !fileExists(cfg.AdminMacPath) &&


The original logic behind this was that you could delete the admin macaroon and leave a read-only one without an admin macaroon being regenerated. If you still prefer I change this to a ||, I will do so.

aakselrod · 2017-08-15T00:43:53Z

rpcserver.go

+	// Check macaroon to see if this is allowed.
+	if r.server.authSvc != nil {
+		if err := macaroons.ValidateMacaroon(updateStream.Context(),
+			"listinvoices", r.server.authSvc); err != nil {


See here, for example, where SubscribeInvoices actually uses a listinvoices permission.

aakselrod · 2017-08-15T00:45:09Z

lnd.go

@@ -108,7 +136,7 @@ func lndMain() error {
 		net.JoinHostPort("", strconv.Itoa(cfg.PeerPort)),
 	}
 	server, err := newServer(defaultListenAddrs, chanDB, activeChainControl,
-		idPrivKey)
+		idPrivKey, macaroonService)


This makes more sense. I'll change it.

aakselrod · 2017-08-15T00:53:20Z

macaroons/store.go

+		if len(rootKey) == 0 {
+			// Create a 24-byte root key to emulate the memory
+			// storage that comes with bakery.
+			rootKey = make([]byte, 24)


The value 24 isn't included in the package as a relevant constant as far as I can tell, and is specified as a literal in every call to randomBytes. There's a NonceLen constant that specifies 24 bytes, but it's not used for this purpose.

Increasing the root key size to 32 bytes shouldn't cause any problems; there is a semi-relevant constant called KeyLen that specifies 32 bytes which is used for Ed25519 keys. I can use that constant if desired, or create a new constant, or just use a literal.

aakselrod · 2017-08-15T00:54:13Z

macaroons/store.go

+			// Create a 24-byte root key to emulate the memory
+			// storage that comes with bakery.
+			rootKey = make([]byte, 24)
+			length, err := rand.Read(rootKey)


This looks more readable and I'll change the code to use it instead.

aakselrod · 2017-08-15T00:57:30Z

rpcserver.go

@@ -1522,6 +1686,13 @@ func (r *rpcServer) AddInvoice(ctx context.Context,
 // returned.
 func (r *rpcServer) LookupInvoice(ctx context.Context,
 	req *lnrpc.PaymentHash) (*lnrpc.Invoice, error) {
+	// Check macaroon to see if this is allowed.
+	if r.server.authSvc != nil {
+		if err := macaroons.ValidateMacaroon(ctx, "lookupinvoice",


On the topic of using the same permission for the same data, should lookupinvoice be the same permission as listinvoices? The reason I chose not to do this is that lookupinvoice requires a known payment hash and retrieves a single invoice, whereas listinvoices allows the caller to retrieve all invoices indiscriminately and also to subscribe to invoice notifications.

aakselrod · 2017-08-15T23:26:05Z

cmd/lncli/main.go

+	"time"
+
+	"gopkg.in/macaroon-bakery.v1/bakery/checkers"
+	"gopkg.in/macaroon.v1"


Would like some feedback here: should I submit a PR upstream to fix this issue by helping the original author use glide? If so, should I modify lnd's glide configuration to use my fork unless/until the upstream PR is merged?

aakselrod · 2017-08-15T23:26:38Z

lnd.go

+		}
+
+		// Create macaroon files for lncli to use if they don't exist.
+		if !fileExists(cfg.AdminMacPath) &&


Would like some feedback here: based on my comment above, should I change this to || or leave as &&?

aakselrod · 2017-08-15T23:27:43Z

rpcserver.go

+		"channelbalance",
+		"pendingchannels",
+		"listchannels",
+		"lookupinvoice",


Would like some feedback here: should I merge the lookupinvoice and listinvoices permissions? What about the listchannels and pendingchannels permissions? Any others that I'm missing that might be good to merge?

Sure, yeah merging them makes sense as they query the same data in the end.

Roasbeef

LGTM 🍪

aakselrod force-pushed the macaroons branch 3 times, most recently from 1c29791 to 9bfa4ac Compare August 11, 2017 02:21

rodasmith reviewed Aug 11, 2017

View reviewed changes

halseth reviewed Aug 11, 2017

View reviewed changes

aakselrod force-pushed the macaroons branch from 9bfa4ac to 20b8c19 Compare August 11, 2017 23:51

Roasbeef previously approved these changes Aug 14, 2017

View reviewed changes

Roasbeef reviewed Aug 14, 2017

View reviewed changes

aakselrod commented Aug 15, 2017

View reviewed changes

aakselrod dismissed Roasbeef’s stale review via a7adb3e August 15, 2017 22:59

aakselrod commented Aug 15, 2017

View reviewed changes

aakselrod force-pushed the macaroons branch from 433b4c5 to 3d3dd3c Compare August 18, 2017 01:42

aakselrod added 3 commits August 17, 2017 19:50

macaroons: add macaroons package and update glide

4e6937f

main: integrate macaroons into config, startup, and RPC server

83a8420

lncli: add support for macaroons

30dfb40

aakselrod force-pushed the macaroons branch from 3d3dd3c to 30dfb40 Compare August 18, 2017 01:52

Roasbeef approved these changes Aug 18, 2017

View reviewed changes

Roasbeef merged commit 5d971a8 into lightningnetwork:master Aug 18, 2017

Roasbeef mentioned this pull request Aug 18, 2017

Implement RPC Authentication #20

Closed

Initial macaroon implementation #250

Initial macaroon implementation #250

Conversation

aakselrod commented Aug 9, 2017 • edited Loading

mention-bot commented Aug 9, 2017

rodasmith left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rodasmith commented Aug 12, 2017 • edited Loading

aakselrod commented Aug 12, 2017

rodasmith commented Aug 13, 2017

rodasmith commented Aug 13, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Roasbeef left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Roasbeef left a comment

Choose a reason for hiding this comment

aakselrod commented Aug 9, 2017 •

edited

Loading

rodasmith commented Aug 12, 2017 •

edited

Loading