engineccl: Fix data key generation for v2 encryption

pkg/ccl/baseccl/encryption_spec.go

@@ -58,6 +58,11 @@ func (es StoreEncryptionSpec) String() string {
 		es.Path, es.KeyPath, es.OldKeyPath, es.RotationPeriod)
 }
 
+// PathMatches returns true if this StoreEncryptionSpec matches the given store path.
+func (es StoreEncryptionSpec) PathMatches(path string) bool {
+	return es.Path == path || es.Path == "*"
+}
+
 // NewStoreEncryptionSpec parses the string passed in and returns a new
 // StoreEncryptionSpec if parsing succeeds.
 // TODO(mberhault): we should share the parsing code with the StoreSpec.
@@ -91,10 +96,14 @@ func NewStoreEncryptionSpec(value string) (StoreEncryptionSpec, error) {
 
 		switch field {
 		case pathField:
-			var err error
-			es.Path, err = base.GetAbsoluteFSPath(pathField, value)
-			if err != nil {
-				return StoreEncryptionSpec{}, err
+			if value == "*" {
+				es.Path = value
+			} else {
+				var err error
+				es.Path, err = base.GetAbsoluteFSPath(pathField, value)
+				if err != nil {
+					return StoreEncryptionSpec{}, err
+				}
 			}
 		case "key":
 			if value == plaintextFieldValue {
@@ -195,7 +204,7 @@ func PopulateWithEncryptionOpts(
 	for _, es := range encryptionSpecs.Specs {
 		var found bool
 		for i := range storeSpecs.Specs {
-			if storeSpecs.Specs[i].Path != es.Path {
+			if !es.PathMatches(storeSpecs.Specs[i].Path) {
 				continue
 			}
 
@@ -215,7 +224,7 @@ func PopulateWithEncryptionOpts(
 		}
 
 		for _, externalPath := range [2]*base.ExternalPath{&walFailoverConfig.Path, &walFailoverConfig.PrevPath} {
-			if !externalPath.IsSet() || externalPath.Path != es.Path {
+			if !externalPath.IsSet() || !es.PathMatches(externalPath.Path) {
 				continue
 			}
 			// NB: The external paths WALFailoverConfig.Path and
@@ -250,7 +259,7 @@ func EncryptionOptionsForStore(dir string, encryptionSpecs EncryptionSpecList) (
 		return nil, errors.Wrapf(err, "could not find absolute path for %s ", dir)
 	}
 	for _, es := range encryptionSpecs.Specs {
-		if es.Path == path {
+		if es.PathMatches(path) {
 			return es.ToEncryptionOptions()
 		}
 	}

pkg/ccl/baseccl/encryption_spec_test.go

@@ -10,6 +10,7 @@ package baseccl
 
 import (
 	"fmt"
+	"path/filepath"
 	"reflect"
 	"testing"
 	"time"
@@ -22,6 +23,11 @@ import (
 func TestNewStoreEncryptionSpec(t *testing.T) {
 	defer leaktest.AfterTest(t)()
 
+	absDataPath, err := filepath.Abs("data")
+	if err != nil {
+		t.Fatal(err)
+	}
+
 	testCases := []struct {
 		value       string
 		expectedErr string
@@ -45,11 +51,18 @@ func TestNewStoreEncryptionSpec(t *testing.T) {
 		{"path=data,key=new.key,old-key=old.key,rotation-period=1", `could not parse rotation-duration value: 1: time: missing unit in duration "1"`, StoreEncryptionSpec{}},
 		{"path=data,key=new.key,old-key=old.key,rotation-period=1d", `could not parse rotation-duration value: 1d: time: unknown unit "d" in duration "1d"`, StoreEncryptionSpec{}},
 
-		// Good values.
+		// Good values. Note that paths get absolutized so we start most of them
+		// with / so we can used fixed expected values.
 		{"path=/data,key=/new.key,old-key=/old.key", "", StoreEncryptionSpec{Path: "/data", KeyPath: "/new.key", OldKeyPath: "/old.key", RotationPeriod: DefaultRotationPeriod}},
 		{"path=/data,key=/new.key,old-key=/old.key,rotation-period=1h", "", StoreEncryptionSpec{Path: "/data", KeyPath: "/new.key", OldKeyPath: "/old.key", RotationPeriod: time.Hour}},
 		{"path=/data,key=plain,old-key=/old.key,rotation-period=1h", "", StoreEncryptionSpec{Path: "/data", KeyPath: "plain", OldKeyPath: "/old.key", RotationPeriod: time.Hour}},
 		{"path=/data,key=/new.key,old-key=plain,rotation-period=1h", "", StoreEncryptionSpec{Path: "/data", KeyPath: "/new.key", OldKeyPath: "plain", RotationPeriod: time.Hour}},
+
+		// One relative path to test absolutization.
+		{"path=data,key=/new.key,old-key=/old.key", "", StoreEncryptionSpec{Path: absDataPath, KeyPath: "/new.key", OldKeyPath: "/old.key", RotationPeriod: DefaultRotationPeriod}},
+
+		// Special path * is not absolutized.
+		{"path=*,key=/new.key,old-key=/old.key", "", StoreEncryptionSpec{Path: "*", KeyPath: "/new.key", OldKeyPath: "/old.key", RotationPeriod: DefaultRotationPeriod}},
 	}
 
 	for i, testCase := range testCases {

pkg/ccl/cliccl/cliflagsccl/flags.go

@@ -20,17 +20,12 @@ stores exist, the flag must be specified for each store.
 
 A valid enterprise license is required to use this functionality.
 
-</PRE>
-Key files must be of size 32 bytes + AES key size, such as:
-<PRE>
-AES-128: 48 bytes
-AES-192: 56 bytes
-AES-256: 64 bytes
+Key files should be generated by "cockroach gen encryption-key".
 
-</PRE>
 Valid fields:
-<PRE>
-* path    (required): must match the path of one of the stores
+
+* path    (required): must match the path of one of the stores, or the special
+                      value "*" to match all stores
 * key     (required): path to the current key file, or "plain"
 * old-key (required): path to the previous key file, or "plain"
 * rotation-period   : amount of time after which data keys should be rotated

pkg/ccl/storageccl/engineccl/pebble_key_manager.go

@@ -412,11 +412,11 @@ func generateAndSetNewDataKey(
 	} else {
 		var keyLength int
 		switch activeStoreKey.EncryptionType {
-		case enginepbccl.EncryptionType_AES128_CTR:
+		case enginepbccl.EncryptionType_AES128_CTR, enginepbccl.EncryptionType_AES_128_CTR_V2:
 			keyLength = 16
-		case enginepbccl.EncryptionType_AES192_CTR:
+		case enginepbccl.EncryptionType_AES192_CTR, enginepbccl.EncryptionType_AES_192_CTR_V2:
 			keyLength = 24
-		case enginepbccl.EncryptionType_AES256_CTR:
+		case enginepbccl.EncryptionType_AES256_CTR, enginepbccl.EncryptionType_AES_256_CTR_V2:
 			keyLength = 32
 		default:
 			return nil, fmt.Errorf("unknown encryption type %d for key ID %s",

pkg/ccl/storageccl/engineccl/pebble_key_manager_test.go

@@ -314,7 +314,18 @@ func TestDataKeyManager(t *testing.T) {
 			case "set-active-store-key":
 				var id string
 				d.ScanArgs(t, "id", &id)
-				return setActiveStoreKey(dkm, id, enginepbccl.EncryptionType_AES128_CTR)
+				version := 1
+				d.MaybeScanArgs(t, "version", &version)
+				var et enginepbccl.EncryptionType
+				switch version {
+				case 1:
+					et = enginepbccl.EncryptionType_AES128_CTR
+				case 2:
+					et = enginepbccl.EncryptionType_AES_128_CTR_V2
+				default:
+					t.Fatalf("unknown version %d", version)
+				}
+				return setActiveStoreKey(dkm, id, et)
 			case "set-active-store-key-plain":
 				var id string
 				d.ScanArgs(t, "id", &id)

pkg/ccl/storageccl/engineccl/testdata/data_key_manager

@@ -245,3 +245,10 @@ wait
 compare-active-data-key
 ----
 different
+
+set-active-store-key id=v2key version=2
+----
+
+get-active-data-key
+----
+encryption_type:AES_128_CTR_V2 creation_time:26 source:"data key manager" parent_key_id:"v2key"

pkg/cmd/roachtest/tests/BUILD.bazel

@@ -201,6 +201,7 @@ go_library(
         "//pkg/ccl/changefeedccl",
         "//pkg/ccl/changefeedccl/cdctest",
         "//pkg/ccl/changefeedccl/changefeedbase",
+        "//pkg/ccl/storageccl/engineccl/enginepbccl",
         "//pkg/ccl/streamingccl/replicationutils",
         "//pkg/cli",
         "//pkg/cli/clisqlclient",

pkg/cmd/roachtest/tests/encryption.go

@@ -13,85 +13,97 @@ package tests
 import (
 	"context"
 	"fmt"
+	"strings"
 
+	"github.com/cockroachdb/cockroach/pkg/ccl/storageccl/engineccl/enginepbccl"
 	"github.com/cockroachdb/cockroach/pkg/cmd/roachtest/cluster"
 	"github.com/cockroachdb/cockroach/pkg/cmd/roachtest/option"
 	"github.com/cockroachdb/cockroach/pkg/cmd/roachtest/registry"
+	"github.com/cockroachdb/cockroach/pkg/cmd/roachtest/roachtestutil"
 	"github.com/cockroachdb/cockroach/pkg/cmd/roachtest/test"
 	"github.com/cockroachdb/cockroach/pkg/roachprod/install"
+	"github.com/cockroachdb/cockroach/pkg/server/serverpb"
+	"github.com/cockroachdb/cockroach/pkg/util/protoutil"
 	"github.com/cockroachdb/errors"
 )
 
 func registerEncryption(r registry.Registry) {
-	// Note that no workload is run in this roachtest because kv roachtest
-	// ideally runs with encryption turned on to see the performance impact and
-	// to test the correctness of encryption at rest.
-	runEncryption := func(ctx context.Context, t test.Test, c cluster.Cluster) {
+	// Note that no workload is run in this roachtest: it's testing basic operations
+	// and not performance. Performance tests of encryption are performed in other
+	// tests based on the TestSpec.EncryptionSupport field.
+	runEncryptionRotation := func(ctx context.Context, t test.Test, c cluster.Cluster) {
 		nodes := c.Spec().NodeCount
-		c.Start(ctx, t.L(), option.DefaultStartOpts(), install.MakeClusterSettings(), c.Range(1, nodes))
+		httpClient := roachtestutil.DefaultHTTPClient(c, t.L())
 
-		// Check that /_status/stores/local endpoint has encryption status.
-		adminAddrs, err := c.InternalAdminUIAddr(ctx, t.L(), c.Range(1, nodes))
-		if err != nil {
-			t.Fatal(err)
-		}
-		for _, addr := range adminAddrs {
-			if err := c.RunE(ctx, option.WithNodes(c.Node(nodes)), fmt.Sprintf(`curl http://%s/_status/stores/local | (! grep '"encryptionStatus": null')`, addr)); err != nil {
-				t.Fatalf("encryption status from /_status/stores/local endpoint is null")
-			}
-		}
-
-		for i := 1; i <= nodes; i++ {
-			if err := c.StopCockroachGracefullyOnNode(ctx, t.L(), i); err != nil {
-				t.Fatal(err)
+		// Keys are configured in (key, old-key) pairs. We want to start out
+		// unencrypted so we set both key and old-key to "plain" to avoid
+		// special-casing the first iteration of the loop.
+		keys := []string{"plain", "plain"}
+		// Rotate through multiple keys with different key sizes and both
+		// implementation versions.
+		for _, size := range []int{128, 192, 256} {
+			for _, version := range []int{1, 2} {
+				filename := fmt.Sprintf("aes-%d-v%d.key", size, version)
+				if err := c.RunE(ctx, option.WithNodes(c.Node(nodes)),
+					fmt.Sprintf("./cockroach gen encryption-key -s %d --version=%d %s",
+						size, version, filename)); err != nil {
+					t.Fatal(errors.Wrapf(err, "failed to generate AES key"))
+				}
+				keys = append(keys, filename)
 			}
 		}
 
-		// Restart node with encryption turned on to verify old key works.
-		c.Start(ctx, t.L(), option.DefaultStartOpts(), install.MakeClusterSettings(), c.Range(1, nodes))
-
-		testCLIGenKey := func(size int) error {
-			// Generate encryption store key through `./cockroach gen encryption-key -s=size aes-size.key`.
-			if err := c.RunE(ctx, option.WithNodes(c.Node(nodes)), fmt.Sprintf("./cockroach gen encryption-key -s=%[1]d aes-%[1]d.key", size)); err != nil {
-				return errors.Wrapf(err, "failed to generate AES key with size %d through CLI", size)
+		for keyIdx := 1; keyIdx < len(keys); keyIdx++ {
+			opts := option.DefaultStartOpts()
+			opts.RoachprodOpts.ExtraArgs = []string{
+				fmt.Sprintf("--enterprise-encryption=path=*,key=%s,old-key=%s",
+					keys[keyIdx], keys[keyIdx-1]),
 			}
+			c.Start(ctx, t.L(), opts, install.MakeClusterSettings(), c.Range(1, nodes))
+			WaitForReady(ctx, t, c, c.Range(1, nodes))
 
-			// Check the size of generated aes key has expected size.
-			if err := c.RunE(ctx, option.WithNodes(c.Node(nodes)), fmt.Sprintf(`size=$(wc -c <"aes-%d.key"); test $size -eq %d && exit 0 || exit 1`, size, 32+size/8)); err != nil {
-				return errors.Errorf("expected aes-%d.key has size %d bytes, but got different size", size, 32+size/8)
-			}
-
-			return nil
-		}
-
-		// Check that CLI can generated key with specified sizes.
-		for _, size := range []int{128, 192, 256} {
-			if err := testCLIGenKey(size); err != nil {
+			// Check that /_status/stores/local endpoint has encryption status.
+			adminAddrs, err := c.ExternalAdminUIAddr(ctx, t.L(), c.Range(1, nodes))
+			if err != nil {
 				t.Fatal(err)
 			}
-		}
+			for _, addr := range adminAddrs {
+				url := fmt.Sprintf("http://%s/_status/stores/local", addr)
+				var storesResponse serverpb.StoresResponse
+				if err := httpClient.GetJSON(ctx, url, &storesResponse); err != nil {
+					t.Fatal(err)
+				}
+				var encryptionStatus enginepbccl.EncryptionStatus
+				if err := protoutil.Unmarshal(storesResponse.Stores[0].EncryptionStatus,
+					&encryptionStatus); err != nil {
+					t.Fatal(err)
+				}
+				if !strings.HasSuffix(encryptionStatus.ActiveStoreKey.Source, keys[keyIdx]) {
+					t.Fatalf("expected active key %s, but found %s",
+						keys[keyIdx], encryptionStatus.ActiveStoreKey.Source)
+				}
+			}
 
-		// Check that CLI returns error if AES key size is incorrect.
-		for _, size := range []int{20, 88, 90} {
-			// Cannot check for specific error message from CLI because command
-			// is run through roachprod and it will return exist status 1.
-			if err := testCLIGenKey(size); err == nil {
-				t.Fatalf("expected error from CLI gen encryption-key, but got nil")
+			for i := 1; i <= nodes; i++ {
+				if err := c.StopCockroachGracefullyOnNode(ctx, t.L(), i); err != nil {
+					t.Fatal(err)
+				}
 			}
 		}
 	}
 
 	for _, n := range []int{1} {
 		r.Add(registry.TestSpec{
-			Name:              fmt.Sprintf("encryption/nodes=%d", n),
-			EncryptionSupport: registry.EncryptionAlwaysEnabled,
+			Name: fmt.Sprintf("encryption/rotation/nodes=%d", n),
+			// This test controls encryption manually.
+			EncryptionSupport: registry.EncryptionAlwaysDisabled,
 			Leases:            registry.MetamorphicLeases,
 			Owner:             registry.OwnerStorage,
 			Cluster:           r.MakeClusterSpec(n),
 			CompatibleClouds:  registry.AllExceptAWS,
 			Suites:            registry.Suites(registry.Nightly),
 			Run: func(ctx context.Context, t test.Test, c cluster.Cluster) {
-				runEncryption(ctx, t, c)
+				runEncryptionRotation(ctx, t, c)
 			},
 		})
 	}