Repository: code-ready/crc (go)

Issue:
Add a secret config type to store secrets in the OS provided secret store
It'll be useful to have a way to store a password/pull-secret in the OS key store directly using the `crc config` commands.

The end result would be:

- e.g `crc config set smb-share-password crcpass` (its stored in the OS secret store and 'crcpass' is not written to ~/.crc/crc.json)
- `crc config view` should mask the value of `smb-share-password` in its output

Pull request:
Add storage backend for configs based on OS provided secret store
Fixes #3312 

Patch:
diff --git a/cmd/crc/cmd/config/view.go b/cmd/crc/cmd/config/view.go
index f574e3f1a..1a8109a5c 100644
--- a/cmd/crc/cmd/config/view.go
+++ b/cmd/crc/cmd/config/view.go
@@ -16,7 +16,10 @@ const (
 	DefaultConfigViewFormat = "- {{.ConfigKey | printf \"%-38s\"}}: {{.ConfigValue}}"
 )
 
-var configViewFormat string
+var (
+	configViewFormat string
+	showSecrets      bool
+)
 
 type configViewTemplate struct {
 	ConfigKey   string
@@ -38,6 +41,7 @@ func configViewCmd(config config.Storage) *cobra.Command {
 	}
 	configViewCmd.Flags().StringVar(&configViewFormat, "format", DefaultConfigViewFormat,
 		`Go template format to apply to the configuration file. For more information about Go templates, see: https://golang.org/pkg/text/template/`)
+	configViewCmd.Flags().BoolVar(&showSecrets, "show-secrets", false, "Show values of secret config properties")
 	return configViewCmd
 }
 
