fix: gate clusterview auth skip behind SKIP_HUB_PERMISSION_CHECK env var - #765
Conversation
WalkthroughThe HCP proxy now fails closed when clusterview is unavailable unless permission checks are explicitly skipped. Unit tests cover both branches, shared test setup enables skipping, and the e2e deployment sets the skip environment variable. ChangesHub permission enforcement
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 9 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (9 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
| p.log.Info("clusterview API not installed, skipping hub permission check (SKIP_HUB_PERMISSION_CHECK=true)") | ||
| return nil | ||
| } | ||
| return fmt.Errorf("clusterview API not installed and SKIP_HUB_PERMISSION_CHECK is not set") |
There was a problem hiding this comment.
Can we change to "UserPermission is required in production. Please ensure the cluster has UserPermission configured." because SKIP_HUB_PERMISSION_CHECK is for our dev and ci
Previously, when the clusterview API was absent the proxy unconditionally skipped the hub permission check. This is a security risk on production clusters where a partial MCE install could leave the userpermissions API missing, bypassing authorization. Now the skip only happens when SKIP_HUB_PERMISSION_CHECK=true is explicitly set in the pod environment (kind E2E only). On production clusters without the env var, the proxy fails closed. Co-authored-by: Cursor <cursoragent@cursor.com>
45d3803 to
7033c4a
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/manager/hcp_proxy_test.go (1)
481-496: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winAdd meaningful failure messages to the branch assertions.
pkg/manager/hcp_proxy_test.go#L481-L496: explain that the absent API must be allowed only when the explicit skip is enabled.pkg/manager/hcp_proxy_test.go#L499-L515: explain that a missing API must deny access and report the production configuration requirement.Proposed fix
- assert.NoError(t, err) + assert.NoError(t, err, "explicit skip must allow kind environments without the clusterview API") ... - assert.Error(t, err) - assert.Contains(t, err.Error(), "UserPermission is required in production") + assert.Error(t, err, "missing clusterview API must fail closed when skipping is disabled") + assert.Contains(t, err.Error(), "UserPermission is required in production", + "the error must identify the required production configuration")As per coding guidelines, “Assertions should include meaningful failure messages.” As per path instructions, assertion messages must explain what was expected and why it matters.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/manager/hcp_proxy_test.go` around lines 481 - 496, Add meaningful failure messages to the assertions in Test_checkHubPermission_WhenClusterviewAPIAbsent_WithEnvVar_ItShouldSkipAndAllow, stating that an absent API is allowed only when the explicit skip setting is enabled. Also update the assertions in pkg/manager/hcp_proxy_test.go lines 499-515 to state that a missing API must deny access and report the required production configuration.Sources: Coding guidelines, Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@pkg/manager/hcp_proxy_test.go`:
- Around line 481-496: Add meaningful failure messages to the assertions in
Test_checkHubPermission_WhenClusterviewAPIAbsent_WithEnvVar_ItShouldSkipAndAllow,
stating that an absent API is allowed only when the explicit skip setting is
enabled. Also update the assertions in pkg/manager/hcp_proxy_test.go lines
499-515 to state that a missing API must deny access and report the required
production configuration.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: stolostron/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: ef1d8567-748f-49a2-823e-b2a8119f16f5
📒 Files selected for processing (3)
pkg/manager/hcp_proxy.gopkg/manager/hcp_proxy_test.gotest/e2e/addon-manager-deployment.yaml
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: kurwang, yiraeChristineKim The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
2 participants
Summary
SKIP_HUB_PERMISSION_CHECK=trueenvironment variable.userpermissionsAPI is missing — prevents security bypass on partial MCE installs.SKIP_HUB_PERMISSION_CHECK=truein the kind E2E deployment where the API is intentionally absent.Context
Follow-up to PR #764 per reviewer feedback from @yiraeChristineKim on backplane-operator PR #3716: the unconditional skip when the clusterview API is absent could be a security issue if MCE fails to install only the userpermissions component.
Changes
pkg/manager/hcp_proxy.go— checkos.Getenv("SKIP_HUB_PERMISSION_CHECK") == "true"before skippingpkg/manager/hcp_proxy_test.go— split test into two: with env var (skip allowed) and without (fail closed)test/e2e/addon-manager-deployment.yaml— addSKIP_HUB_PERMISSION_CHECK=trueenv var for kind E2ETest plan
Test_checkHubPermission_WhenClusterviewAPIAbsent_WithEnvVar_ItShouldSkipAndAllowTest_checkHubPermission_WhenClusterviewAPIAbsent_WithoutEnvVar_ItShouldFailClosedMade with Cursor
Summary by CodeRabbit
Bug Fixes
UserPermissionis not configured, rather than silently skipping authorization.Configuration