Fix panic if target issuer referenced but not allowed (#371)

* handle panic if target issuer is referenced but not allowed * add integration test for unallowed target issuer * fix error handling arguments
gardener · Nov 29, 2024 · 7ad639e · 7ad639e
1 parent 5506cd1
commit 7ad639e
Show file tree

Hide file tree

Showing 5 changed files with 82 additions and 19 deletions.
diff --git a/pkg/controller/issuer/acme/handler.go b/pkg/controller/issuer/acme/handler.go
@@ -126,7 +126,11 @@ func (r *acmeIssuerHandler) Reconcile(logger logger.LogContext, obj resources.Ob
 		if err != nil {
 			return r.failedAcme(logger, obj, api.StateError, fmt.Errorf("registration marshalling failed: %w", err))
 		}
-		newObj, err := r.support.GetIssuerResources(issuerKey).Update(issuer)
+		issuerResources, err := r.support.GetIssuerResources(issuerKey)
+		if err != nil {
+			return r.failedAcme(logger, obj, api.StateError, fmt.Errorf("invalid issuer: %w", err))
+		}
+		newObj, err := issuerResources.Update(issuer)
 		if err != nil {
 			return r.failedAcmeRetry(logger, obj, api.StateError, fmt.Errorf("updating resource failed: %w", err))
 		}

diff --git a/pkg/controller/issuer/certificate/reconciler.go b/pkg/controller/issuer/certificate/reconciler.go
@@ -572,8 +572,12 @@ func (r *certReconciler) restoreCA(issuerKey utils.IssuerKey, issuer *api.Issuer
 		return nil, fmt.Errorf("CA registration? missing in status")
 	}
 	issuerSecretObjectName := resources.NewObjectName(secretRef.Namespace, secretRef.Name)
+	secretResources, err := r.support.GetIssuerSecretResources(issuerKey)
+	if err != nil {
+		return nil, fmt.Errorf("fetching issuer secret failed: %w", err)
+	}
 	issuerSecret := &corev1.Secret{}
-	_, err := r.support.GetIssuerSecretResources(issuerKey).GetInto(issuerSecretObjectName, issuerSecret)
+	_, err = secretResources.GetInto(issuerSecretObjectName, issuerSecret)
 	if err != nil {
 		return nil, fmt.Errorf("fetching issuer secret failed: %w", err)
 	}

diff --git a/pkg/controller/issuer/core/support.go b/pkg/controller/issuer/core/support.go
@@ -89,6 +89,7 @@ func NewHandlerSupport(c controller.Interface) (*Support, error) {
 		defaultSecretResources: defaultSecretResources,
 		targetIssuerResources:  targetIssuerResources,
 		targetSecretResources:  targetSecretResources,
+		targetIssuerAllowed:    allowTargetIssuers,
 		defaultCluster:         defaultCluster,
 		targetCluster:          targetCluster,
 	}
@@ -221,6 +222,7 @@ type Support struct {
 	defaultSecretResources     resources.Interface
 	targetIssuerResources      resources.Interface
 	targetSecretResources      resources.Interface
+	targetIssuerAllowed        bool
 	defaultIssuerName          string
 	issuerNamespace            string
 	defaultRequestsPerDayQuota int
@@ -262,7 +264,11 @@ func (s *Support) WriteIssuerSecretFromRegistrationUser(issuerKey utils.IssuerKe
 		return nil, nil, err
 	}
 
-	obj, err := s.GetIssuerSecretResources(issuerKey).CreateOrUpdate(secret)
+	secretResources, err := s.GetIssuerSecretResources(issuerKey)
+	if err != nil {
+		return nil, nil, fmt.Errorf("getting issuer secret resources failed: %w", err)
+	}
+	obj, err := secretResources.CreateOrUpdate(secret)
 	if err != nil {
 		return nil, nil, fmt.Errorf("creating/updating issuer secret failed: %w", err)
 	}
@@ -278,7 +284,11 @@ func (s *Support) UpdateIssuerSecret(issuerKey utils.IssuerKey, reguser *legobri
 	if err != nil {
 		return err
 	}
-	obj, err := s.GetIssuerSecretResources(issuerKey).Wrap(secret)
+	secretResources, err := s.GetIssuerSecretResources(issuerKey)
+	if err != nil {
+		return fmt.Errorf("getting issuer secret resources failed: %w", err)
+	}
+	obj, err := secretResources.Wrap(secret)
 	if err != nil {
 		return fmt.Errorf("wrapping issuer secret failed: %w", err)
 	}
@@ -292,11 +302,14 @@ func (s *Support) UpdateIssuerSecret(issuerKey utils.IssuerKey, reguser *legobri
 
 // ReadIssuerSecret reads a issuer secret
 func (s *Support) ReadIssuerSecret(issuerKey utils.IssuerKey, ref *corev1.SecretReference) (*corev1.Secret, error) {
-	res := s.GetIssuerSecretResources(issuerKey)
+	secretResources, err := s.GetIssuerSecretResources(issuerKey)
+	if err != nil {
+		return nil, fmt.Errorf("getting issuer secret resources failed: %w", err)
+	}
 
 	secret := &corev1.Secret{}
 	objName := resources.NewObjectName(ref.Namespace, ref.Name)
-	_, err := res.GetInto(objName, secret)
+	_, err = secretResources.GetInto(objName, secret)
 	if err != nil {
 		return nil, err
 	}
@@ -663,25 +676,31 @@ func (s *Support) IsDefaultIssuer(issuerKey utils.IssuerKey) bool {
 }
 
 // GetIssuerResources returns the resources for issuer.
-func (s *Support) GetIssuerResources(issuerKey utils.IssuerKey) resources.Interface {
+func (s *Support) GetIssuerResources(issuerKey utils.IssuerKey) (resources.Interface, error) {
 	switch issuerKey.Cluster() {
 	case utils.ClusterDefault:
-		return s.defaultIssuerResources
+		return s.defaultIssuerResources, nil
 	case utils.ClusterTarget:
-		return s.targetIssuerResources
+		if !s.targetIssuerAllowed {
+			return nil, fmt.Errorf("target issuers not allowed")
+		}
+		return s.targetIssuerResources, nil
 	}
-	return nil
+	return nil, fmt.Errorf("unexpected issuer cluster: %s", issuerKey.ClusterName())
 }
 
 // GetIssuerSecretResources returns the resources for issuer secrets.
-func (s *Support) GetIssuerSecretResources(issuerKey utils.IssuerKey) resources.Interface {
+func (s *Support) GetIssuerSecretResources(issuerKey utils.IssuerKey) (resources.Interface, error) {
 	switch issuerKey.Cluster() {
 	case utils.ClusterDefault:
-		return s.defaultSecretResources
+		return s.defaultSecretResources, nil
 	case utils.ClusterTarget:
-		return s.targetSecretResources
+		if !s.targetIssuerAllowed {
+			return nil, fmt.Errorf("target issuers not allowed")
+		}
+		return s.targetSecretResources, nil
 	}
-	return nil
+	return nil, fmt.Errorf("unexpected issuer cluster: %s", issuerKey.ClusterName())
 }
 
 // CalcSecretHash calculates the secret hash
@@ -742,7 +761,7 @@ func (s *Support) ClearCertRevoked(certName resources.ObjectName) {
 	}
 }
 
-// GetAllRevoked gets all certificate object object names which are revoked
+// GetAllRevoked gets all certificate object names which are revoked
 func (s *Support) GetAllRevoked() []resources.ObjectName {
 	return s.state.GetAllRevoked()
 }
@@ -754,9 +773,12 @@ func (s *Support) reportRevokedCount() {
 
 // LoadIssuer loads the issuer for the given Certificate
 func (s *Support) LoadIssuer(issuerKey utils.IssuerKey) (*api.Issuer, error) {
-	res := s.GetIssuerResources(issuerKey)
+	issuerResources, err := s.GetIssuerResources(issuerKey)
+	if err != nil {
+		return nil, err
+	}
 	issuer := &api.Issuer{}
-	_, err := res.GetInto(issuerKey.ObjectName(s.IssuerNamespace()), issuer)
+	_, err = issuerResources.GetInto(issuerKey.ObjectName(s.IssuerNamespace()), issuer)
 	if err != nil {
 		return nil, fmt.Errorf("fetching issuer failed: %w", err)
 	}

diff --git a/pkg/controller/issuer/reconciler.go b/pkg/controller/issuer/reconciler.go
@@ -77,8 +77,11 @@ func (r *compoundReconciler) Setup() error {
 
 func (r *compoundReconciler) setupIssuers(cluster utils.Cluster) error {
 	dummyKey := utils.NewIssuerKey(cluster, "dummy", "dummy")
-	res := r.handler.Support().GetIssuerResources(dummyKey)
-	list, err := res.Namespace(r.handler.Support().IssuerNamespace()).List(v1.ListOptions{})
+	issuerResources, err := r.handler.Support().GetIssuerResources(dummyKey)
+	if err != nil {
+		return fmt.Errorf("cannot get issuer resources: %w", err)
+	}
+	list, err := issuerResources.Namespace(r.handler.Support().IssuerNamespace()).List(v1.ListOptions{})
 	if err != nil {
 		return err
 	}

diff --git a/test/integration/controller/issuer/issuer_test.go b/test/integration/controller/issuer/issuer_test.go
@@ -14,6 +14,7 @@ import (
 	. "github.com/gardener/gardener/pkg/utils/test/matchers"
 	. "github.com/onsi/ginkgo/v2"
 	. "github.com/onsi/gomega"
+	. "github.com/onsi/gomega/gstruct"
 	corev1 "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/utils/ptr"
@@ -152,6 +153,35 @@ var _ = Describe("Issuer controller tests", func() {
 				return cert.Status.State
 			}).WithPolling(500 * time.Millisecond).WithTimeout(20 * time.Second).Should(Equal("Ready"))
 		})
+
+		It("should reconcile a certificate referencing unallowed target issuer", func() {
+			cert := &v1alpha1.Certificate{
+				ObjectMeta: metav1.ObjectMeta{
+					Namespace: testRunID,
+					Name:      "certificate-with-unallowed-issuer",
+				},
+				Spec: v1alpha1.CertificateSpec{
+					CommonName: ptr.To("example.com"),
+					IssuerRef: &v1alpha1.IssuerRef{
+						Namespace: "namespace1",
+						Name:      "foo",
+					},
+				},
+			}
+			Expect(testClient.Create(ctx, cert)).To(Succeed())
+			DeferCleanup(func() {
+				Expect(testClient.Delete(ctx, cert)).To(Succeed())
+			})
+
+			By("Wait for certificate to become ready")
+			Eventually(func(g Gomega) v1alpha1.CertificateStatus {
+				g.Expect(testClient.Get(ctx, client.ObjectKeyFromObject(cert), cert)).To(Succeed())
+				return cert.Status
+			}).Should(MatchFields(IgnoreExtras, Fields{
+				"State":   Equal("Error"),
+				"Message": PointTo(Equal("target issuers not allowed")),
+			}))
+		})
 	})
 
 	Context("Self-signed issuer", func() {