diff --git a/cmd/config-migrate.go b/cmd/config-migrate.go index 0cceb1859..170279ab5 100644 --- a/cmd/config-migrate.go +++ b/cmd/config-migrate.go @@ -167,7 +167,9 @@ func readConfigWithoutMigrate(ctx context.Context, objAPI ObjectLayer) (config.C notify.SetNotifyMQTT(newCfg, k, args) } for k, args := range cfg.Notify.MySQL { - notify.SetNotifyMySQL(newCfg, k, args) + if err := notify.SetNotifyMySQL(newCfg, k, args); err != nil { + return nil, err + } } for k, args := range cfg.Notify.NATS { notify.SetNotifyNATS(newCfg, k, args) @@ -176,7 +178,9 @@ func readConfigWithoutMigrate(ctx context.Context, objAPI ObjectLayer) (config.C notify.SetNotifyNSQ(newCfg, k, args) } for k, args := range cfg.Notify.PostgreSQL { - notify.SetNotifyPostgres(newCfg, k, args) + if err := notify.SetNotifyPostgres(newCfg, k, args); err != nil { + return nil, err + } } for k, args := range cfg.Notify.Redis { notify.SetNotifyRedis(newCfg, k, args) diff --git a/cmd/config-migrate_test.go b/cmd/config-migrate_test.go new file mode 100644 index 000000000..76a616d9b --- /dev/null +++ b/cmd/config-migrate_test.go @@ -0,0 +1,264 @@ +// Copyright (c) 2015-2026 MinIO, Inc. +// +// This file is part of MinIO Object Storage stack +// +// This program is free software: you can redistribute it and/or modify +// it under the terms of the GNU Affero General Public License as published by +// the Free Software Foundation, either version 3 of the License, or +// (at your option) any later version. +// +// This program is distributed in the hope that it will be useful +// but WITHOUT ANY WARRANTY; without even the implied warranty of +// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +// GNU Affero General Public License for more details. +// +// You should have received a copy of the GNU Affero General Public License +// along with this program. If not, see . + +package cmd + +import ( + "bytes" + "context" + "encoding/json" + "errors" + "os" + "reflect" + "strings" + "testing" + + "github.com/minio/minio/internal/config" + "github.com/minio/minio/internal/config/notify" + "github.com/minio/minio/internal/event/target" +) + +func installLegacyConfigFile(t *testing.T, configure func(*serverConfigV33)) (string, []byte) { + t.Helper() + + cfg := &serverConfigV33{ + Version: "33", + Notify: notify.NewConfig(), + } + configure(cfg) + + data, err := json.Marshal(cfg) + if err != nil { + t.Fatal(err) + } + + oldConfigDir := globalConfigDir + globalConfigDir = &ConfigDir{path: t.TempDir()} + t.Cleanup(func() { globalConfigDir = oldConfigDir }) + + configFile := getConfigFile() + if err = os.WriteFile(configFile, data, 0o600); err != nil { + t.Fatal(err) + } + return configFile, data +} + +func assertLegacyMigrationError(t *testing.T, err error, subsystem, name, key, secret string) { + t.Helper() + var targetErr *notify.LegacyDatabaseTargetError + if !errors.As(err, &targetErr) { + t.Fatalf("error = %v, want *notify.LegacyDatabaseTargetError", err) + } + msg := err.Error() + for _, want := range []string{subsystem + config.SubSystemSeparator + name, key} { + if !strings.Contains(msg, want) { + t.Errorf("error %q does not contain %q", msg, want) + } + } + if strings.Contains(msg, secret) { + t.Errorf("migration error leaks database password %q: %s", secret, msg) + } +} + +func TestReadConfigWithoutMigrateRejectsLegacyDatabaseTargets(t *testing.T) { + tests := []struct { + name string + subsystem string + key string + secret string + configure func(*serverConfigV33) + }{ + { + name: "postgres", + subsystem: config.NotifyPostgresSubSys, + key: target.PostgresConnectionString, + secret: "postgres-migration-secret", + configure: func(cfg *serverConfigV33) { + cfg.Notify.PostgreSQL["archive"] = target.PostgreSQLArgs{ + Enable: true, + Port: "5432", + Username: "legacy-user", + Password: "postgres-migration-secret", + Database: "events", + } + }, + }, + { + name: "mysql", + subsystem: config.NotifyMySQLSubSys, + key: target.MySQLDSNString, + secret: "mysql-migration-secret", + configure: func(cfg *serverConfigV33) { + cfg.Notify.MySQL["archive"] = target.MySQLArgs{ + Enable: true, + Port: "3306", + User: "legacy-user", + Password: "mysql-migration-secret", + Database: "events", + } + }, + }, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + configFile, original := installLegacyConfigFile(t, test.configure) + got, err := readConfigWithoutMigrate(t.Context(), nil) + if got != nil { + t.Fatalf("config = %v, want nil on failed migration", got) + } + assertLegacyMigrationError(t, err, test.subsystem, "archive", test.key, test.secret) + + after, readErr := os.ReadFile(configFile) + if readErr != nil { + t.Fatal(readErr) + } + if !bytes.Equal(after, original) { + t.Fatal("failed migration rewrote the legacy source config") + } + if _, statErr := os.Stat(configFile + ".old"); !errors.Is(statErr, os.ErrNotExist) { + t.Fatalf("failed migration created a backup/persistence artifact: %v", statErr) + } + }) + } +} + +func TestReadConfigWithoutMigrateMigratesCanonicalDatabaseTargets(t *testing.T) { + const ( + postgresConnection = "host=postgres.example port=5432 dbname=events user=app password=secret sslmode=disable" + mysqlDSN = "app:secret@tcp(mysql.example:3306)/events?parseTime=true" + discardedLegacyValue = "discarded-legacy-value" + ) + installLegacyConfigFile(t, func(cfg *serverConfigV33) { + cfg.Notify.PostgreSQL["archive"] = target.PostgreSQLArgs{ + Enable: true, + Format: "namespace", + ConnectionString: postgresConnection, + Table: "events", + Port: discardedLegacyValue, + Username: discardedLegacyValue, + Password: discardedLegacyValue, + Database: discardedLegacyValue, + } + cfg.Notify.MySQL["archive"] = target.MySQLArgs{ + Enable: true, + Format: "namespace", + DSN: mysqlDSN, + Table: "events", + Port: discardedLegacyValue, + User: discardedLegacyValue, + Password: discardedLegacyValue, + Database: discardedLegacyValue, + } + }) + + got, err := readConfigWithoutMigrate(t.Context(), nil) + if err != nil { + t.Fatalf("readConfigWithoutMigrate: %v", err) + } + tests := []struct { + subsystem string + key string + want string + discarded string + }{ + {config.NotifyPostgresSubSys, target.PostgresConnectionString, postgresConnection, discardedLegacyValue}, + {config.NotifyMySQLSubSys, target.MySQLDSNString, mysqlDSN, discardedLegacyValue}, + } + for _, test := range tests { + kvs := got[test.subsystem]["archive"] + if value := kvs.Get(test.key); value != test.want { + t.Errorf("%s %s = %q, want %q", test.subsystem, test.key, value, test.want) + } + if err := config.CheckValidKeys(test.subsystem+config.SubSystemSeparator+"archive", kvs, notify.DefaultNotificationKVS[test.subsystem]); err != nil { + t.Errorf("migrated %s target failed key validation: %v", test.subsystem, err) + } + for _, key := range []string{"host", "port", "username", "password", "database"} { + if _, ok := kvs.Lookup(key); ok { + t.Errorf("migrated %s target contains legacy key %q", test.subsystem, key) + } + } + for _, kv := range kvs { + if strings.Contains(kv.Value, test.discarded) { + t.Errorf("migrated %s target contains discarded legacy value in %q", test.subsystem, kv.Key) + } + } + } + + postgresTargets, err := notify.GetNotifyPostgres(got[config.NotifyPostgresSubSys]) + if err != nil { + t.Fatalf("GetNotifyPostgres: %v", err) + } + if value := postgresTargets["archive"].ConnectionString; value != postgresConnection { + t.Errorf("Postgres connection string = %q, want %q", value, postgresConnection) + } + mysqlTargets, err := notify.GetNotifyMySQL(got[config.NotifyMySQLSubSys]) + if err != nil { + t.Fatalf("GetNotifyMySQL: %v", err) + } + if value := mysqlTargets["archive"].DSN; value != mysqlDSN { + t.Errorf("MySQL DSN = %q, want %q", value, mysqlDSN) + } +} + +func TestInitConfigSubsystemReturnsLegacyDatabaseTargetError(t *testing.T) { + obj, fsDir, err := prepareFS(t.Context()) + if err != nil { + t.Fatal(err) + } + t.Cleanup(func() { + _ = obj.Shutdown(context.Background()) + _ = os.RemoveAll(fsDir) + }) + + const secret = "startup-migration-secret" + installLegacyConfigFile(t, func(cfg *serverConfigV33) { + cfg.Notify.PostgreSQL["archive"] = target.PostgreSQLArgs{ + Enable: true, + Port: "5432", + Username: "legacy-user", + Password: secret, + Database: "events", + } + }) + + globalServerConfigMu.RLock() + var before config.Config + if globalServerConfig != nil { + before = globalServerConfig.Clone() + } + globalServerConfigMu.RUnlock() + + err = initConfigSubsystem(t.Context(), obj) + assertLegacyMigrationError(t, err, config.NotifyPostgresSubSys, "archive", target.PostgresConnectionString, secret) + if configRetriableErrors(err) { + t.Fatal("legacy database migration error must be startup-fatal, not retriable") + } + if !fatalServerConfigError(err) { + t.Fatal("legacy database migration error must abort server startup") + } + + globalServerConfigMu.RLock() + var after config.Config + if globalServerConfig != nil { + after = globalServerConfig.Clone() + } + globalServerConfigMu.RUnlock() + if !reflect.DeepEqual(after, before) { + t.Fatal("failed migration activated a partial server configuration") + } +} diff --git a/cmd/server-main.go b/cmd/server-main.go index a547581f7..bedd39043 100644 --- a/cmd/server-main.go +++ b/cmd/server-main.go @@ -48,6 +48,7 @@ import ( "github.com/minio/minio/internal/color" "github.com/minio/minio/internal/config" "github.com/minio/minio/internal/config/api" + "github.com/minio/minio/internal/config/notify" "github.com/minio/minio/internal/handlers" "github.com/minio/minio/internal/hash/sha256" xhttp "github.com/minio/minio/internal/http" @@ -535,6 +536,15 @@ func configRetriableErrors(err error) bool { notInitialized } +func fatalServerConfigError(err error) bool { + var configErr config.Err + if errors.As(err, &configErr) { + return true + } + var migrationErr *notify.LegacyDatabaseTargetError + return errors.As(err, &migrationErr) +} + func bootstrapTraceMsg(msg string) { info := madmin.TraceInfo{ TraceType: madmin.TraceBootstrap, @@ -632,7 +642,10 @@ func initConfigSubsystem(ctx context.Context, newObject ObjectLayer) error { // Initialize config system. if err := globalConfigSys.Init(newObject); err != nil { - if configRetriableErrors(err) { + var migrationErr *notify.LegacyDatabaseTargetError + // Do not use fatalServerConfigError here: existing config.Err values + // retain the historical log-and-continue behavior at this boundary. + if configRetriableErrors(err) || errors.As(err, &migrationErr) { return fmt.Errorf("Unable to initialize config system: %w", err) } @@ -965,10 +978,9 @@ func serverMain(ctx *cli.Context) { var err error bootstrapTrace("initServerConfig", func() { if err = initServerConfig(GlobalContext, newObject); err != nil { - var cerr config.Err // For any config error, we don't need to drop into safe-mode // instead its a user error and should be fixed by user. - if errors.As(err, &cerr) { + if fatalServerConfigError(err) { logger.FatalIf(err, "Unable to initialize the server") } diff --git a/internal/config/notify/legacy.go b/internal/config/notify/legacy.go index d2a1d67f3..21417f0da 100644 --- a/internal/config/notify/legacy.go +++ b/internal/config/notify/legacy.go @@ -26,6 +26,25 @@ import ( "github.com/minio/minio/internal/event/target" ) +// LegacyDatabaseTargetError reports a pre-KV database notification target +// that cannot be migrated safely. It deliberately carries no configuration +// values so credentials cannot escape through startup logs. +type LegacyDatabaseTargetError struct { + subsystem string + target string + connectionKey string + invalid bool +} + +func (e *LegacyDatabaseTargetError) Error() string { + if e.invalid { + return fmt.Sprintf("%s:%s has invalid %s or target settings; fix the target before migrating to SILO", + e.subsystem, e.target, e.connectionKey) + } + return fmt.Sprintf("%s:%s requires %s; discrete database connection fields are not migrated to SILO", + e.subsystem, e.target, e.connectionKey) +} + // SetNotifyKafka - helper for config migration from older config. func SetNotifyKafka(s config.Config, name string, cfg target.KafkaArgs) error { if !cfg.Enable { @@ -325,8 +344,21 @@ func SetNotifyPostgres(s config.Config, psqName string, cfg target.PostgreSQLArg return nil } + if cfg.ConnectionString == "" { + return &LegacyDatabaseTargetError{ + subsystem: config.NotifyPostgresSubSys, + target: psqName, + connectionKey: target.PostgresConnectionString, + } + } + if err := cfg.Validate(); err != nil { - return err + return &LegacyDatabaseTargetError{ + subsystem: config.NotifyPostgresSubSys, + target: psqName, + connectionKey: target.PostgresConnectionString, + invalid: true, + } } s[config.NotifyPostgresSubSys][psqName] = config.KVS{ @@ -346,26 +378,6 @@ func SetNotifyPostgres(s config.Config, psqName string, cfg target.PostgreSQLArg Key: target.PostgresTable, Value: cfg.Table, }, - config.KV{ - Key: target.PostgresHost, - Value: cfg.Host.String(), - }, - config.KV{ - Key: target.PostgresPort, - Value: cfg.Port, - }, - config.KV{ - Key: target.PostgresUsername, - Value: cfg.Username, - }, - config.KV{ - Key: target.PostgresPassword, - Value: cfg.Password, - }, - config.KV{ - Key: target.PostgresDatabase, - Value: cfg.Database, - }, config.KV{ Key: target.PostgresQueueDir, Value: cfg.QueueDir, @@ -538,8 +550,21 @@ func SetNotifyMySQL(s config.Config, sqlName string, cfg target.MySQLArgs) error return nil } + if cfg.DSN == "" { + return &LegacyDatabaseTargetError{ + subsystem: config.NotifyMySQLSubSys, + target: sqlName, + connectionKey: target.MySQLDSNString, + } + } + if err := cfg.Validate(); err != nil { - return err + return &LegacyDatabaseTargetError{ + subsystem: config.NotifyMySQLSubSys, + target: sqlName, + connectionKey: target.MySQLDSNString, + invalid: true, + } } s[config.NotifyMySQLSubSys][sqlName] = config.KVS{ @@ -559,26 +584,6 @@ func SetNotifyMySQL(s config.Config, sqlName string, cfg target.MySQLArgs) error Key: target.MySQLTable, Value: cfg.Table, }, - config.KV{ - Key: target.MySQLHost, - Value: cfg.Host.String(), - }, - config.KV{ - Key: target.MySQLPort, - Value: cfg.Port, - }, - config.KV{ - Key: target.MySQLUsername, - Value: cfg.User, - }, - config.KV{ - Key: target.MySQLPassword, - Value: cfg.Password, - }, - config.KV{ - Key: target.MySQLDatabase, - Value: cfg.Database, - }, config.KV{ Key: target.MySQLQueueDir, Value: cfg.QueueDir, diff --git a/internal/config/notify/legacy_test.go b/internal/config/notify/legacy_test.go index 50f298d7f..011ad95fa 100644 --- a/internal/config/notify/legacy_test.go +++ b/internal/config/notify/legacy_test.go @@ -18,6 +18,8 @@ package notify import ( + "errors" + "strings" "testing" "github.com/minio/minio/internal/config" @@ -26,6 +28,25 @@ import ( "github.com/rabbitmq/amqp091-go" ) +func assertLegacyDatabaseTargetError(t *testing.T, err error, subsystem, name, key string, secrets ...string) { + t.Helper() + var targetErr *LegacyDatabaseTargetError + if !errors.As(err, &targetErr) { + t.Fatalf("error = %v, want *LegacyDatabaseTargetError", err) + } + msg := err.Error() + for _, want := range []string{subsystem + config.SubSystemSeparator + name, key} { + if !strings.Contains(msg, want) { + t.Errorf("error %q does not contain %q", msg, want) + } + } + for _, secret := range secrets { + if secret != "" && strings.Contains(msg, secret) { + t.Errorf("error leaks configuration value %q: %s", secret, msg) + } + } +} + // T5 (NATS): a config produced by the legacy migration must survive validation // and round-trip back through the parser unchanged. Before the fix the // migration wrote an env var name as a config key, so every migrated NATS @@ -113,3 +134,172 @@ func TestSetNotifyAMQPRoundTrip(t *testing.T) { t.Errorf("Internal = true, want false (immediate must not be written to the internal key)") } } + +func TestSetNotifyDatabaseTargetsRequireConnectionStrings(t *testing.T) { + postgresHost, err := xnet.ParseHost("legacy-postgres.example") + if err != nil { + t.Fatal(err) + } + mysqlHost, err := xnet.ParseURL("legacy-mysql.example") + if err != nil { + t.Fatal(err) + } + + tests := []struct { + name string + subsystem string + key string + set func(config.Config) error + secrets []string + }{ + { + name: "postgres", + subsystem: config.NotifyPostgresSubSys, + key: target.PostgresConnectionString, + set: func(s config.Config) error { + return SetNotifyPostgres(s, testTargetName, target.PostgreSQLArgs{ + Enable: true, + Format: formatNamespace, + Table: "events", + Host: *postgresHost, + Port: "5432", + Username: "legacy-user", + Password: "legacy-postgres-password", + Database: "legacy-database", + }) + }, + secrets: []string{postgresHost.String(), "5432", "legacy-user", "legacy-postgres-password", "legacy-database"}, + }, + { + name: "mysql", + subsystem: config.NotifyMySQLSubSys, + key: target.MySQLDSNString, + set: func(s config.Config) error { + return SetNotifyMySQL(s, testTargetName, target.MySQLArgs{ + Enable: true, + Format: formatNamespace, + Table: "events", + Host: *mysqlHost, + Port: "3306", + User: "legacy-user", + Password: "legacy-mysql-password", + Database: "legacy-database", + }) + }, + secrets: []string{mysqlHost.String(), "3306", "legacy-user", "legacy-mysql-password", "legacy-database"}, + }, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + s := config.Config{test.subsystem: map[string]config.KVS{}} + err := test.set(s) + assertLegacyDatabaseTargetError(t, err, test.subsystem, testTargetName, test.key, test.secrets...) + if _, ok := s[test.subsystem][testTargetName]; ok { + t.Fatal("unsupported target was emitted despite migration error") + } + }) + } +} + +func TestSetNotifyDisabledDatabaseTargetsAreIgnored(t *testing.T) { + s := config.Config{ + config.NotifyPostgresSubSys: map[string]config.KVS{}, + config.NotifyMySQLSubSys: map[string]config.KVS{}, + } + if err := SetNotifyPostgres(s, testTargetName, target.PostgreSQLArgs{Password: "discarded-postgres-secret"}); err != nil { + t.Fatalf("SetNotifyPostgres: %v", err) + } + if err := SetNotifyMySQL(s, testTargetName, target.MySQLArgs{Password: "discarded-mysql-secret"}); err != nil { + t.Fatalf("SetNotifyMySQL: %v", err) + } + if _, ok := s[config.NotifyPostgresSubSys][testTargetName]; ok { + t.Fatal("disabled Postgres target was emitted") + } + if _, ok := s[config.NotifyMySQLSubSys][testTargetName]; ok { + t.Fatal("disabled MySQL target was emitted") + } +} + +func TestSetNotifyInvalidDatabaseTargetsDoNotLeak(t *testing.T) { + tests := []struct { + name string + subsystem string + key string + secret string + set func(config.Config) error + }{ + { + name: "postgres", + subsystem: config.NotifyPostgresSubSys, + key: target.PostgresConnectionString, + secret: "postgres-dsn-secret", + set: func(s config.Config) error { + return SetNotifyPostgres(s, testTargetName, target.PostgreSQLArgs{ + Enable: true, + Format: formatNamespace, + ConnectionString: "host=db password=postgres-dsn-secret", + }) + }, + }, + { + name: "mysql", + subsystem: config.NotifyMySQLSubSys, + key: target.MySQLDSNString, + secret: "mysql-dsn-secret", + set: func(s config.Config) error { + return SetNotifyMySQL(s, testTargetName, target.MySQLArgs{ + Enable: true, + Format: formatNamespace, + DSN: "user:mysql-dsn-secret@tcp(db:3306/events", + Table: "events", + }) + }, + }, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + s := config.Config{test.subsystem: map[string]config.KVS{}} + err := test.set(s) + assertLegacyDatabaseTargetError(t, err, test.subsystem, testTargetName, test.key, test.secret) + }) + } +} + +func TestDatabaseConnectionStringsSurviveKVTokenization(t *testing.T) { + tests := []struct { + name string + subsystem string + key string + input string + want string + }{ + { + name: "postgres", + subsystem: config.NotifyPostgresSubSys, + key: target.PostgresConnectionString, + input: `notify_postgres:dsn connection_string="host=db port=5432 dbname=events user=app password=inside" table="events"`, + want: "host=db port=5432 dbname=events user=app password=inside", + }, + { + name: "mysql", + subsystem: config.NotifyMySQLSubSys, + key: target.MySQLDSNString, + input: `notify_mysql:dsn dsn_string="user:pass@tcp(db:3306)/events?host=db&port=3306&password=inside" table="events"`, + want: "user:pass@tcp(db:3306)/events?host=db&port=3306&password=inside", + }, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + s := config.Config{test.subsystem: map[string]config.KVS{}} + if _, err := s.SetKVS(test.input, DefaultNotificationKVS); err != nil { + t.Fatalf("SetKVS: %v", err) + } + if got := s[test.subsystem]["dsn"].Get(test.key); got != test.want { + t.Errorf("%s = %q, want %q", test.key, got, test.want) + } + }) + } +} diff --git a/internal/config/notify/parse_test.go b/internal/config/notify/parse_test.go index 51e99747f..074461b1e 100644 --- a/internal/config/notify/parse_test.go +++ b/internal/config/notify/parse_test.go @@ -414,21 +414,9 @@ var configPkgConsts = map[string]string{ "Comment": config.Comment, } -// knownUnregisteredWrites records pre-existing instances of the exact defect -// this audit exists to catch: a legacy migration writing config keys that no -// default KVS registers, so the migrated config is rejected on the next load. -// -// These are inherited from upstream and are the same class as issue #39, but -// they are NOT part of the issue #39 fix and were left untouched deliberately. -// The Postgres/MySQL keys below are the pre-connection-string DSN fields; the -// migration still writes them and `password` carries a plaintext database -// password. -// -// This list must only ever shrink. Do not add entries to silence a new gap. -var knownUnregisteredWrites = map[string][]string{ - "SetNotifyPostgres": {"host", "port", "username", "password", "database"}, - "SetNotifyMySQL": {"host", "port", "username", "password", "database"}, -} +// knownUnregisteredWrites is a shrink-only ratchet for inherited migration +// gaps. Do not add entries to silence a new mismatch. +var knownUnregisteredWrites = map[string][]string{} func TestNotifyConfigKeysAreRegistered(t *testing.T) { targetConsts, err := parseTargetPkgStringConsts("../../event/target")