From 8027423cd0e7a45a040c99ea8f74183ab47b4ad6 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Dominykas=20Blyz=CC=8Ce=CC=87?= <hello@dominykas.com>
Date: Wed, 22 Nov 2023 15:32:38 +0200
Subject: [PATCH] Fill out rationale, security implications, etc

---
 hips/hip-00NN.md | 23 ++++++++++++++++++-----
 1 file changed, 18 insertions(+), 5 deletions(-)

diff --git a/hips/hip-00NN.md b/hips/hip-00NN.md
index 67ca5c08..26cc0418 100644
--- a/hips/hip-00NN.md
+++ b/hips/hip-00NN.md
@@ -20,13 +20,15 @@ There are existing ways to achieve installation of charts without using a regist
 
 - A Helm chart repository is effectively an `index.yaml` with links to downloads. Maintaining such a repository does create a burden of scripts and automation (e.g. Github Actions). This is not always feasible for smaller projects. It also does not really offer an obvious and readily available way of testing pre-releases.
 - For the testing use cases, charts can be packaged using `helm package`, however this does introduce manual steps and requires extra work to replicate in CI/CD scenarios.
-- There is a [`aslafy-z/helm-git`](https://github.com/aslafy-z/helm-git) plugin available, however using plugins requires additional setup, which may not always be feasible (esp. in more complex team structures and cluster setups with advanced tooling, e.g. ArgoCD). An additional drawback is that the `Chart.yaml` does not provide a way to specify the plugin requirements.
+- There are several plugins available to solve this problem ([`aslafy-z/helm-git`](https://github.com/aslafy-z/helm-git), [`diwakar-s-maurya/helm-git`](https://github.com/diwakar-s-maurya/helm-git), [`sagansystems/helm-github`](https://github.com/sagansystems/helm-github)), however using plugins requires additional setup, which may not always be feasible (esp. in more complex team structures and cluster setups with advanced tooling, e.g. ArgoCD). An additional drawback is that the `Chart.yaml` does not provide a way to specify the plugin requirements, which leaves it up to the consumer to figure this out.
+
+## Rationale
 
 Installing dependencies from git is an established pattern in other ecosystems even when they are registry-based, e.g. npm (Node.js), pipenv (Python), bundler (Gem) have this option - it would make sense to have the behavior replicated in Helm.
 
-## Rationale
+At least the npm and the pip ecosystems have already established a syntax of `vcs+protocol` for defining the dependency source, so it should be familiar to some users.
 
-(TBD)
+One alternative to consider would be to exclude defining the vcs/protocol, similar to what Go does, esp. given that Helm is built using Go. This does limit the flexibility somewhat - while Go does allow adding a VCS qualifier at the end of the URL (allowing future support for other VCSs), it does not allow specifying the protocol, which means that the users might have to override the default protocol in their VCS configuration.
 
 ## Specification
 
@@ -51,6 +53,13 @@ dependencies:
   version: "main"
 ```
 
+When Helm is installing a dependency from git, it should:
+
+- create a temporary directory
+- clone the repo at the specified branch/tag into the temp dir
+- treat the cloned git repo similar to a `file:///path/to/temp/dir` style requirement; use `chart.LoadDir` to load that directory (which in turn applied the logic for filtering the files through `.helmignore`) and archives it to `charts/`
+- delete the temp dir
+
 ## Backwards compatibility  
 
 This is backwards compatible from Helm perspective - the existing formats for `dependencies` are still supported.
@@ -59,11 +68,15 @@ Charts that start using the new format will effectively be changing their minimu
 
 ## Security implications
 
-(TBD)
+Pulling the dependencies from git may introduce additional attack surfaces, as it would need to rely on an implementation of `git` (most likely the official `git` executable), and there have been recent vulnerabilities disclosed, including Remote Code Execution (RCE).
+
+This is something that needs to be taken into account in security conscious environments and might need to be documented for the end users. Users with high security requirements, should probably avoid using the feature and instead rely on a registry.
 
 ## How to teach this
 
-(TBD)
+- The documentation should note the security caveat listed above
+- The documentation should provide the recommendation to prefer registries to git, if possible
+- The documentation should note the implications of git being mutable with a recommendation of pinning to specific hashes
 
 ## Reference implementation