HIP for download cache (charts & provenance files) #185

dragonchaser · 2021-05-07T14:30:11Z

This is a proposal for adding a download-cache for helm charts and provenance files to helm.

Co-authored-by: Matt Farina matt.farina@suse.com
Signed-off-by: Matt Farina matt.farina@suse.com

Co-authored-by: Matt Farina <matt.farina@suse.com> Signed-off-by: Matt Farina <matt.farina@suse.com> Signed-off-by: Christian Richter <crichter@suse.com>

joejulian · 2021-05-13T16:40:35Z

Maybe not for this iteration, but what about storing chart in a configmap and using that first? This would allow a shared cache that could be used by CD systems as well as by multiple users or even the same user on multiple machines.

dragonchaser · 2021-05-17T07:51:16Z

@joejulian Then there is no advantage for admins maintaining multiple clusters at once.

bacongobbler

Thanks for writing this up. I think this is a great candidate as a HIP. Approved as HIP 13. Please change the name accordingly.

Out of curiosity, have you considered how the cache would handle the chart provenance file? If the user provides the --sign flag with helm install, do we cache the signature along with the chart? What security concerns would that bring?

mattfarina · 2021-06-02T13:42:25Z

@bacongobbler I think I missed that this wasn't in there. The gist is that there is a provenance cache as well. It is separate but follows the same patterns. Since provenance files contain the chart archive hash there is a complete coupling of a provenance file to a specific archive.

If it helps, we have already been working on an implementation. You can find a provenance cache commit here.

We will update the HIP to account for this.

mattfarina · 2021-06-02T13:49:11Z

@joejulian That is an interesting idea. There are a couple reasons I would not want to do it...

How would it work with helm template and other commands specifically designed to work without being connected to a cluster. Not connecting to a cluster is part of the security model for some commands. The cache wouldn't work there. The UX would also not work as a cache in a cluster for these commands would mean needing to make sure you were pointed at the right cluster before running the commands or risk polluting another clusters cache. If you were pointed at a cluster for an open source project and running helm template for a company project you could even risk leaking some company details. We wouldn't want that either.
A cache provides a performance boost when performing operations. Checking and pulling from a k8s cluster would go over a network and still need to download something. We would need to look at the performance difference of a Helm repository vs a k8s cluster. When both are handled over HTTP. For many cases, the cache won't be a performance boost. Something on local disk would be because it skips all the network traffic.

This is a cache rather than a common store.

Thoughts?

Signed-off-by: Christian Richter <crichter@suse.com>

bacongobbler

Looking good! Just a few questions for clarity

bacongobbler · 2021-06-17T16:05:17Z

hips/hip-0013.md

+
+The existing Helm cache directory is used for the cache storage. This location needs to exist for storing index files locally. There is no impact to the cache location.
+
+Those who use the `ChartDownloader` from the Helm SDK will have two new properties to define when instantiating it.


Are these properties optional?

What do these properties do when set?

What happens if the user does not set these new properties?

Same question. I feel like this needs some clarification to show that it is not a breaking change. In order to not be breaking, existing userland code must not need to be changed.

If you look at the chart downloader you'll see there is a property for the Repository Cache. This is the specific cache for the repository and not the cache location in general. See my other comment on the environment variables for the way those are setup.

So, the idea is to pass in the locations for the new cache here following the existing pattern the repository cache uses. The design is simply based to follow existing patterns. We might want to discuss this in a dev call.

The new properties would need to be optional. If they are not set a default location would then need to be used.

bacongobbler · 2021-06-17T16:06:58Z

hips/hip-0013.md

