diff --git a/cmd/app/main.go b/cmd/app/main.go index 8cb9b4234..0b34e1b0a 100644 --- a/cmd/app/main.go +++ b/cmd/app/main.go @@ -11,6 +11,8 @@ import ( "strings" "unicode/utf8" + "golang.org/x/crypto/bcrypt" + "github.com/device-management-toolkit/go-wsman-messages/v2/pkg/security" "github.com/device-management-toolkit/console/config" @@ -414,33 +416,103 @@ func shufflePassword(password []byte) error { } // handleAdminPassword ensures cfg.AdminPassword is set, generating one and -// persisting it to config.yml on first run if nothing was provided via config -// or environment. +// persisting it as a bcrypt hash to config.yml on first run if nothing was +// provided via config or environment. func handleAdminPassword(cfg *config.Config) { - if cfg.AdminPassword != "" { - warnOnWeakAdminPassword(cfg.AdminPassword) + if cfg.AdminPassword == "" { + password, err := generateRandomPassword(adminPasswordLength) + if err != nil { + log.Fatalf("Failed to generate admin password: %v", err) + } + + hashedPassword, err := hashAdminPassword(password) + if err != nil { + log.Fatalf("Failed to hash generated admin password: %v", err) + } + + cfg.AdminPassword = hashedPassword + + if err := config.SaveAdminPassword(cfg.AdminPassword); err != nil { + log.Fatalf( + "Generated admin password but failed to persist it to config (%v).\n"+ + "Refusing to start with an unsaved credential that would vanish on restart.\n"+ + "Set AUTH_ADMIN_PASSWORD in the environment (or auth.adminPassword in config) "+ + "to provide the admin password directly.", + err, + ) + } + + log.Printf( + "Generated a new admin password. It is shown here once and cannot be "+ + "recovered later, because only its bcrypt hash is written to "+ + "auth.adminPassword in config.yml:\n\n %s\n\n"+ + "Store it now, or set AUTH_ADMIN_PASSWORD to supply your own.", + password, + ) return } - password, err := generateRandomPassword(adminPasswordLength) + originalPassword := cfg.AdminPassword + + hashedPassword, converted, err := normalizeAdminPasswordHash(cfg.AdminPassword) if err != nil { - log.Fatalf("Failed to generate admin password: %v", err) + log.Fatalf("Failed to normalize admin password: %v", err) } - cfg.AdminPassword = password + cfg.AdminPassword = hashedPassword - if err := config.SaveAdminPassword(cfg.AdminPassword); err != nil { - log.Fatalf( - "Generated admin password but failed to persist it to config (%v).\n"+ - "Refusing to start with an unsaved credential that would vanish on restart.\n"+ - "Set AUTH_ADMIN_PASSWORD in the environment (or auth.adminPassword in config) "+ - "to provide the admin password directly.", - err, - ) + if converted { + // The config file may be read-only (password supplied via env or a mounted + // secret); the in-memory hash still authenticates this run. + if err := config.SaveAdminPassword(cfg.AdminPassword); err != nil { + log.Printf( + "WARNING: could not persist the hashed admin password to config (%v). "+ + "Console is starting with the in-memory hash; the plaintext password "+ + "will be re-hashed on every restart.", + err, + ) + } + } + + if !isBcryptHash(originalPassword) { + warnOnWeakAdminPassword(originalPassword) } +} + +func hashAdminPassword(password string) (string, error) { + hash, err := bcrypt.GenerateFromPassword([]byte(password), bcrypt.DefaultCost) + if err != nil { + return "", err + } + + return string(hash), nil +} + +func normalizeAdminPasswordHash(password string) (normalized string, converted bool, err error) { + if password == "" { + return "", false, nil + } + + if isBcryptHash(password) { + return password, false, nil + } + + normalized, err = hashAdminPassword(password) + if err != nil { + return "", false, err + } + + return normalized, true, nil +} + +// isBcryptHash reports whether s is already a bcrypt hash. Parsing the cost is +// stricter than a prefix check, so a plaintext password that happens to start +// with "$2a$" is not mistaken for a hash and left unhashed. +func isBcryptHash(s string) bool { + _, err := bcrypt.Cost([]byte(s)) - log.Printf("Generated new admin password and persisted to config; see auth.adminPassword in config.yml.") + return err == nil } // warnOnWeakAdminPassword warns but does not stop startup: migrated MPS/RPS diff --git a/cmd/app/main_test.go b/cmd/app/main_test.go index c66f4af00..176c40378 100644 --- a/cmd/app/main_test.go +++ b/cmd/app/main_test.go @@ -4,13 +4,17 @@ import ( "bytes" "crypto/rsa" "crypto/x509" + "flag" "log" "os" + "path/filepath" + "regexp" "strings" "testing" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + "golang.org/x/crypto/bcrypt" "github.com/device-management-toolkit/go-wsman-messages/v2/pkg/security" @@ -171,7 +175,8 @@ func TestHandleAdminPassword_AlreadyConfigured(t *testing.T) { handleAdminPassword(cfg) - assert.Equal(t, "already-set", cfg.AdminPassword) + assert.True(t, isBcryptHash(cfg.AdminPassword)) + assert.NoError(t, bcrypt.CompareHashAndPassword([]byte(cfg.AdminPassword), []byte("already-set"))) } func TestIsStrongAdminPassword(t *testing.T) { @@ -270,6 +275,137 @@ func TestHandleAdminPassword_WeakConfiguredPasswordStillStarts(t *testing.T) { / handleAdminPassword(cfg) - assert.Equal(t, "weak", cfg.AdminPassword) + assert.True(t, isBcryptHash(cfg.AdminPassword)) + assert.NoError(t, bcrypt.CompareHashAndPassword([]byte(cfg.AdminPassword), []byte("weak"))) assert.Contains(t, buf.String(), "Console is starting anyway") } + +func TestHandleAdminPassword_GeneratesAndPersistsWhenUnset(t *testing.T) { //nolint:paralleltest // rebinds the global log output and config flag + var buf bytes.Buffer + + orig := log.Writer() + + log.SetOutput(&buf) + t.Cleanup(func() { log.SetOutput(orig) }) + + configPath := filepath.Join(t.TempDir(), "config.yml") + + if flag.Lookup("config") == nil { + flag.String("config", "", "path to config file") + } + + prev := flag.Lookup("config").Value.String() + + require.NoError(t, flag.Set("config", configPath)) + t.Cleanup(func() { _ = flag.Set("config", prev) }) + + cfg := &config.Config{} + + handleAdminPassword(cfg) + + assert.True(t, isBcryptHash(cfg.AdminPassword), "generated password must be stored as a hash") + + // The operator can only ever learn the generated password from this output, + // so it must be printed and must match the stored hash. + shown := regexp.MustCompile(`\n\n {4}(\S+)\n\n`).FindStringSubmatch(buf.String()) + require.Len(t, shown, 2, "generated password must be shown once: %s", buf.String()) + assert.NoError(t, bcrypt.CompareHashAndPassword([]byte(cfg.AdminPassword), []byte(shown[1]))) + + saved, err := os.ReadFile(configPath) + require.NoError(t, err) + assert.Contains(t, string(saved), cfg.AdminPassword, "hash must be persisted so it survives restart") + assert.NotContains(t, string(saved), shown[1], "plaintext must never be written to config") +} + +func TestHandleAdminPassword_KeepsExistingHashUnchanged(t *testing.T) { + t.Parallel() + + hash, err := bcrypt.GenerateFromPassword([]byte("P@ssw0rdd"), bcrypt.DefaultCost) + require.NoError(t, err) + + cfg := &config.Config{Auth: config.Auth{AdminPassword: string(hash)}} + + handleAdminPassword(cfg) + + assert.Equal(t, string(hash), cfg.AdminPassword, "an already-hashed password must not be re-hashed") +} + +func TestHandleAdminPassword_StartsWhenConfigIsNotWritable(t *testing.T) { //nolint:paralleltest // rebinds the global log output and config flag + var buf bytes.Buffer + + orig := log.Writer() + + log.SetOutput(&buf) + t.Cleanup(func() { log.SetOutput(orig) }) + + // A path under a regular file cannot be written, standing in for a read-only + // config or a password supplied entirely via the environment. + blocker := filepath.Join(t.TempDir(), "not-a-dir") + require.NoError(t, os.WriteFile(blocker, []byte("x"), 0o600)) + + if flag.Lookup("config") == nil { + flag.String("config", "", "path to config file") + } + + prev := flag.Lookup("config").Value.String() + + require.NoError(t, flag.Set("config", filepath.Join(blocker, "config.yml"))) + t.Cleanup(func() { _ = flag.Set("config", prev) }) + + cfg := &config.Config{Auth: config.Auth{AdminPassword: "P@ssw0rdd"}} + + handleAdminPassword(cfg) + + assert.NoError(t, bcrypt.CompareHashAndPassword([]byte(cfg.AdminPassword), []byte("P@ssw0rdd")), + "startup must continue with the in-memory hash") + assert.Contains(t, buf.String(), "could not persist the hashed admin password") +} + +func TestNormalizeAdminPasswordHash(t *testing.T) { + t.Parallel() + + t.Run("empty stays empty", func(t *testing.T) { + t.Parallel() + + got, converted, err := normalizeAdminPasswordHash("") + + require.NoError(t, err) + assert.False(t, converted) + assert.Empty(t, got) + }) + + t.Run("plaintext beginning with a bcrypt prefix is still hashed", func(t *testing.T) { + t.Parallel() + + plaintext := "$2a$notarealhash" + + got, converted, err := normalizeAdminPasswordHash(plaintext) + + require.NoError(t, err) + assert.True(t, converted, "a prefix alone must not be treated as a hash") + assert.NoError(t, bcrypt.CompareHashAndPassword([]byte(got), []byte(plaintext))) + }) + + t.Run("plaintext is hashed", func(t *testing.T) { + t.Parallel() + + got, converted, err := normalizeAdminPasswordHash("P@ssw0rdd") + + require.NoError(t, err) + assert.True(t, converted) + assert.NoError(t, bcrypt.CompareHashAndPassword([]byte(got), []byte("P@ssw0rdd"))) + }) + + t.Run("existing hash is returned as is", func(t *testing.T) { + t.Parallel() + + hash, err := bcrypt.GenerateFromPassword([]byte("P@ssw0rdd"), bcrypt.DefaultCost) + require.NoError(t, err) + + got, converted, err := normalizeAdminPasswordHash(string(hash)) + + require.NoError(t, err) + assert.False(t, converted) + assert.Equal(t, string(hash), got) + }) +} diff --git a/config/config.go b/config/config.go index f6982229b..e8fb795a6 100644 --- a/config/config.go +++ b/config/config.go @@ -392,14 +392,19 @@ func SaveAdminPassword(adminPassword string) error { return err } - data, err := os.ReadFile(configPath) - if err != nil { - return err - } - fileCfg := defaultConfig() - if err := yaml.Unmarshal(data, fileCfg); err != nil { - return err + + if _, statErr := os.Stat(configPath); statErr == nil { + data, readErr := os.ReadFile(configPath) + if readErr != nil { + return readErr + } + + if unmarshalErr := yaml.Unmarshal(data, fileCfg); unmarshalErr != nil { + return unmarshalErr + } + } else if !errors.Is(statErr, os.ErrNotExist) { + return statErr } fileCfg.AdminPassword = adminPassword diff --git a/config/config_test.go b/config/config_test.go index c48e4ac78..8c2d219b0 100644 --- a/config/config_test.go +++ b/config/config_test.go @@ -1,6 +1,7 @@ package config import ( + "flag" "os" "path/filepath" "runtime" @@ -9,6 +10,7 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + "gopkg.in/yaml.v3" ) func clearEnv() { @@ -522,3 +524,109 @@ func TestValidate_ValidDefaults(t *testing.T) { err := cfg.validate() require.NoError(t, err) } + +// pointConfigFlagAt redirects SaveAdminPassword to a throwaway path. +func pointConfigFlagAt(t *testing.T, path string) { + t.Helper() + + if flag.Lookup("config") == nil { + flag.String("config", "", "path to config file") + } + + f := flag.Lookup("config") + orig := f.Value.String() + + require.NoError(t, flag.Set("config", path)) + t.Cleanup(func() { _ = flag.Set("config", orig) }) +} + +func TestSaveAdminPassword_CreatesConfigWhenMissing(t *testing.T) { //nolint:paralleltest // mutates the global config flag + configPath := filepath.Join(t.TempDir(), "nested", "config.yml") + pointConfigFlagAt(t, configPath) + + require.NoError(t, SaveAdminPassword("$2a$10$hashedvalue")) + + data, err := os.ReadFile(configPath) + require.NoError(t, err) + + var written Config + + require.NoError(t, yaml.Unmarshal(data, &written)) + assert.Equal(t, "$2a$10$hashedvalue", written.AdminPassword) +} + +func TestSaveAdminPassword_PreservesOtherFileValues(t *testing.T) { //nolint:paralleltest // mutates the global config flag + configPath := filepath.Join(t.TempDir(), "config.yml") + pointConfigFlagAt(t, configPath) + + existing := defaultConfig() + existing.Port = "9999" + existing.AdminPassword = "stale" + + data, err := yaml.Marshal(existing) + require.NoError(t, err) + require.NoError(t, os.WriteFile(configPath, data, configFilePerm)) + + require.NoError(t, SaveAdminPassword("$2a$10$replacement")) + + saved, err := os.ReadFile(configPath) + require.NoError(t, err) + + var written Config + + require.NoError(t, yaml.Unmarshal(saved, &written)) + assert.Equal(t, "$2a$10$replacement", written.AdminPassword) + assert.Equal(t, "9999", written.Port) +} + +func TestSaveAdminPassword_UnreadableConfigReturnsError(t *testing.T) { //nolint:paralleltest // mutates the global config flag + configPath := filepath.Join(t.TempDir(), "config.yml") + pointConfigFlagAt(t, configPath) + + require.NoError(t, os.WriteFile(configPath, []byte("\tnot: [valid"), configFilePerm)) + + require.Error(t, SaveAdminPassword("$2a$10$hashedvalue")) +} + +func TestSaveAdminPassword_ReadFailureReturnsError(t *testing.T) { //nolint:paralleltest // mutates the global config flag + if runtime.GOOS == goosWindows { + t.Skip("chmod-based read denial does not apply on Windows") + } + + if os.Geteuid() == 0 { + t.Skip("root bypasses file permission checks") + } + + configPath := filepath.Join(t.TempDir(), "config.yml") + pointConfigFlagAt(t, configPath) + + require.NoError(t, os.WriteFile(configPath, []byte("auth:\n"), configFilePerm)) + require.NoError(t, os.Chmod(configPath, 0o000)) + + require.Error(t, SaveAdminPassword("$2a$10$hashedvalue")) +} + +func TestSaveAdminPassword_StatFailureReturnsError(t *testing.T) { //nolint:paralleltest // mutates the global config flag + notADir := filepath.Join(t.TempDir(), "regular-file") + require.NoError(t, os.WriteFile(notADir, []byte("x"), configFilePerm)) + + // Stat fails with ENOTDIR rather than ErrNotExist, so the error must surface. + pointConfigFlagAt(t, filepath.Join(notADir, "config.yml")) + + require.Error(t, SaveAdminPassword("$2a$10$hashedvalue")) +} + +func TestSaveAdminPassword_KeepsFileOwnerOnly(t *testing.T) { //nolint:paralleltest // mutates the global config flag + if runtime.GOOS == goosWindows { + t.Skip("POSIX file modes are not enforced on Windows") + } + + configPath := filepath.Join(t.TempDir(), "config.yml") + pointConfigFlagAt(t, configPath) + + require.NoError(t, SaveAdminPassword("$2a$10$hashedvalue")) + + info, err := os.Stat(configPath) + require.NoError(t, err) + assert.Equal(t, configFilePerm, info.Mode().Perm()) +} diff --git a/internal/controller/httpapi/v1/login.go b/internal/controller/httpapi/v1/login.go index b727b2dd3..0bff68c5c 100644 --- a/internal/controller/httpapi/v1/login.go +++ b/internal/controller/httpapi/v1/login.go @@ -2,6 +2,7 @@ package v1 import ( "context" + "crypto/subtle" "crypto/tls" "errors" "fmt" @@ -12,6 +13,7 @@ import ( "github.com/coreos/go-oidc/v3/oidc" "github.com/gin-gonic/gin" "github.com/golang-jwt/jwt/v5" + "golang.org/x/crypto/bcrypt" "github.com/device-management-toolkit/console/config" "github.com/device-management-toolkit/console/internal/entity/dto/v1" @@ -83,7 +85,10 @@ func (lr LoginRoute) Login(c *gin.Context) { } func (lr LoginRoute) handleBasicAuth(creds dto.Credentials, c *gin.Context) { - if creds.Username != lr.Config.AdminUsername || creds.Password != lr.Config.AdminPassword { + usernameMatches := subtle.ConstantTimeCompare([]byte(creds.Username), []byte(lr.Config.AdminUsername)) == 1 + passwordMatches := bcrypt.CompareHashAndPassword([]byte(lr.Config.AdminPassword), []byte(creds.Password)) == nil + + if !usernameMatches || !passwordMatches { c.JSON(http.StatusUnauthorized, gin.H{errorKey: "invalid credentials", messageKey: "Incorrect Username and/or Password!"}) return diff --git a/internal/controller/httpapi/v1/login_test.go b/internal/controller/httpapi/v1/login_test.go index 1d25d7710..248db2ba7 100644 --- a/internal/controller/httpapi/v1/login_test.go +++ b/internal/controller/httpapi/v1/login_test.go @@ -10,6 +10,7 @@ import ( "github.com/gin-gonic/gin" "github.com/stretchr/testify/require" + "golang.org/x/crypto/bcrypt" "github.com/device-management-toolkit/console/config" ) @@ -26,9 +27,14 @@ const ( // cookieAuthTestConfig is a basic-auth (non-OIDC) config with cookies enabled. func cookieAuthTestConfig() *config.Config { + hash, err := bcrypt.GenerateFromPassword([]byte(testAdminPass), bcrypt.DefaultCost) + if err != nil { + panic(err) + } + cfg := &config.Config{} cfg.AdminUsername = testAdminUser - cfg.AdminPassword = testAdminPass + cfg.AdminPassword = string(hash) cfg.JWTKey = testJWTKey cfg.JWTExpiration = time.Hour cfg.CookieEnabled = true diff --git a/internal/controller/tcp/cira/handler.go b/internal/controller/tcp/cira/handler.go index 02f68dd75..70fb92b42 100644 --- a/internal/controller/tcp/cira/handler.go +++ b/internal/controller/tcp/cira/handler.go @@ -2,6 +2,7 @@ package cira import ( "context" + "crypto/subtle" "strings" "github.com/device-management-toolkit/go-wsman-messages/v2/pkg/apf" @@ -98,17 +99,13 @@ func (h *APFHandler) validateCredentials(username, password string) bool { return false } - // Compare credentials - // MPSUsername is the field used for CIRA authentication - if device.MPSUsername != username { - h.log.Debug("Username mismatch for device %s", h.deviceID) + // Both comparisons always run so the response time does not reveal which + // field failed. MPSUsername is the field used for CIRA authentication. + usernameMatches := subtle.ConstantTimeCompare([]byte(device.MPSUsername), []byte(username)) + passwordMatches := subtle.ConstantTimeCompare([]byte(device.MPSPassword), []byte(password)) - return false - } - - // Compare password - if device.MPSPassword != password { - h.log.Debug("Password mismatch for device %s", h.deviceID) + if usernameMatches&passwordMatches != 1 { + h.log.Debug("Credential mismatch for device %s", h.deviceID) return false } diff --git a/internal/controller/tcp/cira/tunnel_test.go b/internal/controller/tcp/cira/tunnel_test.go index 6d299e47e..2f9ab6c28 100644 --- a/internal/controller/tcp/cira/tunnel_test.go +++ b/internal/controller/tcp/cira/tunnel_test.go @@ -12,6 +12,7 @@ import ( "github.com/device-management-toolkit/go-wsman-messages/v2/pkg/apf" + "github.com/device-management-toolkit/console/internal/entity/dto/v1" "github.com/device-management-toolkit/console/internal/mocks" "github.com/device-management-toolkit/console/internal/usecase/devices" "github.com/device-management-toolkit/console/internal/usecase/devices/wsman" @@ -219,6 +220,86 @@ func verifyConnectionRemoved(t *testing.T, authenticated bool, deviceID string) } } +func TestAPFHandler_ValidatesCredentialsAndProtocolVersion(t *testing.T) { + t.Parallel() + + ctrl := gomock.NewController(t) + mockDevices := mocks.NewMockDeviceManagementFeature(ctrl) + mockDevices.EXPECT().GetByID(gomock.Any(), "device-123", "", true).Return(&dto.Device{ + MPSUsername: "admin", + MPSPassword: "s3cret", + }, nil).AnyTimes() + + handler := NewAPFHandler(mockDevices, logger.New("debug")) + require.NoError(t, handler.OnProtocolVersion(apf.ProtocolVersionInfo{UUID: "DEVICE-123"})) + assert.Equal(t, "device-123", handler.DeviceID()) + + assert.True(t, handler.validateCredentials("admin", "s3cret")) + assert.False(t, handler.validateCredentials("admin", "wrong-pass")) + assert.False(t, handler.validateCredentials("wrong-user", "s3cret")) + assert.False(t, handler.validateCredentials("wrong-user", "wrong-pass")) + assert.False(t, handler.validateCredentials("", "")) + + resp := handler.OnAuthRequest(apf.AuthRequest{Username: "admin", Password: "s3cret", MethodName: "password"}) + assert.True(t, resp.Authenticated) + + unsupported := handler.OnAuthRequest(apf.AuthRequest{Username: "admin", Password: "s3cret", MethodName: "keyboard-interactive"}) + assert.False(t, unsupported.Authenticated) +} + +func TestAPFHandler_validateCredentials_FailurePaths(t *testing.T) { + t.Parallel() + + t.Run("device id not set", func(t *testing.T) { + t.Parallel() + + handler := NewAPFHandler(nil, logger.New("error")) + + assert.False(t, handler.validateCredentials("admin", "s3cret")) + }) + + t.Run("lookup error", func(t *testing.T) { + t.Parallel() + + ctrl := gomock.NewController(t) + mockDevices := mocks.NewMockDeviceManagementFeature(ctrl) + mockDevices.EXPECT().GetByID(gomock.Any(), "dev-err", "", true).Return(nil, errors.New("db error")) + + handler := NewAPFHandler(mockDevices, logger.New("error")) + handler.deviceID = "dev-err" + + assert.False(t, handler.validateCredentials("admin", "s3cret")) + }) + + t.Run("device not found", func(t *testing.T) { + t.Parallel() + + ctrl := gomock.NewController(t) + mockDevices := mocks.NewMockDeviceManagementFeature(ctrl) + mockDevices.EXPECT().GetByID(gomock.Any(), "dev-missing", "", true).Return(nil, nil) + + handler := NewAPFHandler(mockDevices, logger.New("error")) + handler.deviceID = "dev-missing" + + assert.False(t, handler.validateCredentials("admin", "s3cret")) + }) +} + +func TestAPFHandler_TracksKeepAliveThreshold(t *testing.T) { + t.Parallel() + + handler := NewAPFHandler(nil, logger.New("debug")) + assert.False(t, handler.ShouldSendKeepAlive()) + + for i := 0; i < globalRequestThreshold-1; i++ { + assert.False(t, handler.OnGlobalRequest(apf.GlobalRequest{})) + assert.False(t, handler.ShouldSendKeepAlive()) + } + + assert.True(t, handler.OnGlobalRequest(apf.GlobalRequest{})) + assert.True(t, handler.ShouldSendKeepAlive()) +} + // fakeConn is a minimal net.Conn implementation for tests. type fakeConn struct{ net.Conn }