Allow KeyStore to be configured with multiple keys (#3335)

Fixes #3304

Add a new configuration structure for encryption keys. This allows a
default key and algorithm to be specified, along with fallbacks. A
backwards compatibility mechanism is provided which will allow the
keystore to work with the existing configuration structure.

cmd/server/app/webhook_update.go

@@ -219,7 +219,7 @@ func updateGithubRepoHooks(
 }
 
 func wireUpProviderManager(cfg *serverconfig.Config, store db.Store) (manager.ProviderManager, error) {
-	cryptoEng, err := crypto.NewEngineFromAuthConfig(&cfg.Auth)
+	cryptoEng, err := crypto.NewEngineFromConfig(cfg)
 	if err != nil {
 		return nil, fmt.Errorf("failed to create crypto engine: %w", err)
 	}

config/server-config.yaml.example

@@ -55,7 +55,6 @@ identity:
 #   openssl rand -base64 32 > .ssh/token_key_passphrase
 auth:
   nonce_period: 3600
-  token_key: "./.ssh/token_key_passphrase"
 
 # Webhook Configuration
 # change example.com to an exposed IP / domain
@@ -104,4 +103,13 @@ authz:
 #    - stacklok-health-check
 #  bundle:
 #    namespace: stacklok
-#    name: healthcheck
+#    name: healthcheck
+
+crypto:
+    keystore:
+        type: local
+        config:
+            key_dir: "./.ssh"
+    default:
+        key_id:    token_key_passphrase
+        algorithm: aes-256-cfb

internal/config/server/auth.go

@@ -15,17 +15,10 @@
 
 package server
 
-import "github.com/stacklok/minder/internal/config"
-
 // AuthConfig is the configuration for the auth package
 type AuthConfig struct {
 	// NoncePeriod is the period in seconds for which a nonce is valid
 	NoncePeriod int64 `mapstructure:"nonce_period" default:"3600"`
 	// TokenKey is the key used to store the provider's token in the database
 	TokenKey string `mapstructure:"token_key" default:"./.ssh/token_key_passphrase"`
 }
-
-// GetTokenKey returns a key used to encrypt the provider's token in the database
-func (acfg *AuthConfig) GetTokenKey() ([]byte, error) {
-	return config.ReadKey(acfg.TokenKey)
-}

internal/config/server/config.go

@@ -44,6 +44,7 @@ type Config struct {
 	Provider        ProviderConfig        `mapstructure:"provider"`
 	Marketplace     MarketplaceConfig     `mapstructure:"marketplace"`
 	DefaultProfiles DefaultProfilesConfig `mapstructure:"default_profiles"`
+	Crypto          CryptoConfig          `mapstructure:"crypto"`
 }
 
 // DefaultConfigForTest returns a configuration with all the struct defaults set,

internal/config/server/config_test.go

@@ -17,9 +17,6 @@ package server_test
 
 import (
 	"bytes"
-	"os"
-	"path/filepath"
-	"strings"
 	"testing"
 
 	"github.com/spf13/pflag"
@@ -181,57 +178,6 @@ func TestReadDefaultConfig(t *testing.T) {
 	require.Equal(t, "./.ssh/token_key_passphrase", cfg.Auth.TokenKey)
 }
 
-func TestReadAuthConfig(t *testing.T) {
-	t.Parallel()
-
-	testCases := []struct {
-		name     string
-		mutate   func(*testing.T, *serverconfig.AuthConfig)
-		keyError string
-	}{
-		{
-			name: "valid config",
-		},
-		{
-			name: "missing keys",
-			mutate: func(t *testing.T, cfg *serverconfig.AuthConfig) {
-				t.Helper()
-				require.NoError(t, os.Remove(cfg.TokenKey))
-			},
-			keyError: "no such file or directory",
-		},
-	}
-
-	for _, tc := range testCases {
-		tc := tc
-		t.Run(tc.name, func(t *testing.T) {
-			t.Parallel()
-			tmpdir := t.TempDir()
-			cfg := serverconfig.AuthConfig{
-				TokenKey: filepath.Join(tmpdir, "token_key"),
-			}
-			if err := os.WriteFile(cfg.TokenKey, []byte("test"), 0600); err != nil {
-				t.Fatalf("Error generating access token key pair: %v", err)
-			}
-
-			if tc.mutate != nil {
-				tc.mutate(t, &cfg)
-			}
-
-			if _, err := cfg.GetTokenKey(); !errMatches(err, tc.keyError) {
-				t.Errorf("Expected error containing %q, but got %v", tc.keyError, err)
-			}
-		})
-	}
-}
-
-func errMatches(got error, want string) bool {
-	if want == "" {
-		return got == nil
-	}
-	return strings.Contains(got.Error(), want)
-}
-
 const (
 	viperPath  = "test.path"
 	cmdLineArg = "test-arg"

internal/config/server/crypto.go

@@ -0,0 +1,48 @@
+// Copyright 2024 Stacklok, Inc.
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//     http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package server
+
+// CryptoConfig is the configuration for the crypto engine
+type CryptoConfig struct {
+	KeyStore KeyStoreConfig `mapstructure:"key_store"`
+	Default  DefaultCrypto  `mapstructure:"default"`
+	Fallback FallbackCrypto `mapstructure:"fallback"`
+}
+
+// KeyStoreConfig specifies the type of keystore to use and its configuration
+type KeyStoreConfig struct {
+	Type string `mapstructure:"type" default:"local"`
+	// is currently expected to match the structure of LocalFileKeyStoreConfig
+	Config map[string]any `mapstructure:"config"`
+}
+
+// DefaultCrypto defines the default crypto to be used for new data
+type DefaultCrypto struct {
+	KeyID     string `mapstructure:"key_id"`
+	Algorithm string `mapstructure:"algorithm"`
+}
+
+// FallbackCrypto defines the optional list of keys and algorithms to fall
+// back to.
+// When rotating keys or algorithms, add the old ones here.
+type FallbackCrypto struct {
+	KeyIDs     []string `mapstructure:"key_ids"`
+	Algorithms []string `mapstructure:"algorithms"`
+}
+
+// LocalFileKeyStoreConfig contains configuration for the local file keystore
+type LocalFileKeyStoreConfig struct {
+	KeyDir string `mapstructure:"key_dir"`
+}

internal/config/utils.go

@@ -233,31 +233,7 @@ func SetViperStructDefaults(v *viper.Viper, prefix string, s any) {
 			continue
 		}
 
-		defaultValue := reflect.Zero(field.Type).Interface()
-		var err error // We handle errors at the end of the switch
-		//nolint:golint,exhaustive
-		switch fieldType.Kind() {
-		case reflect.String:
-			defaultValue = value
-		case reflect.Int64:
-			defaultValue, err = getDefaultValueForInt64(value)
-		case reflect.Int32, reflect.Int16, reflect.Int8, reflect.Int,
-			reflect.Uint64, reflect.Uint32, reflect.Uint16, reflect.Uint8, reflect.Uint:
-			defaultValue, err = strconv.Atoi(value)
-		case reflect.Float64:
-			defaultValue, err = strconv.ParseFloat(value, 64)
-		case reflect.Bool:
-			defaultValue, err = strconv.ParseBool(value)
-		case reflect.Slice:
-			defaultValue = nil
-		default:
-			err = fmt.Errorf("unhandled type %s", fieldType)
-		}
-		if err != nil {
-			// This is effectively a compile-time error, so exit early
-			panic(fmt.Sprintf("Bad value for field %q (%s): %q", valueName, fieldType, err))
-		}
-
+		defaultValue := getDefaultValue(field, value, valueName)
 		if err := v.BindEnv(strings.ToUpper(valueName)); err != nil {
 			panic(fmt.Sprintf("Failed to bind %q to env var: %v", valueName, err))
 		}
@@ -297,3 +273,33 @@ func getDefaultValueForInt64(value string) (any, error) {
 	// Return the original error, not time.ParseDuration's error
 	return nil, err
 }
+
+func getDefaultValue(field reflect.StructField, value string, valueName string) any {
+	defaultValue := reflect.Zero(field.Type).Interface()
+	var err error // We handle errors at the end of the switch
+	//nolint:golint,exhaustive
+	switch field.Type.Kind() {
+	case reflect.String:
+		defaultValue = value
+	case reflect.Int64:
+		defaultValue, err = getDefaultValueForInt64(value)
+	case reflect.Int32, reflect.Int16, reflect.Int8, reflect.Int,
+		reflect.Uint64, reflect.Uint32, reflect.Uint16, reflect.Uint8, reflect.Uint:
+		defaultValue, err = strconv.Atoi(value)
+	case reflect.Float64:
+		defaultValue, err = strconv.ParseFloat(value, 64)
+	case reflect.Bool:
+		defaultValue, err = strconv.ParseBool(value)
+	case reflect.Slice:
+		defaultValue = nil
+	case reflect.Map:
+		defaultValue = nil
+	default:
+		err = fmt.Errorf("unhandled type %s", field.Type)
+	}
+	if err != nil {
+		// This is effectively a compile-time error, so exit early
+		panic(fmt.Sprintf("Bad value for field %q (%s): %q", valueName, field.Type, err))
+	}
+	return defaultValue
+}

internal/controlplane/server_test.go

@@ -97,7 +97,7 @@ func newDefaultServer(
 	// Needed to keep these tests working as-is.
 	// In future, beef up unit test coverage in the dependencies
 	// of this code, and refactor these tests to use stubs.
-	eng, err := crypto.NewEngineFromAuthConfig(&c.Auth)
+	eng, err := crypto.NewEngineFromConfig(c)
 	require.NoError(t, err)
 	ghClientService := ghService.NewGithubProviderService(
 		mockStore,

internal/crypto/engine.go

@@ -62,34 +62,53 @@ type engine struct {
 	defaultAlgorithm    algorithms.Type
 }
 
-// NewEngineFromAuthConfig creates a new crypto engine from the service config
+// NewEngineFromConfig creates a new crypto engine from the service config
 // TODO: modify to support multiple keys/algorithms
-func NewEngineFromAuthConfig(config *serverconfig.AuthConfig) (Engine, error) {
-	if config == nil {
-		return nil, errors.New("auth config is nil")
-	}
-
-	keystore, err := keystores.NewKeyStoreFromConfig(config)
+func NewEngineFromConfig(config *serverconfig.Config) (Engine, error) {
+	// Use fallback if the new config structure is missing
+	var cryptoCfg serverconfig.CryptoConfig
+	if config.Crypto.Default.KeyID != "" && config.Crypto.Default.Algorithm != "" {
+		cryptoCfg = config.Crypto
+	} else if config.Auth.TokenKey != "" {
+		fallbackConfig, err := convertToCryptoConfig(&config.Auth)
+		if err != nil {
+			return nil, fmt.Errorf("unable to load fallback config: %w", err)
+		}
+		cryptoCfg = fallbackConfig
+	} else {
+		return nil, errors.New("no encryption keys configured")
+	}
+
+	keystore, err := keystores.NewKeyStoreFromConfig(cryptoCfg)
 	if err != nil {
 		return nil, fmt.Errorf("failed to read token key file: %s", err)
 	}
 
-	aes, err := algorithms.NewFromType(algorithms.Aes256Cfb)
+	defaultAlgorithm, err := algorithms.TypeFromString(cryptoCfg.Default.Algorithm)
 	if err != nil {
-		return nil, err
+		return nil, fmt.Errorf("unexpected default algorithm: %w", err)
 	}
-	supportedAlgorithms := map[algorithms.Type]algorithms.EncryptionAlgorithm{
-		algorithms.Aes256Cfb: aes,
+
+	// Instantiate all the algorithms we need
+	algos := append([]string{cryptoCfg.Default.Algorithm}, cryptoCfg.Fallback.Algorithms...)
+	supportedAlgorithms := make(algorithmsByName, len(algos))
+	for _, algoName := range algos {
+		algoType, err := algorithms.TypeFromString(algoName)
+		if err != nil {
+			return nil, err
+		}
+		algorithm, err := algorithms.NewFromType(algoType)
+		if err != nil {
+			return nil, err
+		}
+		supportedAlgorithms[algoType] = algorithm
 	}
 
 	return &engine{
 		keystore:            keystore,
 		supportedAlgorithms: supportedAlgorithms,
-		defaultAlgorithm:    algorithms.Aes256Cfb,
-		// Use the key filename as the key ID.
-		// This will be cleaned up in a future PR
-		// Right now, by the time we get here, this should return a valid result
-		defaultKeyID: filepath.Base(config.TokenKey),
+		defaultAlgorithm:    defaultAlgorithm,
+		defaultKeyID:        cryptoCfg.Default.KeyID,
 	}, nil
 }
 
@@ -172,7 +191,13 @@ func (e *engine) decrypt(data EncryptedData) ([]byte, error) {
 		return nil, fmt.Errorf("%w: %s", algorithms.ErrUnknownAlgorithm, e.defaultAlgorithm)
 	}
 
-	key, err := e.keystore.GetKey(e.defaultKeyID)
+	// for backwards compatibility with encrypted data which doesn't have the
+	// key ID stored in the DB.
+	if data.KeyVersion == "" {
+		data.KeyVersion = e.defaultKeyID
+	}
+
+	key, err := e.keystore.GetKey(data.KeyVersion)
 	if err != nil {
 		// error from keystore is good enough - we do not need more context
 		return nil, err
@@ -191,3 +216,24 @@ func (e *engine) decrypt(data EncryptedData) ([]byte, error) {
 	}
 	return result, nil
 }
+
+// This is for config transition purposes, and will eventually be removed.
+func convertToCryptoConfig(a *serverconfig.AuthConfig) (serverconfig.CryptoConfig, error) {
+	abspath, err := filepath.Abs(a.TokenKey)
+	if err != nil {
+		return serverconfig.CryptoConfig{}, fmt.Errorf("could not get absolute path: %w", err)
+	}
+	name := filepath.Base(abspath)
+	dir := filepath.Dir(abspath)
+
+	return serverconfig.CryptoConfig{
+		KeyStore: serverconfig.KeyStoreConfig{
+			Type:   keystores.LocalKeyStore,
+			Config: map[string]any{"key_dir": dir},
+		},
+		Default: serverconfig.DefaultCrypto{
+			KeyID:     name,
+			Algorithm: string(algorithms.Aes256Cfb),
+		},
+	}, nil
+}