HIP to include repository URL and tarball digest in release object under chart metadata #224

lucasreed · 2021-11-24T14:55:37Z

Signed-off-by: Luke Reed luke@lreed.net

Opening this HIP after conversation with @mattfarina in this PR comment section: helm/helm#10369

This would help solve this longstanding issue: helm/helm#4256

Signed-off-by: Luke Reed <luke@lreed.net>

hips/hip-9999.md

bacongobbler · 2021-11-24T15:05:41Z

hips/hip-9999.md

+
+Nothing would change from the user's perspective. With some modifications in the current code it's possible to add this metadata into the release object transparently.
+
+The current idea for this implementation is to add a field in both the `ChartDownloader` and `Metadata` (in the `pkg/chart/metadata.go` file) structs to hold the repository URL. The Downloader is the logical place to derive the proper URL and can then that storage location can populate the Metadata field at install/upgrade time.


What about charts that are pulled down via helm pull before issuing a helm install?

Great question! I'll look into that.

I think the answer here is that if you install it from a local filesystem, this new field would just stay empty. I'm not sure a good method for populating it without the actual download happening.

I guess a question I'd like to hear feedback on is if people want this field to be populated in a release where someone installed it from a chart on their local filesystem?

Signed-off-by: Luke Reed <luke@lreed.net>

lucasreed · 2021-12-02T15:43:30Z

A potential scope increase to also include the tar sha256sum in the release metadata might be nice as well. If installed with a local tarball, this could still be populated where repoURL would not get populated in that case. I can extend the proposal with this.

This came out of a conversation in this PR: FairwindsOps/nova#61

I'm also planning on attending the helm dev meeting today in case there is time to talk about this HIP.

…ingly Signed-off-by: Luke Reed <luke@lreed.net>

technosophos

I left a couple comments here, but the most important thing to figure out is whether this constitutes a breaking change (which is dependent largely on whether we use strict YAML parsing for release YAML objects).

technosophos · 2021-12-02T18:03:26Z

hips/hip-9999.md

+
+Nothing would change from the user's perspective. With some modifications in the current code it's possible to add this metadata into the release object transparently.
+
+The current idea for this implementation is to add a field in both the `ChartDownloader` and `Metadata` (in the `pkg/chart/metadata.go` file) structs to hold the repository URL. The Downloader is the logical place to derive the proper URL and can then that storage location can populate the Metadata field.


The only potential compatibility checking item I see here (and @mattfarina raised this in the Helm call) is that we sometimes do strict checking of some YAML objects. In this case, an unknown field will cause a parse error. This behavior was added to address some security concerns. IF this behavior is enabled for the release object, then an addition constitutes a breaking change. Otherwise, our rules for additive changes mean this is only a regular feature and we can do it as part of a Minor release.

If we determine this to be a breaking change, this HIP should be added to Helm 4.

If not, then I think we should push for adding this feature ASAP into Helm 3.

The only strict parsing I am able to find so far is for a repository index file and a helm plugin's plugin.yaml file. I don't think the ChartDownloader struct should get unmarshaled in a strict way (that I can find).

technosophos · 2021-12-02T18:06:02Z

hips/hip-9999.md

+
+A potential security issue would be private repository URLs becoming present in a cluster via this implementation. A potential fix for this would be to add a command line argument that would keep this field in the release object empty, but that would introduce something a user has to change in their workflow which may not be desirable. Another way would be to attempt a DNS match against a publicly available DNS server and if it's not found, then we do not populate the repository URL field.
+
+Another option is to ignore this potential downside. Unclear if this would truly be an issue or not. Would love to have feedback on why it would be bad for an internal repository URL to be stored in the cluster.


As an additional (positive) security implementation, giving the tarballDigest would mean that utilities could independently verify that a chart has not been altered. However, a caveat should be noted here: There is no guarantee that helm will rebuild exactly the same tarball if presented exactly the same input data because some input metadata (timestamps, ordering) may change. So we wouldn't necessarily want to assume that just because a tarball has a different SHA, it's data has changed and thus it is untrustworthy.

Since this HIP does not have a suggested implementation of how tarballDigest should be implemented in the client, I am fine with merely noting this issue under Security Implications and then letting implementors figure out how to cope.

technosophos · 2021-12-02T18:07:30Z

hips/hip-9999.md

