Helm chart mangles backslashes in passwords

[dimbleby]

Describe the bug

The helm operator's helm chart adds double quotes around password strings - which gives them a different meaning than was intended.

To Reproduce

values.yaml:

helm:
  versions: v3
configureRepositories:
  enable: true
  repositories:
    - name: fluxcd
      password: this-\-is-a-literal-backslash
      url: https://charts.fluxcd.io
      username: dimbleby

Then helm template helm-operator fluxcd/helm-operator -f values.yaml.

Extract the data in the resulting flux-helm-repositories secret and base64-decode it, the result is this:

generated: 0001-01-01T00:00:00Z
repositories:
- name: fluxcd
  url: https://charts.fluxcd.io
  cache: /var/fluxd/helm/repository/cache/fluxcd-index.yaml
  caFile: ""
  certFile: ""
  keyFile: ""
  password: "this-\-is-a-literal-backslash"
  username: "dimbleby"

The problem is that in YAML, backslashes in a double-quoted string act as an escape character. So although I passed in a password with a literal backslash in it, I have ended up with a password that contains an invalid escape sequence.

Equivalently I could have set password: "this-\\-is-a-literal-backslash" in values.yaml: this results in the exact same faulty repositories.yaml.

Expected behavior

The password that is provided in values.yaml should be passed through unaltered.

Logs

Additional context

Possibly the solution is as simple as: don't add the double quotes in the template here?

Edit: actually it looks as though the expected thing to do is to pass the string through quote - per https://helm.sh/docs/chart_template_guide/functions_and_pipelines/.

Presumably the same issue is true for other fields here, but the password is where I hit it and where it seems most likely to be hit.

[dimbleby]

I've submitted #630.

I understand that this project is in maintenance mode so perhaps you are not likely to release this fix; but if you are able to publish a 1.4.1 chart including this that would make me happy...

[yebyen]

We can definitely get this merged! I appreciate you taking the time to document and explain the issue. Thanks very much.

[yebyen]

I may have spoke too soon, I have to validate that this will not represent a breaking change. It looks like it might.

I appreciate it either way, will get back to you before the next release is cut.

[dimbleby]

Thanks!

I would think this is breaking only for folk who are relying on bugged behaviour - which normally wouldn't "count" as breaking, though it certainly wants calling out in the changelog.

Eg we ourselves have written some code to replace \ with \\ in any password that we pass here, so as to work around the problem: and that workaround will definitely want removing if and when the helm chart is fixed.

[dimbleby]

Ran into another example that's broken by the bad quoting today: a password that itself contains a quotation mark produces faulty configuration.

[kingdonb]

I do think this sounds like a bug.

However I have to point out the Flux v1 Migration Timetable specifies that Helm Operator v1 "maintenance" after June 30 is limited to CVEs.

The specific language I'm looking at:

Superseded

• All existing projects encouraged to migrate to Flux 2, and report any bugs
• Flux 1 Helm Operator code freeze – no further updates except CVEs

I have to limit what I can merge into Helm Operator based on what was agreed here by the dev team. If there is a way this issue represented a security risk, I could see merging it based on the language above. (Can you frame it as a CVE in any way?)

If there is an issue with Helm Controller that blocks your migration, I would be much more interested to fix that and help you get migrated rather than try to justify another Helm Operator release when there is no CVE. There is acknowledged that there can be bugs in Helm Operator which cannot be fixed. This one, we have a PR and I would be inclined to merge it, as long as the other maintainers don't object. I would be less inclined to devote any resources to fixing a bug if you hadn't provided a fix.

If you would consider joining the Flux CNCF Dev Meeting tomorrow you can get an audience with the dev team and we can get the last word on the issue of merging or not merging this! If you can't make it, then I will still try to find out, but without your presence it unfortunately may wind up falling right off the agenda as it will have to go at the bottom of the priority list.

https://fluxcd.io/community/#meetings the time is 11:00 AM Eastern Time / 15:00 UTC, with the Zoom link on the calendar!

[dimbleby]

I'm not going to make it to that meeting, sorry.

We can update our workaround to do a more thorough job of pre-escaping the password, so that when the helm chart adds the ~single~ quotes around it we get the right value back after all. So my motivation at this point is more about doing the right thing for this project than making our particular use case work.

I think we agree that the fix is clearly correct, and not risky; and I hope I've made it as easy as I can for you just to accept the PR. But I understand that Helm Operator is in its twilight; and - though I hope for the opposite - you are certainly within your rights to declare that you simply won't be accepting such fixes.

I will watch with interest.

[kingdonb]

It's on the agenda here:

Flux Dev Agenda & Meeting notes

We are halfway through the meeting now, I will try to get this question answered. Two out of three core maintainers are in attendance today

[kingdonb]

This issue was aired during the meeting. There were no strong opinions voiced one way or another, and the Helm Operator maintainer was unfortunately not in attendance today, but we will get back with him soon and decide one way or another whether it can be merged, and if it needs a MAJOR increment in the helm chart version or not.

I think what would be a fair compromise is publishing a development chart with a 2.0.0-prerelease tag, since that way we can accept other potentially breaking bugfixes without compromising semver. I don't know if we will go for that, but it seems like the most well-engineered way that I can think of to release a change like this. What was expressed on an unrelated topic is that Flux project takes our commitment to honor semver very seriously.

Merging this change in a devel branch and cutting a new chart release as prerelease does not necessarily conflict with our specific and public commitments to SemVer in Flux's legacy projects. I don't know if we will ever release Helm Operator Chart 2.0.0 as a final version, I would keep my expectations low because for some time now, the focus of the effort is on Helm Controller and Flux v2, and it seems clear we really need all outward projections of the Flux project to expressly honor that.

But at the same time if you are still getting value out of Helm Operator and willing to contribute changes, it would be a great shame and negative mark on us if we did not also be sure to try our best to find a way to honor that! Thanks again for your PR and the friendly discussion.

[kingdonb]

Sorry if your issue remains unresolved. The Helm Operator is in maintenance mode, we recommend everybody upgrades to Flux v2 and Helm Controller.

A new release of Helm Operator is out this week, 1.4.4.

We will continue to support Helm Operator in maintenance mode for an indefinite period of time, and eventually archive this repository.

Please be aware that Flux v2 has a vibrant and active developer community who are actively working through minor releases and delivering new features on the way to General Availability for Flux v2.

In the mean time, this repo will still be monitored, but support is basically limited to migration issues only. I will have to close many issues today without reading them all in detail because of time constraints. If your issue is very important, you are welcome to reopen it, but due to staleness of all issues at this point a new report is more likely to be in order. Please open another issue if you have unresolved problems that prevent your migration in the appropriate Flux v2 repo.

Helm Operator releases will continue as possible for a limited time, as a courtesy for those who still cannot migrate yet, but these are strongly not recommended for ongoing production use as our strict adherence to semver backward compatibility guarantees limit many dependencies and we can only upgrade them so far without breaking compatibility. So there are likely known CVEs that cannot be resolved.

We recommend upgrading to Flux v2 which is actively maintained ASAP.

I am going to go ahead and close every issue at once today, Thanks for participating in Helm Operator and Flux! 💚 💙