macaroons: macaroon.Clone() doesn't copy caveats

[yyforyongyu]

When calling macaroons.NewMacaroonCredential, this function uses a Clone method defined in the package gopkg.in/macaroon.v2. Its intention is to make a copy so that the new macaroon object won't be affectted if any mutation happened to the old one.

However, because slicing does not copy the slice's data, the Clone method won't copy the caveats.

Currently it should be fine. Whenever AddConstraints is called, it uses AddFirstPartyCaveat to add the caveat, which always creates a new slice of caveat in the macaroon(through append).

This won't be true in the following edge case,

// Suppose m is a macaroon.Macaroon with caveats attached.

// make a copy of m
m1 = m.Clone()

// make change to the first caveat
m.Caveats()[0].Location = "mars"

// the copied one will be changed too!
m1.Caveats()[0].Location == "mars"  // true

Although the caveats should probably not be modified this way, it might create a subtle bug in the future.

[yyforyongyu]

Suggested fix

Modify NewMacaroonCredential, so that it copies caveats too.

For demonstration, locally, I've created the following test to address this bug, under macaroons/constrains_test.go.

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)
	if err != nil {
		t.Fatalf("Error applying timeout constraint: %v", 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 := macaroons.NewMacaroonCredential(testMacaroon)
	require.Equal(t,
		"", newMacCred.Macaroon.Caveats()[0].Location,
		"expected new caveat location to be empty, found: %s",
		newMacCred.Macaroon.Caveats()[0].Location,
	)

	// 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,
		"", newMacCred.Macaroon.Caveats()[0].Location,
		"expected new caveat location to be empty, found: %s",
		newMacCred.Macaroon.Caveats()[0].Location,
	)
}