+The feature can be documented by describing how we get the URL (taken from the description in the [Specification](#Specification) section). The usage of this URL is up to a user or third-party tool developer, but the URL would be included in the release object as one more informational piece with no prescribed usage.
+
+## Reference implementation
+A PR has been created to show a possible implementation: https://github.com/helm/helm/pull/10369


The reference implementation, as far as I can tell, does not do anything with the tarballDigest field, which is probably the correct behavior for now (given the note I left on the Security Implications)

This is correct, I have not done that work yet.

marckhouzam

I like this!

hips/hip-9999.md

marckhouzam · 2021-12-02T18:11:58Z

hips/hip-9999.md

+
+## Security implications
+
+A potential security issue would be private repository URLs becoming present in a cluster via this implementation. A potential fix for this would be to add a command line argument that would keep this field in the release object empty, but that would introduce something a user has to change in their workflow which may not be desirable. Another way would be to attempt a DNS match against a publicly available DNS server and if it's not found, then we do not populate the repository URL field.


If it is decided not to store a private URL by default, we should add a flag to allow it.

marckhouzam · 2021-12-02T18:20:32Z

hips/hip-9999.md

+Currently there is no way of knowing the exact repository that a chart originated from for a given release. There are ways to guess by looking at the sources list in the chart metadata, but since this is provided via the `Chart.yaml` file, any forks of that repository will look identical. The only way to concretely know what the repository URL is that was used at install/upgrade time is to populate a repository URL metadata field in the release during the install or upgrade. It should work for both traditional repositories behind an http web server, as well as an OCI compliant repository.
+
+The second part of this proposal would include the tarball digest of a chart in the metadata. This aspect is less fleshed out in code yet, but would be a nice alternative for installing a local chart from a tarball when the repository URL would not be known. The reason this is useful is because it can be used when interacting with other third-party databases, such as artifact-hub.
+


What will be done for an installation done from a local filesystem directory?

Currently, in my reference implementation, the field would not get populated and would be omitted from the release metadata.

marckhouzam · 2021-12-02T18:24:11Z

hips/hip-9999.md

+```
+"repoURL": "oci://${ACCOUNT_NUMBER}.dkr.ecr.us-east-1.amazonaws.com/goldilocks"
+"tarballDigest": "sha256:f9a9f9a9f9a9f9a9f9a9f9a9f9a9f9a9f9a9f9a9f9a9f9a9f9a9f9a9f9a9f9a9"
+```


How would this information be accessed? Is it meant to be easily accessible by users? Is it sufficient to add it to the metadata and leave it up to users to figure out how to access it using kubectl or some other means?

I leave it up to the users here. If it's decided that it should be made a part of helm ls or something later, I think that can be a separate request/HIP.

Signed-off-by: Luke Reed <luke@lreed.net>

mattfarina · 2022-01-06T16:39:42Z

@technosophos can you look at the latest changes?

jiangfoxi · 2022-03-02T04:54:47Z

will this MR be merged? look forward for this function:)

sudermanjr · 2022-05-02T20:02:06Z

Also very interested to see this move forward. Is there anything we can do to assist?

kekscode · 2022-05-30T15:21:24Z

I am also looking forward for having this functionality. What needs to be done next to push this forward?

ahmed82

Looking forward for this function

lucasreed · 2022-08-22T13:16:52Z

Closing this in lieu of @sudermanjr opening a newer one here: #269
Thank you, and sorry I haven't had the time to pursue this further!

HIP to include repository URL in release object under chart metadata

52d8970

Signed-off-by: Luke Reed <luke@lreed.net>

helm-bot added the size/M label Nov 24, 2021

This was referenced Nov 24, 2021

Populate repo url in chart meta helm/helm#10369

Closed

Show Installed Chart Source Location or Source Repository URL helm/helm#4256

Open

bacongobbler reviewed Nov 24, 2021

View reviewed changes

address review comments

e087693

Signed-off-by: Luke Reed <luke@lreed.net>

lucasreed requested a review from bacongobbler November 24, 2021 18:42

lucasreed mentioned this pull request Dec 1, 2021

[RD-24] Utilize ArtifactHub instead of helm-hub FairwindsOps/nova#61

Merged

lucasreed changed the title ~~HIP to include repository URL in release object under chart metadata~~ HIP to include repository URL and tarball digest in release object under chart metadata Dec 2, 2021

add pieces about tarballDigest metadata and adjust name of HIP accord…

a2f3d5e

…ingly Signed-off-by: Luke Reed <luke@lreed.net>

technosophos requested changes Dec 2, 2021

View reviewed changes

marckhouzam reviewed Dec 2, 2021

View reviewed changes

grammar fixes

6e2af4f

Signed-off-by: Luke Reed <luke@lreed.net>

ahmed82 reviewed Jun 30, 2022

View reviewed changes

sudermanjr mentioned this pull request Jul 22, 2022

Nova unable to flag deprecated Helm charts FairwindsOps/nova#134

Open

2 tasks

sudermanjr mentioned this pull request Aug 21, 2022

HIP to include repository URL and tarball digest in release object under chart metadata #269

Merged

lucasreed closed this Aug 22, 2022

lucasreed deleted the hip_repourl_release_manifest branch August 22, 2022 13:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

HIP to include repository URL and tarball digest in release object under chart metadata #224

HIP to include repository URL and tarball digest in release object under chart metadata #224

lucasreed commented Nov 24, 2021

bacongobbler Nov 24, 2021

lucasreed Nov 24, 2021

lucasreed Nov 24, 2021 •

edited

Loading

lucasreed Nov 24, 2021

lucasreed commented Dec 2, 2021

technosophos left a comment

technosophos Dec 2, 2021

lucasreed Dec 20, 2021

technosophos Dec 2, 2021

technosophos Dec 2, 2021

lucasreed Dec 2, 2021

marckhouzam left a comment

marckhouzam Dec 2, 2021

marckhouzam Dec 2, 2021

lucasreed Dec 2, 2021

marckhouzam Dec 2, 2021

lucasreed Dec 2, 2021

mattfarina commented Jan 6, 2022

jiangfoxi commented Mar 2, 2022

sudermanjr commented May 2, 2022

kekscode commented May 30, 2022

ahmed82 left a comment

lucasreed commented Aug 22, 2022


		Nothing would change from the user's perspective. With some modifications in the current code it's possible to add this metadata into the release object transparently.

		The current idea for this implementation is to add a field in both the `ChartDownloader` and `Metadata` (in the `pkg/chart/metadata.go` file) structs to hold the repository URL. The Downloader is the logical place to derive the proper URL and can then that storage location can populate the Metadata field at install/upgrade time.


		A potential security issue would be private repository URLs becoming present in a cluster via this implementation. A potential fix for this would be to add a command line argument that would keep this field in the release object empty, but that would introduce something a user has to change in their workflow which may not be desirable. Another way would be to attempt a DNS match against a publicly available DNS server and if it's not found, then we do not populate the repository URL field.

		Another option is to ignore this potential downside. Unclear if this would truly be an issue or not. Would love to have feedback on why it would be bad for an internal repository URL to be stored in the cluster.


		## Security implications

		A potential security issue would be private repository URLs becoming present in a cluster via this implementation. A potential fix for this would be to add a command line argument that would keep this field in the release object empty, but that would introduce something a user has to change in their workflow which may not be desirable. Another way would be to attempt a DNS match against a publicly available DNS server and if it's not found, then we do not populate the repository URL field.

		Currently there is no way of knowing the exact repository that a chart originated from for a given release. There are ways to guess by looking at the sources list in the chart metadata, but since this is provided via the `Chart.yaml` file, any forks of that repository will look identical. The only way to concretely know what the repository URL is that was used at install/upgrade time is to populate a repository URL metadata field in the release during the install or upgrade. It should work for both traditional repositories behind an http web server, as well as an OCI compliant repository.

		The second part of this proposal would include the tarball digest of a chart in the metadata. This aspect is less fleshed out in code yet, but would be a nice alternative for installing a local chart from a tarball when the repository URL would not be known. The reason this is useful is because it can be used when interacting with other third-party databases, such as artifact-hub.

HIP to include repository URL and tarball digest in release object under chart metadata #224

HIP to include repository URL and tarball digest in release object under chart metadata #224

Conversation

lucasreed commented Nov 24, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lucasreed Nov 24, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lucasreed commented Dec 2, 2021

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

marckhouzam 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

mattfarina commented Jan 6, 2022

jiangfoxi commented Mar 2, 2022

sudermanjr commented May 2, 2022

kekscode commented May 30, 2022

ahmed82 left a comment

Choose a reason for hiding this comment

lucasreed commented Aug 22, 2022

lucasreed Nov 24, 2021 •

edited

Loading