[DOM-47678] Add support for passing buildkit secrets via k8s secrets (#…

…106)

* Update docs for service Secrets

* Add secrets support to ImageBuild CR

 - Adds the ability to pass k8s secret names by namespace / name to be
   consumed by the ImageBuild request using buildkit secrets. The
   `secrets` field is added to the CR as optional, and is fully
   backwards compatible with previous requests.

   Previously, the Helm chart supported exposing service level secrets
   into *all* builds, but this adds supports on a per request basis.

   Buildkit secrets must be mounted in via Dockerfile syntax:
   https://docs.docker.com/engine/reference/builder/#run---mounttypesecret

   Consumption via Dockerfile is therefore similar to:

   RUN --mount=type=secret,id=domino-compute/mysecret/foo cat /run/secrets/foo

 - For the secret to be accessible by Hephaestus, it must have the
   label `hephaestus-accessible: "true"`. This prevents the build
   service from having access to arbitrary secrets in the cluster and
   requires clients to specifically opt-in.

 - Additionally the `hephaestus-owned: "true"` label can be added to
   secrets to help manage their lifecycle. When set, the secret will be
   updated to specify the attached ImageBuild as the owner -- when
   ImageBuild resources are routinely purged by the service, those
   secrets will be cleaned up at the same time. This removes the burden
   of secret cleanup from clients, but changes the cleanup timing to be
   non-determinstic.

   The ClusterRole for Hephaestus is updated to allow for secret
   resource updates to support this feature.

api/api-rules/violation_exceptions.list

@@ -3,6 +3,7 @@ API rule violation: list_type_missing,github.com/dominodatalab/hephaestus/pkg/ap
 API rule violation: list_type_missing,github.com/dominodatalab/hephaestus/pkg/api/hephaestus/v1,ImageBuildSpec,Images
 API rule violation: list_type_missing,github.com/dominodatalab/hephaestus/pkg/api/hephaestus/v1,ImageBuildSpec,ImportRemoteBuildCache
 API rule violation: list_type_missing,github.com/dominodatalab/hephaestus/pkg/api/hephaestus/v1,ImageBuildSpec,RegistryAuth
+API rule violation: list_type_missing,github.com/dominodatalab/hephaestus/pkg/api/hephaestus/v1,ImageBuildSpec,Secrets
 API rule violation: list_type_missing,github.com/dominodatalab/hephaestus/pkg/api/hephaestus/v1,ImageBuildStatus,Conditions
 API rule violation: list_type_missing,github.com/dominodatalab/hephaestus/pkg/api/hephaestus/v1,ImageBuildStatus,Transitions
 API rule violation: list_type_missing,github.com/dominodatalab/hephaestus/pkg/api/hephaestus/v1,ImageBuildStatusTransitionMessage,ImageURLs

api/openapi-spec/swagger.json

@@ -2946,6 +2946,14 @@
             "default": {},
             "$ref": "#/definitions/.RegistryCredentials"
           }
+        },
+        "secrets": {
+          "description": "Secrets provides references to Kubernetes secrets to expose to individual image builds.",
+          "type": "array",
+          "items": {
+            "default": {},
+            "$ref": "#/definitions/.SecretReference"
+          }
         }
       }
     },
@@ -3193,6 +3201,17 @@
           "type": "string"
         }
       }
