Skip to content

cubeops: reject an empty JWT signing secret #1374

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
dwin-gharibi wants to merge 5 commits into
base: master
Choose a base branch
from
Open

cubeops: reject an empty JWT signing secret #1374

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
808f704
fix(CubeOps/internal/store/db.go): fixing up the rejecting an empty J…
dwin-gharibi Aug 18, 2026
d612c33
test(CubeOps/internal/auth/jwt_empty_secret_test.go): adding up some …
dwin-gharibi Aug 18, 2026
9a5973c
test(CubeOps/internal/auth/jwt_empty_secret_test.go): forge tokens wi…
dwin-gharibi Aug 18, 2026
c424a9a
fix(CubeOps/internal/store/db.go): repair an empty JWT secret row and…
dwin-gharibi Aug 18, 2026
334a57a
test(CubeOps/internal/store/jwt_secret_bootstrap_test.go): cover repa…
dwin-gharibi Aug 18, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 21 additions & 0 deletions CubeOps/internal/auth/jwt.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,15 @@ func NewJWTManager(secret string, accessTTL, refreshTTL time.Duration) *JWTManag
}
}

var errEmptyJWTSecret = errors.New("jwt: signing secret is empty")

func (m *JWTManager) checkSecret() error {

@cubesandboxbot cubesandboxbot Bot Aug 18, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

if len(m.secret) == 0 {
return errEmptyJWTSecret
}
return nil

@cubesandboxbot cubesandboxbot Bot Aug 18, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}

// AccessTTL returns the configured access-token TTL.
func (m *JWTManager) AccessTTL() time.Duration { return m.accessTTL }

Expand All @@ -67,6 +76,9 @@ func (m *JWTManager) RefreshTTL() time.Duration { return m.refreshTTL }

// GenerateAccessToken creates a signed JWT access token.
func (m *JWTManager) GenerateAccessToken(username string) (string, error) {
if err := m.checkSecret(); err != nil {
return "", err
}
now := time.Now()
claims := AccessClaims{
RegisteredClaims: jwt.RegisteredClaims{
Expand All @@ -86,6 +98,9 @@ func (m *JWTManager) GenerateAccessToken(username string) (string, error) {

// GenerateRefreshToken creates a signed JWT refresh token.
func (m *JWTManager) GenerateRefreshToken(username string) (string, string, error) {
if err := m.checkSecret(); err != nil {
return "", "", err
}
now := time.Now()
tokenID := uuid.New().String()
claims := RefreshClaims{
Expand All @@ -111,6 +126,9 @@ func (m *JWTManager) GenerateRefreshToken(username string) (string, string, erro
// tokens by checking the "typ" claim and the audience, so a long-lived
// refresh token cannot be used as an access token.
func (m *JWTManager) VerifyAccessToken(tokenStr string) (*AccessClaims, error) {
if err := m.checkSecret(); err != nil {
return nil, err
}
token, err := jwt.ParseWithClaims(tokenStr, &AccessClaims{}, func(t *jwt.Token) (interface{}, error) {
if _, ok := t.Method.(*jwt.SigningMethodHMAC); !ok {
return nil, fmt.Errorf("unexpected signing method: %v", t.Header["alg"])
Expand All @@ -136,6 +154,9 @@ func (m *JWTManager) VerifyAccessToken(tokenStr string) (*AccessClaims, error) {
// than on this package's internals. It rejects access tokens via the "typ"
// claim and the audience.
func (m *JWTManager) VerifyRefreshToken(tokenStr string) (*service.RefreshClaims, error) {
if err := m.checkSecret(); err != nil {
return nil, err
}
token, err := jwt.ParseWithClaims(tokenStr, &RefreshClaims{}, func(t *jwt.Token) (interface{}, error) {
if _, ok := t.Method.(*jwt.SigningMethodHMAC); !ok {
return nil, fmt.Errorf("unexpected signing method: %v", t.Header["alg"])
Expand Down
54 changes: 54 additions & 0 deletions CubeOps/internal/auth/jwt_empty_secret_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,54 @@
// Copyright (c) 2026 Tencent Inc.
// SPDX-License-Identifier: Apache-2.0

package auth_test

import (
"testing"
"time"

"github.com/tencentcloud/CubeSandbox/CubeOps/internal/auth"
)

func TestEmptySecretRejectedOnGenerate(t *testing.T) {
jm := auth.NewJWTManager("", 15*time.Minute, 168*time.Hour)

if _, err := jm.GenerateAccessToken("admin"); err == nil {
t.Fatal("GenerateAccessToken accepted an empty signing secret")
}
if _, _, err := jm.GenerateRefreshToken("admin"); err == nil {
t.Fatal("GenerateRefreshToken accepted an empty signing secret")
}
}

func TestEmptySecretRejectedOnVerify(t *testing.T) {
signer := auth.NewJWTManager("forged-secret-32-bytes-long-ok!!", 15*time.Minute, 168*time.Hour)

@cubesandboxbot cubesandboxbot Bot Aug 18, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

minted, err := signer.GenerateAccessToken("admin")
if err != nil {
t.Fatalf("failed to mint a token for the test: %v", err)
}

empty := auth.NewJWTManager("", 15*time.Minute, 168*time.Hour)
if _, err := empty.VerifyAccessToken(minted); err == nil {
t.Fatal("VerifyAccessToken accepted a token while configured with an empty secret")
}
if _, err := empty.VerifyRefreshToken(minted); err == nil {
t.Fatal("VerifyRefreshToken accepted a token while configured with an empty secret")
}
}

func TestNonEmptySecretStillWorks(t *testing.T) {
jm := auth.NewJWTManager("good-secret-32-bytes-long-enough!", 15*time.Minute, 168*time.Hour)

access, err := jm.GenerateAccessToken("admin")
if err != nil {
t.Fatalf("GenerateAccessToken: %v", err)
}
claims, err := jm.VerifyAccessToken(access)
if err != nil {
t.Fatalf("VerifyAccessToken: %v", err)
}
if claims.Username != "admin" {
t.Fatalf("Username = %q, want admin", claims.Username)
}
}
4 changes: 4 additions & 0 deletions CubeOps/internal/store/db.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ package store

import (
"context"
"errors"
"fmt"

"github.com/tencentcloud/CubeSandbox/CubeDB/dao"
Expand Down Expand Up @@ -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)
}

@cubesandboxbot cubesandboxbot Bot Aug 18, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

if winner == "" {

@cubesandboxbot cubesandboxbot Bot Aug 18, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@cubesandboxbot cubesandboxbot Bot Aug 18, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

return "", errors.New("JWT secret resolved to an empty value in t_system_setting")
}
if winner == generated {
logging.G(ctx).Info("JWT secret auto-generated and persisted to database (t_system_setting)")
} else {
Expand Down
Loading