diff --git a/docs/release-notes/release-notes-0.17.1.md b/docs/release-notes/release-notes-0.17.1.md index 54238d898e..fbb2ef89f5 100644 --- a/docs/release-notes/release-notes-0.17.1.md +++ b/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 diff --git a/itest/lnd_macaroons_test.go b/itest/lnd_macaroons_test.go index 22767ec933..3adfafffe9 100644 --- a/itest/lnd_macaroons_test.go +++ b/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 diff --git a/macaroons/constraints_test.go b/macaroons/constraints_test.go index d690bda8c0..0a758c11d5 100644 --- a/macaroons/constraints_test.go +++ b/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) diff --git a/macaroons/service.go b/macaroons/service.go index adbd586258..d4319567b8 100644 --- a/macaroons/service.go +++ b/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...) diff --git a/macaroons/service_test.go b/macaroons/service_test.go index b351367170..aad8af8db0 100644 --- a/macaroons/service_test.go +++ b/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) + }) +}