From 34e4214ac238c6570cbc97f5cc48cb0beeda189b Mon Sep 17 00:00:00 2001 From: James Munnelly Date: Thu, 4 Jul 2019 17:16:23 +0100 Subject: [PATCH] Set max commonName length to 64 characters Signed-off-by: James Munnelly --- deploy/manifests/00-crds.yaml | 10 +++---- .../output/reference/api-docs/index.html | 4 +-- .../certmanager/v1alpha1/types_certificate.go | 9 +++--- .../certmanager/validation/certificate.go | 12 ++++---- .../validation/certificate_test.go | 30 ++++++++++++------- 5 files changed, 37 insertions(+), 28 deletions(-) diff --git a/deploy/manifests/00-crds.yaml b/deploy/manifests/00-crds.yaml index f82783a9a..4da49d4dc 100644 --- a/deploy/manifests/00-crds.yaml +++ b/deploy/manifests/00-crds.yaml @@ -78,16 +78,16 @@ spec: commonName: description: CommonName is a common name to be used on the Certificate. If no CommonName is given, then the first entry in DNSNames is used - as the CommonName. The CommonName should have a length shorter than - 64 bytes to avoid generating invalid CSRs; in order to have longer + as the CommonName. The CommonName should have a length of 64 characters + or fewer to avoid generating invalid CSRs; in order to have longer domain names, set the CommonName (or first DNSNames entry) to have - less than 64 bytes, and then add the longer domain name to DNSNames. + 64 characters or fewer, and then add the longer domain name to DNSNames. type: string dnsNames: description: DNSNames is a list of subject alt names to be used on the Certificate. If no CommonName is given, then the first entry in DNSNames - is used as the CommonName - any requirements for the CommonName would - then also apply to this first entry. + is used as the CommonName and must have a length of 64 characters + or fewer. items: type: string type: array diff --git a/docs/generated/reference/output/reference/api-docs/index.html b/docs/generated/reference/output/reference/api-docs/index.html index 793d19246..2d5a9d153 100755 --- a/docs/generated/reference/output/reference/api-docs/index.html +++ b/docs/generated/reference/output/reference/api-docs/index.html @@ -85,11 +85,11 @@ Appears In: commonName
string -CommonName is a common name to be used on the Certificate. If no CommonName is given, then the first entry in DNSNames is used as the CommonName. The CommonName should have a length shorter than 64 bytes to avoid generating invalid CSRs; in order to have longer domain names, set the CommonName (or first DNSNames entry) to have less than 64 bytes, and then add the longer domain name to DNSNames. +CommonName is a common name to be used on the Certificate. If no CommonName is given, then the first entry in DNSNames is used as the CommonName. The CommonName should have a length of 64 characters or fewer to avoid generating invalid CSRs; in order to have longer domain names, set the CommonName (or first DNSNames entry) to have 64 characters or fewer, and then add the longer domain name to DNSNames. dnsNames
string array -DNSNames is a list of subject alt names to be used on the Certificate. If no CommonName is given, then the first entry in DNSNames is used as the CommonName - any requirements for the CommonName would then also apply to this first entry. +DNSNames is a list of subject alt names to be used on the Certificate. If no CommonName is given, then the first entry in DNSNames is used as the CommonName and must have a length of 64 characters or fewer. duration
*Duration* diff --git a/pkg/apis/certmanager/v1alpha1/types_certificate.go b/pkg/apis/certmanager/v1alpha1/types_certificate.go index 4aefa81ac..e9c9a8ad6 100644 --- a/pkg/apis/certmanager/v1alpha1/types_certificate.go +++ b/pkg/apis/certmanager/v1alpha1/types_certificate.go @@ -66,10 +66,10 @@ type CertificateSpec struct { // CommonName is a common name to be used on the Certificate. // If no CommonName is given, then the first entry in DNSNames is used as // the CommonName. - // The CommonName should have a length shorter than 64 bytes to avoid + // The CommonName should have a length of 64 characters or fewer to avoid // generating invalid CSRs; in order to have longer domain names, set the - // CommonName (or first DNSNames entry) to have less than 64 bytes, and - // then add the longer domain name to DNSNames. + // CommonName (or first DNSNames entry) to have 64 characters or fewer, + // and then add the longer domain name to DNSNames. // +optional CommonName string `json:"commonName,omitempty"` @@ -87,8 +87,7 @@ type CertificateSpec struct { // DNSNames is a list of subject alt names to be used on the Certificate. // If no CommonName is given, then the first entry in DNSNames is used as - // the CommonName - any requirements for the CommonName would then also - // apply to this first entry. + // the CommonName and must have a length of 64 characters or fewer. // +optional DNSNames []string `json:"dnsNames,omitempty"` diff --git a/pkg/apis/certmanager/validation/certificate.go b/pkg/apis/certmanager/validation/certificate.go index 5a0b22b2c..af332580f 100644 --- a/pkg/apis/certmanager/validation/certificate.go +++ b/pkg/apis/certmanager/validation/certificate.go @@ -50,14 +50,14 @@ func ValidateCertificateSpec(crt *v1alpha1.CertificateSpec, fldPath *field.Path) if len(crt.CommonName) == 0 && len(crt.DNSNames) == 0 { el = append(el, field.Required(fldPath.Child("dnsNames"), "at least one dnsName is required if commonName is not set")) } - // if a common name has been specified, ensure it is no longer than 63 chars - if len(crt.CommonName) > 63 { - el = append(el, field.TooLong(fldPath.Child("commonName"), crt.CommonName, 63)) + // if a common name has been specified, ensure it is no longer than 64 chars + if len(crt.CommonName) > 64 { + el = append(el, field.TooLong(fldPath.Child("commonName"), crt.CommonName, 64)) } - // if the common name has *not* been specified, ensure the first dnsName is no longer than 63 chars + // if the common name has *not* been specified, ensure the first dnsName is no longer than 64 chars // as it will be used as the commonName - if crt.CommonName == "" && len(crt.DNSNames) > 0 && len(crt.DNSNames[0]) > 63 { - el = append(el, field.TooLong(fldPath.Child("dnsNames").Index(0), crt.DNSNames[0], 63)) + if crt.CommonName == "" && len(crt.DNSNames) > 0 && len(crt.DNSNames[0]) > 64 { + el = append(el, field.TooLong(fldPath.Child("dnsNames").Index(0), crt.DNSNames[0], 64)) } if len(crt.IPAddresses) > 0 { diff --git a/pkg/apis/certmanager/validation/certificate_test.go b/pkg/apis/certmanager/validation/certificate_test.go index 09350c663..3460b2ce2 100644 --- a/pkg/apis/certmanager/validation/certificate_test.go +++ b/pkg/apis/certmanager/validation/certificate_test.go @@ -395,53 +395,63 @@ func TestValidateCertificate(t *testing.T) { field.Invalid(fldPath.Child("ipAddresses").Index(0), "blah", "invalid IP address"), }, }, - "invalid certificate with commonName longer than 63 bytes": { + "valid certificate with commonName exactly 64 bytes": { cfg: &v1alpha1.Certificate{ Spec: v1alpha1.CertificateSpec{ - CommonName: "this-is-a-certificate-common-name-which-is-longer-than-sixty-three-bytes", + CommonName: "this-is-a-big-long-string-which-is-exactly-sixty-four-characters", + SecretName: "abc", + IssuerRef: validIssuerRef, + }, + }, + errs: []*field.Error{}, + }, + "invalid certificate with commonName longer than 64 bytes": { + cfg: &v1alpha1.Certificate{ + Spec: v1alpha1.CertificateSpec{ + CommonName: "this-is-a-big-long-string-which-has-exactly-sixty-five-characters", SecretName: "abc", IssuerRef: validIssuerRef, }, }, errs: []*field.Error{ - field.TooLong(fldPath.Child("commonName"), "this-is-a-certificate-common-name-which-is-longer-than-sixty-three-bytes", 63), + field.TooLong(fldPath.Child("commonName"), "this-is-a-big-long-string-which-has-exactly-sixty-five-characters", 64), }, }, - "invalid certificate with no commonName and first dnsName longer than 63 bytes": { + "invalid certificate with no commonName and first dnsName longer than 64 bytes": { cfg: &v1alpha1.Certificate{ Spec: v1alpha1.CertificateSpec{ SecretName: "abc", IssuerRef: validIssuerRef, DNSNames: []string{ - "this-is-a-certificate-dns-name-which-is-longer-than-sixty-three-bytes", + "this-is-a-big-long-string-which-has-exactly-sixty-five-characters", "dnsName", }, }, }, errs: []*field.Error{ - field.TooLong(fldPath.Child("dnsNames").Index(0), "this-is-a-certificate-dns-name-which-is-longer-than-sixty-three-bytes", 63), + field.TooLong(fldPath.Child("dnsNames").Index(0), "this-is-a-big-long-string-which-has-exactly-sixty-five-characters", 64), }, }, - "valid certificate with no commonName and second dnsName longer than 63 bytes": { + "valid certificate with no commonName and second dnsName longer than 64 bytes": { cfg: &v1alpha1.Certificate{ Spec: v1alpha1.CertificateSpec{ SecretName: "abc", IssuerRef: validIssuerRef, DNSNames: []string{ "dnsName", - "this-is-a-certificate-dns-name-which-is-longer-than-sixty-three-bytes", + "this-is-a-big-long-string-which-has-exactly-sixty-five-characters", }, }, }, }, - "valid certificate with commonName and first dnsName longer than 63 bytes": { + "valid certificate with commonName and first dnsName longer than 64 bytes": { cfg: &v1alpha1.Certificate{ Spec: v1alpha1.CertificateSpec{ CommonName: "testcn", SecretName: "abc", IssuerRef: validIssuerRef, DNSNames: []string{ - "this-is-a-certificate-dns-name-which-is-longer-than-sixty-three-bytes", + "this-is-a-big-long-string-which-has-exactly-sixty-five-characters", "dnsName", }, },