Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Ensure external certs have a hash #3535

Merged
merged 6 commits into from
Oct 24, 2024
Merged

Ensure external certs have a hash #3535

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
ffdde8b
Ideally, we create the trusted bundle from the certificate manager ob…
rene-dekker Oct 10, 2024
3e6889b
If a certificate is external, we must ensure that there is a hash for…
rene-dekker Oct 10, 2024
3270939
We should create a hash for both byo secrets even if the same pem is …
rene-dekker Oct 21, 2024
dd63db6
Address PR comments.
rene-dekker Oct 22, 2024
f3197db
Address PR comments.
rene-dekker Oct 22, 2024
4a3e020
Keep the sorting logic for certificates in the bundle
rene-dekker Oct 23, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion pkg/controller/apiserver/apiserver_controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -360,7 +360,7 @@ func (r *ReconcileAPIServer) Reconcile(ctx context.Context, request reconcile.Re
r.status.SetDegraded(operatorv1.ResourceReadError, "Failed to get certificate", err, reqLogger)
return reconcile.Result{}, err
} else if prometheusCertificate != nil {
trustedBundle = certificatemanagement.CreateTrustedBundle(prometheusCertificate)
trustedBundle = certificateManager.CreateTrustedBundle(prometheusCertificate)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed for consistency, to ensure that every bundle will contain the ca as well.

}
}

Expand Down
8 changes: 4 additions & 4 deletions pkg/controller/certificatemanager/certificatemanager.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -329,7 +329,7 @@ func (cm *certificateManager) GetOrCreateKeyPair(cli client.Client, secretName,
cm.log.V(3).Info("secret %s has invalid DNS names, the expected names are: %v", secretName, dnsNames)
return keyPair, nil
}
} else if keyPair == nil {
} else {
Copy link
Member Author

@rene-dekker rene-dekker Oct 18, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The condition if keyPair == nil is always true here, so I removed it.

cm.log.V(1).Info("Keypair wasn't found, create a new one", "namespace", secretNamespace, "name", secretName)
}

Expand Down Expand Up @@ -561,19 +561,19 @@ func HasExpectedDNSNames(secretName, secretNamespace string, cert *x509.Certific
// It will include:
// - A bundle with Calico's root certificates + any user supplied certificates in /etc/pki/tls/certs/tigera-ca-bundle.crt.
func (cm *certificateManager) CreateTrustedBundle(certificates ...certificatemanagement.CertificateInterface) certificatemanagement.TrustedBundle {
return certificatemanagement.CreateTrustedBundle(append([]certificatemanagement.CertificateInterface{cm.keyPair}, certificates...)...)
return certificatemanagement.CreateTrustedBundle(cm.keyPair, certificates...)
}

// CreateTrustedBundleWithSystemRootCertificates creates a TrustedBundle, which provides standardized methods for mounting a bundle of certificates to trust.
// It will include:
// - A bundle with Calico's root certificates + any user supplied certificates in /etc/pki/tls/certs/tigera-ca-bundle.crt.
// - A system root certificate bundle in /etc/pki/tls/certs/ca-bundle.crt.
func (cm *certificateManager) CreateTrustedBundleWithSystemRootCertificates(certificates ...certificatemanagement.CertificateInterface) (certificatemanagement.TrustedBundle, error) {
return certificatemanagement.CreateTrustedBundleWithSystemRootCertificates(append([]certificatemanagement.CertificateInterface{cm.keyPair}, certificates...)...)
return certificatemanagement.CreateTrustedBundleWithSystemRootCertificates(cm.keyPair, certificates...)
}

func (cm *certificateManager) CreateMultiTenantTrustedBundleWithSystemRootCertificates(certificates ...certificatemanagement.CertificateInterface) (certificatemanagement.TrustedBundle, error) {
return certificatemanagement.CreateMultiTenantTrustedBundleWithSystemRootCertificates(append([]certificatemanagement.CertificateInterface{cm.keyPair}, certificates...)...)
return certificatemanagement.CreateMultiTenantTrustedBundleWithSystemRootCertificates(cm.keyPair, certificates...)
}

func (cm *certificateManager) LoadTrustedBundle(ctx context.Context, client client.Client, ns string) (certificatemanagement.TrustedBundleRO, error) {
Expand Down
37 changes: 37 additions & 0 deletions pkg/controller/certificatemanager/certificatemanager_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -671,6 +671,43 @@ var _ = Describe("Test CertificateManagement suite", func() {
},
}))
})

