multi: use safe copy for macaroons

Fixes #4383 by adding a new SafeCopyMacaroon function that correctly
clones all caveats and prevents modifications on the copy from affecting
the original.

cmd/lncli/main.go

@@ -167,7 +167,10 @@ func getClientConn(ctx *cli.Context, skipMacaroons bool) *grpc.ClientConn {
 		}
 
 		// Now we append the macaroon credentials to the dial options.
-		cred := macaroons.NewMacaroonCredential(constrainedMac)
+		cred, err := macaroons.NewMacaroonCredential(constrainedMac)
+		if err != nil {
+			fatal(fmt.Errorf("error cloning mac: %v", err))
+		}
 		opts = append(opts, grpc.WithPerRPCCredentials(cred))
 	}
 

lnd.go

@@ -111,7 +111,10 @@ func AdminAuthOptions(cfg *Config, skipMacaroons bool) ([]grpc.DialOption, error
 		}
 
 		// Now we append the macaroon credentials to the dial options.
-		cred := macaroons.NewMacaroonCredential(mac)
+		cred, err := macaroons.NewMacaroonCredential(mac)
+		if err != nil {
+			return nil, fmt.Errorf("error cloning mac: %v", err)
+		}
 		opts = append(opts, grpc.WithPerRPCCredentials(cred))
 	}
 

lntest/node.go

@@ -1195,7 +1195,10 @@ func (hn *HarnessNode) ConnectRPCWithMacaroon(mac *macaroon.Macaroon) (
 	if mac == nil {
 		return grpc.DialContext(ctx, hn.Cfg.RPCAddr(), opts...)
 	}
-	macCred := macaroons.NewMacaroonCredential(mac)
+	macCred, err := macaroons.NewMacaroonCredential(mac)
+	if err != nil {
+		return nil, fmt.Errorf("error cloning mac: %v", err)
+	}
 	opts = append(opts, grpc.WithPerRPCCredentials(macCred))
 
 	return grpc.DialContext(ctx, hn.Cfg.RPCAddr(), opts...)

macaroons/auth.go

@@ -37,8 +37,17 @@ func (m MacaroonCredential) GetRequestMetadata(ctx context.Context,
 
 // NewMacaroonCredential returns a copy of the passed macaroon wrapped in a
 // MacaroonCredential struct which implements PerRPCCredentials.
-func NewMacaroonCredential(m *macaroon.Macaroon) MacaroonCredential {
+func NewMacaroonCredential(m *macaroon.Macaroon) (MacaroonCredential, error) {
 	ms := MacaroonCredential{}
-	ms.Macaroon = m.Clone()
-	return ms
+
+	// The macaroon library's Clone() method has a subtle bug that doesn't
+	// correctly clone all caveats. We need to use our own, safe clone
+	// function instead.
+	var err error
+	ms.Macaroon, err = SafeCopyMacaroon(m)
+	if err != nil {
+		return ms, err
+	}
+
+	return ms, nil
 }

macaroons/constraints.go

@@ -50,7 +50,14 @@ type Checker func() (string, checkers.Func)
 func AddConstraints(mac *macaroon.Macaroon,
 	cs ...Constraint) (*macaroon.Macaroon, error) {
 
-	newMac := mac.Clone()
+	// The macaroon library's Clone() method has a subtle bug that doesn't
+	// correctly clone all caveats. We need to use our own, safe clone
+	// function instead.
+	newMac, err := SafeCopyMacaroon(mac)
+	if err != nil {
+		return nil, err
+	}
+
 	for _, constraint := range cs {
 		if err := constraint(newMac); err != nil {
 			return nil, err

macaroons/service.go

@@ -275,3 +275,21 @@ func RawMacaroonFromContext(ctx context.Context) (string, error) {
 
 	return md["macaroon"][0], nil
 }
+
+// SafeCopyMacaroon creates a copy of a macaroon that is safe to be used and
+// modified. This is necessary because the macaroon library's own Clone() method
+// is unsafe for certain edge cases, resulting in both the cloned and the
+// original macaroons to be modified.
+func SafeCopyMacaroon(mac *macaroon.Macaroon) (*macaroon.Macaroon, error) {
+	macBytes, err := mac.MarshalBinary()
+	if err != nil {
+		return nil, err
+	}
+
+	newMac := &macaroon.Macaroon{}
+	if err := newMac.UnmarshalBinary(macBytes); err != nil {
+		return nil, err
+	}
+
+	return newMac, nil
+}

macaroons/service_test.go

@@ -253,3 +253,58 @@ func TestDeleteMacaroonID(t *testing.T) {
 	ids, _ := service.ListMacaroonIDs(ctxb)
 	require.Equal(t, expectedIDs[1:], ids, "root key IDs mismatch")
 }
+
+// TestCloneMacaroons tests that macaroons can be cloned correctly and that
+// modifications to the copy don't affect the original.
+func TestCloneMacaroons(t *testing.T) {
+	// Get a configured version of the constraint function.
+	constraintFunc := macaroons.TimeoutConstraint(3)
+
+	// Now we need a dummy macaroon that we can apply the constraint
+	// function to.
+	testMacaroon := createDummyMacaroon(t)
+	err := constraintFunc(testMacaroon)
+	require.NoError(t, err)
+
+	// Check that the caveat has an empty location.
+	require.Equal(
+		t, "", testMacaroon.Caveats()[0].Location,
+		"expected caveat location to be empty, found: %s",
+		testMacaroon.Caveats()[0].Location,
+	)
+
+	// Make a copy of the macaroon.
+	newMacCred, err := macaroons.NewMacaroonCredential(testMacaroon)
+	require.NoError(t, err)
+
+	newMac := newMacCred.Macaroon
+	require.Equal(
+		t, "", newMac.Caveats()[0].Location,
+		"expected new caveat location to be empty, found: %s",
+		newMac.Caveats()[0].Location,
+	)
+
+	// They should be deep equal as well.
+	testMacaroonBytes, err := testMacaroon.MarshalBinary()
+	require.NoError(t, err)
+	newMacBytes, err := newMac.MarshalBinary()
+	require.NoError(t, err)
+	require.Equal(t, testMacaroonBytes, newMacBytes)
+
+	// Modify the caveat location on the old macaroon.
+	testMacaroon.Caveats()[0].Location = "mars"
+
+	// The old macaroon's caveat location should be changed.
+	require.Equal(
+		t, "mars", testMacaroon.Caveats()[0].Location,
+		"expected caveat location to be empty, found: %s",
+		testMacaroon.Caveats()[0].Location,
+	)
+
+	// The new macaroon's caveat location should stay untouched.
+	require.Equal(
+		t, "", newMac.Caveats()[0].Location,
+		"expected new caveat location to be empty, found: %s",
+		newMac.Caveats()[0].Location,
+	)
+}