storageconfig: encryption YAML support and validation

Support yaml for the encryption options and add validation.

Epic: none
Release note: None

Author: RaduBerinde

pkg/cli/flags_test.go

@@ -1751,55 +1751,60 @@ func TestParseEncryptionSpec(t *testing.T) {
 		expected    storeEncryptionSpec
 	}{
 		// path
-		{",", "no path specified", storeEncryptionSpec{}},
-		{"", "no path specified", storeEncryptionSpec{}},
-		{"/mnt/hda1", "field not in the form <key>=<value>: /mnt/hda1", storeEncryptionSpec{}},
-		{"path=", "no value specified for path", storeEncryptionSpec{}},
-		{"path=~/data", "path cannot start with '~': ~/data", storeEncryptionSpec{}},
-		{"path=data,path=data2", "path field was used twice in encryption definition", storeEncryptionSpec{}},
+		{value: ",", expectedErr: "no path specified"},
+		{expectedErr: "no path specified"},
+		{value: "/mnt/hda1", expectedErr: "field not in the form <key>=<value>: /mnt/hda1"},
+		{value: "path=", expectedErr: "no value specified for path"},
+		{value: "path=~/data", expectedErr: "path cannot start with '~': ~/data"},
+		{value: "path=data,path=data2", expectedErr: "path field was used twice in encryption definition"},
 
 		// The same logic applies to key and old-key, don't repeat everything.
-		{"path=data", "no key specified", storeEncryptionSpec{}},
-		{"path=data,key=new.key", "no old-key specified", storeEncryptionSpec{}},
+		{value: "path=data", expectedErr: "no key specified"},
+		{value: "path=data,key=new.key", expectedErr: "no old key specified"},
 
 		// Rotation period.
-		{"path=data,key=new.key,old-key=old.key,rotation-period", "field not in the form <key>=<value>: rotation-period", storeEncryptionSpec{}},
-		{"path=data,key=new.key,old-key=old.key,rotation-period=", "no value specified for rotation-period", storeEncryptionSpec{}},
-		{"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{}},
+		{value: "path=data,key=new.key,old-key=old.key,rotation-period", expectedErr: "field not in the form <key>=<value>: rotation-period"},
+		{value: "path=data,key=new.key,old-key=old.key,rotation-period=", expectedErr: "no value specified for rotation-period"},
+		{value: "path=data,key=new.key,old-key=old.key,rotation-period=1", expectedErr: `could not parse rotation-duration value: 1: time: missing unit in duration "1"`},
+		{value: "path=data,key=new.key,old-key=old.key,rotation-period=1d", expectedErr: `could not parse rotation-duration value: 1d: time: unknown unit "d" in duration "1d"`},
+		{value: "path=data,key=new.key,old-key=old.key,rotation-period=1ms", expectedErr: `invalid rotation period 1ms`},
 
 		// 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",
+			value: "path=/data,key=/new.key,old-key=/old.key",
+			expected: storeEncryptionSpec{
+				Path: "/data",
 				Options: storageconfig.EncryptionOptions{
 					KeyFiles:       &storageconfig.EncryptionKeyFiles{CurrentKey: "/new.key", OldKey: "/old.key"},
 					RotationPeriod: storageconfig.DefaultRotationPeriod,
 				},
 			},
 		},
 		{
-			"path=/data,key=/new.key,old-key=/old.key,rotation-period=1h", "",
-			storeEncryptionSpec{Path: "/data",
+			value: "path=/data,key=/new.key,old-key=/old.key,rotation-period=1h",
+			expected: storeEncryptionSpec{
+				Path: "/data",
 				Options: storageconfig.EncryptionOptions{
 					KeyFiles:       &storageconfig.EncryptionKeyFiles{CurrentKey: "/new.key", OldKey: "/old.key"},
 					RotationPeriod: time.Hour,
 				},
 			},
 		},
 		{
-			"path=/data,key=plain,old-key=/old.key,rotation-period=1h", "",
-			storeEncryptionSpec{Path: "/data",
+			value: "path=/data,key=plain,old-key=/old.key,rotation-period=1h",
+			expected: storeEncryptionSpec{
+				Path: "/data",
 				Options: storageconfig.EncryptionOptions{
 					KeyFiles:       &storageconfig.EncryptionKeyFiles{CurrentKey: "plain", OldKey: "/old.key"},
 					RotationPeriod: time.Hour,
 				},
 			},
 		},
 		{
-			"path=/data,key=/new.key,old-key=plain,rotation-period=1h", "",
-			storeEncryptionSpec{Path: "/data",
+			value: "path=/data,key=/new.key,old-key=plain,rotation-period=1h",
+			expected: storeEncryptionSpec{
+				Path: "/data",
 				Options: storageconfig.EncryptionOptions{
 					KeyFiles:       &storageconfig.EncryptionKeyFiles{CurrentKey: "/new.key", OldKey: "plain"},
 					RotationPeriod: time.Hour,
@@ -1809,8 +1814,9 @@ func TestParseEncryptionSpec(t *testing.T) {
 
 		// One relative path to test absolutization.
 		{
-			"path=data,key=/new.key,old-key=/old.key", "",
-			storeEncryptionSpec{Path: absDataPath,
+			value: "path=data,key=/new.key,old-key=/old.key",
+			expected: storeEncryptionSpec{
+				Path: absDataPath,
 				Options: storageconfig.EncryptionOptions{
 					KeyFiles:       &storageconfig.EncryptionKeyFiles{CurrentKey: "/new.key", OldKey: "/old.key"},
 					RotationPeriod: storageconfig.DefaultRotationPeriod,
@@ -1820,8 +1826,9 @@ func TestParseEncryptionSpec(t *testing.T) {
 
 		// Special path * is not absolutized.
 		{
-			"path=*,key=/new.key,old-key=/old.key", "",
-			storeEncryptionSpec{Path: "*",
+			value: "path=*,key=/new.key,old-key=/old.key",
+			expected: storeEncryptionSpec{
+				Path: "*",
 				Options: storageconfig.EncryptionOptions{
 					KeyFiles:       &storageconfig.EncryptionKeyFiles{CurrentKey: "/new.key", OldKey: "/old.key"},
 					RotationPeriod: storageconfig.DefaultRotationPeriod,

pkg/cli/flags_util.go

@@ -530,22 +530,13 @@ func (es storeEncryptionSpec) PathMatches(path string) bool {
 	return es.Path == path || es.Path == "*"
 }
 
-// Special value of key paths to mean "no encryption". We do not accept empty fields.
-const plaintextFieldValue = "plain"
-
 // parseStoreEncryptionSpec parses the string passed in and returns a new
 // storeEncryptionSpec if parsing succeeds.
 // TODO(mberhault): we should share the parsing code with the StoreSpec.
 func parseStoreEncryptionSpec(value string) (storeEncryptionSpec, error) {
 	const pathField = "path"
-	es := storeEncryptionSpec{
-		Path: "",
-		Options: storageconfig.EncryptionOptions{
-			KeySource:      storageconfig.EncryptionKeyFromFiles,
-			KeyFiles:       &storageconfig.EncryptionKeyFiles{},
-			RotationPeriod: storageconfig.DefaultRotationPeriod,
-		},
-	}
+	var es storeEncryptionSpec
+	es.Options.KeyFiles = &storageconfig.EncryptionKeyFiles{}
 
 	used := make(map[string]struct{})
 	for _, split := range strings.Split(value, ",") {
@@ -582,25 +573,9 @@ func parseStoreEncryptionSpec(value string) (storeEncryptionSpec, error) {
 				}
 			}
 		case "key":
-			if value == plaintextFieldValue {
-				es.Options.KeyFiles.CurrentKey = plaintextFieldValue
-			} else {
-				var err error
-				es.Options.KeyFiles.CurrentKey, err = getAbsoluteFSPath("key", value)
-				if err != nil {
-					return storeEncryptionSpec{}, err
-				}
-			}
+			es.Options.KeyFiles.CurrentKey = value
 		case "old-key":
-			if value == plaintextFieldValue {
-				es.Options.KeyFiles.OldKey = plaintextFieldValue
-			} else {
-				var err error
-				es.Options.KeyFiles.OldKey, err = getAbsoluteFSPath("old-key", value)
-				if err != nil {
-					return storeEncryptionSpec{}, err
-				}
-			}
+			es.Options.KeyFiles.OldKey = value
 		case "rotation-period":
 			dur, err := time.ParseDuration(value)
 			if err != nil {
@@ -616,11 +591,8 @@ func parseStoreEncryptionSpec(value string) (storeEncryptionSpec, error) {
 	if es.Path == "" {
 		return storeEncryptionSpec{}, fmt.Errorf("no path specified")
 	}
-	if es.Options.KeyFiles.CurrentKey == "" {
-		return storeEncryptionSpec{}, fmt.Errorf("no key specified")
-	}
-	if es.Options.KeyFiles.OldKey == "" {
-		return storeEncryptionSpec{}, fmt.Errorf("no old-key specified")
+	if err := es.Options.Validate(); err != nil {
+		return storeEncryptionSpec{}, err
 	}
 
 	return es, nil

pkg/storage/storageconfig/encryption_spec.go

@@ -5,22 +5,31 @@
 
 package storageconfig
 
-import "time"
+import (
+	"path/filepath"
+	"strings"
+	"time"
+
+	"github.com/cockroachdb/errors"
+	"gopkg.in/yaml.v3"
+)
 
 // EncryptionOptions defines the per-store encryption options.
 type EncryptionOptions struct {
 	// The store key source. Defines which fields are useful.
-	KeySource EncryptionKeySource
-	// Set if key_source == KeyFiles.
-	KeyFiles *EncryptionKeyFiles
-	// Data key rotation period.
-	RotationPeriod time.Duration
+	KeySource EncryptionKeySource `yaml:"key-source,omitempty"`
+	// Set if KeySource == EncryptionKeyFiles.
+	KeyFiles *EncryptionKeyFiles `yaml:",inline"`
+	// Data key rotation period. Defaults to 7 days if not specified.
+	RotationPeriod time.Duration `yaml:"rotation-period,omitempty"`
 }
 
 // EncryptionKeyFiles is used when plain key files are passed.
 type EncryptionKeyFiles struct {
-	CurrentKey string
-	OldKey     string
+	// Path to the current encryption key file, or "plain" for no encryption.
+	CurrentKey string `yaml:"key"`
+	// Path to the previous encryption key file, or "plain" for no encryption.
+	OldKey string `yaml:"old-key"`
 }
 
 // EncryptionKeySource is an enum identifying the source of the encryption key.
@@ -30,5 +39,67 @@ const (
 	EncryptionKeyFromFiles EncryptionKeySource = 0
 )
 
+var _ yaml.IsZeroer = EncryptionKeyFromFiles
+
+func (e EncryptionKeySource) IsZero() bool {
+	return e == 0
+}
+
 // DefaultRotationPeriod is the rotation period used if not specified.
 const DefaultRotationPeriod = time.Hour * 24 * 7 // 1 week, give or take time changes.
+
+// PlaintextKeyValue is the value of a key field that indicates no encryption.
+const PlaintextKeyValue = "plain"
+
+// Validate checks all fields, possibly normalizing them and filling in
+// defaults.
+func (e *EncryptionOptions) Validate() error {
+	if e.RotationPeriod == 0 {
+		e.RotationPeriod = DefaultRotationPeriod
+	} else if e.RotationPeriod < time.Second {
+		return errors.Newf("invalid rotation period %s", e.RotationPeriod)
+	}
+	switch e.KeySource {
+	case EncryptionKeyFromFiles:
+		if e.KeyFiles == nil {
+			return errors.New("no key files specified")
+		}
+		currentKey, err := getAbsoluteFSPath("key", e.KeyFiles.CurrentKey)
+		if err != nil {
+			return err
+		}
+		oldKey, err := getAbsoluteFSPath("old key", e.KeyFiles.OldKey)
+		if err != nil {
+			return err
+		}
+		e.KeyFiles.CurrentKey = currentKey
+		e.KeyFiles.OldKey = oldKey
+		return nil
+
+	default:
+		return errors.Newf("unknown key source %d", e.KeySource)
+	}
+}
+
+// getAbsoluteFSPath takes a (possibly relative) path to an encryption key and
+// returns the absolute path. Handles PlaintextKeyValue as a special case.
+//
+// Returns an error if the path begins with '~' or Abs fails.
+// 'fieldName' is used in error strings.
+func getAbsoluteFSPath(fieldName string, p string) (string, error) {
+	if p == "" {
+		return "", errors.Newf("no %s specified", fieldName)
+	}
+	if p == PlaintextKeyValue {
+		return p, nil
+	}
+	if strings.HasPrefix(p, "~") {
+		return "", errors.Newf("%s %q cannot start with '~'", fieldName, p)
+	}
+
+	ret, err := filepath.Abs(p)
+	if err != nil {
+		return "", errors.Wrapf(err, "could not find absolute path for %s %q", fieldName, p)
+	}
+	return ret, nil
+}

pkg/storage/storageconfig/store.go

@@ -57,6 +57,8 @@ func (s *Store) IsEncrypted() bool {
 // hard coded to 640MiB.
 const MinimumStoreSize = 10 * 64 << 20
 
+// Validate checks all fields, possibly normalizing them and filling in
+// defaults.
 func (s *Store) Validate() error {
 	if s.Size.IsBytes() && s.Size.Bytes() < MinimumStoreSize {
 		return errors.Newf("store size (%s) must be at least %s",
@@ -88,6 +90,11 @@ func (s *Store) Validate() error {
 			return errors.Newf("on-disk store cannot use a sticky VFS ID")
 		}
 	}
+	if s.EncryptionOptions != nil {
+		if err := s.EncryptionOptions.Validate(); err != nil {
+			return errors.Wrap(err, "invalid encryption options")
+		}
+	}
 
 	return nil
 }

pkg/storage/storageconfig/store_test.go

@@ -10,6 +10,7 @@ import (
 	"math"
 	"math/rand/v2"
 	"testing"
+	"time"
 
 	"github.com/cockroachdb/cockroach/pkg/testutils/datapathutils"
 	"github.com/cockroachdb/cockroach/pkg/util/yamlutil"
@@ -35,6 +36,14 @@ func TestStoreYAMLOutput(t *testing.T) {
 			BallastSize:     PercentSize(10),
 			Attributes:      []string{"foo", "bar"},
 			ProvisionedRate: ProvisionedRate{ProvisionedBandwidth: 1000 * 1024},
+			EncryptionOptions: &EncryptionOptions{
+				KeySource: EncryptionKeyFromFiles,
+				KeyFiles: &EncryptionKeyFiles{
+					CurrentKey: "/path/to/current-key",
+					OldKey:     "/path/to/old-key",
+				},
+				RotationPeriod: time.Hour,
+			},
 		},
 		"pebble-opts": {
 			Path: "/mnt/data1",
@@ -73,7 +82,9 @@ l0_stop_writes_threshold=10`,
 			if err := s.Validate(); err != nil {
 				return err.Error()
 			}
-			return "ok"
+			out, err := yaml.Marshal(s)
+			require.NoError(t, err)
+			return string(out)
 
 		default:
 			t.Fatalf("unknown command %q", td.Cmd)