It("should create a hash for both secrets even if the same pem is used twice", func() {
By("creating 2 secrets with identical pem in the datastore", func() {

byoSecretCopy := byoSecret.DeepCopyObject().(*corev1.Secret)

byoSecret.Name, byoSecret.Namespace = "byo-secret", common.OperatorNamespace()
Expect(cli.Create(ctx, byoSecret)).NotTo(HaveOccurred())

byoSecretCopy.Name, byoSecretCopy.Namespace = "byo-secret-copy", common.OperatorNamespace()
Expect(cli.Create(ctx, byoSecretCopy)).NotTo(HaveOccurred())
})

var trustedBundle certificatemanagement.TrustedBundle
By("creating the bundle from the certificates", func() {
byo, err := certificateManager.GetCertificate(cli, "byo-secret", common.OperatorNamespace())
Expect(err).NotTo(HaveOccurred())
byoCopy, err := certificateManager.GetCertificate(cli, "byo-secret-copy", common.OperatorNamespace())
Expect(err).NotTo(HaveOccurred())
Expect(byo.GetIssuer()).NotTo(Equal(certificateManager.KeyPair()))
Expect(byoCopy.GetIssuer()).NotTo(Equal(certificateManager.KeyPair()))
trustedBundle = certificateManager.CreateTrustedBundle(byo, byoCopy)
})

By("verifying the hash annotations and pem blocks of the bundle", func() {
bundle := trustedBundle.ConfigMap("").Data[certificatemanagement.TrustedCertConfigMapKeyName]
numBlocks := strings.Count(bundle, "certificate name:")
// While we have the ca + 4 certs, we expect 3 cert blocks (+ 2):
// - the certificateManager: this covers for all certs signed by the tigera root ca
// - the byo block (+ its ca block)
// - the legacy cert that already has ExtKeyUsageClient configured.
Expect(numBlocks).To(Equal(3))
Expect(trustedBundle.HashAnnotations()).To(HaveKey("tigera-operator.hash.operator.tigera.io/tigera-ca-private"))
Expect(trustedBundle.HashAnnotations()).To(HaveKey("tigera-operator.hash.operator.tigera.io/byo-secret-copy"))
Expect(trustedBundle.HashAnnotations()).To(HaveKey("tigera-operator.hash.operator.tigera.io/byo-secret"))
})
})
})
})

