Skip to content

fix: prevent yurthub from overwriting valid kube-system token with empty bearer token in tenant mode - #2764

Open
WorrierKhushal wants to merge 1 commit into
openyurtio:masterfrom
WorrierKhushal:fix/2761-tenant-token-substitute
Open

fix: prevent yurthub from overwriting valid kube-system token with empty bearer token in tenant mode#2764
WorrierKhushal wants to merge 1 commit into
openyurtio:masterfrom
WorrierKhushal:fix/2761-tenant-token-substitute

Conversation

@WorrierKhushal

Copy link
Copy Markdown
Contributor

What type of PR is this?

/kind bug

What this PR does / why we need it:

WithSaTokenSubstitute (pkg/yurthub/proxy/util/util.go) replaces the
request's Authorization header with the tenant's token whenever the
request is authenticated as a kube-system service account and the tenant
secret informer has synced. It unconditionally called GetTenantToken()
and, if that returned an empty string, the header became
Authorization: Bearer (empty) — destroying the original, still-valid
token.

GetTenantToken() returns "" whenever TenantSecret == nil, and
TenantSecret is only set once the informer observes a default
service-account token secret in the tenant namespace. On Kubernetes

=1.24, legacy SA token secrets are no longer auto-created for the
default SA, so in a tenant deployment this secret may not exist at hub
startup (or at all until explicitly provisioned). WaitForCacheSync()
only guarantees the informer's initial list was processed — it does not
guarantee the token secret exists.

Consequence: as soon as the tenant informer synced but before the tenant
token secret appeared, every kube-system request through the hub had its
valid token overwritten with an empty one, and the tenant kube-apiserver
returned 401 for all of them — with no fallback and no error surfaced,
fully cutting off the edge node in tenant mode until the secret showed
up.

Secondary issue: the existing log line printed the full old and new
token values in plaintext.

This PR:

  • Only rewrites the Authorization header when GetTenantToken()
    returns a non-empty token; otherwise keeps the original header
    untouched and logs a warning instead.
  • Removes the plaintext token logging.
  • Adds TestWithSaTokenSubstituteWithStubTenant with a stub
    tenant.Interface, covering three cases: non-empty token → header
    replaced, empty token → original header preserved,
    WaitForCacheSync() false → original header preserved.

Verified locally (WSL2, matching CI's Go 1.25.0 / golangci-lint v2.11.4):
all clean — 0 test failures, 0 data races, 0 vet issues, 0 lint issues.

Which issue(s) this PR fixes:

Fixes #2761

Special notes for your reviewer:

When the tenant token is unavailable, requests now go out with the
original kube-system token instead of an empty one. The tenant apiserver
may still reject it (401/403) if RBAC doesn't map that identity, which is
the same outcome as today during the missing-secret window — but we no
longer actively destroy a token that could be valid (e.g., if the tenant
apiserver is configured to accept the kube-system identity).

No other callers of GetTenantToken/GetTenantNs are affected by this
change.

Does this PR introduce a user-facing change?

fix(yurthub): stop overwriting valid kube-system service-account tokens with an empty bearer token in tenant mode when the tenant token secret is not yet available, which was cutting off all edge traffic until the secret appeared.

other Note

- Only rewrite Authorization header when GetTenantToken() returns a valid, non-empty token.
- Avoids stripping credentials on k8s >=1.24 where legacy SA secrets aren't auto-created, preventing edge traffic disruption.
- Removed plaintext token logging for security.
@WorrierKhushal
WorrierKhushal requested a review from a team as a code owner August 20, 2026 21:48
@sonarqubecloud

Copy link
Copy Markdown

@codecov

codecov Bot commented Aug 20, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 46.22%. Comparing base (ddf22f7) to head (e0f83c3).
⚠️ Report is 7 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #2764      +/-   ##
==========================================
+ Coverage   46.20%   46.22%   +0.02%     
==========================================
  Files         405      405              
  Lines       27540    27569      +29     
==========================================
+ Hits        12724    12743      +19     
- Misses      13649    13655       +6     
- Partials     1167     1171       +4     
Flag Coverage Δ
unittests 46.22% <100.00%> (+0.02%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@WorrierKhushal

Copy link
Copy Markdown
Contributor Author

Hi @zyjhtangtang @luc99hen sir ,
fixes #2761.

yurthub was replacing a valid kube-system token with an empty one
whenever the tenant token wasn't ready yet (common on k8s 1.24+), which
cut off all edge traffic until the tenant secret appeared. Fixed by only
replacing the token when it's actually available, otherwise keeping the
original. Also removed a line that was logging tokens in plaintext.

Added a test covering all three cases: token available, token missing,
cache not synced. Verified with go test -race, go vet, and golangci-lint
— all clean.

If CI shows a failure on prepare_autonomy_tests, that's unrelated too —
traced it and confirmed this code path isn't even active in the e2e
cluster (tenant mode is off there). Same known flake as #1999.

Thanks for reviewing!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[BUG] Yurthub WithSaTokenSubstitute empties Authorization header when tenant token secret is missing

1 participant