From eb0d9eeac2e74b660c0f0acce2a3e0ec3062ce4f Mon Sep 17 00:00:00 2001
From: Samuel Gaist <samuel.gaist@idiap.ch>
Date: Thu, 17 Nov 2022 16:59:32 +0100
Subject: [PATCH] refactor: improve git credential handling in Kubernetes

The git credentials are now stored in a temporary secret following recommended
best practice. The secret will have the same lifespan as the builder pod.
---
 binderhub/build.py            | 63 +++++++++++++++++++++++++----------
 binderhub/tests/test_build.py | 11 ++++--
 2 files changed, 53 insertions(+), 21 deletions(-)

diff --git a/binderhub/build.py b/binderhub/build.py
index 50d683792a..a48c6eca13 100644
--- a/binderhub/build.py
+++ b/binderhub/build.py
@@ -2,6 +2,7 @@
 Contains build of a docker image from a git repository.
 """
 
+import base64
 import datetime
 import json
 import os
@@ -382,8 +383,24 @@ def submit(self):
 
         env = []
         if self.git_credentials:
+            secret_content = base64.b64encode(
+                self.git_credentials.encode("utf-8")
+            ).decode("utf-8")
+            data = {"credentials": secret_content}
+
+            secret = client.V1Secret()
+            secret.data = data
+            secret.metadata = {"name": self.name}
+            secret.type = "Opaque"
+
+            self.api.create_namespaced_secret(self.namespace, secret)
+
+            secret_key_ref = client.V1SecretKeySelector(
+                name=self.name, key="credentials", optional=False
+            )
+            value_from = client.V1EnvVarSource(secret_key_ref=secret_key_ref)
             env.append(
-                client.V1EnvVar(name="GIT_CREDENTIAL_ENV", value=self.git_credentials)
+                client.V1EnvVar(name="GIT_CREDENTIAL_ENV", value_from=value_from)
             )
 
         self.pod = client.V1Pod(
@@ -515,10 +532,9 @@ def submit(self):
                                 f"Found unknown phase {phase} when building {self.name}"
                             )
 
-                    if self.pod.status.phase == "Succeeded":
-                        self.cleanup()
-                    elif self.pod.status.phase == "Failed":
+                    if self.pod.status.phase in ["Succeeded", "Failed"]:
                         self.cleanup()
+
             except Exception:
                 app_log.exception("Error in watch stream for %s", self.name)
                 raise
@@ -568,21 +584,32 @@ def stream_logs(self):
 
     def cleanup(self):
         """
-        Delete the kubernetes build pod
+        Delete the kubernetes build pod and secret if exists
         """
-        try:
-            self.api.delete_namespaced_pod(
-                name=self.name,
-                namespace=self.namespace,
-                body=client.V1DeleteOptions(grace_period_seconds=0),
-                _request_timeout=KUBE_REQUEST_TIMEOUT,
-            )
-        except client.rest.ApiException as e:
-            if e.status == 404:
-                # Is ok, someone else has already deleted it
-                pass
-            else:
-                raise
+
+        exceptions = []
+        deletion_methods = [self.api.delete_namespaced_pod]
+
+        if self.git_credentials:
+            deletion_methods.append(self.api.delete_namespaced_secret)
+
+        for deletion_method in deletion_methods:
+            try:
+                deletion_method(
+                    name=self.name,
+                    namespace=self.namespace,
+                    body=client.V1DeleteOptions(grace_period_seconds=0),
+                    _request_timeout=KUBE_REQUEST_TIMEOUT,
+                )
+            except client.rest.ApiException as e:
+                if e.status == 404:
+                    # Is ok, someone else has already deleted it
+                    pass
+                else:
+                    exceptions.append(str(e))
+
+        if exceptions:
+            raise RuntimeError("Error(s) occurred during cleanup", exceptions)
 
 
 class KubernetesCleaner(LoggingConfigurable):
diff --git a/binderhub/tests/test_build.py b/binderhub/tests/test_build.py
index 1a14d07762..9ae7cfe809 100644
--- a/binderhub/tests/test_build.py
+++ b/binderhub/tests/test_build.py
@@ -234,9 +234,14 @@ def test_git_credentials_passed_to_podspec_upon_submit():
 
     assert len(pod.spec.containers) == 1
 
-    env = {env_var.name: env_var.value for env_var in pod.spec.containers[0].env}
-
-    assert env["GIT_CREDENTIAL_ENV"] == git_credentials
+    env = {env_var.name: env_var.value_from for env_var in pod.spec.containers[0].env}
+
+    credentials_value = env["GIT_CREDENTIAL_ENV"]
+    assert isinstance(credentials_value, client.V1EnvVarSource)
+    assert isinstance(credentials_value.secret_key_ref, client.V1SecretKeySelector)
+    assert credentials_value.secret_key_ref.key == "credentials"
+    assert credentials_value.secret_key_ref.name == "test_build"
+    assert credentials_value.secret_key_ref.optional == False
 
 
 async def test_local_repo2docker_build():