helm: add authentication support to Helm repository operations#5120
helm: add authentication support to Helm repository operations#5120Athang69 wants to merge 1 commit into
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: Athang69 The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
There was a problem hiding this comment.
Pull request overview
This PR extends the backend Helm repository add/update endpoints to support Helm repository authentication and TLS-related settings by plumbing additional request fields through to helm.sh/helm/v3/pkg/repo.Entry.
Changes:
- Expanded
AddUpdateRepoRequestto include username/password and TLS/auth-related fields supported by Helm’srepo.Entry. - Updated repository add/update flows to pass the full request through to Helm’s repository entry creation/update.
Comments suppressed due to low confidence (1)
backend/pkg/helm/repository.go:141
- This PR introduces persisted credentials (e.g.,
Password, TLS key material) in the Helm repo config, but the file is written withdefaultNewConfigFileMode(currently 0644). That makes credentials world-readable on multi-user systems. Use a restrictive mode (e.g., 0600) when writing/creating the repo config (and consider tightening the directory mode as well).
repoFile.Update(newRepo)
err = repoFile.WriteFile(settings.RepositoryConfig, defaultNewConfigFileMode)
if err != nil {
|
@sniok @illume |
skoeva
left a comment
There was a problem hiding this comment.
hi, could you squash the linting fixes into one atomic commit? and make sure to address the failing test in the CI
85f7e4f to
b76c7f8
Compare
b76c7f8 to
3c1ffc7
Compare
ad7b353 to
655ad01
Compare
|
Hi @skoeva I’ve squashed the linting fixes into a single atomic commit as requested and also fixed the failing CI test |
|
@illume Resolved the copilot's review PTAL. |
062d4cd to
b2ef92f
Compare
b2ef92f to
00b3af9
Compare
00b3af9 to
18d980a
Compare
illume
left a comment
There was a problem hiding this comment.
Thanks for working on this.
Would you mind addressing the open Copilot review comments? Please mark each comment as resolved after addressing it.
|
@illume The exported UpdateRepository function is only called from its own HTTP handler within the same file with no external callers in the codebase, so no deprecation cycle is needed. The godoc has been updated to document the new 404 behavior explicitly so any future external caller is aware |
illume
left a comment
There was a problem hiding this comment.
Thank you.
It's looking very good now.
Just left a few notes about some missing function docs. I think after that it looks good to go.
Signed-off-by: Athang69 <athangkali21@gmail.com>
18d980a to
a91fcfa
Compare
|
@illume Added doc comments PTAL. |
Summary
This PR adds authentication support to Helm repository add and update operations by extending
AddUpdateRepoRequestto include auth fields supported byhelm.sh/helm/v3/pkg/repo.Entry.Changes
AddUpdateRepoRequeststruct withusername,password,insecureSkipTLSverify, andpassCredentialsAllfields.certFile,keyFile, andcaFileare intentionally omitted as accepting filesystem paths from HTTP clients would allow arbitrary local file reads on the server; these values are preserved from the existing repo entry only.Validate()method toAddUpdateRepoRequestto reject emptynameorurlwith HTTP 400.addRepositoryto accept the full request struct and pass auth fields torepo.Entry.UpdateRepositoryto accept the full request struct, preserve existing auth fields, and return HTTP 404 if the named repository does not exist.updateRepositoryWithRequestto keep it unexported since there are no external callers outside this file.applyRequestFieldshelper to apply optional auth fields to arepo.Entry.findRepoEntryhelper to look up an existing repo entry by name.createFileIfNotThereto properly close the file handle and propagate non-IsNotExist errors fromos.Stat.ListRepoandAddRepohandlers to buffer the JSON response before writing headers, preventing malformed responses on encode errors.RemoveRepositoryto pass the repository name via structured metadata instead of as the message string.Steps to Test
HEADLAMP_CONFIG_ENABLE_HELM=true ./headlamp-serverusernameandpasswordfields in the body.go test -v -p 1 ./pkg/helm/...and confirm all tests pass.Notes for the Reviewer
addRepositoryis unexported so its signature change has no impact outside this file.updateRepositoryWithRequestis unexported.UpdateRepositoryremains the single exported function and delegates to it.certFile,keyFile, andcaFileare not accepted from the HTTP request body. This is a deliberate security decision to prevent server-side path traversal via authenticated API calls.TestListChartwas already failing before this PR due to a missing local Helm cache file. This PR does not introduce or fix that failure.