HELM-613: Reject basic auth over non-HTTPS for Helm chart repositories#16317
HELM-613: Reject basic auth over non-HTTPS for Helm chart repositories#16317sowmya-sl wants to merge 1 commit intoopenshift:mainfrom
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@sowmya-sl: This pull request references HELM-613 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "5.0.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
openshift-ci
Bot
added
component/backend
📝 WalkthroughWalkthroughThe changes add scheme validation for Helm repositories configured with basic authentication, enforcing HTTPS URLs. When a repository has basic auth configuration but is not HTTPS, the unmarshalling now fails early with a descriptive error. The test suite is updated to parametrize the 🚥 Pre-merge checks | ✅ 9 | ❌ 3❌ Failed checks (3 warnings)
✅ Passed checks (9 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
pkg/helm/chartproxy/repos_test.go (1)
618-643: Tighten the HTTP negative test to assert the expected error reason.Line 639 currently accepts any error. Prefer checking that error text matches the HTTPS enforcement path, so this test can’t pass for unrelated failures. (Also, there’s a typo in the test name: “respositories”.)
Suggested assertion refinement
tests := []struct { name string helmCRS *unstructured.Unstructured repoName string wantsErr bool + errContains string createSecret bool namespace string createNamespace bool clusterScoped bool }{ @@ { - name: "Basic auth is supported only for https respositories", + name: "Basic auth is supported only for https repositories", @@ repoName: "repo7", wantsErr: true, + errContains: "basic auth is only supported for https repository", createSecret: false, createNamespace: false, clusterScoped: false, }, @@ - if tt.wantsErr { - require.Error(t, err) + if tt.wantsErr { + require.Error(t, err) + if tt.errContains != "" { + require.ErrorContains(t, err, tt.errContains) + } } else { require.NoError(t, err) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/helm/chartproxy/repos_test.go` around lines 618 - 643, Update the negative test so it asserts the specific HTTPS-enforcement error instead of any error and fix the typo in the test name: rename the case title string "Basic auth is supported only for https respositories" to "Basic auth is supported only for https repositories" and, where the test currently checks wantsErr == true, replace the loose check with an assertion that the returned error message contains both "basic auth" and "https" (or otherwise matches the specific enforcement error text from the validation logic); locate the failing test case by the helmCRS object literal and the test case name and add the tighter error message assertion there.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/helm/chartproxy/repos_test.go`:
- Around line 590-616: The HTTPS cluster-scope test case "Basic auth is
supported for https repositories - cluster scope" is asserting the wrong
outcome; update the test case so it expects success by setting wantsErr: false
and ensure the referenced secret exists by setting createSecret: true (leave
createNamespace as-is since cluster-scoped tests use an empty namespace). Modify
the test case fields wantsErr and createSecret in the HelmChartRepository test
table to reflect the intended success path.
---
Nitpick comments:
In `@pkg/helm/chartproxy/repos_test.go`:
- Around line 618-643: Update the negative test so it asserts the specific
HTTPS-enforcement error instead of any error and fix the typo in the test name:
rename the case title string "Basic auth is supported only for https
respositories" to "Basic auth is supported only for https repositories" and,
where the test currently checks wantsErr == true, replace the loose check with
an assertion that the returned error message contains both "basic auth" and
"https" (or otherwise matches the specific enforcement error text from the
validation logic); locate the failing test case by the helmCRS object literal
and the test case name and add the tighter error message assertion there.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: 3ed1785b-0dad-46eb-bc84-9d0436f607ab
📒 Files selected for processing (2)
pkg/helm/chartproxy/repos.gopkg/helm/chartproxy/repos_test.go
📜 Review details
🔇 Additional comments (2)
pkg/helm/chartproxy/repos.go (1)
231-233: Security check is correctly placed and fail-fast.Line 231-Line 233 correctly enforce HTTPS before any basic-auth secret retrieval, which is the right security boundary.
pkg/helm/chartproxy/repos_test.go (1)
534-535: Good improvement: scope is now part of test inputs.Passing
clusterScopedthrough to Line 670 is a solid testability improvement for cluster-vs-namespace behavior.Also applies to: 562-563, 588-589, 670-670
Enforce HTTPS when basic auth credentials are configured on HelmChartRepository and ProjectHelmChartRepository CRs to prevent sending credentials over plain HTTP. This adds server-side validation matching the existing frontend check.
sowmya-sl
force-pushed
the
fix-https-check
branch
from
a913d31 to
7044fa1
Compare
|
@sowmya-sl: all tests passed! Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
webbnh
left a comment
webbnh
left a comment
There was a problem hiding this comment.
The change looks good to me, but Code Rabbit identified a couple of things that you should consider addressing.
/lgtm
| clusterScoped: true, | ||
| }, | ||
| { | ||
| name: "Basic auth is supported only for https respositories", |
There was a problem hiding this comment.
As Code Rabbit pointed out, you've got an extra 's' in "repositories", here.
| }, | ||
| }, | ||
| repoName: "repo7", | ||
| wantsErr: true, |
There was a problem hiding this comment.
I concur with Code Rabbit that it would be better if wantsErr identified the expected error rather than being just a boolean. (That is, it could be nil or an empty string for the non-error case, and an error or a string to match in the error case.) That way, the test would only pass if it received the right error response, and we wouldn't get a false positive in other cases.
Since none of the existing test cases test error paths, it wouldn't be too hard to make this change.
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: sowmya-sl, webbnh 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 |
3 participants
Enforce HTTPS when basic auth credentials are configured on HelmChartRepository and ProjectHelmChartRepository CRs to prevent sending credentials over plain HTTP. This adds backend server-side validation matching the existing frontend check.
Summary by CodeRabbit
Bug Fixes
Tests