Document YAML injection risks in chart templates#2158
Conversation
Signed-off-by: Shaohan He <275881371+Shaohan-He@users.noreply.github.com>
|
Sure, let's keep it small and focused for now. But wouldn't it be a better fit for the YAML Techniques appendix? Just because it's YAML related? |
| Using `toYaml` with the correct indentation preserves the intended YAML | ||
| structure, but it does not make arbitrary content safe or validate that the | ||
| result is appropriate for a particular Kubernetes field. Chart authors should | ||
| validate and constrain accepted values when they may come from untrusted | ||
| sources. |
There was a problem hiding this comment.
It's good to mention validation. But how to do it, exactly? I could not find anything about validation elsewhere in the Helm docs.
For comparison, Terraform/OpenTofu have a dedicated mechanism for validating input variables. And they have a dedicated guide that covers all sorts of additional validation mechanisms.
I'm not an active Helm user, but afaik, Helm does not have similar mechanisms. If I'm wrong here, how about linking to existing documentation?
Of course, there's always the possibility of validating inside the template itself. Do go templates recommend a standard way to do validation? I couldn't find it in the package docs.
In my demo project, I've used a rather crude validation approach: I added some validation logic in the beginning, and I simply emit an empty YAML file, if any of the validations fail. Could not find a more idiomatic approach, e.g. failing the template run through an exception or similar.
I can do more research on this... I feel that template authors need more guidance here.
|
|
||
| ```yaml | ||
| data: | ||
| drink: {{ .Values.favorite.drink | quote }} |
There was a problem hiding this comment.
This works fine for values that map to a single YAML scalar.
But it gets tricky, when you want construct a YAML string from multiple values. In my demo project, I've recommended the following approach:
{{ print "static text " .value " more static text " .otherValue | toYaml | nindent 4 }}
I guess there are many other alternatives, e.g. using printf. I think we should mention at least one of them.
| Values may come from users or other untrusted sources. Rendering an untrusted | ||
| string directly into a template can allow the string to change the structure of | ||
| the generated YAML. For example, this template renders a scalar value without | ||
| quoting it: |
There was a problem hiding this comment.
It would be helpful to explain the risk more thoroughly. It may not be a risk at all in certain scenarios. E.g. when I'm writing a Helm chart that I'll only use myself, and I always know which values will go into it, I might get away without bullet-proof YAML encoding.
But in my experience, some values will ultimately come from somewhere unexpected. E.g. some orchestration tool. Which in turn may pull some values from a dynamic source, e.g. label names that someone has entered into some web UI. See the problem statement in my demo project.
On the other hand, it might be worthwhile mentioning the other benefits of consistent YAML encoding. Templates will be more robust against non-malicious input values that just contain YAML control characters. Proper encoding helps prevent such errors and can save a lot of time debugging template code.
| ```yaml | ||
| metadata: | ||
| annotations: | ||
| {{- .Values.podAnnotations | toYaml | nindent 4 }} |
There was a problem hiding this comment.
I think all this different whitespace handling is getting a little confusing here. First we remove a line-break through go template syntax {{-, then we re-add the line-break by using nintent instead of indent.
I think this would be better:
metadata:
annotations: {{ .Values.podAnnotations | toYaml | nindent 4 }}
|
|
||
| A value containing YAML syntax, such as a newline followed by another key, may | ||
| then add unexpected data to the rendered manifest. Quote string values so that | ||
| YAML treats the result as a scalar: |
There was a problem hiding this comment.
What about other types of scalars (numbers, bools, null)?
I forgot if kubectl or the k8s API do type coercion here? From a pure YAML perspective, 42 is different from "42". Will do some more testing with Helm asap...
Three alternatives to address this issue, if necessary:
A. Use toYaml and nindent for scalar values, too. (May not work, if Helm treats values as strings only, need to test...)
B. Use quote and explicit YAML type conversion, such as !!int {{ .integerValue | quote }}.
C. Use type validation and emit numbers, bools, and null without quoting. (Not sure what's the best way to validate, see comment further below.)
|
I've left review comments. Some of them are based on the YAML encoding recommendations from my demo project. I've originally written these recommendations ages ago for the general case of YAML encoding. Some of them may not apply to how Helm and Kubernetes process YAML. Will try to do more reading and testing, and provide more feedback... |
|
Side-note: I've done all my testing on Helm 3 so far. Does Helm 4 bring any relevant changes? |
Signed-off-by: Shaohan He <275881371+Shaohan-He@users.noreply.github.com>
|
Thanks for the careful review, @meeque . You are right — I overlooked that this belongs more naturally in the YAML Techniques appendix. Sorry about that, and I appreciate the detailed feedback. I pushed a follow-up commit that moves the guidance from The updated version now:
I also kept the Please let me know if this now fits the direction you had in mind. |
| This guidance is for string values. Other scalar types, such as integers and | ||
| booleans, may need to remain unquoted when the Kubernetes field expects that | ||
| type. Define the expected type in the chart values and render it accordingly. | ||
|
|
There was a problem hiding this comment.
The trouble is that chart authors generally cannot know what type a value is. So omitting the quote is dangerous.
Further above, this page already mentions how to "force a particular type inference using YAML node tags". I think this should be the preferred approach for other scalar types. E.g.:
listen: foo: !!bool {{ .Values.server.enable | quote }}
port: !!int {{ .Values.server.port | quote }}
Here, we first generate YAML strings securely. Then we convert them to the expected YAML scalar type. If the string values do not match the specified YAML type, the YAML parser will fail with an error.
Of course, validation could alternatively be done through values.schema.json. But we only mention that further below.
Personally, I'm a big fan of encapsulation. The component (in this case the template) that generates structured data (in this case YAML), should generate secure output under all circumstances. It should not rely on validation happening elsewhere. Otherwise there's a risk that future changes in one place (the JSON schema) might break stuff in some other place (the template).
| When a value is intentionally structured data, serialize the complete value and | ||
| indent it for its location in the manifest: | ||
|
|
||
| ```yaml | ||
| metadata: | ||
| annotations: | ||
| {{- .Values.podAnnotations | toYaml | nindent 4 }} | ||
| ``` |
There was a problem hiding this comment.
We're still in a section about strings here. Might be preferable to move this part to somewhere else in this page.
| Using `toYaml` with the correct indentation preserves the intended YAML | ||
| structure, but it does not make arbitrary content safe or validate that the | ||
| result is appropriate for a particular Kubernetes field. For values that may | ||
| come from untrusted sources, chart authors should constrain the accepted values. | ||
| Helm can validate chart values with a | ||
| [`values.schema.json`](/topics/charts.mdx#schema-files) schema, and templates can | ||
| fail rendering with functions such as | ||
| [`required`](/chart_template_guide/function_list.mdx#required) or | ||
| [`fail`](/chart_template_guide/function_list.mdx#fail) for simple checks. |
There was a problem hiding this comment.
Again, we're still in a section about strings. The notes on validation don't seem to fit well here. Maybe they should get their own section?
|
Thanks for the changes, @Shaohan-He! I wasn't aware of I'm fine with sticking to the That said, I'm having doubts that the YAML Techniques appendix is a good place for the new contents after all. Mostly because the existing structure of this page is rather confusing:
I guess the root cause is that YAML syntax and terminology is tremendously confusing itself. Not sure if restructuring this whole page will make matters better or worse? Or a separate appendix about "template security" after all? |
Signed-off-by: Shaohan He <290298661@qq.com>
4f4df45 to
179c7e8
Compare
|
Thanks again for the careful follow-up, @meeque ! I agree that the broader information architecture question is still a bit tricky here. The existing YAML Techniques appendix already mixes scalar/collection terminology with a separate strings section, so I do not want to assume that this PR settles the best long-term location for this guidance. For this update, I tried a smaller compromise: keep the PR focused, but move the new guidance out of the The new text is now split near the scalars/collections discussion into:
Locally, If you think this still belongs in a separate template security appendix, or somewhere else in the guide, I am happy to adjust the structure further. |
Summary
Addresses #2154.
This PR adds a focused section to the Chart Template Guide documenting YAML injection risks when rendering untrusted values directly into chart templates.
It intentionally keeps the scope small:
quote;toYamlandnindent;This does not attempt to introduce a broader chart security guide in the first pass.
Testing
yarn buildgit diff --check