From e840c17bc8e5b6ea3e92e55770ba88dab75f6856 Mon Sep 17 00:00:00 2001 From: madhavilosetty-intel Date: Fri, 10 Jul 2026 16:35:28 -0700 Subject: [PATCH] fix(config): remove hardcoded JWT signing key; resolve securely The JWT signing key defaulted to the public literal "your_secret_jwt_key" in defaultConfig, .env.example and config/config.yml. Because every JWT (REST middleware and the KVM/SOL/IDER WebSocket) is validated with HS256 against this key, anyone could forge a token and bypass authentication. The env-required tag gave false assurance: cleanenv is satisfied by the non-empty default, so the app booted with the insecure key. Resolve the key with no hardcoded default, mirroring the AMT encryption key: secret store (Vault) -> OS keyring -> generate a 256-bit key, stored securely and never written to config.yml. Refuse to start when the key is empty or the known-insecure literal. Tighten config.yml to 0600 / dir 0700 (it persists the admin password in plaintext) and scrub the literal from .env.example and config/config.yml. This is Phase 1 of #1067 (secure credential storage); admin-password migration and the disable-plaintext toggle follow in Phase 2. Refs: #1067 --- .env.example | 4 +- cmd/app/main.go | 203 +++++++++++++++++------------------- cmd/app/main_test.go | 64 ++++++++++++ cmd/app/secretstore.go | 63 +++++++++++ cmd/app/secretstore_test.go | 127 ++++++++++++++++++++++ config/config.go | 19 +++- config/config.yml | 2 +- config/config_test.go | 3 + 8 files changed, 369 insertions(+), 116 deletions(-) create mode 100644 cmd/app/secretstore.go create mode 100644 cmd/app/secretstore_test.go diff --git a/.env.example b/.env.example index f49e4e2e6..305f518cf 100644 --- a/.env.example +++ b/.env.example @@ -43,7 +43,9 @@ AUTH_DISABLED=false AUTH_ADMIN_USERNAME=standalone # AUTH_ADMIN_PASSWORD: If not set, a random password is generated AUTH_ADMIN_PASSWORD= -AUTH_JWT_KEY=your_secret_jwt_key +# AUTH_JWT_KEY: If not set, a strong key is generated and stored in the secret store / OS keyring. +# Set a strong random value here only to supply your own; never use a shared/example value. +AUTH_JWT_KEY= AUTH_JWT_EXPIRATION=24h AUTH_REDIRECTION_JWT_EXPIRATION=5m AUTH_CLIENT_ID= diff --git a/cmd/app/main.go b/cmd/app/main.go index 3bbaa563c..18c909123 100644 --- a/cmd/app/main.go +++ b/cmd/app/main.go @@ -22,11 +22,21 @@ import ( var ( ErrSecretStoreAddressNotConfigured = errors.New("secret store address not configured") ErrSecretStoreTokenNotConfigured = errors.New("secret store token not configured") + ErrJWTKeyMissing = errors.New("JWT signing key is empty") + ErrJWTKeyInsecure = errors.New("JWT signing key is the known-insecure default") ) // adminPasswordLength is the length of generated admin passwords. const adminPasswordLength = 16 +const ( + insecureDefaultJWTKey = "your_secret_jwt_key" // rejected at startup + jwtKeyStorageKey = "jwt-signing-key" // secret store / keyring key + encryptionKeyStorageKey = "default-security-key" // secret store / keyring key + jwtKeyBytes = 32 // 256-bit, matches HS256 + keyringServiceName = "device-management-toolkit" +) + // Function pointers for better testability. var ( initializeConfigFunc = config.NewConfig @@ -67,6 +77,7 @@ func main() { handleEncryptionKey(cfg) handleAdminPassword(cfg) + handleJWTKey(cfg) // Run with system tray (if built with tray tag and --tray flag) or standard mode if config.TrayMode && !trayBuildEnabled { @@ -133,92 +144,16 @@ func handleSecretsConfig(cfg *config.Config) (security.Storager, error) { } func handleEncryptionKey(cfg *config.Config) { - // If encryption key is already provided via config/env, just use it if cfg.EncryptionKey != "" { log.Println("Encryption key loaded from environment") return } - toolkitCrypto := security.Crypto{} + store := newSecretStore(cfg) - // Try to initialize secret store client for encryption key retrieval - remoteStorage, err := handleSecretsConfig(cfg) + key, err := store.get(encryptionKeyStorageKey) if err != nil { - remoteStorage = nil - } - - // Try remote storage first - if done := tryRemoteStorage(cfg, remoteStorage); done { - return - } - - // Try local keyring storage - localStorage := security.NewKeyRingStorage("device-management-toolkit") - - if done := tryLocalStorage(cfg, localStorage, remoteStorage); done { - return - } - - // Key not found anywhere, generate a new one - cfg.EncryptionKey = handleKeyNotFound(toolkitCrypto, remoteStorage, localStorage) - - if err := saveEncryptionKey(cfg.EncryptionKey, remoteStorage, localStorage); err != nil { - log.Printf("Warning: Failed to save encryption key: %v", err) - } -} - -// tryRemoteStorage attempts to store/retrieve the encryption key from remote storage. -func tryRemoteStorage(cfg *config.Config, remoteStorage security.Storager) bool { - if remoteStorage == nil { - return false - } - - if cfg.EncryptionKey != "" { - // Store static key in secret store (not recommended) - if err := remoteStorage.SetKeyValue("default-security-key", cfg.EncryptionKey); err == nil { - log.Println("Encryption key stored in secret store") - - return true - } - } else { - // Retrieve from secret store - key, err := remoteStorage.GetKeyValue("default-security-key") - if err == nil { - cfg.EncryptionKey = key - - log.Println("Encryption key loaded from secret store") - - return true - } - } - - return false -} - -// tryLocalStorage attempts to store/retrieve the encryption key from local keyring. -func tryLocalStorage(cfg *config.Config, localStorage, remoteStorage security.Storager) bool { - var err error - - if cfg.EncryptionKey != "" { - err = localStorage.SetKeyValue("default-security-key", cfg.EncryptionKey) - if err == nil { - log.Println("Encryption key stored in local keyring") - - return true - } - } else { - cfg.EncryptionKey, err = localStorage.GetKeyValue("default-security-key") - if err == nil { - log.Println("Encryption key loaded from local keyring") - syncKeyToRemote(cfg.EncryptionKey, remoteStorage) - - return true - } - } - - // Check for unexpected errors - if err != nil && !errors.Is(err, security.ErrKeyNotFound) { log.Fatalf( "Local keyring unavailable (%v).\n"+ "Set APP_ENCRYPTION_KEY in the environment (or encryption_key in config) "+ @@ -227,45 +162,23 @@ func tryLocalStorage(cfg *config.Config, localStorage, remoteStorage security.St ) } - return false -} - -// syncKeyToRemote syncs an encryption key to the remote storage if available. -func syncKeyToRemote(key string, remoteStorage security.Storager) { - if remoteStorage == nil { - return - } - - if err := remoteStorage.SetKeyValue("default-security-key", key); err != nil { - log.Printf("Warning: Failed to sync key to secret store: %v", err) - } else { - log.Println("Encryption key synced to secret store") - } -} - -func saveEncryptionKey(key string, remoteStorage, localStorage security.Storager) error { - if remoteStorage != nil { - err := remoteStorage.SetKeyValue("default-security-key", key) - if err == nil { - log.Println("Encryption key saved to secret store") + if key != "" { + cfg.EncryptionKey = key - return nil - } + log.Println("Encryption key loaded from secure storage") - return err + return } - err := localStorage.SetKeyValue("default-security-key", key) - if err == nil { - log.Println("Encryption key saved to local keyring") + // Not found anywhere: prompt (losing this key prevents access to existing data), then store. + cfg.EncryptionKey = handleKeyNotFound() - return nil + if err := store.set(encryptionKeyStorageKey, cfg.EncryptionKey); err != nil { + log.Printf("Warning: Failed to save encryption key: %v", err) } - - return err } -func handleKeyNotFound(toolkitCrypto security.Crypto, _, _ security.Storager) string { +func handleKeyNotFound() string { log.Print("\033[31mWarning: Key Not Found, Generate new key? -- This will prevent access to existing data? Y/N: \033[0m") var response string @@ -283,7 +196,7 @@ func handleKeyNotFound(toolkitCrypto security.Crypto, _, _ security.Storager) st return "" } - return toolkitCrypto.GenerateKey() + return security.Crypto{}.GenerateKey() } // generateRandomPassword creates a cryptographically secure random password. @@ -325,3 +238,73 @@ func handleAdminPassword(cfg *config.Config) { log.Printf("Generated new admin password and persisted to config; see auth.adminPassword in config.yml.") } + +// handleJWTKey resolves the JWT key: env/config, else secret store / keyring, else generate. +// Rejects the insecure default; never writes the key to config.yml. +func handleJWTKey(cfg *config.Config) { + if cfg.JWTKey == insecureDefaultJWTKey { + log.Fatalf("insecure default JWT key; unset auth.jwtKey or set AUTH_JWT_KEY to a strong value") + } + + if cfg.JWTKey != "" { + log.Println("JWT signing key loaded from environment/config") + + return + } + + store := newSecretStore(cfg) + + // Losing the JWT key is harmless (tokens re-issue), so warn on storage errors rather than fatal. + key, err := store.get(jwtKeyStorageKey) + if err != nil { + log.Printf("Warning: keyring unavailable for JWT signing key: %v", err) + } + + if key != "" { + cfg.JWTKey = key + + log.Println("JWT signing key loaded from secure storage") + + return + } + + generated, genErr := generateJWTKey() + if genErr != nil { + log.Fatalf("Failed to generate JWT signing key: %v", genErr) + } + + cfg.JWTKey = generated + + if saveErr := store.set(jwtKeyStorageKey, cfg.JWTKey); saveErr != nil { + log.Printf("Warning: generated JWT signing key but could not persist it (%v); "+ + "a new key will be generated on restart", saveErr) + } else { + log.Println("Generated and stored new JWT signing key") + } + + if valErr := validateJWTKey(cfg.JWTKey); valErr != nil { + log.Fatalf("JWT signing key unusable: %v", valErr) + } +} + +// validateJWTKey rejects an empty or known-insecure signing key. +func validateJWTKey(key string) error { + switch key { + case "": + return ErrJWTKeyMissing + case insecureDefaultJWTKey: + return ErrJWTKeyInsecure + default: + return nil + } +} + +// generateJWTKey returns a base64-encoded 256-bit random key. +func generateJWTKey() (string, error) { + b := make([]byte, jwtKeyBytes) + if _, err := rand.Read(b); err != nil { + return "", err + } + + return base64.StdEncoding.EncodeToString(b), nil +} diff --git a/cmd/app/main_test.go b/cmd/app/main_test.go index be738baa1..af104de2a 100644 --- a/cmd/app/main_test.go +++ b/cmd/app/main_test.go @@ -3,6 +3,7 @@ package main import ( "crypto/rsa" "crypto/x509" + "encoding/base64" "os" "testing" @@ -101,3 +102,66 @@ func TestHandleAdminPassword_AlreadyConfigured(t *testing.T) { assert.Equal(t, "already-set", cfg.AdminPassword) } + +// TestGenerateJWTKey checks the key is a decodable 256-bit random value. +func TestGenerateJWTKey(t *testing.T) { + t.Parallel() + + key, err := generateJWTKey() + require.NoError(t, err) + + decoded, err := base64.StdEncoding.DecodeString(key) + require.NoError(t, err) + assert.Len(t, decoded, jwtKeyBytes) +} + +// TestGenerateJWTKey_Uniqueness ensures generated keys are unique. +func TestGenerateJWTKey_Uniqueness(t *testing.T) { + t.Parallel() + + keys := make(map[string]bool) + + for range 100 { + key, err := generateJWTKey() + require.NoError(t, err) + assert.False(t, keys[key], "generated duplicate JWT key") + + keys[key] = true + } +} + +// TestValidateJWTKey covers empty, known-insecure, and valid keys. +func TestValidateJWTKey(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + key string + wantErr error + }{ + {"empty", "", ErrJWTKeyMissing}, + {"insecure default", insecureDefaultJWTKey, ErrJWTKeyInsecure}, + {"valid", "a-strong-random-key", nil}, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + + assert.ErrorIs(t, validateJWTKey(tc.key), tc.wantErr) + }) + } +} + +// TestHandleJWTKey_AlreadyConfigured keeps an operator-supplied key untouched. +func TestHandleJWTKey_AlreadyConfigured(t *testing.T) { + t.Parallel() + + cfg := &config.Config{ + Auth: config.Auth{JWTKey: "operator-supplied-key"}, + } + + handleJWTKey(cfg) + + assert.Equal(t, "operator-supplied-key", cfg.JWTKey) +} diff --git a/cmd/app/secretstore.go b/cmd/app/secretstore.go new file mode 100644 index 000000000..23735e90b --- /dev/null +++ b/cmd/app/secretstore.go @@ -0,0 +1,63 @@ +package main + +import ( + "errors" + "log" + + "github.com/device-management-toolkit/go-wsman-messages/v2/pkg/security" + + "github.com/device-management-toolkit/console/config" +) + +// secretStore resolves named secrets from a remote store (Vault) with an OS keyring fallback. +type secretStore struct { + remote security.Storager + local security.Storager +} + +// newSecretStore builds a store from the optional Vault client and the local keyring. +func newSecretStore(cfg *config.Config) *secretStore { + remote, err := handleSecretsConfig(cfg) + if err != nil { + remote = nil + } + + return &secretStore{remote: remote, local: security.NewKeyRingStorage(keyringServiceName)} +} + +// get returns the secret for name, preferring the remote store and falling back to the +// keyring (syncing a keyring-only value back to remote). Returns "" when absent; a non-nil +// error is a real storage failure the caller must decide how to handle. +func (s *secretStore) get(name string) (string, error) { + if s.remote != nil { + if v, err := s.remote.GetKeyValue(name); err == nil && v != "" { + return v, nil + } + } + + v, err := s.local.GetKeyValue(name) + if err != nil { + if errors.Is(err, security.ErrKeyNotFound) { + return "", nil + } + + return "", err + } + + if v != "" && s.remote != nil { + if syncErr := s.remote.SetKeyValue(name, v); syncErr != nil { + log.Printf("Warning: failed to sync %s to secret store: %v", name, syncErr) + } + } + + return v, nil +} + +// set stores value under name in the remote store when available, else the keyring. +func (s *secretStore) set(name, value string) error { + if s.remote != nil { + return s.remote.SetKeyValue(name, value) + } + + return s.local.SetKeyValue(name, value) +} diff --git a/cmd/app/secretstore_test.go b/cmd/app/secretstore_test.go new file mode 100644 index 000000000..a6e0ab5a5 --- /dev/null +++ b/cmd/app/secretstore_test.go @@ -0,0 +1,127 @@ +package main + +import ( + "errors" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/device-management-toolkit/go-wsman-messages/v2/pkg/security" +) + +var errStorage = errors.New("storage boom") + +// fakeStorage is an in-memory security.Storager for testing secretStore. +type fakeStorage struct { + data map[string]string + getErr error + setErr error +} + +func newFakeStorage() *fakeStorage { return &fakeStorage{data: map[string]string{}} } + +func (f *fakeStorage) GetKeyValue(key string) (string, error) { + if f.getErr != nil { + return "", f.getErr + } + + v, ok := f.data[key] + if !ok { + return "", security.ErrKeyNotFound + } + + return v, nil +} + +func (f *fakeStorage) SetKeyValue(key, value string) error { + if f.setErr != nil { + return f.setErr + } + + f.data[key] = value + + return nil +} + +func (f *fakeStorage) DeleteKeyValue(key string) error { + delete(f.data, key) + + return nil +} + +func TestSecretStoreGet_RemoteHit(t *testing.T) { + t.Parallel() + + remote := newFakeStorage() + remote.data["k"] = "remote-val" + local := newFakeStorage() + local.data["k"] = "local-val" + + s := &secretStore{remote: remote, local: local} + + v, err := s.get("k") + require.NoError(t, err) + assert.Equal(t, "remote-val", v) +} + +func TestSecretStoreGet_LocalFallbackSyncsToRemote(t *testing.T) { + t.Parallel() + + remote := newFakeStorage() // empty + local := newFakeStorage() + local.data["k"] = "local-val" + + s := &secretStore{remote: remote, local: local} + + v, err := s.get("k") + require.NoError(t, err) + assert.Equal(t, "local-val", v) + assert.Equal(t, "local-val", remote.data["k"], "keyring-only value should sync to remote") +} + +func TestSecretStoreGet_NotFound(t *testing.T) { + t.Parallel() + + s := &secretStore{remote: nil, local: newFakeStorage()} + + v, err := s.get("missing") + require.NoError(t, err) + assert.Empty(t, v) +} + +func TestSecretStoreGet_RealKeyringError(t *testing.T) { + t.Parallel() + + local := newFakeStorage() + local.getErr = errStorage + + s := &secretStore{remote: nil, local: local} + + _, err := s.get("k") + require.ErrorIs(t, err, errStorage) +} + +func TestSecretStoreSet_PrefersRemote(t *testing.T) { + t.Parallel() + + remote := newFakeStorage() + local := newFakeStorage() + + s := &secretStore{remote: remote, local: local} + + require.NoError(t, s.set("k", "v")) + assert.Equal(t, "v", remote.data["k"]) + assert.Empty(t, local.data["k"], "should not write keyring when remote is available") +} + +func TestSecretStoreSet_FallsBackToLocal(t *testing.T) { + t.Parallel() + + local := newFakeStorage() + + s := &secretStore{remote: nil, local: local} + + require.NoError(t, s.set("k", "v")) + assert.Equal(t, "v", local.data["k"]) +} diff --git a/config/config.go b/config/config.go index 74f238dfd..b166c7611 100644 --- a/config/config.go +++ b/config/config.go @@ -94,7 +94,7 @@ type ( Disabled bool `yaml:"disabled" env:"AUTH_DISABLED"` AdminUsername string `yaml:"adminUsername" env:"AUTH_ADMIN_USERNAME"` AdminPassword string `yaml:"adminPassword" env:"AUTH_ADMIN_PASSWORD"` - JWTKey string `env-required:"true" yaml:"jwtKey" env:"AUTH_JWT_KEY"` + JWTKey string `yaml:"jwtKey" env:"AUTH_JWT_KEY"` JWTExpiration time.Duration `yaml:"jwtExpiration" env:"AUTH_JWT_EXPIRATION"` RedirectionJWTExpiration time.Duration `yaml:"redirectionJWTExpiration" env:"AUTH_REDIRECTION_JWT_EXPIRATION"` ClientID string `yaml:"clientId" env:"AUTH_CLIENT_ID"` @@ -187,7 +187,7 @@ func defaultConfig() *Config { Auth: Auth{ AdminUsername: "standalone", AdminPassword: "", // Generated and stored in config on first run if not provided - JWTKey: "your_secret_jwt_key", + JWTKey: "", // resolved/generated at startup JWTExpiration: 24 * time.Hour, RedirectionJWTExpiration: 5 * time.Minute, // OAUTH CONFIG, if provided will not use basic auth @@ -247,19 +247,30 @@ func readOrInitConfig(configPath string, cfg *Config) error { return err } +// Owner-only permissions: config.yml persists the admin password in plaintext. +const ( + configDirPerm = 0o700 + configFilePerm = 0o600 +) + // writeConfig serializes cfg to configPath, creating the parent directory if needed. func writeConfig(configPath string, cfg *Config) error { configDir := filepath.Dir(configPath) - if mkErr := os.MkdirAll(configDir, os.ModePerm); mkErr != nil { + if mkErr := os.MkdirAll(configDir, configDirPerm); mkErr != nil { return mkErr } - file, err := os.Create(configPath) + file, err := os.OpenFile(configPath, os.O_WRONLY|os.O_CREATE|os.O_TRUNC, configFilePerm) if err != nil { return err } defer file.Close() + // Enforce perms even if the file pre-existed with looser bits. + if chmodErr := os.Chmod(configPath, configFilePerm); chmodErr != nil { + return chmodErr + } + encoder := yaml.NewEncoder(file) defer encoder.Close() diff --git a/config/config.yml b/config/config.yml index 782bd309e..90804906f 100644 --- a/config/config.yml +++ b/config/config.yml @@ -39,7 +39,7 @@ auth: disabled: false adminUsername: standalone adminPassword: - jwtKey: your_secret_jwt_key + jwtKey: jwtExpiration: 24h0m0s redirectionJWTExpiration: 5m0s clientId: "" diff --git a/config/config_test.go b/config/config_test.go index 24e4360e7..fb2f39b4b 100644 --- a/config/config_test.go +++ b/config/config_test.go @@ -40,6 +40,9 @@ func TestNewConfig_Defaults(t *testing.T) { //nolint:paralleltest // cannot have assert.Equal(t, "info", cfg.Level) assert.Equal(t, 2, cfg.PoolMax) + + // JWT key has no hardcoded default; it is resolved/generated at startup. + assert.Equal(t, "", cfg.JWTKey) } func TestNewConfig_EnvVars(t *testing.T) { //nolint:paralleltest // cannot have simultaneous tests modifying environment variables