cubeops: reject an empty JWT signing secret - #1374
Conversation
…WT signing secret
…amazing tests for covering the fixed bug
![cubesandboxbot[bot]](https://avatars.githubusercontent.com/in/3653584?s=60&v=4){kind=small}
| } | ||
|
|
||
| func TestEmptySecretRejectedOnVerify(t *testing.T) { | ||
| signer := auth.NewJWTManager("forged-secret-32-bytes-long-ok!!", 15*time.Minute, 168*time.Hour) |
There was a problem hiding this comment.
This test does not actually exercise the empty-secret guard on the verify path — it would pass even if checkSecret() were removed from VerifyAccessToken/VerifyRefreshToken.
minted is signed with the non-empty key "forged-secret-32-bytes-long-ok!!", so when the empty-key manager parses it, the HMAC signature check itself fails (key mismatch) and err != nil regardless of the guard. The test is effectively a cross-key rejection test, not an empty-secret rejection test. It therefore gives no regression coverage for the verify-side guard — which is exactly the layer that stops forged tokens at the middleware (the attack surface described in #1373).
To lock in the fix, mint a token with the empty key directly (bypassing the generator guard, e.g. jwt.NewWithClaims(jwt.SigningMethodHS256, claims).SignedString([]byte(""))) and assert the empty-secret manager rejects it. Before the fix such a token verifies successfully; after the fix checkSecret() rejects it — that is the precise forged-token scenario.
| if err != nil { | ||
| return "", fmt.Errorf("persist JWT secret: %w", err) | ||
| } | ||
| if winner == "" { |
There was a problem hiding this comment.
The primary fix for #1373 (startup aborts when the resolved secret is empty) has no test coverage. A regression — e.g. reverting this winner == "" check — would not be caught by CI, since all the new tests live in internal/auth and only cover the defense-in-depth layer.
Consider adding a store-level test using the existing dockertest fixture: insert a t_system_setting row for jwt_secret with an empty setting_value, then assert BootstrapJWTSecret(ctx, "") returns an error (and that main.go would consequently abort). That directly pins the reported vulnerability.
Review: cubeops: reject an empty JWT signing secret (PR #1374)Verdict: Approve. The vulnerability is real and correctly characterized — Findings below are minor; none block this landing. F1 (low) — Whitespace-only
|
…th the empty key and cover the bootstrap guard
| } | ||
| } | ||
|
|
||
| func TestTokenSignedWithADifferentSecretIsRejected(t *testing.T) { |
There was a problem hiding this comment.
This test passes with or without the new checkSecret() guard, so it doesn't actually regress-test this fix. minted is signed with a non-empty key, so empty.VerifyAccessToken(minted) already fails via an HMAC signature mismatch in jwt.ParseWithClaims — even if the empty-secret guard were removed, this assertion still holds. (The test that genuinely pins the fix is TestForgedTokenSignedWithEmptySecretIsRejected, where the token is signed with the empty key.) It also overlaps with the existing TestS1_DifferentSecretsRejected in jwt_test.go. Suggest dropping it, or — if you want to pin the guard itself — assert the rejection is the empty-secret error (e.g. strings.Contains(err.Error(), "empty")) rather than relying on the signature mismatch, so the test fails if the guard is ever removed.
| if err != nil { | ||
| return "", fmt.Errorf("persist JWT secret: %w", err) | ||
| } | ||
| if winner == "" { |
There was a problem hiding this comment.
Minor: this branch also fires when the read-back inside GetOrCreateSystemSetting fails with a real DB error, not just when the stored value is genuinely empty. GetSystemSetting returns ("", nil) whenever the scanned value is empty — even if the SELECT itself errored (the error-swallowing this PR scopes out). So a transient connection drop between the INSERT IGNORE and the read-back surfaces as "JWT secret resolved to an empty value in t_system_setting" instead of the underlying DB error. Still fail-closed (safe direction), but the message could mislead an operator debugging a startup failure. Worth a comment noting the conflation, or fixing once GetSystemSetting stops swallowing errors.
| @@ -140,6 +141,9 @@ func (s *Store) BootstrapJWTSecret(ctx context.Context, envSecret string) (strin | |||
| if err != nil { | |||
| return "", fmt.Errorf("persist JWT secret: %w", err) | |||
| } | |||
There was a problem hiding this comment.
Consideration (not a blocker — the PR documents fail-closed as intentional): when the stored row is empty, a fresh non-empty generated secret is already in hand. Repairing the row with SetSystemSetting(ctx, "jwt_secret", generated) and continuing would (a) un-break the one-click deploy this PR itself identifies as the affected configuration, and (b) remain secure — any tokens forged with the old empty key would fail verification against the new non-empty key. The chosen path instead forces manual SQL (DELETE FROM t_system_setting WHERE setting_key = 'jwt_secret') on the default deployment. Worth confirming self-healing was weighed, since it would be both safer-by-default and non-breaking here.
|
|
||
| var errEmptyJWTSecret = errors.New("jwt: signing secret is empty") | ||
|
|
||
| func (m *JWTManager) checkSecret() error { |
There was a problem hiding this comment.
Nit: len(m.secret) == 0 only catches a fully-empty key. A whitespace-only secret (e.g. JWT_SECRET=" " via env or jwt_secret: " " in YAML) passes the guard while being trivially guessable — effectively a single-value keyspace. If the intent is to reject degenerate signing keys, strings.TrimSpace(string(m.secret)) == "" would cover both cases.
… reject whitespace-only secrets
…ir, convergence and whitespace secrets
| if strings.TrimSpace(string(m.secret)) == "" { | ||
| return errEmptyJWTSecret | ||
| } | ||
| return nil |
There was a problem hiding this comment.
checkSecret treats whitespace-only as empty, but the sibling guard in BootstrapJWTSecret (if envSecret != "") does not trim. A JWT_SECRET that resolves to spaces (e.g. JWT_SECRET=" " in a .env file) is returned from bootstrap as a nil-error success, so the server starts, the DB row is never repaired, and every token operation then fails at runtime with errEmptyJWTSecret. That undercuts the PR's stated fail-fast guarantee ("startup aborts with a clear message") for this input, and the two guards disagree on what "empty" means. Suggest if strings.TrimSpace(envSecret) != "" in BootstrapJWTSecret (or trimming at config load) so both defenses agree. Fail-closed, low severity — not a security hole, since the runtime check still rejects.
|
|
||
| func (s *Store) repairEmptySystemSetting(ctx context.Context, key, value string) error { | ||
| return s.db.WithContext(ctx).Exec( | ||
| "UPDATE t_system_setting SET setting_value = ? WHERE setting_key = ? AND (setting_value IS NULL OR setting_value = '')", |
There was a problem hiding this comment.
The caller detects "empty" with strings.TrimSpace(winner) == "", but this WHERE only matches NULL and ''. A stored value of ' ' / '\t' is detected as empty by the caller yet never matched by this UPDATE, so the repair is a silent no-op and BootstrapJWTSecret then returns its hard error instead of the auto-repair path it logs about. It's fail-closed, so acceptable, but the repair is inconsistent with the detection. Using TRIM(setting_value) = '' in the WHERE would make them agree (and wouldn't match ' a ', only whitespace-only values). Minor.
Closes #1373.
Motivation
BootstrapJWTSecretcould hand back an empty string with anilerror, after whichNewJWTManagerstored a zero-length HMAC key.golang-jwt/v5treats[]byte{}as a perfectlyvalid HMAC key, so anyone who guessed the secret was empty could mint a
sub=admin,aud=cubeops:access,typ=accesstoken that passedVerifyAccessTokenunchanged — full access toevery
/api/v1route behindauth.Middleware.The default one-click deployment is the affected configuration: it deliberately leaves
JWT_SECRETunset so the secret is auto-generated into
t_system_setting, which is the code path that canreturn
"".What this changes
Two independent guards, so neither alone has to be perfect:
CubeOps/internal/store/db.go—BootstrapJWTSecretnow fails when the resolved secret isempty instead of returning it as a success.
main.goalready exits on that error, so startupaborts with a clear message rather than coming up insecure.
CubeOps/internal/auth/jwt.go—JWTManagerrefuses a zero-length key in all four tokenoperations (
GenerateAccessToken,GenerateRefreshToken,VerifyAccessToken,VerifyRefreshToken) via a sharedcheckSecret()helper. This keeps the exportedNewJWTManagersignature unchanged while making forged-token acceptance impossible even if anempty secret reaches the manager some other way.
Deliberately not changed here:
GetSystemSetting/GetSetting/GetUserPasswordstill swallow DB errors. That is theupstream cause and is a behaviour change with a wider blast radius, so it is a separate fix
(see the sibling branch for report finding ci: add builder image workflow and update protobuf tooling #5). This PR is safe to land on its own and does not
depend on it.
Testing
New:
CubeOps/internal/auth/jwt_empty_secret_test.goTestEmptySecretRejectedOnGenerate— both generators refuse an empty key.TestEmptySecretRejectedOnVerify— both verifiers refuse a token when configured with an emptykey, including a token that was legitimately signed with a different, non-empty key.
TestNonEmptySecretStillWorks— regression guard that the normal sign/verify round trip isuntouched.
CI gates checked locally:
gofmt -l ./internal ./cmd— clean (fmt-check).go build ./...— clean.go test ./...— 11 ok, 0 failures (unit-test-checkrunsmake cubeops-test, which isgo test ./... -v -count=1).Risk / rollout
Low risk, but it is a fail-closed change: an operator whose
jwt_secretrow is currently emptywill see CubeOps refuse to start instead of silently accepting forged tokens. That is the intended
behaviour. The remedy is to clear the row so it is regenerated, or to set
JWT_SECRETexplicitly:Existing valid sessions are unaffected — the secret itself is not rotated by this change.