+    },
+    ".SecretReference": {
+      "type": "object",
+      "properties": {
+        "name": {
+          "type": "string"
+        },
+        "namespace": {
+          "type": "string"
+        }
+      }
     }
   },
   "securityDefinitions": {

deployments/crds/hephaestus.dominodatalab.com_imagebuilds.yaml

@@ -125,6 +125,17 @@ spec:
                       type: string
                   type: object
                 type: array
+              secrets:
+                description: Secrets provides references to Kubernetes secrets to
+                  expose to individual image builds.
+                items:
+                  properties:
+                    name:
+                      type: string
+                    namespace:
+                      type: string
+                  type: object
+                type: array
             type: object
           status:
             properties:

deployments/helm/hephaestus/templates/controller/clusterrole.yaml

@@ -83,9 +83,15 @@ rules:
       - ""
     resources:
       - nodes
+    verbs:
+      - get
+  - apiGroups:
+      - ""
+    resources:
       - secrets
     verbs:
       - get
+      - update
   - apiGroups:
       - apps
     resources:

deployments/helm/hephaestus/values.yaml

@@ -159,7 +159,7 @@ controller:
     # Defaults to 4.25 mins for fetch retries and an unlimited amount of time to extract.
     fetchAndExtractTimeout: null
 
-    # Secrets (name: path) to expose into builds that request it
+    # Global secrets (name: path) to expose into all image builds
     secrets: {}
 
     # Cloud-based registry credentials configuration

examples/resources/imagebuild.yaml

@@ -8,6 +8,9 @@ spec:
     - username/repo:tag
   buildArgs:
     - ENV=development
+  secrets:
+    - name: mySecret
+      namespace: default
   cacheMode: min
   cacheTag: local-test
   disableCacheExports: false

pkg/api/hephaestus/v1/imagebuild_types.go

@@ -32,6 +32,8 @@ type ImageBuildSpec struct {
 	DisableLocalBuildCache bool `json:"disableBuildCache,omitempty"`
 	// DisableCacheLayerExport will remove the "inline" cache metadata from the image configuration.
 	DisableCacheLayerExport bool `json:"disableCacheExport,omitempty"`
+	// Secrets provides references to Kubernetes secrets to expose to individual image builds.
+	Secrets []SecretReference `json:"secrets,omitempty"`
 }
 
 type ImageBuildTransition struct {

pkg/api/hephaestus/v1/types.go

@@ -26,6 +26,15 @@ const (
 	PhaseFailed Phase = "Failed"
 )
 
+const (
+	// Kubernetes metadata set by clients required to allow reading secrets by Hephaestus.
+	// Safeguards against accidental secret exposure / exfiltration.
+	AccessLabel = "hephaestus-accessible"
+	// Kubernetes metadata set by clients, to manage secret lifetime.
+	// When set, the given secret gets deleted at the same time as the ImageBuild that uses it.
+	OwnedLabel = "hephaestus-owned"
+)
+
 type BasicAuthCredentials struct {
 	Username string `json:"username,omitempty"`
 	Password string `json:"password,omitempty"`
@@ -49,6 +58,11 @@ type RegistryCredentials struct {
 	Secret        *SecretCredentials    `json:"secret,omitempty"`
 }
 
+type SecretReference struct {
+	Name      string `json:"name,omitempty"`
+	Namespace string `json:"namespace,omitempty"`
+}
+
 // ImageBuildStatusTransitionMessage contains information about ImageBuild status transitions.
 //
 // This type is used to publish JSON-formatted messages to one or more configured messaging

pkg/buildkit/buildkit.go

@@ -104,6 +104,7 @@ type BuildOptions struct {
 	ImportCache              []string
 	DisableInlineCacheExport bool
 	Secrets                  map[string]string
+	SecretsData              map[string][]byte
 	FetchAndExtractTimeout   time.Duration
 }
 
@@ -177,6 +178,11 @@ func (c *Client) Build(ctx context.Context, opts BuildOptions) error {
 		secrets[name] = contents
 	}
 
+	// merge in preloaded data
+	for name, contents := range opts.SecretsData {
+		secrets[name] = contents
+	}
+
 	// build solve options
 	solveOpt := bkclient.SolveOpt{
 		Frontend:      "dockerfile.v0",

pkg/config/config.go

@@ -105,7 +105,7 @@ type Buildkit struct {
 	PoolEndpointWatchTimeout *int64 `json:"poolEndpointWatchTimeout" yaml:"poolEndpointWatchTimeout"`
 	// MTLS parameters.
 	MTLS *BuildkitMTLS `json:"mtls,omitempty" yaml:"mtls,omitempty"`
-	// Secrets provided to buildkitd during the build process.
+	// Global secrets provided to buildkitd during the build process for all image builds.
 	Secrets map[string]string `json:"secrets" yaml:"secrets,omitempty"`
 	// Registries parameters.
 	Registries map[string]RegistryConfig `json:"registries,omitempty" yaml:"registries,omitempty"`

pkg/controller/imagebuild/component/builddispatcher.go

@@ -19,6 +19,7 @@ import (
 	"github.com/dominodatalab/hephaestus/pkg/config"
 	"github.com/dominodatalab/hephaestus/pkg/controller/support/credentials"
 	"github.com/dominodatalab/hephaestus/pkg/controller/support/phase"
+	"github.com/dominodatalab/hephaestus/pkg/controller/support/secrets"
 )
 
 type BuildDispatcherComponent struct {
@@ -91,6 +92,21 @@ func (c *BuildDispatcherComponent) Reconcile(ctx *core.Context) (ctrl.Result, er
 	}
 	c.phase.SetInitializing(ctx, obj)
 
+	// Extracts cluster secrets into data to pass to buildkit
+	log.Info("Processing references to build secrets")
+	secretsReadSeq := txn.StartSegment("cluster-secrets-read")
+	secretsData, err := secrets.ReadSecrets(ctx, obj, log, ctx.Config, ctx.Scheme)
+	if err != nil {
+		err = fmt.Errorf("cluster secrets processing failed: %w", err)
+		txn.NoticeError(newrelic.Error{
+			Message: err.Error(),
+			Class:   "ClusterSecretsReadError",
+		})
+
+		return ctrl.Result{}, c.phase.SetFailed(ctx, obj, err)
+	}
+	secretsReadSeq.End()
+
 	log.Info("Processing and persisting registry credentials")
 	persistCredsSeg := txn.StartSegment("credentials-persist")
 	configDir, helpMessage, err := credentials.Persist(ctx, buildLog, ctx.Config, obj.Spec.RegistryAuth)
@@ -189,6 +205,7 @@ func (c *BuildDispatcherComponent) Reconcile(ctx *core.Context) (ctrl.Result, er
 		ImportCache:              obj.Spec.ImportRemoteBuildCache,
 		DisableInlineCacheExport: obj.Spec.DisableCacheLayerExport,
 		Secrets:                  c.cfg.Secrets,
+		SecretsData:              secretsData,
 		FetchAndExtractTimeout:   c.cfg.FetchAndExtractTimeout,
 	}
 	log.Info("Dispatching image build", "images", buildOpts.Images)

pkg/controller/support/secrets/secrets.go

@@ -0,0 +1,81 @@
+package secrets
+
+import (
+	"context"
+	"fmt"
+	"strings"
+
+	"github.com/go-logr/logr"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/apimachinery/pkg/fields"
+	"k8s.io/apimachinery/pkg/labels"
+	"k8s.io/apimachinery/pkg/runtime"
+	"k8s.io/client-go/kubernetes"
+	"k8s.io/client-go/rest"
+	"sigs.k8s.io/controller-runtime/pkg/controller/controllerutil"
+
+	hephv1 "github.com/dominodatalab/hephaestus/pkg/api/hephaestus/v1"
+)
+
+// exists only so it can overridden by tests with a fake client
+var clientsetFunc = func(config *rest.Config) (kubernetes.Interface, error) {
+	return kubernetes.NewForConfig(config)
+}
+
+func ReadSecrets(
+	ctx context.Context,
+	obj *hephv1.ImageBuild,
+	log logr.Logger,
+	cfg *rest.Config,
+	scheme *runtime.Scheme,
+) (map[string][]byte, error) {
+	clientset, err := clientsetFunc(cfg)
+	if err != nil {
+		return map[string][]byte{}, fmt.Errorf("failure to get kubernetes client: %w", err)
+	}
+	v1 := clientset.CoreV1()
+
+	// Extracts secrets into data to pass to buildkit
+	secretsData := make(map[string][]byte)
+	for _, secretRef := range obj.Spec.Secrets {
+		secretClient := v1.Secrets(secretRef.Namespace)
+
+		path := strings.Join([]string{secretRef.Namespace, secretRef.Name}, "/")
+		log.Info("Finding secret", "path", path)
+		fields := fields.SelectorFromSet(map[string]string{"Namespace": secretRef.Namespace, "Name": secretRef.Name})
+		// prevent exfiltration of arbitrary secret values by using the presence of this label
+		labels := labels.SelectorFromSet(map[string]string{hephv1.AccessLabel: "true"})
+		secrets, err := secretClient.List(ctx,
+			metav1.ListOptions{FieldSelector: fields.String(), LabelSelector: labels.String()})
+
+		if err != nil {
+			return map[string][]byte{}, fmt.Errorf("failure querying for secret %q: %w", path, err)
+		}
+
+		if len(secrets.Items) == 0 {
+			return map[string][]byte{}, fmt.Errorf("secret %q unreadable or missing required label %q", path, hephv1.AccessLabel)
+		}
+		secret := &secrets.Items[0]
+
+		// adopt the secret resource if hephaestus-owned is true to delete when ImageBuild is deleted
+		if _, ok := secret.Labels[hephv1.OwnedLabel]; ok {
+			log.Info("Taking ownership of secret", "owner", obj.Name, "secret", path)
+
+			// non-fatal error thats logged but ignored
+			if err = controllerutil.SetOwnerReference(obj, secret, scheme); err != nil {
+				log.Info("Ignoring error taking ownership of secret", "secret", path, "error", err)
+			} else if _, err = secretClient.Update(ctx, secret, metav1.UpdateOptions{}); err != nil {
+				log.Info("Ignoring error taking ownership of secret", "secret", path, "error", err)
+			}
+		}
+
+		// builds a path for the secret like {namespace}/{name}/{key} to avoid hash key collisions
+		for filename, data := range secret.Data {
+			name := strings.Join([]string{path, filename}, "/")
+			secretsData[name] = data
+			log.Info("Read secret bytes", "path", name, "bytes", len(data))
+		}
+	}
+
+	return secretsData, nil
+}