Merge pull request cert-manager#130 from SgtCoDFish/annotationsan

Move parsing of custom annotations to testable function

internal/annotations/annotations.go

@@ -0,0 +1,24 @@
+/*
+Copyright 2024 The cert-manager Authors.
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+// Package annotations holds constants which represent built-in csi-driver-spiffe annotations
+package annotations
+
+const (
+	Prefix = "spiffe.csi.cert-manager.io"
+
+	SPIFFEIdentityAnnnotationKey = "spiffe.csi.cert-manager.io/identity"
+)

internal/csi/app/app.go

@@ -19,7 +19,6 @@ package app
 import (
 	"context"
 	"fmt"
-	"strings"
 
 	"github.com/spf13/cobra"
 
@@ -62,15 +61,6 @@ func NewCommand(ctx context.Context) *cobra.Command {
 				log.Info("propagating root CA bundle disabled")
 			}
 
-			annotations := map[string]string{}
-			for key, value := range opts.CertManager.CertificateRequestAnnotations {
-				if strings.HasPrefix(key, "spiffe.csi.cert-manager.io") {
-					log.Error(nil, "custom annotations must not begin with spiffe.csi.cert-manager.io", "annotation-key", key)
-				} else {
-					annotations[key] = value
-				}
-			}
-
 			driver, err := driver.New(opts.Logr, driver.Options{
 				DriverName: opts.DriverName,
 				NodeID:     opts.Driver.NodeID,
@@ -79,7 +69,7 @@ func NewCommand(ctx context.Context) *cobra.Command {
 
 				RestConfig:                    opts.RestConfig,
 				TrustDomain:                   opts.CertManager.TrustDomain,
-				CertificateRequestAnnotations: annotations,
+				CertificateRequestAnnotations: opts.CertManager.CertificateRequestAnnotations,
 				CertificateRequestDuration:    opts.CertManager.CertificateRequestDuration,
 				IssuerRef:                     opts.CertManager.IssuerRef,
 

internal/csi/driver/driver.go

@@ -21,8 +21,10 @@ import (
 	"crypto"
 	"crypto/x509"
 	"encoding/pem"
+	"errors"
 	"fmt"
 	"net/url"
+	"strings"
 	"sync"
 	"time"
 
@@ -39,6 +41,7 @@ import (
 	"k8s.io/client-go/rest"
 	"k8s.io/utils/clock"
 
+	"github.com/cert-manager/csi-driver-spiffe/internal/annotations"
 	"github.com/cert-manager/csi-driver-spiffe/internal/csi/rootca"
 	"github.com/cert-manager/csi-driver-spiffe/internal/version"
 )
@@ -139,6 +142,12 @@ type Driver struct {
 
 // New constructs a new Driver instance.
 func New(log logr.Logger, opts Options) (*Driver, error) {
+	sanitizedAnnotations, err := sanitizeAnnotations(opts.CertificateRequestAnnotations)
+	if err != nil {
+		log.Error(err, "some custom annotations were removed")
+		// don't exit, not a fatal error as sanitizeAnnotations will trim bad annotations
+	}
+
 	d := &Driver{
 		log:          log.WithName("csi"),
 		trustDomain:  opts.TrustDomain,
@@ -148,28 +157,30 @@ func New(log logr.Logger, opts Options) (*Driver, error) {
 		rootCAs:      opts.RootCAs,
 
 		certificateRequestDuration:    opts.CertificateRequestDuration,
-		certificateRequestAnnotations: opts.CertificateRequestAnnotations,
+		certificateRequestAnnotations: sanitizedAnnotations,
 	}
 
-	// Set sane defaults.
 	if len(d.certFileName) == 0 {
 		d.certFileName = "tls.crt"
 	}
+
 	if len(d.keyFileName) == 0 {
 		d.keyFileName = "tls.key"
 	}
+
 	if len(d.caFileName) == 0 {
 		d.caFileName = "ca.crt"
 	}
+
 	if d.certificateRequestDuration == 0 {
 		d.certificateRequestDuration = time.Hour
 	}
 
-	var err error
 	store, err := storage.NewFilesystem(d.log, opts.DataRoot)
 	if err != nil {
 		return nil, fmt.Errorf("failed to setup filesystem: %w", err)
 	}
+
 	// Used by clients to set the stored file's file-system group before
 	// mounting.
 	store.FSGroupVolumeAttributeKey = "spiffe.csi.cert-manager.io/fs-group"
@@ -278,9 +289,13 @@ func (d *Driver) generateRequest(meta metadata.Metadata) (*manager.CertificateRe
 	if err != nil {
 		return nil, fmt.Errorf("internal error crafting X.509 URI, this is a bug, please report on GitHub: %w", err)
 	}
-	annotations := map[string]string{"spiffe.csi.cert-manager.io/identity": spiffeID}
+
+	crAnnotations := map[string]string{
+		annotations.SPIFFEIdentityAnnnotationKey: spiffeID,
+	}
+
 	for key, value := range d.certificateRequestAnnotations {
-		annotations[key] = value
+		crAnnotations[key] = value
 	}
 
 	return &manager.CertificateRequestBundle{
@@ -297,7 +312,7 @@ func (d *Driver) generateRequest(meta metadata.Metadata) (*manager.CertificateRe
 			cmapi.UsageClientAuth,
 		},
 		IssuerRef:   d.issuerRef,
-		Annotations: annotations,
+		Annotations: crAnnotations,
 	}, nil
 }
 
@@ -344,3 +359,20 @@ func (d *Driver) writeKeypair(meta metadata.Metadata, key crypto.PrivateKey, cha
 
 	return nil
 }
+
+func sanitizeAnnotations(in map[string]string) (map[string]string, error) {
+	out := map[string]string{}
+
+	var errs []error
+
+	for key, value := range in {
+		if strings.HasPrefix(key, annotations.Prefix) {
+			errs = append(errs, fmt.Errorf("custom annotation %q was not valid; must not begin with %s", key, annotations.Prefix))
+			continue
+		}
+
+		out[key] = value
+	}
+
+	return out, errors.Join(errs...)
+}

internal/csi/driver/driver_test.go

@@ -21,6 +21,7 @@ import (
 	"crypto/ecdsa"
 	"crypto/elliptic"
 	"crypto/rand"
+	"reflect"
 	"testing"
 
 	cmapi "github.com/cert-manager/cert-manager/pkg/apis/certmanager/v1"
@@ -30,6 +31,7 @@ import (
 	"github.com/spiffe/go-spiffe/v2/svid/x509svid"
 	"github.com/stretchr/testify/require"
 
+	"github.com/cert-manager/csi-driver-spiffe/internal/annotations"
 	"github.com/cert-manager/csi-driver-spiffe/internal/csi/rootca"
 )
 
@@ -89,3 +91,48 @@ func Test_writeKeyPair(t *testing.T) {
 	_, err = x509svid.Parse(files["crt.pem"], files["key.pem"])
 	require.NoError(t, err)
 }
+
+func Test_DriverAnnotationSanitization(t *testing.T) {
+	badAnnotation := annotations.Prefix + "/customannotation"
+
+	tests := map[string]struct {
+		in          map[string]string
+		expectErr   bool
+		expectedOut map[string]string
+	}{
+		"bad annotations are removed": {
+			in: map[string]string{
+				badAnnotation:              "abc123",
+				"example.com/myannotation": "should-not-be-removed",
+			},
+			expectErr: true,
+			expectedOut: map[string]string{
+				"example.com/myannotation": "should-not-be-removed",
+			},
+		},
+		"good annotations don't produce an error": {
+			in: map[string]string{
+				"example.com/myannotation": "should-not-be-removed",
+			},
+			expectErr: false,
+			expectedOut: map[string]string{
+				"example.com/myannotation": "should-not-be-removed",
+			},
+		},
+	}
+
+	for name, test := range tests {
+		test := test
+		t.Run(name, func(t *testing.T) {
+			out, err := sanitizeAnnotations(test.in)
+
+			if err != nil != test.expectErr {
+				t.Errorf("expectErr=%v but err=%v", test.expectErr, err)
+			}
+
+			if !reflect.DeepEqual(out, test.expectedOut) {
+				t.Errorf("wanted out=%v but got %v", test.expectedOut, out)
+			}
+		})
+	}
+}