fix: register NATS/AMQP notify config keys read by parsers

GetNotifyNATS reads user_credentials, nkey_seed and tls_handshake_first
and GetNotifyAMQP reads immediate, but none of them were registered in
DefaultNATSKVS/DefaultAMQPKVS or the help schema, so CheckValidKeys
rejected any enable=on target carrying them. Worse, the legacy config
migration wrote exactly these keys - including the env var name
MINIO_NOTIFY_NATS_USER_CREDENTIALS used as a config key, because the
NATSUserCredentials constant doubled as both - so a migrated NATS config
failed validation on every load, and the FetchEnabledTargets fail-fast
then silently disabled all bucket notification targets.

- Register user_credentials/nkey_seed/tls_handshake_first (NATS) and
  immediate (AMQP) in the default KVS and help schema; split
  NATSUserCredentials into a real config key plus EnvNATSUserCredentials
  (all env var names byte-stable)
- Fix legacy migration: SetNotifyNATS writes the proper key;
  SetNotifyAMQP no longer writes cfg.Immediate under the internal key
  and now carries both immediate and internal
- Tolerate the legacy MINIO_NOTIFY_NATS_USER_CREDENTIALS key written by
  pre-fix migrations (NATS-scoped, load path only) with fallback read;
  env > user_credentials > legacy key
- Print key names only, never values, in the invalid-keys error of both
  CheckValidKeys forms; rejected values can carry credentials
- Add an AST-based audit test asserting parser reads, migration writes
  and help entries stay within the registered key set for all ten notify
  subsystems, with floor assertions so collector drift fails loudly
- Document (unchanged) FetchEnabledTargets fail-fast and pin it with a
  characterization test

Known same-class gap left in place and pinned by the audit's allowlist:
SetNotifyPostgres/SetNotifyMySQL write five unregistered DSN-era keys;
tracked for a follow-up issue.

Closes #39

Co-authored-by: ChatGPT <noreply@openai.com>
Co-authored-by: Claude <noreply@anthropic.com>
This commit is contained in:
Feng Ruohang
2026-08-03 23:39:06 +08:00
parent fe6dc47804
commit 162ded3438
8 changed files with 1029 additions and 7 deletions
+54
View File
@@ -18,6 +18,7 @@
package config
import (
"strings"
"testing"
)
@@ -128,3 +129,56 @@ func TestValidRegion(t *testing.T) {
})
}
}
// The invalid-keys error is logged by the server and printed by `mc`. A
// rejected key may carry a credential, so only key names may appear in it.
func TestCheckValidKeysDoesNotLeakValues(t *testing.T) {
const (
badKey = "unknown_key"
badSecret = "s3cr3t-must-not-appear"
)
assertRedacted := func(t *testing.T, err error) {
t.Helper()
if err == nil {
t.Fatal("expected an error for an unregistered key")
}
msg := err.Error()
if strings.Contains(msg, badSecret) {
t.Errorf("error leaks the rejected value: %s", msg)
}
if !strings.Contains(msg, badKey) {
t.Errorf("error does not name the rejected key: %s", msg)
}
if !strings.Contains(msg, "mc admin config reset") {
t.Errorf("error lost the remediation hint: %s", msg)
}
}
t.Run("func", func(t *testing.T) {
kv := KVS{
KV{Key: Enable, Value: EnableOn},
KV{Key: badKey, Value: badSecret},
}
validKVS := KVS{KV{Key: Enable, Value: EnableOff}}
assertRedacted(t, CheckValidKeys("test_subsys", kv, validKVS))
})
t.Run("method", func(t *testing.T) {
const subSys = "test_subsys_method"
RegisterDefaultKVS(map[string]KVS{
subSys: {KV{Key: Enable, Value: EnableOff}},
})
t.Cleanup(func() { delete(DefaultKVS, subSys) })
c := Config{
subSys: map[string]KVS{
Default: {
KV{Key: Enable, Value: EnableOn},
KV{Key: badKey, Value: badSecret},
},
},
}
assertRedacted(t, c.CheckValidKeys(subSys, nil))
})
}