From 9960deae6ce045353b9ecc9a20dd63fad4ec0411 Mon Sep 17 00:00:00 2001 From: Alex Chvatal Date: Wed, 9 Oct 2024 13:38:08 -0400 Subject: [PATCH] use correct common name when correcting cert issuer --- pkg/cluster/correct_cert_issuer.go | 27 ++++++++++---- pkg/cluster/correct_cert_issuer_test.go | 48 ++++++++++++++++--------- 2 files changed, 51 insertions(+), 24 deletions(-) diff --git a/pkg/cluster/correct_cert_issuer.go b/pkg/cluster/correct_cert_issuer.go index 255628ae114..7f1ed97e1a7 100644 --- a/pkg/cluster/correct_cert_issuer.go +++ b/pkg/cluster/correct_cert_issuer.go @@ -5,6 +5,8 @@ package cluster import ( "context" + "errors" + "fmt" "strings" "github.com/Azure/ARO-RP/pkg/util/keyvault" @@ -16,18 +18,29 @@ import ( // signing algorithm in use by DigiCert func (m *manager) correctCertificateIssuer(ctx context.Context) error { if !strings.Contains(m.doc.OpenShiftCluster.Properties.ClusterProfile.Domain, ".") { - for _, certName := range []string{m.doc.ID + "-apiserver", m.doc.ID + "-ingress"} { - err := m.ensureCertificateIssuer(ctx, certName, "OneCertV2-PublicCA") - if err != nil { - return err - } + apiCertName := m.doc.ID + "-apiserver" + apiHostname := strings.Split(strings.TrimPrefix(m.doc.OpenShiftCluster.Properties.APIServerProfile.URL, "https://"), ":")[0] + err := m.ensureCertificateIssuer(ctx, apiCertName, apiHostname, "OneCertV2-PublicCA") + if err != nil { + return err + } + + ingressCertName := m.doc.ID + "-ingress" + ingressHostname := "*" + strings.TrimSuffix(strings.TrimPrefix(m.doc.OpenShiftCluster.Properties.ConsoleProfile.URL, "https://console-openshift-console"), "/") + err = m.ensureCertificateIssuer(ctx, ingressCertName, ingressHostname, "OneCertV2-PublicCA") + if err != nil { + return err } } return nil } -func (m *manager) ensureCertificateIssuer(ctx context.Context, certificateName, issuerName string) error { +func (m *manager) ensureCertificateIssuer(ctx context.Context, certificateName, dnsName, issuerName string) error { + if strings.Count(dnsName, ".") < 2 { + return errors.New(fmt.Sprintf("%s is not a valid DNS name", dnsName)) + } + clusterKeyvault := m.env.ClusterKeyvault() bundle, err := clusterKeyvault.GetCertificate(ctx, certificateName) @@ -47,7 +60,7 @@ func (m *manager) ensureCertificateIssuer(ctx context.Context, certificateName, return err } - err = clusterKeyvault.CreateSignedCertificate(ctx, issuerName, certificateName, certificateName, keyvault.EkuServerAuth) + err = clusterKeyvault.CreateSignedCertificate(ctx, issuerName, certificateName, dnsName, keyvault.EkuServerAuth) if err != nil { return err } diff --git a/pkg/cluster/correct_cert_issuer_test.go b/pkg/cluster/correct_cert_issuer_test.go index 8d97334419d..3aa988a8168 100644 --- a/pkg/cluster/correct_cert_issuer_test.go +++ b/pkg/cluster/correct_cert_issuer_test.go @@ -18,6 +18,7 @@ func TestEnsureCertificateIssuer(t *testing.T) { tests := []struct { Name string CertificateName string + DNSName string CurrentIssuerName string NewIssuerName string ExpectError bool @@ -25,15 +26,25 @@ func TestEnsureCertificateIssuer(t *testing.T) { { Name: "current issuer matches new issuer", CertificateName: "testCert", + DNSName: "*.apps.test.asdf.tld", CurrentIssuerName: "fakeIssuer", NewIssuerName: "fakeIssuer", }, { Name: "current issuer different from new issuer", CertificateName: "testCert", + DNSName: "*.apps.test.asdf.tld", CurrentIssuerName: "OldFakeIssuer", NewIssuerName: "NewFakeIssuer", }, + { + Name: "malformed dns data", + CertificateName: "testCert", + DNSName: "f898896d-c9ce-4d2b-b218-95a1f858df3a-something", + CurrentIssuerName: "OldFakeIssuer", + NewIssuerName: "NewFakeIssuer", + ExpectError: true, + }, } for _, test := range tests { @@ -42,33 +53,36 @@ func TestEnsureCertificateIssuer(t *testing.T) { defer controller.Finish() clusterKeyvault := mock_keyvault.NewMockManager(controller) - clusterKeyvault.EXPECT().GetCertificate(gomock.Any(), test.CertificateName).Return(azkeyvault.CertificateBundle{ - Policy: &azkeyvault.CertificatePolicy{ - IssuerParameters: &azkeyvault.IssuerParameters{ - Name: &test.CurrentIssuerName, - }, - }, - }, nil) + env := mock_env.NewMockInterface(controller) - if test.CurrentIssuerName != test.NewIssuerName { - clusterKeyvault.EXPECT().GetCertificatePolicy(gomock.Any(), test.CertificateName).Return(azkeyvault.CertificatePolicy{ - IssuerParameters: &azkeyvault.IssuerParameters{ - Name: &test.CurrentIssuerName, + if !test.ExpectError { + clusterKeyvault.EXPECT().GetCertificate(gomock.Any(), test.CertificateName).Return(azkeyvault.CertificateBundle{ + Policy: &azkeyvault.CertificatePolicy{ + IssuerParameters: &azkeyvault.IssuerParameters{ + Name: &test.CurrentIssuerName, + }, }, }, nil) - clusterKeyvault.EXPECT().UpdateCertificatePolicy(gomock.Any(), test.CertificateName, gomock.Any()).Return(nil) - clusterKeyvault.EXPECT().CreateSignedCertificate(gomock.Any(), test.NewIssuerName, test.CertificateName, test.CertificateName, gomock.Any()).Return(nil) - } + if test.CurrentIssuerName != test.NewIssuerName { + clusterKeyvault.EXPECT().GetCertificatePolicy(gomock.Any(), test.CertificateName).Return(azkeyvault.CertificatePolicy{ + IssuerParameters: &azkeyvault.IssuerParameters{ + Name: &test.CurrentIssuerName, + }, + }, nil) - env := mock_env.NewMockInterface(controller) - env.EXPECT().ClusterKeyvault().AnyTimes().Return(clusterKeyvault) + clusterKeyvault.EXPECT().UpdateCertificatePolicy(gomock.Any(), test.CertificateName, gomock.Any()).Return(nil) + clusterKeyvault.EXPECT().CreateSignedCertificate(gomock.Any(), test.NewIssuerName, test.CertificateName, test.DNSName, gomock.Any()).Return(nil) + } + + env.EXPECT().ClusterKeyvault().AnyTimes().Return(clusterKeyvault) + } m := &manager{ env: env, } - err := m.ensureCertificateIssuer(context.TODO(), test.CertificateName, test.NewIssuerName) + err := m.ensureCertificateIssuer(context.TODO(), test.CertificateName, test.DNSName, test.NewIssuerName) if test.ExpectError == (err == nil) { t.Errorf("TestCorrectCertificateIssuer() %s: ExpectError: %t, actual error: %s\n", test.Name, test.ExpectError, err) }