Skip to content

fix: log security warning when auth disabled and remove API key from 401 response#391

Open
oindrilakha12-ui wants to merge 1 commit into
openkruise:masterfrom
oindrilakha12-ui:fix/auth-bypass-nil-keys-and-key-leakage
Open

fix: log security warning when auth disabled and remove API key from 401 response#391
oindrilakha12-ui wants to merge 1 commit into
openkruise:masterfrom
oindrilakha12-ui:fix/auth-bypass-nil-keys-and-key-leakage

Conversation

@oindrilakha12-ui
Copy link
Copy Markdown

What

Two security hardening changes to pkg/servers/e2b/routes.go:

  1. Log a security warning when authentication is disabled — when keyCfg is nil, every request silently received admin-level AnonymousUser access with no operator visibility. Now logs an Error-level message on every request so misconfigured deployments are immediately detectable in logs/alerts.

  2. Stop echoing submitted API key in 401 responses — the previous error message "Invalid API Key: <submitted-key>" reflected the caller's key back in the response body, aiding brute-force key validation. Replaced with the static message "Invalid API Key".

Why

  • A single Helm misconfiguration (missing key Secret) silently exposed all sandbox management APIs with admin privileges
  • Operators had no signal that auth was disabled unless they audited the startup config
  • Leaking the submitted key in 401 aids attackers in confirming partial key guesses

Changes

  • routes.go: Add klog.Error warning in CheckApiKey when sc.keys == nil
  • routes.go: Replace fmt.Sprintf("Invalid API Key: %s", apiKey) with "Invalid API Key"
  • routes_test.go: Update test expectations to match new generic 401 message

Testing

  • go test github.com/openkruise/agents/pkg/servers/e2b -run TestCheckApiKey
  • go build ./pkg/servers/e2b/...

Fixes #389

@kruise-bot
Copy link
Copy Markdown

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign furykerry for approval by writing /assign @furykerry in a comment. For more information see:The Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@codecov
Copy link
Copy Markdown

codecov Bot commented May 14, 2026

Codecov Report

❌ Patch coverage is 50.00000% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 76.50%. Comparing base (5d3212b) to head (d736ac4).
⚠️ Report is 3 commits behind head on master.

Files with missing lines Patch % Lines
pkg/servers/e2b/routes.go 50.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #391      +/-   ##
==========================================
+ Coverage   75.90%   76.50%   +0.60%     
==========================================
  Files         145      146       +1     
  Lines       10626    10685      +59     
==========================================
+ Hits         8066     8175     +109     
+ Misses       2212     2169      -43     
+ Partials      348      341       -7     
Flag Coverage Δ
unittests 76.50% <50.00%> (+0.60%) ⬆️

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

☔ View full report in Codecov by Sentry.
📢 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.

@oindrilakha12-ui oindrilakha12-ui force-pushed the fix/auth-bypass-nil-keys-and-key-leakage branch from 7a49abc to acc880f Compare May 14, 2026 14:22
…401 response

When keyCfg is nil, CheckApiKey silently grants every request admin-level
AnonymousUser access with no indication to operators. This change adds a
prominent error-level log on every such request so operators can detect
misconfigured deployments immediately from logs.

Additionally, the 401 error response body previously echoed the submitted
API key value back to the caller, aiding brute-force key validation. Replaced
with a static 'Invalid API Key' message.

Updated test expectations to reflect the new generic 401 message.

Fixes openkruise#389
@oindrilakha12-ui oindrilakha12-ui force-pushed the fix/auth-bypass-nil-keys-and-key-leakage branch from acc880f to d736ac4 Compare May 14, 2026 14:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

bug: authentication fully bypassed when keyCfg is nil, all endpoints exposed as admin

2 participants