+
+## Motivation
+
+There are three primary motivations for leveraging a cache the impact [both the application operator and the application distributor](https://github.com/helm/community/blob/main/user-profiles.md).


typo: that* impact

bacongobbler · 2021-06-17T16:07:32Z

hips/hip-0013.md

+
+When repeatedly installing, upgrading, or showing information about a chart version within a repository, Helm will download the chart each time. When downloading the chart Helm currently puts the chart into the repositories cache directory (where index files are cached) but never leverages the cache. Helm cannot use the chart version in the cache because the identifier is the name and charts version (e.g., `wordpress-1.2.3.tgz`). A name and version is insufficient as the same chart and version could come from two or more different repositories and have different content.
+
+The first motivation is to make the cache useful by handling it in a manner where the downloaded chart's identifier can be make unique enough to be useful.


typo: made*

bacongobbler · 2021-06-17T16:09:09Z

hips/hip-0013.md

+
+The [cache provided for the Helm client](https://github.com/rancher/sandbox/gofilecache) is based on the Go build cache and uses the file system. It uses the first two characters of the hash as the top directory followed by the rest of the hash for the second level directory. This is similar to the way in with Git stores objects on disk. Both the chart and provenance caches will live within the cache directory Helm already uses.
+
+The Helm Client would have two new environment variables and flags to specify the locations for the provenance files and chart archive cache. These would follow the same format as the current environment variable use for individual caching locations.


For posterity, can you list those environment variable names here? That way users can refer to this document instead of looking up the source code.

I'm not entirely sure that allowing caches to be outside of a single top-level directory is a good idea, and I don't see this pattern replicated in similar systems. This is hard to debug, hard to trace, and could lead to bewildering behavior when multiple users/tools are attempting to share the same cache.

Can you please explain the rationale for allowing this very specific feature of Helm to be configured?

Helm currently has HELM_CACHE_HOME and HELM_REPOSITORY_CACHE. The HELM_REPOSITORY_CACHE is where index files and charts are downloaded to. The two additional ones were to continue the pattern of allowing them to be overridden.

HELM_CACHE_HOME currently provides a default base for caching.

HELM_REPOSITORY_CACHE, by default, is relative to HELM_CACHE_HOME.

If we allow the current cache to be changed should the new caches be able to be changed, too? We just tried to follow the existing pattern. It is easy enough not to.

Is there a command line flag for overriding download chart name? e.g. my complex service may have an Istio gateway, then some other gateway. and both of them named their charts 'gateway'.

bacongobbler · 2021-06-17T16:13:39Z

hips/hip-0013.md

+
+For this to work the malicious user would need access to the system Helm is running on with write access to the cache. The current cache directory does not provide write access to those who are not the user or a system admin. That would mean the malicious user would need access as the user or a system admin to exploit this issue.
+
+To further mitigate the issue, `ChartDownloader` will check that the archive conforms to the digest that is being requested. If it does not the `ChartDownloader` will retrieve it from the source.


This is checked using the cached index file, correct? Could it be possible for an attacker to modify both digests (the .tgz in the cache, and the string in the index) to forge a "valid" chart digest?

I think there is an underlying assumption that the --sign | --verify flag will check for malicious intent, but I just wanted to check and clarify how can we mitigate a local attack vector. Or rather, if it's an unreasonable attack vector to mitigate.

This solution does not mitigate the above.

One could attempt to match the local cached metadata (chart name and version, for instance) with the info in the index. That would help, but would not totally solve the problem.

Checking that the actual SHA is correct would otherwise require Helm to fetch the entire binary package and digest it, which completely defeats the purpose of a cache.

A provenance file solves the problem if they are used. But otherwise, there is likely a whole in the security model of this feature.

bacongobbler · 2021-06-17T16:14:15Z

hips/hip-0013.md

+
+## How to teach this
+
+The Helm client will have a flag to bypass the cache. This will be in the client documentation alongside the other flags.


Again for posterity, can we give that flag a name so this document can be self-describing?

technosophos

I think there is a substantial security issue with the proposed solution. I believe the proposed solution would allow a malicious user with access to the index (or MITM-style) to force the Helm client to install the wrong package by forging the SHA field in the index to point to another package.

technosophos · 2021-08-09T23:15:53Z

hips/hip-0013.md

+
+The first motivation is to make the cache useful by handling it in a manner where the downloaded chart's identifier can be make unique enough to be useful.
+
+The second motivation is around use and experience. Downloading the chart version each time an operation is run on a chart in a repository can lead to wasted time. For example, if one run two show commands (i.e., `helm show readme example-repo/foo` and `helm show values example-repo/foo`) the chart will be downloaded twice from the repository. This can be avoided with the use of a cache.


"if one run two show" -- not sure I understand

Oh... I think it's supposed to be "If one runs two show commands".

technosophos · 2021-08-09T23:20:07Z

hips/hip-0013.md

+
+The `ChartDownloader` will be extended to provide the caching features. To do this, two new properties will be added to the `ChartDownloader` to accommodate a cache for both the chart archives and the provenance files. These can be stored in two separate locations.
+
+When the `ChartDownloader` resolves a chart in a repository it will also obtain the digest from the repository index. This piece of information is already available in the index. Using the digest, `ChartDownloader` will check if the chart archive and provenance file are already in the cache. If the chart archive is not in the cache the `ChartDownloader` will retrieve it and place it in the cache prior to returning as before. The same will happen for provenance files. If no provenance file is found on download the cache will be marked in a manner to note that none was available. If the files are in the cache they will be returned rather than downloading the files again.


So let's say that an attacker compromises a chart repository index file, but not the packages (a possible scenario, given Helm's design).

What if the attacker replaced the correct SHA with a fake SHA that pointed to another package? Might this result in a way for an attacker to replace the user's intended package with another package (provided that other package is in the user's cache)?

