Repository: spegel-org/spegel (go)

Issue:
Some sort of authorisation
### Describe the problem to be solved

(as discussed in person as FOSDEM):

I'd love to run spegel in some clusters where customers have the ability to set up custom webhooks where our services call out over the network to theirs. As-is, a customer could set their webhook to the node ip/spegel's service ip, and make a call that could result in e.g. exposing internal container images to the customer; or just causing a huge blob to get served to the webhook server.

### Proposed solution to the problem

Add some sort of really basic authentication to the spegel server; e.g. a static registry password configured via env var.
This secret should be verified before serving a response (or even reading a request body).

Pull request:
Add basic authentication support
This change adds support for basic auth to Spegel. It works by allowing the user to specify a secret name which is expected to be of the type `kubernetes.io/basic-auth`, which will be mounted on the pod. The configuration init container will write the username and password as a basic auth header. While the registry container will use the secret to validate incoming requests. 

Updating the secret will require a restart of all of the pods so that the new secret can be written to the configuration. During this time some outage may occur as some instances will have the new secret configured while others have the old one.

It is worth noting that this is not a perfect solution as the basic auth is stored in plain text on the host configuration. It is however a easy addition for those who wish to have some sort of authentication.

Fixes #732 

Patch:
diff --git a/charts/spegel/README.md b/charts/spegel/README.md
index 9bc5696..8d386a0 100644
--- a/charts/spegel/README.md
+++ b/charts/spegel/README.md
@@ -9,6 +9,7 @@ Read the [getting started](https://spegel.dev/docs/getting-started/) guide to de
 | Key | Type | Default | Description |
 |-----|------|---------|-------------|
 | affinity | object | `{}` | Affinity settings for pod assignment. |
+| basicAuthSecretName | string | `""` | Name of secret containing basic authentication credentials for registry. |
 | clusterDomain | string | `"cluster.local."` | Domain configured for service domain names. |
 | commonLabels | object | `{}` | Common labels to apply to all rendered resources. |
 | fullnameOverride | string | `""` | Overrides the full name of the chart. |
diff --git a/charts/spegel/templates/daemonset.yaml b/charts/spegel/templates/daemonset.yaml
index 3ba7b33..00911f5 100644
--- a/charts/spegel/templates/daemonset.yaml
+++ b/charts/spegel/templates/daemonset.yaml
@@ -67,6 +67,11 @@ spec:
         volumeMounts:
           - name: containerd-config
             mountPath: {{ .Values.spegel.containerdRegistryConfigPath }}
+          {{- if .Values.basicAuthSecretName }}
+          - name: basic-auth
+            mountPath: "/etc/secrets/basic-auth"
+            readOnly: true
+          {{- end }}
       {{- end }}
       containers:
       - name: registry
@@ -139,6 +144,11 @@ spec:
             path: /healthz
             port: registry
         volumeMounts:
+          {{- if .Values.basicAuthSecretName }}
+          - name: basic-auth
+            mountPath: "/etc/secrets/basic-auth"
+            readOnly: true
+          {{- end }}
           - name: containerd-sock
             mountPath: {{ .Values.spegel.containerdSock }}
           {{- with .Values.spegel.containerdContentPath }}
@@ -149,6 +159,11 @@ spec:
         resources:
           {{- toYaml .Values.resources | nindent 10 }}
       volumes:
+        {{- with .Values.basicAuthSecretName }}
+        - name: basic-auth
+          secret:
+            secretName: {{ . }}
+        {{- end }}
         - name: containerd-sock
           hostPath:
             path: {{ .Values.spegel.containerdSock }}
diff --git a/charts/spegel/values.yaml b/charts/spegel/values.yaml
index f0e721c..5196575 100644
--- a/charts/spegel/values.yaml
+++ b/charts/spegel/values.yaml
@@ -133,6 +133,9 @@ grafanaDashboard:
 # -- Priority class name to use for the pod.
 priorityClassName: system-node-critical
 
+# -- Name of secret containing basic authentication credentials for registry.
+basicAuthSecretName: ""
+
 spegel:
   # -- Minimum log level to output. Value should be DEBUG, INFO, WARN, or ERROR.
   logLevel: "INFO"
diff --git a/main.go b/main.go
index e549c8f..dab0b66 100644
--- a/main.go
+++ b/main.go
@@ -11,6 +11,7 @@ import (
 	"net/url"
 	"os"
 	"os/signal"
+	"path/filepath"
 	"syscall"
 	"time"
 
@@ -104,8 +105,12 @@ func run(ctx context.Context, args *Arguments) error {
 }
 
 func configurationCommand(ctx context.Context, args *ConfigurationCmd) error {
+	username, password, err := loadBasicAuth()
+	if err != nil {
+		return err
+	}
 	fs := afero.NewOsFs()
-	err := oci.AddMirrorConfiguration(ctx, fs, args.ContainerdRegistryConfigPath, args.MirroredRegistries, args.MirrorTargets, args.ResolveTags, args.PrependExisting)
+	err = oci.AddMirrorConfiguration(ctx, fs, args.ContainerdRegistryConfigPath, args.MirroredRegistries, args.MirrorTargets, args.ResolveTags, args.PrependExisting, username, password)
 	if err != nil {
 		return err
 	}
@@ -116,6 +121,11 @@ func registryCommand(ctx context.Context, args *RegistryCmd) (err error) {
 	log := logr.FromContextOrDiscard(ctx)
 	g, ctx := errgroup.WithContext(ctx)
 
+	username, password, err := loadBasicAuth()
+	if err != nil {
+		return err
+	}
+
 	// OCI Client
 	ociClient, err := oci.NewContainerd(args.ContainerdSock, args.ContainerdNamespace, args.ContainerdRegistryConfigPath, args.MirroredRegistries, oci.WithContentPath(args.ContainerdContentPath))
 	if err != nil {
@@ -190,6 +200,7 @@ func registryCommand(ctx context.Context, args *RegistryCmd) (err error) {
 		registry.WithResolveTimeout(args.MirrorResolveTimeout),
 		registry.WithLocalAddress(args.LocalAddr),
 		registry.WithLogger(log),
+		registry.WithBasicAuth(username, password),
 	}
 	reg := registry.NewRegistry(ociClient, router, registryOpts...)
 	regSrv, err := reg.Server(args.RegistryAddr)
@@ -233,3 +244,16 @@ func getBootstrapper(cfg BootstrapConfig) (routing.Bootstrapper, error) { //noli
 		return nil, fmt.Errorf("unknown bootstrap kind %s", cfg.BootstrapKind)
 	}
 }
+
+func loadBasicAuth() (string, string, error) {
+	dirPath := "/etc/secrets/basic-auth"
+	username, err := os.ReadFile(filepath.Join(dirPath, "username"))
+	if err != nil && !errors.Is(err, os.ErrNotExist) {
+		return "", "", err
+	}
+	password, err := os.ReadFile(filepath.Join(dirPath, "password"))
+	if err != nil && !errors.Is(err, os.ErrNotExist) {
+		return "", "", err
+	}
+	return string(username), string(password), nil
+}
diff --git a/pkg/oci/containerd.go b/pkg/oci/containerd.go
index 6dba64f..35bdced 100644
--- a/pkg/oci/containerd.go
+++ b/pkg/oci/containerd.go
@@ -3,6 +3,7 @@ package oci
 import (
 	"bytes"
 	"context"
+	"encoding/base64"
 	"encoding/json"
 	"errors"
 	"fmt"
@@ -350,7 +351,7 @@ func createFilters(mirroredRegistries []url.URL) (string, string) {
 // Refer to containerd registry configuration documentation for more information about required configuration.
 // https://github.com/containerd/containerd/blob/main/docs/cri/config.md#registry-configuration
 // https://github.com/containerd/containerd/blob/main/docs/hosts.md#registry-configuration---examples
-func AddMirrorConfiguration(ctx context.Context, fs afero.Fs, configPath string, mirroredRegistries, mirrorTargets []url.URL, resolveTags, prependExisting bool) error {
+func AddMirrorConfiguration(ctx context.Context, fs afero.Fs, configPath string, mirroredRegistries, mirrorTargets []url.URL, resolveTags, prependExisting bool, username, password string) error {
 	log := logr.FromContextOrDiscard(ctx)
 	err := validateRegistries(mirroredRegistries)
 	if err != nil {
@@ -379,7 +380,7 @@ func AddMirrorConfiguration(ctx context.Context, fs afero.Fs, configPath string,
 
 	// Write mirror configuration
 	for _, mirroredRegistry := range mirroredRegistries {
-		templatedHosts, err := templateHosts(mirroredRegistry, mirrorTargets, capabilities)
+		templatedHosts, err := templateHosts(mirroredRegistry, mirrorTargets, capabilities, username, password)
 		if err != nil {
 			return err
 		}
@@ -477,13 +478,21 @@ func clearConfig(fs afero.Fs, configPath string) error {
 	return nil
 }
 
-func templateHosts(mirroredRegistry url.URL, mirrorTargets []url.URL, capabilities []string) (string, error) {
+func templateHosts(mirroredRegistry url.URL, mirrorTargets []url.URL, capabilities []string, username, password string) (string, error) {
 	server := mirroredRegistry.String()
 	if mirroredRegistry.String() == "https://docker.io" {
 		server = "https://registry-1.docker.io"
 	}
 
+	authorization := ""
+	if username != "" || password != "" {
+		authorization = username + ":" + password
+		authorization = base64.StdEncoding.EncodeToString([]byte(authorization))
+		authorization = "Basic " + authorization
+	}
+
 	hc := struct {
+		Authorization string
 		Server        string
 		Capabilities  string
 		MirrorTargets []url.URL
@@ -491,11 +500,17 @@ func templateHosts(mirroredRegistry url.URL, mirrorTargets []url.URL, capabiliti
 		Server:        server,
 		Capabilities:  fmt.Sprintf("['%s']", strings.Join(capabilities, "', '")),
 		MirrorTargets: mirrorTargets,
+		Authorization: authorization,
 	}
 	tmpl, err := template.New("").Parse(`{{- with .Server }}server = '{{ . }}'{{ end }}
+{{- $authorization := .Authorization }}
 {{ range .MirrorTargets }}
 [host.'{{ .String }}']
 capabilities = {{ $.Capabilities }}
+{{- if $authorization }}
+[host.'{{ .String }}'.header]
+Authorization = '{{ $authorization }}'
+{{- end }}
 {{ end }}`)
 	if err != nil {
 		return "", err
diff --git a/pkg/registry/registry.go b/pkg/registry/registry.go
index 9024c2f..dfb2a38 100644
--- a/pkg/registry/registry.go
+++ b/pkg/registry/registry.go
@@ -33,6 +33,8 @@ type Registry struct {
 	router           routing.Router
 	transport        http.RoundTripper
 	localAddr        string
+	username         string
+	password         string
 	resolveRetries   int
 	resolveTimeout   time.Duration
 	resolveLatestTag bool
@@ -76,6 +78,13 @@ func WithLogger(log logr.Logger) Option {
 	}
 }
 
+func WithBasicAuth(username, password string) Option {
+	return func(r *Registry) {
+		r.username = username
+		r.password = password
+	}
+}
+
 func NewRegistry(ociClient oci.Client, router routing.Router, opts ...Option) *Registry {
 	r := &Registry{
 		ociClient:        ociClient,
@@ -170,6 +179,15 @@ func (r *Registry) readyHandler(rw mux.ResponseWriter, req *http.Request) {
 }
 
 func (r *Registry) registryHandler(rw mux.ResponseWriter, req *http.Request) string {
+	// Check basic authentication
+	if r.username != "" || r.password != "" {
+		username, password, _ := req.BasicAuth()
+		if r.username != username || r.password != password {
+			rw.WriteError(http.StatusUnauthorized, errors.New("invalid basic authentication"))
+			return "registry"
+		}
+	}
+
 	// Quickly return 200 for /v2 to indicate that registry supports v2.
 	if path.Clean(req.URL.Path) == "/v2" {
 		rw.WriteHeader(http.StatusOK)


Test patch:
diff --git a/pkg/oci/containerd_test.go b/pkg/oci/containerd_test.go
index 47b2b75..32ca146 100644
--- a/pkg/oci/containerd_test.go
+++ b/pkg/oci/containerd_test.go
@@ -280,6 +280,8 @@ func TestMirrorConfiguration(t *testing.T) {
 		existingFiles       map[string]string
 		expectedFiles       map[string]string
 		name                string
+		username            string
+		password            string
 		registries          []url.URL
 		mirrors             []url.URL
 		resolveTags         bool
@@ -523,6 +525,28 @@ capabilities = ['pull', 'resolve']`,
 capabilities = ['pull', 'resolve']`,
 			},
 		},
+		{
+			name:            "with basic authentication",
+			resolveTags:     true,
+			registries:      stringListToUrlList(t, []string{"http://foo.bar:5000"}),
+			mirrors:         stringListToUrlList(t, []string{"http://127.0.0.1:5000", "http://127.0.0.1:5001"}),
+			prependExisting: false,
+			username:        "hello",
+			password:        "world",
+			expectedFiles: map[string]string{
+				"/etc/containerd/certs.d/foo.bar:5000/hosts.toml": `server = 'http://foo.bar:5000'
+
+[host.'http://127.0.0.1:5000']
+capabilities = ['pull', 'resolve']
+[host.'http://127.0.0.1:5000'.header]
+Authorization = 'Basic aGVsbG86d29ybGQ='
+
+[host.'http://127.0.0.1:5001']
+capabilities = ['pull', 'resolve']
+[host.'http://127.0.0.1:5001'.header]
+Authorization = 'Basic aGVsbG86d29ybGQ='`,
+			},
+		},
 	}
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
@@ -537,7 +561,7 @@ capabilities = ['pull', 'resolve']`,
 				err := afero.WriteFile(fs, k, []byte(v), 0o644)
 				require.NoError(t, err)
 			}
-			err := AddMirrorConfiguration(context.TODO(), fs, registryConfigPath, tt.registries, tt.mirrors, tt.resolveTags, tt.prependExisting)
+			err := AddMirrorConfiguration(context.TODO(), fs, registryConfigPath, tt.registries, tt.mirrors, tt.resolveTags, tt.prependExisting, tt.username, tt.password)
 			require.NoError(t, err)
 			ok, err := afero.DirExists(fs, "/etc/containerd/certs.d/_backup")
 			require.NoError(t, err)
@@ -568,19 +592,19 @@ func TestMirrorConfigurationInvalidMirrorURL(t *testing.T) {
 	mirrors := stringListToUrlList(t, []string{"http://127.0.0.1:5000"})
 
 	registries := stringListToUrlList(t, []string{"ftp://docker.io"})
-	err := AddMirrorConfiguration(context.TODO(), fs, "/etc/containerd/certs.d", registries, mirrors, true, false)
+	err := AddMirrorConfiguration(context.TODO(), fs, "/etc/containerd/certs.d", registries, mirrors, true, false, "", "")
 	require.EqualError(t, err, "invalid registry url scheme must be http or https: ftp://docker.io")
 
 	registries = stringListToUrlList(t, []string{"https://docker.io/foo/bar"})
-	err = AddMirrorConfiguration(context.TODO(), fs, "/etc/containerd/certs.d", registries, mirrors, true, false)
+	err = AddMirrorConfiguration(context.TODO(), fs, "/etc/containerd/certs.d", registries, mirrors, true, false, "", "")
 	require.EqualError(t, err, "invalid registry url path has to be empty: https://docker.io/foo/bar")
 
 	registries = stringListToUrlList(t, []string{"https://docker.io?foo=bar"})
-	err = AddMirrorConfiguration(context.TODO(), fs, "/etc/containerd/certs.d", registries, mirrors, true, false)
+	err = AddMirrorConfiguration(context.TODO(), fs, "/etc/containerd/certs.d", registries, mirrors, true, false, "", "")
 	require.EqualError(t, err, "invalid registry url query has to be empty: https://docker.io?foo=bar")
 
 	registries = stringListToUrlList(t, []string{"https://foo@docker.io"})
-	err = AddMirrorConfiguration(context.TODO(), fs, "/etc/containerd/certs.d", registries, mirrors, true, false)
+	err = AddMirrorConfiguration(context.TODO(), fs, "/etc/containerd/certs.d", registries, mirrors, true, false, "", "")
 	require.EqualError(t, err, "invalid registry url user has to be empty: https://foo@docker.io")
 }
 
diff --git a/pkg/registry/registry_test.go b/pkg/registry/registry_test.go
index aaba8e9..9ea7a87 100644
--- a/pkg/registry/registry_test.go
+++ b/pkg/registry/registry_test.go
@@ -14,6 +14,81 @@ import (
 	"github.com/spegel-org/spegel/pkg/routing"
 )
 
+func TestBasicAuth(t *testing.T) {
+	t.Parallel()
+
+	tests := []struct {
+		name        string
+		username    string
+		password    string
+		reqUsername string
+		reqPassword string
+		expected    int
+	}{
+		{
+			name:     "no registry authentication",
+			expected: http.StatusOK,
+		},
+		{
+			name:        "unnecessary authentication",
+			reqUsername: "foo",
+			reqPassword: "bar",
+			expected:    http.StatusOK,
+		},
+		{
+			name:        "correct authentication",
+			username:    "foo",
+			password:    "bar",
+			reqUsername: "foo",
+			reqPassword: "bar",
+			expected:    http.StatusOK,
+		},
+		{
+			name:        "invalid username",
+			username:    "foo",
+			password:    "bar",
+			reqUsername: "wrong",
+			reqPassword: "bar",
+			expected:    http.StatusUnauthorized,
+		},
+		{
+			name:        "invalid password",
+			username:    "foo",
+			password:    "bar",
+			reqUsername: "foo",
+			reqPassword: "wrong",
+			expected:    http.StatusUnauthorized,
+		},
+		{
+			name:     "missing authentication",
+			username: "foo",
+			password: "bar",
+			expected: http.StatusUnauthorized,
+		},
+		{
+			name:     "missing authentication",
+			username: "foo",
+			password: "bar",
+			expected: http.StatusUnauthorized,
+		},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			t.Parallel()
+
+			reg := NewRegistry(nil, nil, WithBasicAuth(tt.username, tt.password))
+			rw := httptest.NewRecorder()
+			req := httptest.NewRequest(http.MethodGet, "http://localhost/v2", nil)
+			req.SetBasicAuth(tt.reqUsername, tt.reqPassword)
+			m, err := mux.NewServeMux(reg.handle)
+			require.NoError(t, err)
+			m.ServeHTTP(rw, req)
+
+			require.Equal(t, tt.expected, rw.Result().StatusCode)
+		})
+	}
+}
+
 func TestMirrorHandler(t *testing.T) {
 	t.Parallel()