Expand Down
3 changes: 2 additions & 1 deletion pkg/render/apiserver_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ import (
"github.com/openshift/library-go/pkg/crypto"

calicov3 "github.com/tigera/api/pkg/apis/projectcalico/v3"

operatorv1 "github.com/tigera/operator/api/v1"
"github.com/tigera/operator/pkg/apis"
"github.com/tigera/operator/pkg/common"
Expand Down Expand Up @@ -93,7 +94,7 @@ var _ = Describe("API server rendering tests (Calico Enterprise)", func() {
kp, err := certificateManager.GetOrCreateKeyPair(cli, render.ProjectCalicoAPIServerTLSSecretName(instance.Variant), common.OperatorNamespace(), dnsNames)
Expect(err).NotTo(HaveOccurred())

trustedBundle = certificatemanagement.CreateTrustedBundle()
trustedBundle = certificatemanagement.CreateTrustedBundle(nil)
replicas = 2

cfg = &render.APIServerConfiguration{
Expand Down
59 changes: 29 additions & 30 deletions pkg/tls/certificatemanagement/certificatebundle.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,15 +41,19 @@ type trustedBundle struct {
name string
// systemCertificates is a bundle of certificates loaded from the host systems root location.
systemCertificates []byte

// This is the CA that signs new certificates that are being created.
ca CertificateInterface

// certificates is a map of key: hash, value: certificate.
certificates map[string]CertificateInterface
certificates []CertificateInterface
}

// CreateTrustedBundle creates a TrustedBundle, which provides standardized methods for mounting a bundle of certificates to trust.
// It will include:
// - A bundle with Calico's root certificates + any user supplied certificates in /etc/pki/tls/certs/tigera-ca-bundle.crt.
func CreateTrustedBundle(certificates ...CertificateInterface) TrustedBundle {
bundle, err := createTrustedBundle(false, TrustedCertConfigMapName, certificates...)
func CreateTrustedBundle(ca CertificateInterface, certificates ...CertificateInterface) TrustedBundle {
bundle, err := createTrustedBundle(false, TrustedCertConfigMapName, ca, certificates...)
if err != nil {
panic(err) // This should never happen.
}
Expand All @@ -60,18 +64,18 @@ func CreateTrustedBundle(certificates ...CertificateInterface) TrustedBundle {
// It will include:
// - A bundle with Calico's root certificates + any user supplied certificates in /etc/pki/tls/certs/tigera-ca-bundle.crt.
// - A system root certificate bundle in /etc/pki/tls/certs/ca-bundle.crt.
func CreateTrustedBundleWithSystemRootCertificates(certificates ...CertificateInterface) (TrustedBundle, error) {
return createTrustedBundle(true, TrustedCertConfigMapName, certificates...)
func CreateTrustedBundleWithSystemRootCertificates(ca CertificateInterface, certificates ...CertificateInterface) (TrustedBundle, error) {
return createTrustedBundle(true, TrustedCertConfigMapName, ca, certificates...)
}

// CreateMultiTenantTrustedBundleWithSystemRootCertificates creates a TrustedBundle with system root certificates that is
// appropraite for a multi-tenant cluster, in which each tenant needs multiple trusted bundles.
func CreateMultiTenantTrustedBundleWithSystemRootCertificates(certificates ...CertificateInterface) (TrustedBundle, error) {
return createTrustedBundle(true, TrustedCertConfigMapNamePublic, certificates...)
func CreateMultiTenantTrustedBundleWithSystemRootCertificates(ca CertificateInterface, certificates ...CertificateInterface) (TrustedBundle, error) {
return createTrustedBundle(true, TrustedCertConfigMapNamePublic, ca, certificates...)
}

// createTrustedBundle creates a TrustedBundle, which provides standardized methods for mounting a bundle of certificates to trust.
func createTrustedBundle(includeSystemBundle bool, name string, certificates ...CertificateInterface) (TrustedBundle, error) {
func createTrustedBundle(includeSystemBundle bool, name string, ca CertificateInterface, certificates ...CertificateInterface) (TrustedBundle, error) {
var systemCertificates []byte
var err error
if includeSystemBundle {
Expand All @@ -83,8 +87,12 @@ func createTrustedBundle(includeSystemBundle bool, name string, certificates ...

bundle := &trustedBundle{
name: name,
ca: ca,
systemCertificates: systemCertificates,
certificates: make(map[string]CertificateInterface),
certificates: []CertificateInterface{},
}
if ca != nil {
bundle.certificates = append(bundle.certificates, ca)
}
bundle.AddCertificates(certificates...)

Expand All @@ -94,21 +102,13 @@ func createTrustedBundle(includeSystemBundle bool, name string, certificates ...
// AddCertificates Adds the certificates to the bundle.
func (t *trustedBundle) AddCertificates(certificates ...CertificateInterface) {
for _, cert := range certificates {
// Check if we already trust an issuer of this cert. In practice, this will be 0 or 1 iteration,
// because the issuer is only set when the tigera-ca-private is the issuer.
cur := cert
var skip bool
for cur != nil && !skip {
hash := rmeta.AnnotationHash(cur.GetCertificatePEM())
cur = cur.GetIssuer()
if _, found := t.certificates[hash]; found {
skip = true
if cert != nil {
// Only if a certificate was not signed by our operator, we should add it to the bundle.
if cert.GetIssuer() != nil && t.ca != nil &&
string(cert.GetIssuer().GetCertificatePEM()) == string(t.ca.GetCertificatePEM()) {
continue
}
}
if cert != nil && !skip {
// Add the leaf certificate
hash := rmeta.AnnotationHash(cert.GetCertificatePEM())
t.certificates[hash] = cert
t.certificates = append(t.certificates, cert)
}
}
}
Expand All @@ -119,8 +119,8 @@ func (t *trustedBundle) MountPath() string {

func (t *trustedBundle) HashAnnotations() map[string]string {
annotations := make(map[string]string)
for hash, cert := range t.certificates {
annotations[fmt.Sprintf("%s.hash.operator.tigera.io/%s", cert.GetNamespace(), cert.GetName())] = hash
for _, cert := range t.certificates {
annotations[fmt.Sprintf("%s.hash.operator.tigera.io/%s", cert.GetNamespace(), cert.GetName())] = rmeta.AnnotationHash(cert.GetCertificatePEM())
}
if len(t.systemCertificates) > 0 {
annotations["hash.operator.tigera.io/system"] = rmeta.AnnotationHash(t.systemCertificates)
Expand Down Expand Up @@ -174,10 +174,9 @@ func (t *trustedBundle) ConfigMap(namespace string) *corev1.ConfigMap {

// Sort the certificates so that we get a consistent ordering.
// This reduces the number of changes we see in the configmap.
certs := []CertificateInterface{}
for _, cert := range t.certificates {
certs = append(certs, cert)
}
var certs []CertificateInterface
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed because it failed static checks.

certs = append(certs, t.certificates...)

sort.Slice(certs, func(i, j int) bool {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this sorting still needed ? If so, why can't we move this sorting after adding all certificates so that it is available for everybody ?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point, I think that there should not be a need for it, as the order in which things are added to the bundle (which is now backed by a slice, rather than a map) is deterministic.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We decided to keep the sorting regardless. (See last commit message).

return certs[i].GetName() < certs[j].GetName()
})
Expand Down Expand Up @@ -252,7 +251,7 @@ func getSystemCertificates() ([]byte, error) {
if err == nil {
return data, nil
}
if err != nil && !os.IsNotExist(err) {
if !os.IsNotExist(err) {
Copy link
Member Author

@rene-dekker rene-dekker Oct 18, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The condition err != nil is always true here, so I removed it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are we not checking if err != nil before err == nil ? It is easier to read that this. If we have an error and it is not because the file is missing we return the error.

Copy link
Member Author

@rene-dekker rene-dekker Oct 22, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added, but doubtful how much of an improvement it really is.

return nil, fmt.Errorf(fmt.Sprintf("error occurred when loading system root certificate with name %s", filename), err)
}
}
Expand Down
Loading