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

BUGFIX: issuer-ref and certificate-name annotations are incorrectly updated on Secrets #6147

Merged
merged 4 commits into from Jun 14, 2023
Merged

BUGFIX: issuer-ref and certificate-name annotations are incorrectly updated on Secrets #6147

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
8ddf016
fix a bug that caused the issuer-ref and certificate-name annotations…
inteon Jun 13, 2023
3aa7b82
Update internal/controller/certificates/policies/checks.go
inteon Jun 14, 2023
9c9e833
add TODO comment that explains that we don't understand the reason fo…
inteon Jun 14, 2023
fe4f4e4
re-add TODO comment and make the message more clear
inteon Jun 14, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
9 changes: 8 additions & 1 deletion internal/controller/certificates/policies/checks.go
View file
Open in desktop
Expand Up @@ -347,11 +347,18 @@ func SecretTemplateMismatchesSecretManagedFields(fieldManager string) Func {
}
}

baseAnnotations, err := internalcertificates.AnnotationsForCertificateSecret(input.Certificate, x509cert)
baseAnnotations, err := internalcertificates.AnnotationsForCertificate(x509cert)
if err != nil {
return InvalidCertificate, fmt.Sprintf("Failed getting secret annotations: %v", err), true
}

// We don't use the values of these annotations, but we need to make sure
// that the keys are present in the map so that we can compare the sets.
baseAnnotations[cmapi.CertificateNameKey] = "<certificate-nalue>"
inteon marked this conversation as resolved.
Show resolved Hide resolved
baseAnnotations[cmapi.IssuerNameAnnotationKey] = "<issuer-name>"
baseAnnotations[cmapi.IssuerKindAnnotationKey] = "<issuer-kind>"
baseAnnotations[cmapi.IssuerGroupAnnotationKey] = "<issuer-group>"

managedLabels, managedAnnotations := sets.NewString(), sets.NewString()

