fix: require DSNs for legacy database notifications

Reject pre-KV PostgreSQL and MySQL targets that lack a canonical connection string, propagate the typed migration error to the fatal startup boundary, and stop emitting unregistered discrete connection keys.\n\nCloses the implementation for #53; release and issue closure remain separate gates.
This commit is contained in:
Feng Ruohang
2026-08-24 02:22:20 +08:00
parent 43f4bb7ed4
commit f1ba683582
6 changed files with 525 additions and 62 deletions
+6 -2
View File
@@ -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)
+264
View File
@@ -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 <http://www.gnu.org/licenses/>.
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")
}
}
+15 -3
View File
@@ -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")
}
+47 -42
View File
@@ -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,
+190
View File
@@ -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)
}
})
}
}
+3 -15
View File
@@ -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")