Skip to content

Conversation

@yrobla
Copy link
Contributor

@yrobla yrobla commented Nov 27, 2025

instead of falling back to unauthenticated, make the process fail when there is a problem in discovering auth config

@yrobla yrobla requested review from JAORMX, Copilot and jhrozek November 27, 2025 10:27
@github-actions github-actions bot added the size/XS Extra small PR: < 100 lines changed label Nov 27, 2025
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR implements a critical security improvement by changing the authentication failure handling from "fail open" (falling back to unauthenticated access) to "fail closed" (refusing to create a backend). When an MCPServer references authentication configuration that fails to load (missing auth config or secrets), the system now returns nil instead of silently proceeding without authentication.

Key Changes:

  • Modified mcpServerToBackend to return nil when auth discovery fails instead of logging a warning and continuing
  • Added nil check in GetWorkloadAsVMCPBackend to handle auth discovery failures
  • Updated error logging from Warnf to Errorf to reflect the severity of auth failures
  • Updated tests to expect nil backend when auth config or secrets are missing

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File Description
pkg/vmcp/workloads/k8s.go Implements fail-closed behavior by returning nil from mcpServerToBackend when auth discovery fails, and adds nil check in GetWorkloadAsVMCPBackend
pkg/vmcp/workloads/k8s_test.go Updates test expectations for TestDiscoverAuth_AuthConfigNotFound and TestDiscoverAuth_SecretNotFound to verify nil backend is returned

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@yrobla yrobla force-pushed the fix/defensive_auth branch from e984709 to 65d73b6 Compare November 27, 2025 10:40
@github-actions github-actions bot added size/XS Extra small PR: < 100 lines changed and removed size/XS Extra small PR: < 100 lines changed labels Nov 27, 2025
@codecov
Copy link

codecov bot commented Nov 27, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 56.30%. Comparing base (66fff4a) to head (f339996).
⚠️ Report is 2 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2758      +/-   ##
==========================================
+ Coverage   56.28%   56.30%   +0.01%     
==========================================
  Files         319      319              
  Lines       30749    30753       +4     
==========================================
+ Hits        17308    17315       +7     
+ Misses      11953    11951       -2     
+ Partials     1488     1487       -1     

☔ 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.

instead of falling back to unauthenticated, make the process
fail when there is a problem in discovering auth config
@yrobla yrobla force-pushed the fix/defensive_auth branch from 65d73b6 to f339996 Compare November 27, 2025 10:50
@github-actions github-actions bot added size/XS Extra small PR: < 100 lines changed and removed size/XS Extra small PR: < 100 lines changed labels Nov 27, 2025
@yrobla yrobla merged commit 6e0e96c into main Nov 27, 2025
33 checks passed
@yrobla yrobla deleted the fix/defensive_auth branch November 27, 2025 11:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size/XS Extra small PR: < 100 lines changed

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants