Skip to content

HELM-613: Reject basic auth over non-HTTPS for Helm chart repositories #16317

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
sowmya-sl wants to merge 1 commit into
base: main
Choose a base branch
from
+61 −1
Open

HELM-613: Reject basic auth over non-HTTPS for Helm chart repositories #16317

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions pkg/helm/chartproxy/repos.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -228,6 +228,9 @@ func (b helmRepoGetter) unmarshallConfig(repo unstructured.Unstructured, namespa
}

if basicAuthReference != "" {
if h.URL.Scheme != "https" {
return nil, fmt.Errorf("basic auth is only supported for https repository %s", h.Name)
}
secret, err := b.CoreClient.Secrets(basicAuthRefNamespace).Get(context.TODO(), basicAuthReference, v1.GetOptions{})
if err != nil {
return nil, fmt.Errorf("Failed to GET secret %q from %q reason %v", basicAuthReference, basicAuthRefNamespace, err)
Expand Down
59 changes: 58 additions & 1 deletion pkg/helm/chartproxy/repos_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -531,6 +531,7 @@ func TestHelmRepoGetter_unmarshallConfig(t *testing.T) {
createSecret bool
namespace string
createNamespace bool
clusterScoped bool
}{
{
name: "Namespace present",
Expand Down Expand Up @@ -558,6 +559,7 @@ func TestHelmRepoGetter_unmarshallConfig(t *testing.T) {
createSecret: true,
namespace: "testing",
createNamespace: true,
clusterScoped: false,
},
{
name: "Namespace not present",
Expand All @@ -583,6 +585,61 @@ func TestHelmRepoGetter_unmarshallConfig(t *testing.T) {
wantsErr: false,
createSecret: true,
createNamespace: false,
clusterScoped: false,
},
{
name: "Basic auth is not supported for http repositories - cluster scope",
helmCRS: &unstructured.Unstructured{
Object: map[string]interface{}{
"apiVersion": "helm.openshift.io/v1beta1",
"kind": "HelmChartRepository",
"metadata": map[string]interface{}{
"namespace": "",
"name": "repo6",
},
"spec": map[string]interface{}{
"connectionConfig": map[string]interface{}{
"url": "http://localhost:9553",
"basicAuthConfig": map[string]interface{}{
"name": "fooSecret",
"namespace": "testing",
},
},
},
},
},
repoName: "repo6",
wantsErr: true,
createSecret: false,
createNamespace: false,
clusterScoped: true,
},
Comment thread
coderabbitai[bot] marked this conversation as resolved.
{
name: "Basic auth is supported only for https respositories",
Copy link
Copy Markdown
Contributor

@webbnh webbnh Apr 24, 2026

Choose a reason for hiding this comment

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

As Code Rabbit pointed out, you've got an extra 's' in "repositories", here.

helmCRS: &unstructured.Unstructured{
Object: map[string]interface{}{
"apiVersion": "helm.openshift.io/v1beta1",
"kind": "ProjectHelmChartRepository",
"metadata": map[string]interface{}{
"namespace": "",
"name": "repo4",
},
"spec": map[string]interface{}{
"connectionConfig": map[string]interface{}{
"url": "http://localhost:9553",
"basicAuthConfig": map[string]interface{}{
"name": "fooSecret",
"namespace": "testing",
},
},
},
},
},
repoName: "repo7",
wantsErr: true,
Copy link
Copy Markdown
Contributor

@webbnh webbnh Apr 24, 2026

Choose a reason for hiding this comment

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

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.

createSecret: false,
createNamespace: false,
clusterScoped: false,
},
}
for _, tt := range tests {
Expand Down Expand Up @@ -610,7 +667,7 @@ func TestHelmRepoGetter_unmarshallConfig(t *testing.T) {
Client: fake.K8sDynamicClient("helm.openshift.io/v1beta1", "HelmChartRepository", ""),
CoreClient: k8sfake.NewSimpleClientset(objs...).CoreV1(),
}
_, err := repoGetter.unmarshallConfig(*tt.helmCRS, tt.namespace, false)
_, err := repoGetter.unmarshallConfig(*tt.helmCRS, tt.namespace, tt.clusterScoped)
if tt.wantsErr {
require.Error(t, err)
} else {
Expand Down