If an attacker can compromise the index file they could point a chart version to a different file to download install. There is already a vector there if there is no provenance file.

Using just a the hash could lead to an issue if someone modifies it. Since the index file contains more information than the hash, we could use that metadata (e.g. name and version) to verify it is the right package after looking it up in the cache. If that does not match we could download the file listed in the index.

Would that work?

technosophos · 2021-08-09T23:23:17Z

hips/hip-0013.md

+
+The [cache provided for the Helm client](https://github.com/rancher/sandbox/gofilecache) is based on the Go build cache and uses the file system. It uses the first two characters of the hash as the top directory followed by the rest of the hash for the second level directory. This is similar to the way in with Git stores objects on disk. Both the chart and provenance caches will live within the cache directory Helm already uses.
+
+The Helm Client would have two new environment variables and flags to specify the locations for the provenance files and chart archive cache. These would follow the same format as the current environment variable use for individual caching locations.


I'm not entirely sure that allowing caches to be outside of a single top-level directory is a good idea, and I don't see this pattern replicated in similar systems. This is hard to debug, hard to trace, and could lead to bewildering behavior when multiple users/tools are attempting to share the same cache.

Can you please explain the rationale for allowing this very specific feature of Helm to be configured?

technosophos · 2021-08-09T23:23:53Z

hips/hip-0013.md

+
+Index files, where digests are looked up, remain untouched. There is no impact to the index files.
+
+The existing Helm cache directory is used for the cache storage. This location needs to exist for storing index files locally. There is no impact to the cache location.


But if you introduce new env vars for configuring cache dir, it is not clear that this is true.

technosophos · 2021-08-09T23:25:37Z

hips/hip-0013.md

+
+The existing Helm cache directory is used for the cache storage. This location needs to exist for storing index files locally. There is no impact to the cache location.
+
+Those who use the `ChartDownloader` from the Helm SDK will have two new properties to define when instantiating it.


Same question. I feel like this needs some clarification to show that it is not a breaking change. In order to not be breaking, existing userland code must not need to be changed.

technosophos · 2021-08-09T23:29:27Z

hips/hip-0013.md

+
+If a chart archive or a provenance file is in the cache it will be used instead of being downloaded. This means that a malicious user could place a chart archive in the cache in the location where the good one should be. When Helm looks up the chart listed in the repository it would retrieve the the malicious chart from the cache instead.
+
+For this to work the malicious user would need access to the system Helm is running on with write access to the cache. The current cache directory does not provide write access to those who are not the user or a system admin. That would mean the malicious user would need access as the user or a system admin to exploit this issue.


This is not necessarily true. A malicious user needs to only (a) compel a user to fetch another chart, or (b) assume the user has already fetched the corrupted chart.

For example, say there is a chart foo-1.2.3 that has a security vulnerability. The maintainers update and release foo-1.2.4 to fix. The attacker can assume that many of the users who update from 1.2.3 to 1.2.4 may also have the compromised 1.2.3 version in their cache. So an attacker could alter the SHA in the index to point to the older compromised version, and in so doing potentially prevent the user from fetching the updated version, instead using the known-bad version.

Thanks for the example here.

If an attacker can alter the index file they can already point someone to a bad file to install. I think we need to add signing to them but that's another HIP.

As I noted earlier, we could inspect the chart to make sure it is the right name and version. Would that work?

technosophos · 2021-08-09T23:31:56Z

hips/hip-0013.md

+
+For this to work the malicious user would need access to the system Helm is running on with write access to the cache. The current cache directory does not provide write access to those who are not the user or a system admin. That would mean the malicious user would need access as the user or a system admin to exploit this issue.
+
+To further mitigate the issue, `ChartDownloader` will check that the archive conforms to the digest that is being requested. If it does not the `ChartDownloader` will retrieve it from the source.


This solution does not mitigate the above.

One could attempt to match the local cached metadata (chart name and version, for instance) with the info in the index. That would help, but would not totally solve the problem.

Checking that the actual SHA is correct would otherwise require Helm to fetch the entire binary package and digest it, which completely defeats the purpose of a cache.

A provenance file solves the problem if they are used. But otherwise, there is likely a whole in the security model of this feature.

technosophos · 2021-09-16T17:40:08Z

Is there an update coming on this? I talked with @mattfarina about some mitigations that would basically address my concerns, and I'm pretty interested in accepting this HIP if the mitigations land in the doc.

TBBle · 2021-09-21T00:44:21Z

hips/hip-0013.md

+
+To further mitigate the issue, `ChartDownloader` will check that the archive conforms to the digest that is being requested. If it does not the `ChartDownloader` will retrieve it from the source.
+
+This does mean that if the source chart archive does not have the same digest the one listed in the index it will be retrieved every time and bypass the cache.


Coming in late, but I'd suggest the least-surprise approach here would be to fail in this case, rather than accept the source archive with an unexpected digest. Motivated by helm/helm#7623 where we're not doing this correctly now.

As I mentioned in my comment there, I would implement this HIP by never bypassing the cache. I'd always have the downloader (OCI Registry or Chart Repository) downloading into the cache, hashing as it's written to ensure that the cache entry's content matches its address before marking the entry as usable; and separately consumers would always pull from the cache, hashing as they read to ensure the cache entry's content matches its address before using the data so-read.

For the 'no disk access' case, the "cache" there could be in-memory blobs or streaming.

This is mostly based on what I've seen of the OCI container image layer downloading/extracting mechanisms in containerd, using its "content store" the same way we're talking about a "cache" here.

Of course, if I've misunderstood the intent of this HIP, that's fine too.

annaagaf · 2022-10-06T12:18:40Z

Are there any recent updates on this?

Andrioden · 2023-05-03T18:42:21Z

Any chance work can be continued on this?

It makes me developer-waiting-to-work-sad to see:

Running helm dependency update on umbrella charts download the same chart multiple times (because multiple Chart.yaml dependencies point to it)
Next time helm dependency update is run the cached tgz file in the charts folder is not used, and the charts are downloaded again.

MaxWinterstein · 2024-05-08T09:14:40Z

I know - and even myself - we all hate bumps, but it's been another year and there are still a lot of people hoping to see this in the wild <3

Add hip for download cache

139d876

Co-authored-by: Matt Farina <matt.farina@suse.com> Signed-off-by: Matt Farina <matt.farina@suse.com> Signed-off-by: Christian Richter <crichter@suse.com>

helm-bot added the size/M label May 7, 2021

dragonchaser changed the title ~~Add hip for download cache~~ HIP for download cache (charts & provenance files) May 7, 2021

dragonchaser mentioned this pull request May 10, 2021

Local content addressable cache of helm charts rancher-sandbox/hypper#28

Open

bacongobbler requested changes May 26, 2021

View reviewed changes

mladedav mentioned this pull request May 29, 2021

HIP for dependency overrides #176

Closed

Christian Richter added 2 commits June 7, 2021 14:05

Rename file to hip-0013.md

9dcf0e2

Signed-off-by: Christian Richter <crichter@suse.com>

Incorporate changes

7097cba

Signed-off-by: Christian Richter <crichter@suse.com>

bacongobbler reviewed Jun 17, 2021

View reviewed changes

bacongobbler mentioned this pull request Jun 22, 2021

Verify chart digest on download if possible helm/helm#7623

Closed

technosophos requested changes Aug 9, 2021

View reviewed changes

bacongobbler mentioned this pull request Aug 17, 2021

Caching downloaded charts helm/helm#8831

Closed

TBBle reviewed Sep 21, 2021

View reviewed changes

dragonchaser mentioned this pull request May 16, 2022

Cache and use cache of charts helm/helm#9561

Closed

bacongobbler mentioned this pull request Jul 7, 2022

Increment HIP number from most recent #258

Merged

zifter mentioned this pull request Nov 27, 2022

Defined multiple time helm chart stored OCI Registry is always pulled for each release helmfile/helmfile#544

Closed

olevitt mentioned this pull request Jan 30, 2024

Onyxia api could do better caching strategy when remote helm charts repository of catalogs are offline InseeFrLab/onyxia-api#224

Open

EronWright mentioned this pull request Apr 2, 2024

[feature] cache helm YAML rendered result pulumi/pulumi-kubernetes#935

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

HIP for download cache (charts & provenance files) #185

HIP for download cache (charts & provenance files) #185

dragonchaser commented May 7, 2021

joejulian commented May 13, 2021

dragonchaser commented May 17, 2021 •

edited

bacongobbler left a comment

mattfarina commented Jun 2, 2021

mattfarina commented Jun 2, 2021

bacongobbler left a comment

bacongobbler Jun 17, 2021

technosophos Aug 9, 2021

mattfarina Aug 23, 2021

bacongobbler Jun 17, 2021

bacongobbler Jun 17, 2021

bacongobbler Jun 17, 2021

technosophos Aug 9, 2021

mattfarina Aug 23, 2021

lzvoyager Jan 26, 2023

bacongobbler Jun 17, 2021

technosophos Aug 9, 2021

bacongobbler Jun 17, 2021

technosophos left a comment

technosophos Aug 9, 2021

technosophos Aug 9, 2021

technosophos Aug 9, 2021

mattfarina Aug 23, 2021

technosophos Aug 9, 2021

technosophos Aug 9, 2021

technosophos Aug 9, 2021

technosophos Aug 9, 2021

mattfarina Aug 23, 2021

technosophos Aug 9, 2021

technosophos commented Sep 16, 2021

TBBle Sep 21, 2021 •

edited

annaagaf commented Oct 6, 2022

Andrioden commented May 3, 2023

MaxWinterstein commented May 8, 2024


		The existing Helm cache directory is used for the cache storage. This location needs to exist for storing index files locally. There is no impact to the cache location.

		Those who use the `ChartDownloader` from the Helm SDK will have two new properties to define when instantiating it.


		## Motivation

		There are three primary motivations for leveraging a cache the impact [both the application operator and the application distributor](https://github.com/helm/community/blob/main/user-profiles.md).


		When repeatedly installing, upgrading, or showing information about a chart version within a repository, Helm will download the chart each time. When downloading the chart Helm currently puts the chart into the repositories cache directory (where index files are cached) but never leverages the cache. Helm cannot use the chart version in the cache because the identifier is the name and charts version (e.g., `wordpress-1.2.3.tgz`). A name and version is insufficient as the same chart and version could come from two or more different repositories and have different content.

		The first motivation is to make the cache useful by handling it in a manner where the downloaded chart's identifier can be make unique enough to be useful.


		The [cache provided for the Helm client](https://github.com/rancher/sandbox/gofilecache) is based on the Go build cache and uses the file system. It uses the first two characters of the hash as the top directory followed by the rest of the hash for the second level directory. This is similar to the way in with Git stores objects on disk. Both the chart and provenance caches will live within the cache directory Helm already uses.

		The Helm Client would have two new environment variables and flags to specify the locations for the provenance files and chart archive cache. These would follow the same format as the current environment variable use for individual caching locations.


		For this to work the malicious user would need access to the system Helm is running on with write access to the cache. The current cache directory does not provide write access to those who are not the user or a system admin. That would mean the malicious user would need access as the user or a system admin to exploit this issue.

		To further mitigate the issue, `ChartDownloader` will check that the archive conforms to the digest that is being requested. If it does not the `ChartDownloader` will retrieve it from the source.


		## How to teach this

		The Helm client will have a flag to bypass the cache. This will be in the client documentation alongside the other flags.


		The first motivation is to make the cache useful by handling it in a manner where the downloaded chart's identifier can be make unique enough to be useful.

		The second motivation is around use and experience. Downloading the chart version each time an operation is run on a chart in a repository can lead to wasted time. For example, if one run two show commands (i.e., `helm show readme example-repo/foo` and `helm show values example-repo/foo`) the chart will be downloaded twice from the repository. This can be avoided with the use of a cache.


		The `ChartDownloader` will be extended to provide the caching features. To do this, two new properties will be added to the `ChartDownloader` to accommodate a cache for both the chart archives and the provenance files. These can be stored in two separate locations.

		When the `ChartDownloader` resolves a chart in a repository it will also obtain the digest from the repository index. This piece of information is already available in the index. Using the digest, `ChartDownloader` will check if the chart archive and provenance file are already in the cache. If the chart archive is not in the cache the `ChartDownloader` will retrieve it and place it in the cache prior to returning as before. The same will happen for provenance files. If no provenance file is found on download the cache will be marked in a manner to note that none was available. If the files are in the cache they will be returned rather than downloading the files again.


		Index files, where digests are looked up, remain untouched. There is no impact to the index files.

		The existing Helm cache directory is used for the cache storage. This location needs to exist for storing index files locally. There is no impact to the cache location.


		If a chart archive or a provenance file is in the cache it will be used instead of being downloaded. This means that a malicious user could place a chart archive in the cache in the location where the good one should be. When Helm looks up the chart listed in the repository it would retrieve the the malicious chart from the cache instead.

		For this to work the malicious user would need access to the system Helm is running on with write access to the cache. The current cache directory does not provide write access to those who are not the user or a system admin. That would mean the malicious user would need access as the user or a system admin to exploit this issue.


		To further mitigate the issue, `ChartDownloader` will check that the archive conforms to the digest that is being requested. If it does not the `ChartDownloader` will retrieve it from the source.

		This does mean that if the source chart archive does not have the same digest the one listed in the index it will be retrieved every time and bypass the cache.

HIP for download cache (charts & provenance files) #185

Are you sure you want to change the base?

HIP for download cache (charts & provenance files) #185

Conversation

dragonchaser commented May 7, 2021

joejulian commented May 13, 2021

dragonchaser commented May 17, 2021 • edited

bacongobbler left a comment

Choose a reason for hiding this comment

mattfarina commented Jun 2, 2021

mattfarina commented Jun 2, 2021

bacongobbler left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

technosophos left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

technosophos commented Sep 16, 2021

TBBle Sep 21, 2021 • edited

Choose a reason for hiding this comment

annaagaf commented Oct 6, 2022

Andrioden commented May 3, 2023

MaxWinterstein commented May 8, 2024

dragonchaser commented May 17, 2021 •

edited

TBBle Sep 21, 2021 •

edited