Merge pull request #8132 from Roasbeef/strict-macaroon-version

macaroons: reject unknown macaroon versions

docs/release-notes/release-notes-0.17.1.md

@@ -26,6 +26,8 @@
 * A bug that would cause taproot channels to sometimes not display as active
   [has been fixed](https://github.com/lightningnetwork/lnd/pull/8104).
 
+* [`lnd` will now properly reject macaroons with unknown versions.](https://github.com/lightningnetwork/lnd/pull/8132)
+
 # New Features
 ## Functional Enhancements
 

itest/lnd_macaroons_test.go

@@ -65,7 +65,7 @@ func testMacaroonAuthentication(ht *lntest.HarnessTest) {
 			defer cleanup()
 			_, err := client.GetInfo(ctxt, infoReq)
 			require.Error(t, err)
-			require.Contains(t, err.Error(), "cannot get macaroon")
+			require.Contains(t, err.Error(), "invalid ID")
 		},
 	}, {
 		// Third test: Try to access a write method with read-only

macaroons/constraints_test.go

@@ -30,6 +30,8 @@ func createDummyMacaroon(t *testing.T) *macaroon.Macaroon {
 // TestAddConstraints tests that constraints can be added to an existing
 // macaroon and therefore tighten its restrictions.
 func TestAddConstraints(t *testing.T) {
+	t.Parallel()
+
 	// We need a dummy macaroon to start with. Create one without
 	// a bakery, because we mock everything anyway.
 	initialMac := createDummyMacaroon(t)
@@ -55,6 +57,8 @@ func TestAddConstraints(t *testing.T) {
 // TestTimeoutConstraint tests that a caveat for the lifetime of
 // a macaroon is created.
 func TestTimeoutConstraint(t *testing.T) {
+	t.Parallel()
+
 	// Get a configured version of the constraint function.
 	constraintFunc := macaroons.TimeoutConstraint(3)
 
@@ -79,6 +83,8 @@ func TestTimeoutConstraint(t *testing.T) {
 // TestTimeoutConstraint tests that a caveat for the lifetime of
 // a macaroon is created.
 func TestIpLockConstraint(t *testing.T) {
+	t.Parallel()
+
 	// Get a configured version of the constraint function.
 	constraintFunc := macaroons.IPLockConstraint("127.0.0.1")
 
@@ -99,6 +105,8 @@ func TestIpLockConstraint(t *testing.T) {
 // TestIPLockBadIP tests that an IP constraint cannot be added if the
 // provided string is not a valid IP address.
 func TestIPLockBadIP(t *testing.T) {
+	t.Parallel()
+
 	constraintFunc := macaroons.IPLockConstraint("127.0.0/800")
 	testMacaroon := createDummyMacaroon(t)
 	err := constraintFunc(testMacaroon)
@@ -110,6 +118,8 @@ func TestIPLockBadIP(t *testing.T) {
 // TestCustomConstraint tests that a custom constraint with a name and value can
 // be added to a macaroon.
 func TestCustomConstraint(t *testing.T) {
+	t.Parallel()
+
 	// Test a custom caveat with a value first.
 	constraintFunc := macaroons.CustomConstraint("unit-test", "test-value")
 	testMacaroon := createDummyMacaroon(t)

macaroons/service.go

@@ -27,6 +27,13 @@ var (
 	// same entity:action pairs. For example: uri:/lnrpc.Lightning/GetInfo
 	// only gives access to the GetInfo call.
 	PermissionEntityCustomURI = "uri"
+
+	// ErrUnknownVersion is returned when a macaroon is of an unknown
+	// is presented.
+	ErrUnknownVersion = fmt.Errorf("unknown macaroon version")
+
+	// ErrInvalidID is returned when a macaroon ID is invalid.
+	ErrInvalidID = fmt.Errorf("invalid ID")
 )
 
 // MacaroonValidator is an interface type that can check if macaroons are valid.
@@ -208,6 +215,23 @@ func (svc *Service) CheckMacAuth(ctx context.Context, macBytes []byte,
 		return err
 	}
 
+	// Ensure that the macaroon is using the exact same version as we
+	// expect. In the future, we can relax this check to phase in new
+	// versions.
+	if mac.Version() != macaroon.V2 {
+		return fmt.Errorf("%w: %v", ErrUnknownVersion,
+			mac.Version())
+	}
+
+	// Run a similar version check on the ID used for the macaroon as well.
+	const minIDLength = 1
+	if len(mac.Id()) < minIDLength {
+		return ErrInvalidID
+	}
+	if mac.Id()[0] != byte(bakery.Version3) {
+		return ErrInvalidID
+	}
+
 	// Check the method being called against the permitted operation, the
 	// expiration time and IP address and return the result.
 	authChecker := svc.Checker.Auth(macaroon.Slice{mac})
@@ -267,7 +291,7 @@ func (svc *Service) NewMacaroon(
 		return nil, ErrMissingRootKeyID
 	}
 
-	// // Pass the root key ID to context.
+	// Pass the root key ID to context.
 	ctx = ContextWithRootKeyID(ctx, rootKeyID)
 
 	return svc.Oven.NewMacaroon(ctx, bakery.LatestVersion, nil, ops...)

macaroons/service_test.go

@@ -12,6 +12,7 @@ import (
 	"google.golang.org/grpc/metadata"
 	"gopkg.in/macaroon-bakery.v2/bakery"
 	"gopkg.in/macaroon-bakery.v2/bakery/checkers"
+	macaroon "gopkg.in/macaroon.v2"
 )
 
 var (
@@ -53,6 +54,8 @@ func setupTestRootKeyStorage(t *testing.T) kvdb.Backend {
 
 // TestNewService tests the creation of the macaroon service.
 func TestNewService(t *testing.T) {
+	t.Parallel()
+
 	// First, initialize a dummy DB file with a store that the service
 	// can read from. Make sure the file is removed in the end.
 	db := setupTestRootKeyStorage(t)
@@ -104,6 +107,8 @@ func TestNewService(t *testing.T) {
 // TestValidateMacaroon tests the validation of a macaroon that is in an
 // incoming context.
 func TestValidateMacaroon(t *testing.T) {
+	t.Parallel()
+
 	// First, initialize the service and unlock it.
 	db := setupTestRootKeyStorage(t)
 	rootKeyStore, err := macaroons.NewRootKeyStorage(db)
@@ -149,6 +154,8 @@ func TestValidateMacaroon(t *testing.T) {
 
 // TestListMacaroonIDs checks that ListMacaroonIDs returns the expected result.
 func TestListMacaroonIDs(t *testing.T) {
+	t.Parallel()
+
 	// First, initialize a dummy DB file with a store that the service
 	// can read from. Make sure the file is removed in the end.
 	db := setupTestRootKeyStorage(t)
@@ -180,6 +187,8 @@ func TestListMacaroonIDs(t *testing.T) {
 
 // TestDeleteMacaroonID removes the specific root key ID.
 func TestDeleteMacaroonID(t *testing.T) {
+	t.Parallel()
+
 	ctxb := context.Background()
 
 	// First, initialize a dummy DB file with a store that the service
@@ -237,6 +246,8 @@ func TestDeleteMacaroonID(t *testing.T) {
 // TestCloneMacaroons tests that macaroons can be cloned correctly and that
 // modifications to the copy don't affect the original.
 func TestCloneMacaroons(t *testing.T) {
+	t.Parallel()
+
 	// Get a configured version of the constraint function.
 	constraintFunc := macaroons.TimeoutConstraint(3)
 
@@ -288,3 +299,64 @@ func TestCloneMacaroons(t *testing.T) {
 		newMac.Caveats()[0].Location,
 	)
 }
+
+// TestMacaroonVersionDecode tests that we'll reject macaroons with an unknown
+// version.
+func TestMacaroonVersionDecode(t *testing.T) {
+	t.Parallel()
+
+	ctxb := context.Background()
+
+	// First, initialize a dummy DB file with a store that the service
+	// can read from. Make sure the file is removed in the end.
+	db := setupTestRootKeyStorage(t)
+
+	// Second, create the new service instance, unlock it and pass in a
+	// checker that we expect it to add to the bakery.
+	rootKeyStore, err := macaroons.NewRootKeyStorage(db)
+	require.NoError(t, err)
+
+	service, err := macaroons.NewService(
+		rootKeyStore, "lnd", false, macaroons.IPLockChecker,
+	)
+	require.NoError(t, err, "Error creating new service")
+
+	defer service.Close()
+
+	t.Run("invalid_version", func(t *testing.T) {
+		// Now that we have our sample service, we'll make a new custom
+		// macaroon with an unknown version.
+		testMac, err := macaroon.New(
+			testRootKey, testID, testLocation, 1,
+		)
+		require.NoError(t, err)
+
+		// Next, we'll serialize the macaroon to the binary form,
+		// modifying the first byte to signal an unknown version.
+		testMacBytes, err := testMac.MarshalBinary()
+		require.NoError(t, err)
+
+		// If we attempt to check the mac auth, then we should get a
+		// failure that the version is unknown.
+		err = service.CheckMacAuth(ctxb, testMacBytes, nil, "")
+		require.ErrorIs(t, err, macaroons.ErrUnknownVersion)
+	})
+
+	t.Run("invalid_id", func(t *testing.T) {
+		// We'll now make a macaroon with a valid version, but modify
+		// the ID to be rejected.
+		badID := []byte{}
+		testMac, err := macaroon.New(
+			testRootKey, badID, testLocation, testVersion,
+		)
+		require.NoError(t, err)
+
+		testMacBytes, err := testMac.MarshalBinary()
+		require.NoError(t, err)
+
+		// If we attempt to check the mac auth, then we should get a
+		// failure that the ID is bad.
+		err = service.CheckMacAuth(ctxb, testMacBytes, nil, "")
+		require.ErrorIs(t, err, macaroons.ErrInvalidID)
+	})
+}