@@ -55,6 +59,9 @@ func runConfigView(cfg map[string]config.SettingValue, tmpl *template.Template,
 		if v.IsDefault {
 			continue
 		}
+		if v.IsSecret && !showSecrets {
+			continue
+		}
 		viewTmplt := configViewTemplate{k, v.AsString()}
 		var buffer bytes.Buffer
 		if err := tmpl.Execute(&buffer, viewTmplt); err != nil {
diff --git a/cmd/crc/cmd/root.go b/cmd/crc/cmd/root.go
index 95177e4ce..23026033a 100644
--- a/cmd/crc/cmd/root.go
+++ b/cmd/crc/cmd/root.go
@@ -50,7 +50,7 @@ func init() {
 		logging.Fatal(err.Error())
 	}
 	var err error
-	config, viper, err = newViperConfig()
+	config, viper, err = newConfig()
 	if err != nil {
 		logging.Fatal(err.Error())
 	}
@@ -144,12 +144,12 @@ func setProxyDefaults() error {
 	return nil
 }
 
-func newViperConfig() (*crcConfig.Config, *crcConfig.ViperStorage, error) {
+func newConfig() (*crcConfig.Config, *crcConfig.ViperStorage, error) {
 	viper, err := crcConfig.NewViperStorage(constants.ConfigPath, constants.CrcEnvPrefix)
 	if err != nil {
 		return nil, nil, err
 	}
-	cfg := crcConfig.New(viper)
+	cfg := crcConfig.New(viper, crcConfig.NewSecretStorage())
 	crcConfig.RegisterSettings(cfg)
 	preflight.RegisterSettings(cfg)
 	return cfg, viper, nil
diff --git a/pkg/crc/api/handlers.go b/pkg/crc/api/handlers.go
index 97a91e3ba..edee6a2f5 100644
--- a/pkg/crc/api/handlers.go
+++ b/pkg/crc/api/handlers.go
@@ -219,6 +219,9 @@ func (h *Handler) GetConfig(c *context) error {
 		allConfigs := h.Config.AllConfigs()
 		configs := make(map[string]interface{})
 		for k, v := range allConfigs {
+			if v.IsSecret {
+				continue
+			}
 			configs[k] = v.Value
 		}
 		return c.JSON(http.StatusOK, client.GetConfigResult{
@@ -232,6 +235,9 @@ func (h *Handler) GetConfig(c *context) error {
 		if v.Invalid {
 			continue
 		}
+		if v.IsSecret {
+			continue
+		}
 		configs[key] = v.Value
 	}
 
diff --git a/pkg/crc/config/config.go b/pkg/crc/config/config.go
index e2ab71bf0..58c5b0ce1 100644
--- a/pkg/crc/config/config.go
+++ b/pkg/crc/config/config.go
@@ -16,12 +16,14 @@ const (
 
 type Config struct {
 	storage        RawStorage
+	secretStorage  RawStorage
 	settingsByName map[string]Setting
 }
 
-func New(storage RawStorage) *Config {
+func New(storage, secretStorage RawStorage) *Config {
 	return &Config{
 		storage:        storage,
+		secretStorage:  secretStorage,
 		settingsByName: make(map[string]Setting),
 	}
 }
@@ -55,6 +57,7 @@ func (c *Config) AddSetting(name string, defValue interface{}, validationFn Vali
 		defaultValue: defValue,
 		validationFn: validationFn,
 		callbackFn:   callbackFn,
+		isSecret:     isUnderlyingTypeSecret(defValue),
 		Help:         help,
 	}
 }
@@ -79,7 +82,7 @@ func (c *Config) Set(key string, value interface{}) (string, error) {
 		if err != nil {
 			return "", fmt.Errorf(invalidProp, value, key, err)
 		}
-	case string:
+	case string, Secret:
 		castValue = cast.ToString(value)
 	case bool:
 		castValue, err = cast.ToBoolE(value)
@@ -92,8 +95,14 @@ func (c *Config) Set(key string, value interface{}) (string, error) {
 		return "", fmt.Errorf(invalidType, value, key)
 	}
 
-	if err := c.storage.Set(key, castValue); err != nil {
-		return "", err
+	if setting.isSecret {
+		if err := c.secretStorage.Set(key, castValue); err != nil {
+			return "", err
+		}
+	} else {
+		if err := c.storage.Set(key, castValue); err != nil {
+			return "", err
+		}
 	}
 
 	return c.settingsByName[key].callbackFn(key, castValue), nil
@@ -101,15 +110,19 @@ func (c *Config) Set(key string, value interface{}) (string, error) {
 
 // Unset unsets a given config key
 func (c *Config) Unset(key string) (string, error) {
-	_, ok := c.settingsByName[key]
+	setting, ok := c.settingsByName[key]
 	if !ok {
 		return "", fmt.Errorf(configPropDoesntExistMsg, key)
 	}
-
-	if err := c.storage.Unset(key); err != nil {
-		return "", err
+	if setting.isSecret {
+		if err := c.secretStorage.Unset(key); err != nil {
+			return "", err
+		}
+	} else {
+		if err := c.storage.Unset(key); err != nil {
+			return "", err
+		}
 	}
-
 	return fmt.Sprintf("Successfully unset configuration property '%s'", key), nil
 }
 
@@ -120,7 +133,12 @@ func (c *Config) Get(key string) SettingValue {
 			Invalid: true,
 		}
 	}
-	value := c.storage.Get(key)
+	var value interface{}
+	if setting.isSecret {
+		value = c.secretStorage.Get(key)
+	} else {
+		value = c.storage.Get(key)
+	}
 	if value == nil {
 		value = setting.defaultValue
 	}
@@ -149,6 +167,13 @@ func (c *Config) Get(key string) SettingValue {
 				Invalid: true,
 			}
 		}
+	case Secret:
+		value, err = toSecret(value)
+		if err != nil {
+			return SettingValue{
+				Invalid: true,
+			}
+		}
 	default:
 		return SettingValue{
 			Invalid: true,
@@ -157,5 +182,24 @@ func (c *Config) Get(key string) SettingValue {
 	return SettingValue{
 		Value:     value,
 		IsDefault: reflect.DeepEqual(setting.defaultValue, value),
+		IsSecret:  isUnderlyingTypeSecret(value),
+	}
+}
+
+func toSecret(value interface{}) (Secret, error) {
+	if isUnderlyingTypeSecret(value) {
+		return value.(Secret), nil
+	}
+	v, err := cast.ToStringE(value)
+	if err != nil {
+		return Secret(""), err
+	}
+	return Secret(v), nil
+}
+
+func isUnderlyingTypeSecret(value interface{}) bool {
+	if _, ok := value.(Secret); ok {
+		return true
 	}
+	return false
 }
diff --git a/pkg/crc/config/secret_config.go b/pkg/crc/config/secret_config.go
new file mode 100644
index 000000000..444365b80
--- /dev/null
+++ b/pkg/crc/config/secret_config.go
@@ -0,0 +1,83 @@
+package config
+
+import (
+	"errors"
+	"fmt"
+
+	"github.com/code-ready/crc/pkg/crc/logging"
+	"github.com/spf13/cast"
+	"github.com/zalando/go-keyring"
+)
+
+const secretServiceName = "crc"
+
+var ErrSecretsNotAccessible = errors.New("secret store is not accessible")
+
+type Secret string
+
+func (s Secret) String() string {
+	return string(s)
+}
+
+type SecretStorage struct {
+	secretService   string
+	storeAccessible bool
+}
+
+func NewSecretStorage() *SecretStorage {
+	return &SecretStorage{
+		secretService:   secretServiceName,
+		storeAccessible: keyringAccessible(),
+	}
+}
+
+func (c *SecretStorage) Get(key string) interface{} {
+	if !c.storeAccessible {
+		return nil
+	}
+	secret, err := keyring.Get(c.secretService, key)
+	if errors.Is(err, keyring.ErrNotFound) {
+		return nil
+	}
+	return secret
+}
+
+func (c *SecretStorage) Set(key string, value interface{}) error {
+	if !c.storeAccessible {
+		return ErrSecretsNotAccessible
+	}
+	secret, err := cast.ToStringE(value)
+	if err != nil {
+		return fmt.Errorf("Failed to cast secret value to string: %w", err)
+	}
+	return keyring.Set(c.secretService, key, secret)
+}
+
+func (c *SecretStorage) Unset(key string) error {
+	if !c.storeAccessible {
+		return ErrSecretsNotAccessible
+	}
+	err := keyring.Delete(c.secretService, key)
+	if errors.Is(err, keyring.ErrNotFound) {
+		return nil
+	}
+	return err
+}
+
+func keyringAccessible() bool {
+	err := keyring.Set("crc-test", "foo", "bar")
+	if err == nil {
+		_ = keyring.Delete("crc-test", "foo")
+		return true
+	}
+	logging.Debugf("Keyring is not accessible: %v", err)
+	return false
+}
+
+func NewEmptyInMemorySecretStorage() *SecretStorage {
+	keyring.MockInit()
+	return &SecretStorage{
+		secretService:   secretServiceName,
+		storeAccessible: true,
+	}
+}
diff --git a/pkg/crc/config/types.go b/pkg/crc/config/types.go
index 8b0b36a88..208ac50a4 100644
--- a/pkg/crc/config/types.go
+++ b/pkg/crc/config/types.go
@@ -18,6 +18,7 @@ type Setting struct {
 	defaultValue interface{}
 	validationFn ValidationFnType
 	callbackFn   SetFn
+	isSecret     bool
 	Help         string
 }
 
@@ -25,6 +26,7 @@ type SettingValue struct {
 	Value     interface{}
 	Invalid   bool
 	IsDefault bool
+	IsSecret  bool
 }
 
 func (v SettingValue) AsBool() bool {


Test patch:
diff --git a/pkg/crc/api/helpers_test.go b/pkg/crc/api/helpers_test.go
index df5828b12..df6251620 100644
--- a/pkg/crc/api/helpers_test.go
+++ b/pkg/crc/api/helpers_test.go
@@ -11,7 +11,7 @@ func setupNewInMemoryConfig() *config.Config {
 	storage := config.NewEmptyInMemoryStorage()
 	cfg := config.New(&skipPreflights{
 		storage: storage,
-	})
+	}, config.NewEmptyInMemorySecretStorage())
 	cfg.AddSetting("a&a", "foo", nil, nil, "test special string")
 	cfg.AddSetting("b&&&b", "bar", nil, nil, "test special string")
 	config.RegisterSettings(cfg)
diff --git a/pkg/crc/cluster/pullsecret_test.go b/pkg/crc/cluster/pullsecret_test.go
index 2d2e6d9ad..88f623af4 100644
--- a/pkg/crc/cluster/pullsecret_test.go
+++ b/pkg/crc/cluster/pullsecret_test.go
@@ -25,7 +25,7 @@ func TestLoadPullSecret(t *testing.T) {
 	assert.NoError(t, err)
 	defer os.RemoveAll(dir)
 
-	cfg := config.New(config.NewEmptyInMemoryStorage())
+	cfg := config.New(config.NewEmptyInMemoryStorage(), config.NewEmptyInMemorySecretStorage())
 	config.RegisterSettings(cfg)
 
 	loader := &nonInteractivePullSecretLoader{
diff --git a/pkg/crc/config/secret_config_test.go b/pkg/crc/config/secret_config_test.go
new file mode 100644
index 000000000..f77200b9d
--- /dev/null
+++ b/pkg/crc/config/secret_config_test.go
@@ -0,0 +1,69 @@
+package config
+
+import (
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+)
+
+const (
+	password = "password"
+	secret   = "secret"
+)
+
+func newTestConfigSecret() (*Config, error) {
+	cfg := New(NewEmptyInMemoryStorage(), NewEmptyInMemorySecretStorage())
+
+	cfg.AddSetting(password, Secret(""), ValidateString, SuccessfullyApplied, "")
+	cfg.AddSetting(secret, Secret("apples"), ValidateString, SuccessfullyApplied, "")
+	return cfg, nil
+}
+
+func TestGetSecret(t *testing.T) {
+	cfg, err := newTestConfigSecret()
+	require.NoError(t, err)
+
+	assert.Equal(t, SettingValue{
+		Value:     Secret("apples"),
+		IsDefault: true,
+		IsSecret:  true,
+	}, cfg.Get(secret))
+}
+
+func TestSecretConfigUnknown(t *testing.T) {
+	cfg, err := newTestConfigSecret()
+	require.NoError(t, err)
+
+	assert.Equal(t, SettingValue{
+		Invalid: true,
+	}, cfg.Get("baz"))
+}
+
+func TestSecretConfigSetAndGet(t *testing.T) {
+	cfg, err := newTestConfigSecret()
+	require.NoError(t, err)
+
+	_, err = cfg.Set(password, "pass123")
+	assert.NoError(t, err)
+
+	assert.Equal(t, SettingValue{
+		Value:     Secret("pass123"),
+		IsDefault: false,
+		IsSecret:  true,
+	}, cfg.Get(password))
+}
+
+func TestSecretConfigUnsetAndGet(t *testing.T) {
+	cfg, err := newTestConfigSecret()
+	require.NoError(t, err)
+
+	_, err = cfg.Unset(secret)
+	assert.NoError(t, err)
+
+	assert.Equal(t, SettingValue{
+		Value:     Secret("apples"),
+		IsDefault: true,
+		IsSecret:  true,
+	}, cfg.Get(secret))
+}
diff --git a/pkg/crc/config/viper_config_test.go b/pkg/crc/config/viper_config_test.go
index e861f1277..a6c94f965 100644
--- a/pkg/crc/config/viper_config_test.go
+++ b/pkg/crc/config/viper_config_test.go
@@ -25,7 +25,9 @@ func newTestConfig(configFile, envPrefix string) (*Config, error) {
 	if err != nil {
 		return nil, err
 	}
-	config := New(storage)
+
+	secretStorage := NewEmptyInMemorySecretStorage()
+	config := New(storage, secretStorage)
 	config.AddSetting(cpus, 4, validateCPUs, RequiresRestartMsg, "")
 	config.AddSetting(nameServer, "", ValidateIPAddress, SuccessfullyApplied, "")
 	return config, nil
@@ -157,7 +159,7 @@ func TestViperConfigBindFlagSet(t *testing.T) {
 	}
 	storage, err := NewViperStorage(configFile, "CRC")
 	require.NoError(t, err)
-	config := New(storage)
+	config := New(storage, NewEmptyInMemorySecretStorage())
 	config.AddSetting(cpus, 4, validateCPUs, RequiresRestartMsg, "")
 	config.AddSetting(nameServer, "", ValidateIPAddress, SuccessfullyApplied, "")
 
diff --git a/pkg/crc/preflight/preflight_darwin_test.go b/pkg/crc/preflight/preflight_darwin_test.go
index 9563fc66e..27b516238 100644
--- a/pkg/crc/preflight/preflight_darwin_test.go
+++ b/pkg/crc/preflight/preflight_darwin_test.go
@@ -11,7 +11,7 @@ import (
 )
 
 func TestCountConfigurationOptions(t *testing.T) {
-	cfg := config.New(config.NewEmptyInMemoryStorage())
+	cfg := config.New(config.NewEmptyInMemoryStorage(), config.NewEmptyInMemorySecretStorage())
 	RegisterSettings(cfg)
 	assert.Len(t, cfg.AllConfigs(), 12)
 }
diff --git a/pkg/crc/preflight/preflight_linux_test.go b/pkg/crc/preflight/preflight_linux_test.go
index 7c312a910..6c0a6c355 100644
--- a/pkg/crc/preflight/preflight_linux_test.go
+++ b/pkg/crc/preflight/preflight_linux_test.go
@@ -15,7 +15,7 @@ import (
 )
 
 func TestCountConfigurationOptions(t *testing.T) {
-	cfg := config.New(config.NewEmptyInMemoryStorage())
+	cfg := config.New(config.NewEmptyInMemoryStorage(), config.NewEmptyInMemorySecretStorage())
 	RegisterSettings(cfg)
 	options := len(cfg.AllConfigs())
 
diff --git a/pkg/crc/preflight/preflight_test.go b/pkg/crc/preflight/preflight_test.go
index 74aa312ef..342fa7d34 100644
--- a/pkg/crc/preflight/preflight_test.go
+++ b/pkg/crc/preflight/preflight_test.go
@@ -10,7 +10,7 @@ import (
 
 func TestCheckPreflight(t *testing.T) {
 	check, calls := sampleCheck(nil, nil)
-	cfg := config.New(config.NewEmptyInMemoryStorage())
+	cfg := config.New(config.NewEmptyInMemoryStorage(), config.NewEmptyInMemorySecretStorage())
 	doRegisterSettings(cfg, []Check{*check})
 
 	assert.NoError(t, doPreflightChecks(cfg, []Check{*check}))
@@ -20,7 +20,7 @@ func TestCheckPreflight(t *testing.T) {
 
 func TestSkipPreflight(t *testing.T) {
 	check, calls := sampleCheck(nil, nil)
-	cfg := config.New(config.NewEmptyInMemoryStorage())
+	cfg := config.New(config.NewEmptyInMemoryStorage(), config.NewEmptyInMemorySecretStorage())
 	doRegisterSettings(cfg, []Check{*check})
 	_, err := cfg.Set("skip-sample", true)
 	assert.NoError(t, err)
@@ -31,7 +31,7 @@ func TestSkipPreflight(t *testing.T) {
 
 func TestFixPreflight(t *testing.T) {
 	check, calls := sampleCheck(errors.New("check failed"), nil)
-	cfg := config.New(config.NewEmptyInMemoryStorage())
+	cfg := config.New(config.NewEmptyInMemoryStorage(), config.NewEmptyInMemorySecretStorage())
 	doRegisterSettings(cfg, []Check{*check})
 
 	assert.NoError(t, doFixPreflightChecks(cfg, []Check{*check}, false))
@@ -41,7 +41,7 @@ func TestFixPreflight(t *testing.T) {
 
 func TestFixPreflightCheckOnly(t *testing.T) {
 	check, calls := sampleCheck(errors.New("check failed"), nil)
-	cfg := config.New(config.NewEmptyInMemoryStorage())
+	cfg := config.New(config.NewEmptyInMemoryStorage(), config.NewEmptyInMemorySecretStorage())
 	doRegisterSettings(cfg, []Check{*check})
 
 	assert.Error(t, doFixPreflightChecks(cfg, []Check{*check}, true))
diff --git a/pkg/crc/preflight/preflight_windows_test.go b/pkg/crc/preflight/preflight_windows_test.go
index 34413b368..904c678d5 100644
--- a/pkg/crc/preflight/preflight_windows_test.go
+++ b/pkg/crc/preflight/preflight_windows_test.go
@@ -11,7 +11,7 @@ import (
 )
 
 func TestCountConfigurationOptions(t *testing.T) {
-	cfg := config.New(config.NewEmptyInMemoryStorage())
+	cfg := config.New(config.NewEmptyInMemoryStorage(), config.NewEmptyInMemorySecretStorage())
 	RegisterSettings(cfg)
 	assert.Len(t, cfg.AllConfigs(), 12)
 }
diff --git a/pkg/crc/segment/segment_test.go b/pkg/crc/segment/segment_test.go
index 9ada514df..5b3ce5724 100644
--- a/pkg/crc/segment/segment_test.go
+++ b/pkg/crc/segment/segment_test.go
@@ -59,7 +59,8 @@ func mockServer() (chan []byte, *httptest.Server) {
 
 func newTestConfig(value string) (*crcConfig.Config, error) {
 	storage := crcConfig.NewEmptyInMemoryStorage()
-	config := crcConfig.New(storage)
+	secretStorage := crcConfig.NewEmptyInMemorySecretStorage()
+	config := crcConfig.New(storage, secretStorage)
 	crcConfig.RegisterSettings(config)
 
 	if _, err := config.Set(crcConfig.ConsentTelemetry, value); err != nil {
diff --git a/test/extended/util/collect.go b/test/extended/util/collect.go
index dfd336b93..8ea93b763 100644
--- a/test/extended/util/collect.go
+++ b/test/extended/util/collect.go
@@ -230,7 +230,7 @@ func (collector *ContainerLogCollector) Collect(w Writer) error {
 }
 
 func sshClient() (ssh.Client, error) {
-	cfg := crcConfig.New(crcConfig.NewEmptyInMemoryStorage())
+	cfg := crcConfig.New(crcConfig.NewEmptyInMemoryStorage(), crcConfig.NewEmptyInMemorySecretStorage())
 	crcConfig.RegisterSettings(cfg)
 	client := machine.NewClient(constants.DefaultName, true, cfg)
 	connectionDetails, err := client.ConnectionDetails()
diff --git a/test/integration/utilities_test.go b/test/integration/utilities_test.go
index 89e904116..8e7fb2bbb 100644
--- a/test/integration/utilities_test.go
+++ b/test/integration/utilities_test.go
@@ -218,7 +218,9 @@ func RunPodmanExpectFail(args ...string) (string, error) {
 
 // Send command to CRC VM via SSH
 func SendCommandToVM(cmd string) (string, error) {
-	client := machine.NewClient(constants.DefaultName, false, crcConfig.New(crcConfig.NewEmptyInMemoryStorage()))
+	client := machine.NewClient(constants.DefaultName, false,
+		crcConfig.New(crcConfig.NewEmptyInMemoryStorage(), crcConfig.NewEmptyInMemorySecretStorage()),
+	)
 	connectionDetails, err := client.ConnectionDetails()
 	if err != nil {
 		return "", err