for _, managedField := range input.Secret.ManagedFields {
Expand Down
93 changes: 40 additions & 53 deletions internal/controller/certificates/secrets.go
View file
Open in desktop
Expand Up @@ -20,76 +20,63 @@ import (
"bytes"
"crypto/x509"
"encoding/pem"
"strings"

apiutil "github.com/cert-manager/cert-manager/pkg/api/util"
cmapi "github.com/cert-manager/cert-manager/pkg/apis/certmanager/v1"
cmutil "github.com/cert-manager/cert-manager/pkg/util"
utilpki "github.com/cert-manager/cert-manager/pkg/util/pki"
)

// AnnotationsForCertificateSecret returns a map which is set on all
// AnnotationsForCertificate returns a map which is set on all
// Certificate Secret's Annotations when issued. These annotations contain
// information about the Issuer and Certificate.
// If the X.509 certificate is not-nil, additional annotations will be added
// relating to its Common Name and Subject Alternative Names.
func AnnotationsForCertificateSecret(crt *cmapi.Certificate, certificate *x509.Certificate) (map[string]string, error) {
// information about the Certificate.
// If the X.509 certificate is nil, an empty map will be returned.
func AnnotationsForCertificate(certificate *x509.Certificate) (map[string]string, error) {
annotations := make(map[string]string)

// Only add certificate data if certificate is non-nil.
if certificate != nil {
var err error

var errList []error
annotations[cmapi.SubjectOrganizationsAnnotationKey], err = cmutil.JoinWithEscapeCSV(certificate.Subject.Organization)
errList = append(errList, err)

annotations[cmapi.SubjectOrganizationalUnitsAnnotationKey], err = cmutil.JoinWithEscapeCSV(certificate.Subject.OrganizationalUnit)
errList = append(errList, err)

annotations[cmapi.SubjectCountriesAnnotationKey], err = cmutil.JoinWithEscapeCSV(certificate.Subject.Country)
errList = append(errList, err)

annotations[cmapi.SubjectProvincesAnnotationKey], err = cmutil.JoinWithEscapeCSV(certificate.Subject.Province)
errList = append(errList, err)

annotations[cmapi.SubjectLocalitiesAnnotationKey], err = cmutil.JoinWithEscapeCSV(certificate.Subject.Locality)
errList = append(errList, err)
if certificate == nil {
return annotations, nil
}

annotations[cmapi.SubjectPostalCodesAnnotationKey], err = cmutil.JoinWithEscapeCSV(certificate.Subject.PostalCode)
errList = append(errList, err)
var encodingErr error
addStringAnnotation := func(keepEmpty bool, key string, value string) {
if len(value) == 0 && !keepEmpty {
return
}
annotations[key] = value
}
addCSVEncodedAnnotation := func(keepEmpty bool, key string, values []string) {
if len(values) == 0 && !keepEmpty {
return
}

annotations[cmapi.SubjectStreetAddressesAnnotationKey], err = cmutil.JoinWithEscapeCSV(certificate.Subject.StreetAddress)
errList = append(errList, err)
csvString, err := cmutil.JoinWithEscapeCSV(values)
if err != nil {
encodingErr = err
return
}
annotations[key] = csvString
}

annotations[cmapi.SubjectSerialNumberAnnotationKey] = certificate.Subject.SerialNumber
annotations[cmapi.EmailsAnnotationKey] = strings.Join(certificate.EmailAddresses, ",")
addStringAnnotation(true, cmapi.CommonNameAnnotationKey, certificate.Subject.CommonName)
inteon marked this conversation as resolved.
Show resolved Hide resolved
addStringAnnotation(false, cmapi.SubjectSerialNumberAnnotationKey, certificate.Subject.SerialNumber)

// return first error
for _, v := range errList {
if v != nil {
return nil, err
}
}
inteon marked this conversation as resolved.
Show resolved Hide resolved
addCSVEncodedAnnotation(false, cmapi.SubjectOrganizationsAnnotationKey, certificate.Subject.Organization)
addCSVEncodedAnnotation(false, cmapi.SubjectOrganizationalUnitsAnnotationKey, certificate.Subject.OrganizationalUnit)
addCSVEncodedAnnotation(false, cmapi.SubjectCountriesAnnotationKey, certificate.Subject.Country)
addCSVEncodedAnnotation(false, cmapi.SubjectProvincesAnnotationKey, certificate.Subject.Province)
addCSVEncodedAnnotation(false, cmapi.SubjectLocalitiesAnnotationKey, certificate.Subject.Locality)
addCSVEncodedAnnotation(false, cmapi.SubjectPostalCodesAnnotationKey, certificate.Subject.PostalCode)
addCSVEncodedAnnotation(false, cmapi.SubjectStreetAddressesAnnotationKey, certificate.Subject.StreetAddress)

// remove empty subject annotations
for k, v := range annotations {
if v == "" {
delete(annotations, k)
}
}
addCSVEncodedAnnotation(false, cmapi.EmailsAnnotationKey, certificate.EmailAddresses)
addCSVEncodedAnnotation(true, cmapi.AltNamesAnnotationKey, certificate.DNSNames)
addCSVEncodedAnnotation(true, cmapi.IPSANAnnotationKey, utilpki.IPAddressesToString(certificate.IPAddresses))
addCSVEncodedAnnotation(true, cmapi.URISANAnnotationKey, utilpki.URLsToString(certificate.URIs))

annotations[cmapi.CommonNameAnnotationKey] = certificate.Subject.CommonName
annotations[cmapi.AltNamesAnnotationKey] = strings.Join(certificate.DNSNames, ",")
annotations[cmapi.IPSANAnnotationKey] = strings.Join(utilpki.IPAddressesToString(certificate.IPAddresses), ",")
annotations[cmapi.URISANAnnotationKey] = strings.Join(utilpki.URLsToString(certificate.URIs), ",")
if encodingErr != nil {
return nil, encodingErr
}

annotations[cmapi.CertificateNameKey] = crt.Name
annotations[cmapi.IssuerNameAnnotationKey] = crt.Spec.IssuerRef.Name
annotations[cmapi.IssuerKindAnnotationKey] = apiutil.IssuerKind(crt.Spec.IssuerRef)
annotations[cmapi.IssuerGroupAnnotationKey] = crt.Spec.IssuerRef.Group
Copy link
Member Author

@inteon inteon Jun 13, 2023 •
edited

Choose a reason for hiding this comment

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

These 4 fields are now set by the setValues function.


return annotations, nil
}

Expand Down
86 changes: 19 additions & 67 deletions internal/controller/certificates/secrets_test.go
View file
Open in desktop
Expand Up @@ -24,10 +24,6 @@ import (
"testing"

"github.com/stretchr/testify/assert"

cmapi "github.com/cert-manager/cert-manager/pkg/apis/certmanager/v1"
cmmeta "github.com/cert-manager/cert-manager/pkg/apis/meta/v1"
"github.com/cert-manager/cert-manager/test/unit/gen"
)

func Test_AnnotationsForCertificateSecret(t *testing.T) {
Expand All @@ -39,14 +35,10 @@ func Test_AnnotationsForCertificateSecret(t *testing.T) {
}

tests := map[string]struct {
crt *cmapi.Certificate
certificate *x509.Certificate
expAnnotations map[string]string
}{
"if pass non-nil certificate, expect all Annotations to be present": {
crt: gen.Certificate("test-certificate",
gen.SetCertificateIssuer(cmmeta.ObjectReference{Name: "another-test-issuer", Kind: "GoogleCASIssuer", Group: "my-group.hello.world"}),
),
certificate: &x509.Certificate{
Version: 3,
Subject: pkix.Name{
Expand All @@ -66,10 +58,6 @@ func Test_AnnotationsForCertificateSecret(t *testing.T) {
EmailAddresses: []string{"test1@example.com", "test2@cert-manager.io"},
},
expAnnotations: map[string]string{
"cert-manager.io/certificate-name": "test-certificate",
"cert-manager.io/issuer-name": "another-test-issuer",
"cert-manager.io/issuer-kind": "GoogleCASIssuer",
"cert-manager.io/issuer-group": "my-group.hello.world",
"cert-manager.io/common-name": "cert-manager",
"cert-manager.io/alt-names": "example.com,cert-manager.io",
"cert-manager.io/ip-sans": "1.1.1.1,1.2.3.4",
Expand All @@ -86,100 +74,64 @@ func Test_AnnotationsForCertificateSecret(t *testing.T) {
},
},
"if pass non-nil certificate with only CommonName, expect all Annotations to be present": {
crt: gen.Certificate("test-certificate",
gen.SetCertificateIssuer(cmmeta.ObjectReference{Name: "another-test-issuer", Kind: "GoogleCASIssuer", Group: "my-group.hello.world"}),
),
certificate: &x509.Certificate{
Version: 3,
Subject: pkix.Name{
CommonName: "cert-manager",
},
},
expAnnotations: map[string]string{
"cert-manager.io/certificate-name": "test-certificate",
"cert-manager.io/issuer-name": "another-test-issuer",
"cert-manager.io/issuer-kind": "GoogleCASIssuer",
"cert-manager.io/issuer-group": "my-group.hello.world",
"cert-manager.io/common-name": "cert-manager",
"cert-manager.io/alt-names": "",
"cert-manager.io/ip-sans": "",
"cert-manager.io/uri-sans": "",
"cert-manager.io/common-name": "cert-manager",
"cert-manager.io/alt-names": "",
"cert-manager.io/ip-sans": "",
"cert-manager.io/uri-sans": "",
},
},
"if pass non-nil certificate with only IP Addresses, expect all Annotations to be present": {
crt: gen.Certificate("test-certificate",
gen.SetCertificateIssuer(cmmeta.ObjectReference{Name: "another-test-issuer", Kind: "GoogleCASIssuer", Group: "my-group.hello.world"}),
),
certificate: &x509.Certificate{
Version: 3,
IPAddresses: []net.IP{{1, 1, 1, 1}, {1, 2, 3, 4}},
},
expAnnotations: map[string]string{
"cert-manager.io/certificate-name": "test-certificate",
"cert-manager.io/issuer-name": "another-test-issuer",
"cert-manager.io/issuer-kind": "GoogleCASIssuer",
"cert-manager.io/issuer-group": "my-group.hello.world",
"cert-manager.io/common-name": "",
"cert-manager.io/alt-names": "",
"cert-manager.io/ip-sans": "1.1.1.1,1.2.3.4",
"cert-manager.io/uri-sans": "",
"cert-manager.io/common-name": "",
"cert-manager.io/alt-names": "",
"cert-manager.io/ip-sans": "1.1.1.1,1.2.3.4",
"cert-manager.io/uri-sans": "",
},
},
"if pass non-nil certificate with only URI SANs, expect all Annotations to be present": {
crt: gen.Certificate("test-certificate",
gen.SetCertificateIssuer(cmmeta.ObjectReference{Name: "another-test-issuer", Kind: "GoogleCASIssuer", Group: "my-group.hello.world"}),
),
certificate: &x509.Certificate{
Version: 3,
URIs: urls,
},
expAnnotations: map[string]string{
"cert-manager.io/certificate-name": "test-certificate",
"cert-manager.io/issuer-name": "another-test-issuer",
"cert-manager.io/issuer-kind": "GoogleCASIssuer",
"cert-manager.io/issuer-group": "my-group.hello.world",
"cert-manager.io/common-name": "",
"cert-manager.io/alt-names": "",
"cert-manager.io/ip-sans": "",
"cert-manager.io/uri-sans": "spiffe.io//cert-manager.io/test,spiffe.io//hello.world",
"cert-manager.io/common-name": "",
"cert-manager.io/alt-names": "",
"cert-manager.io/ip-sans": "",
"cert-manager.io/uri-sans": "spiffe.io//cert-manager.io/test,spiffe.io//hello.world",
},
},
"if pass non-nil certificate with only DNS names, expect all Annotations to be present": {
crt: gen.Certificate("test-certificate",
gen.SetCertificateIssuer(cmmeta.ObjectReference{Name: "another-test-issuer", Kind: "GoogleCASIssuer", Group: "my-group.hello.world"}),
),
certificate: &x509.Certificate{
Version: 3,
DNSNames: []string{"example.com", "cert-manager.io"},
},
expAnnotations: map[string]string{
"cert-manager.io/certificate-name": "test-certificate",
"cert-manager.io/issuer-name": "another-test-issuer",
"cert-manager.io/issuer-kind": "GoogleCASIssuer",
"cert-manager.io/issuer-group": "my-group.hello.world",
"cert-manager.io/common-name": "",
"cert-manager.io/alt-names": "example.com,cert-manager.io",
"cert-manager.io/ip-sans": "",
"cert-manager.io/uri-sans": "",
"cert-manager.io/common-name": "",
"cert-manager.io/alt-names": "example.com,cert-manager.io",
"cert-manager.io/ip-sans": "",
"cert-manager.io/uri-sans": "",
},
},
"if no certificate data, then expect no X.509 related annotations": {
crt: gen.Certificate("test-certificate",
gen.SetCertificateIssuer(cmmeta.ObjectReference{Name: "test-issuer", Kind: "", Group: "cert-manager.io"}),
),
certificate: nil,
expAnnotations: map[string]string{
"cert-manager.io/certificate-name": "test-certificate",
"cert-manager.io/issuer-name": "test-issuer",
"cert-manager.io/issuer-kind": "Issuer",
"cert-manager.io/issuer-group": "cert-manager.io",
},
certificate: nil,
expAnnotations: map[string]string{},
},
}

for name, test := range tests {
t.Run(name, func(t *testing.T) {
gotAnnotations, err := AnnotationsForCertificateSecret(test.crt, test.certificate)
gotAnnotations, err := AnnotationsForCertificate(test.certificate)
assert.Equal(t, test.expAnnotations, gotAnnotations)
assert.Equal(t, nil, err)
})
Expand Down
44 changes: 28 additions & 16 deletions pkg/controller/certificates/issuing/internal/secret.go
View file
Open in desktop
Expand Up @@ -59,7 +59,9 @@ type SecretsManager struct {

// SecretData is a structure wrapping private key, Certificate and CA data
type SecretData struct {
PrivateKey, Certificate, CA []byte
PrivateKey, Certificate, CA []byte
CertificateName string
IssuerName, IssuerKind, IssuerGroup string
Copy link
Member Author

@inteon inteon Jun 13, 2023 •
edited

Choose a reason for hiding this comment

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

We now explicitly pass the CertificateName, IssuerName, IssuerKind and IssuerGroup values to setValues instead of reading these values from the cmapi.Certificate resource, since cmapi.Certificate might have been updated since the secret was created.

}

// NewSecretsManager returns a new SecretsManager. Setting
Expand Down Expand Up @@ -150,36 +152,46 @@ func (s *SecretsManager) setValues(crt *cmapi.Certificate, secret *corev1.Secret
secret.Data[cmmeta.TLSCAKey] = data.CA
}

if secret.Annotations == nil {
secret.Annotations = make(map[string]string)
}

if secret.Labels == nil {
secret.Labels = make(map[string]string)
}

if crt.Spec.SecretTemplate != nil {
Copy link
Member Author

Choose a reason for hiding this comment

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

I changed the order: first, we apply the SecretTemplate; afterwards, we apply the cert-manager annotations.
This prevents us from overwriting cert-manager annotations using the SecretTemplate.

Copy link
Member

Choose a reason for hiding this comment

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

I'm sure that this was the original behaviour when @jonathansp originally created the feature. See

I wonder when and how this bug got introduced?
Which versions are affected? Do we need to fix this bug in a separate PR and backport to the affected versions?
Is there a unit test for this? If not, please add one.

Copy link
Member Author

@inteon inteon Jun 14, 2023 •
edited

Choose a reason for hiding this comment

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

This issue was introduced in #4746

for k, v := range crt.Spec.SecretTemplate.Labels {
secret.Labels[k] = v
}
for k, v := range crt.Spec.SecretTemplate.Annotations {
secret.Annotations[k] = v
}
}

var certificate *x509.Certificate
if len(data.Certificate) > 0 {
var err error
certificate, err = utilpki.DecodeX509CertificateBytes(data.Certificate)
// TODO: handle InvalidData here?
inteon marked this conversation as resolved.
Show resolved Hide resolved
if err != nil {
return err
}
}

var err error
secret.Annotations, err = certificates.AnnotationsForCertificateSecret(crt, certificate)
certificateDetailsAnnotations, err := certificates.AnnotationsForCertificate(certificate)
if err != nil {
return err
}

if secret.Labels == nil {
secret.Labels = make(map[string]string)
for k, v := range certificateDetailsAnnotations {
secret.Annotations[k] = v
}

secret.Labels[cmapi.PartOfCertManagerControllerLabelKey] = "true"
secret.Annotations[cmapi.CertificateNameKey] = data.CertificateName
secret.Annotations[cmapi.IssuerNameAnnotationKey] = data.IssuerName
secret.Annotations[cmapi.IssuerKindAnnotationKey] = data.IssuerKind
secret.Annotations[cmapi.IssuerGroupAnnotationKey] = data.IssuerGroup
Copy link
Member Author

Choose a reason for hiding this comment

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

Previously, AnnotationsForCertificate was setting these annotations. In this PR, I moved that step out of the AnnotationsForCertificate function, since it now uses values from the data object.


if crt.Spec.SecretTemplate != nil {
for k, v := range crt.Spec.SecretTemplate.Labels {
secret.Labels[k] = v
}
for k, v := range crt.Spec.SecretTemplate.Annotations {
secret.Annotations[k] = v
}
}
secret.Labels[cmapi.PartOfCertManagerControllerLabelKey] = "true"

return nil
}
Expand Down