From 19a1a6f02335bbd38f22f15cb4ed2bcd08cd34d3 Mon Sep 17 00:00:00 2001 From: Rongsong Shen Date: Sat, 10 Oct 2026 08:37:01 +0000 Subject: [PATCH] fix(apiserver): never sign or accept JWTs with the shipped default secret (bug-1945) Cause: config/app.yaml shipped secretKey "xce-api-key" and resolveJWTSecret used it whenever XCE_JWT_SECRET was unset, so anyone could forge tokens for any user on such a deployment. Fix: resolveJWTSecret uses XCE_JWT_SECRET, else the config value, but treats an empty value or the old public default as unset; then it generates a random 256-bit per-process secret and InitConf logs a warning (secret never logged): tokens do not survive a restart and apiservers sharing one endpoint must set the same XCE_JWT_SECRET. The shipped app.yaml no longer carries a secret. The controllers test secret is no longer the old default. Tests: TestResolveJWTSecret_NeverUsesKnownDefaultOrEmpty, TestShippedAppYAML_NoJWTSecret, TestInitConf_NoJWTSecretGeneratesAndWarns, TestInitConf_JWTSecretFromEnvNoWarning. Co-Authored-By: Claude Opus 5.5 (1M context) --- src/frontend/apiserver/config/app.yaml | 7 +- .../apiserver/controllers/integration_test.go | 4 +- src/frontend/apiserver/utils/config.go | 40 +++++++-- src/frontend/apiserver/utils/config_test.go | 88 +++++++++++++++++++ 4 files changed, 131 insertions(+), 8 deletions(-) diff --git a/src/frontend/apiserver/config/app.yaml b/src/frontend/apiserver/config/app.yaml index ae3d54c1..730240b6 100644 --- a/src/frontend/apiserver/config/app.yaml +++ b/src/frontend/apiserver/config/app.yaml @@ -19,7 +19,12 @@ security: authentication: token: type: "jwt" - secretKey: "xce-api-key" + # JWT signing secret. Prefer the XCE_JWT_SECRET environment variable + # (overrides this value). If neither is set, or the value is the old + # public default "xce-api-key", the apiserver uses a random per-process + # secret and logs a warning: tokens then do not survive a restart, and + # several apiservers behind one endpoint must share XCE_JWT_SECRET. + secretKey: "" token_expire_time: 86400 # 1 day rate_limit: diff --git a/src/frontend/apiserver/controllers/integration_test.go b/src/frontend/apiserver/controllers/integration_test.go index 1efcc8d8..a57b989d 100644 --- a/src/frontend/apiserver/controllers/integration_test.go +++ b/src/frontend/apiserver/controllers/integration_test.go @@ -2,7 +2,7 @@ // Integration tests for XCE API server controllers (§2, §6, §8). // -// Test signing secret: "xce-api-key" (test-only, never use in production). +// Test signing secret: testJWTSecret (test-only, never use in production). // JWT tokens must embed "ip": "127.0.0.1" to match req.RemoteAddr set by httptest. package controllers @@ -26,7 +26,7 @@ import ( "go.uber.org/zap" ) -const testJWTSecret = "xce-api-key" +const testJWTSecret = "controllers-test-only-secret" // ========== Test Helpers ========== diff --git a/src/frontend/apiserver/utils/config.go b/src/frontend/apiserver/utils/config.go index 2420723f..ed5a9674 100644 --- a/src/frontend/apiserver/utils/config.go +++ b/src/frontend/apiserver/utils/config.go @@ -1,6 +1,8 @@ package utils import ( + "crypto/rand" + "encoding/hex" "errors" "fmt" "log" @@ -164,10 +166,18 @@ func InitConf() error { AppVersion: viper.GetString("app.version"), } // parse authentication section + configuredSecret := viper.GetString("authentication.token.secretKey") + jwtSecret := resolveJWTSecret(configuredSecret) + if jwtSecret != configuredSecret && jwtSecret != os.Getenv("XCE_JWT_SECRET") { + Warn("No JWT secret configured (XCE_JWT_SECRET or authentication.token.secretKey), " + + "or it is the publicly known default: using a random per-process secret. " + + "Tokens do not survive an apiserver restart, and apiservers sharing one endpoint " + + "must all set the same XCE_JWT_SECRET") + } AuthenticationYamlConfig = &Authentication{ Token: TokenConfig{ Type: viper.GetString("authentication.token.type"), - SecretKey: resolveJWTSecret(viper.GetString("authentication.token.secretKey")), + SecretKey: jwtSecret, TokenExpireTime: int(viper.GetFloat64("authentication.token.token_expire_time")), }, } @@ -340,11 +350,31 @@ func resolveTrustedProxies(entries []string) ([]string, error) { return proxies, nil } -// resolveJWTSecret returns the JWT secret from XCE_JWT_SECRET env var if set and non-empty, -// otherwise falls back to the value from config file. +// knownDefaultJWTSecret is the secret older app.yaml files shipped with; it is +// public, so it is treated as "no secret configured" (bug-1945). +const knownDefaultJWTSecret = "xce-api-key" + +// usableJWTSecret reports whether s may sign tokens: non-empty and not the +// publicly known default. +func usableJWTSecret(s string) bool { + return s != "" && s != knownDefaultJWTSecret +} + +// resolveJWTSecret returns the JWT secret: XCE_JWT_SECRET if usable, else +// the config file value if usable, else a random per-process secret (the +// caller detects that case because the result differs from both inputs). func resolveJWTSecret(configValue string) string { - if env := os.Getenv("XCE_JWT_SECRET"); env != "" { + if env := os.Getenv("XCE_JWT_SECRET"); usableJWTSecret(env) { return env } - return configValue + if usableJWTSecret(configValue) { + return configValue + } + buf := make([]byte, 32) + if _, err := rand.Read(buf); err != nil { + // crypto/rand never fails on supported platforms; refuse to fall + // back to a predictable secret if it ever does. + panic(fmt.Sprintf("cannot generate a JWT secret: %v", err)) + } + return hex.EncodeToString(buf) } diff --git a/src/frontend/apiserver/utils/config_test.go b/src/frontend/apiserver/utils/config_test.go index fd23919f..ee00fcba 100644 --- a/src/frontend/apiserver/utils/config_test.go +++ b/src/frontend/apiserver/utils/config_test.go @@ -40,6 +40,94 @@ func TestResolveJWTSecret_EmptyEnvUsesConfig(t *testing.T) { } } +// bug-1945: the publicly known default secret and an empty secret are never +// used; a random per-process secret replaces them. +func TestResolveJWTSecret_NeverUsesKnownDefaultOrEmpty(t *testing.T) { + cases := []struct { + name string + env string + config string + }{ + {"default in config, no env", "", "xce-api-key"}, + {"empty config, no env", "", ""}, + {"default in env, empty config", "xce-api-key", ""}, + {"default in env and config", "xce-api-key", "xce-api-key"}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + t.Setenv("XCE_JWT_SECRET", tc.env) + got := resolveJWTSecret(tc.config) + if got == "" || got == "xce-api-key" { + t.Fatalf("resolveJWTSecret(%q) with env %q = %q: must be a generated secret", tc.config, tc.env, got) + } + if len(got) < 32 { + t.Errorf("generated secret too short: %d chars", len(got)) + } + if again := resolveJWTSecret(tc.config); again == got { + t.Errorf("generated secrets must be random, got %q twice", got) + } + }) + } +} + +// bug-1945: the shipped app.yaml carries no JWT secret. +func TestShippedAppYAML_NoJWTSecret(t *testing.T) { + shipped, err := os.ReadFile(filepath.Join("..", "config", "app.yaml")) + if err != nil { + t.Fatalf("read shipped app.yaml: %v", err) + } + v := viper.New() + v.SetConfigType("yaml") + if err := v.ReadConfig(strings.NewReader(string(shipped))); err != nil { + t.Fatalf("parse shipped app.yaml: %v", err) + } + if s := v.GetString("authentication.token.secretKey"); s != "" { + t.Errorf("shipped app.yaml must not set authentication.token.secretKey, has %q", s) + } +} + +// bug-1945: InitConf on the shipped app.yaml without XCE_JWT_SECRET runs +// with a random secret and logs a warning (never the secret itself). +func TestInitConf_NoJWTSecretGeneratesAndWarns(t *testing.T) { + t.Setenv("XCE_JWT_SECRET", "") + logs := setupInitConf(t, "job:\n max_array_size: 1000") + oldAuth := AuthenticationYamlConfig + t.Cleanup(func() { AuthenticationYamlConfig = oldAuth }) + if err := InitConf(); err != nil { + t.Fatalf("InitConf: %v", err) + } + secret := AuthenticationYamlConfig.Token.SecretKey + if secret == "" || secret == "xce-api-key" { + t.Fatalf("SecretKey = %q: must be a generated secret", secret) + } + warns := logs.FilterLevelExact(zap.WarnLevel).FilterMessageSnippet("XCE_JWT_SECRET").All() + if len(warns) != 1 { + t.Fatalf("expected 1 JWT secret warning, got %d (all logs: %v)", len(warns), logs.All()) + } + for _, e := range logs.All() { + if strings.Contains(e.Message, secret) { + t.Errorf("log message contains the secret: %q", e.Message) + } + } +} + +// bug-1945: a configured secret is used as is, without a warning. +func TestInitConf_JWTSecretFromEnvNoWarning(t *testing.T) { + t.Setenv("XCE_JWT_SECRET", "a-deployment-secret-of-sufficient-length") + logs := setupInitConf(t, "job:\n max_array_size: 1000") + oldAuth := AuthenticationYamlConfig + t.Cleanup(func() { AuthenticationYamlConfig = oldAuth }) + if err := InitConf(); err != nil { + t.Fatalf("InitConf: %v", err) + } + if got := AuthenticationYamlConfig.Token.SecretKey; got != "a-deployment-secret-of-sufficient-length" { + t.Errorf("SecretKey = %q, want the XCE_JWT_SECRET value", got) + } + if n := logs.FilterMessageSnippet("XCE_JWT_SECRET").Len(); n != 0 { + t.Errorf("unexpected JWT secret warning (%d)", n) + } +} + // bug-1948: security.trusted_proxies accepts IPs and CIDRs only; empty // means no trusted proxy. func TestResolveTrustedProxies(t *testing.T) {