diff --git a/pkg/api/util/names.go b/pkg/api/util/names.go index fdeb82758aa..4a335336bc3 100644 --- a/pkg/api/util/names.go +++ b/pkg/api/util/names.go @@ -17,10 +17,10 @@ limitations under the License. package util import ( + "crypto/sha256" "encoding/json" "fmt" "hash/fnv" - "regexp" ) @@ -44,15 +44,70 @@ func ComputeName(prefix string, obj interface{}) (string, error) { // and pods down the road for ACME resources. prefix = DNSSafeShortenTo52Characters(prefix) + // the prefix is <= 52 characters, the decimal representation of + // the hash is <= 10 characters, and the hyphen is 1 character. + // 52 + 10 + 1 = 63, so we're good. return fmt.Sprintf("%s-%d", prefix, hashF.Sum32()), nil } -// DNSSafeShortenTo52Characters shortens the input string to 52 chars and ensures the last char is an alpha-numeric character. -func DNSSafeShortenTo52Characters(in string) string { - if len(in) >= 52 { - validCharIndexes := regexp.MustCompile(`[a-zA-Z\d]`).FindAllStringIndex(fmt.Sprintf("%.52s", in), -1) - in = in[:validCharIndexes[len(validCharIndexes)-1][1]] +// ComputeSecureUniqueDeterministicNameFromData computes a deterministic name from the given data. +// The algorithm in use is SHA256 and is cryptographically secure. +// The output is a string that is safe to use as a DNS label. +// The output is guaranteed to be unique for the given input. +// The output will be at least 64 characters long. +func ComputeSecureUniqueDeterministicNameFromData(fullName string, maxNameLength int) (string, error) { + const hashLength = 64 + if maxNameLength < hashLength { + return "", fmt.Errorf("maxNameLength must be at least %d", hashLength) + } + + if len(fullName) <= maxNameLength { + return fullName, nil + } + + hash := sha256.New() + + _, err := hash.Write([]byte(fullName)) + if err != nil { + return "", err + } + + // Although fullName is already a DNS subdomain, we can't just cut it + // at N characters and expect another DNS subdomain. That's because + // we might cut it right after a ".", which would give an invalid DNS + // subdomain (eg. test.-). So we make sure the last character + // is an alpha-numeric character. + prefix := DNSSafeShortenToNCharacters(fullName, maxNameLength-hashLength-1) + hashResult := hash.Sum(nil) + + if len(prefix) == 0 { + return fmt.Sprintf("%08x", hashResult), nil } - return in + return fmt.Sprintf("%s-%08x", prefix, hashResult), nil +} + +// DNSSafeShortenToNCharacters shortens the input string to N chars and ensures the last char is an alpha-numeric character. +func DNSSafeShortenToNCharacters(in string, maxLength int) string { + var alphaNumeric = regexp.MustCompile(`[a-zA-Z\d]`) + + if len(in) < maxLength { + return in + } + + if maxLength <= 0 { + return "" + } + + validCharIndexes := alphaNumeric.FindAllStringIndex(in[:maxLength], -1) + if len(validCharIndexes) == 0 { + return "" + } + + return in[:validCharIndexes[len(validCharIndexes)-1][1]] +} + +// DNSSafeShortenTo52Characters shortens the input string to 52 chars and ensures the last char is an alpha-numeric character. +func DNSSafeShortenTo52Characters(in string) string { + return DNSSafeShortenToNCharacters(in, 52) } diff --git a/pkg/api/util/names_test.go b/pkg/api/util/names_test.go index 392026253c7..477e6798981 100644 --- a/pkg/api/util/names_test.go +++ b/pkg/api/util/names_test.go @@ -17,9 +17,11 @@ limitations under the License. package util import ( + "fmt" "testing" cmapi "github.com/cert-manager/cert-manager/pkg/apis/certmanager/v1" + cmutil "github.com/cert-manager/cert-manager/pkg/util" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/util/validation" ) @@ -111,3 +113,202 @@ func TestComputeName(t *testing.T) { }) } } + +func TestDNSSafeShortenToNCharacters(t *testing.T) { + type testcase struct { + in string + maxLength int + expOut string + } + + tests := []testcase{ + { + in: "aaaaaaaaaaaaaaa", + maxLength: 0, + expOut: "", + }, + { + in: "aa-----aaaa", + maxLength: 5, + expOut: "aa", + }, + { + in: "aa11111aaaa", + maxLength: 5, + expOut: "aa111", + }, + { + in: "aaAAAAAaaaa", + maxLength: 5, + expOut: "aaAAA", + }, + { + in: "aaaaaaaaaaaaaaa", + maxLength: 3, + expOut: "aaa", + }, + { + in: ".....", + maxLength: 3, + expOut: "", + }, + { + in: "aa.....", + maxLength: 3, + expOut: "aa", + }, + { + in: "aaa.....", + maxLength: 3, + expOut: "aaa", + }, + { + in: "a*aa.....", + maxLength: 3, + expOut: "a*a", + }, + { + in: "a**aa.....", + maxLength: 3, + expOut: "a", + }, + } + + for i, test := range tests { + test := test + t.Run(fmt.Sprintf("test-%d", i), func(t *testing.T) { + out := DNSSafeShortenToNCharacters(test.in, test.maxLength) + if out != test.expOut { + t.Errorf("expected %q, got %q", test.expOut, out) + } + }) + } +} + +func TestComputeSecureUniqueDeterministicNameFromData(t *testing.T) { + type testcase struct { + in string + maxLength int + expOut string + expErr bool + } + + aString64 := "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa" + randomString64 := cmutil.RandStringRunes(64) + + tests := []testcase{ + { + in: "aaaa", + maxLength: 3, // must be at least 64 + expOut: "", + expErr: true, + }, + { + in: aString64, + maxLength: 64, + expOut: aString64, + }, + { + in: aString64[:10], + maxLength: 64, + expOut: aString64[:10], + }, + { + in: "b" + aString64, + maxLength: 64, + expOut: "08ba353c3a64d6186cac33ae87b2bd29700803754b34f77dc4d3a45e66316745", + }, + { + in: "b" + aString64, + maxLength: 65, + expOut: "b" + aString64, + }, + { + in: "bb" + aString64, + maxLength: 65, + expOut: "824cc1084d15d9bff4dda12c92066ff5d15ef2f9847c47347836cee174138ca0", + }, + { + in: "bbb" + aString64, + maxLength: 66, + expOut: "b-9a956f515497faf6c2e733e5c2a0e35700ff0b9457e6fd163f30bfe5ec81d13c", + }, + { + in: ".bb" + aString64, + maxLength: 66, + expOut: "efd1f8e9b2f02af94b0d00c03eaddbde3a510b626eb92022f1f25bcc74eedb5b", + }, + { + in: "b.b" + aString64, + maxLength: 66, + expOut: "b-f0673c1af88891be1ecfe74876e460de28e073a0bb78d3308fb41617db4c2ca5", + }, + { + in: "bbbbbbbbbbbbbc............." + aString64, + maxLength: 79, + expOut: "bbbbbbbbbbbbbc-d1b69a0803d97526b868335f95a8bc6fcf02e8e08644264c470faded0ca42033", + }, + { + in: "bbbbbbbbbbbbbc............." + aString64, + maxLength: 80, + expOut: "bbbbbbbbbbbbbc-d1b69a0803d97526b868335f95a8bc6fcf02e8e08644264c470faded0ca42033", + }, + { + in: "bbbbbbbbbbbbbc............." + aString64, + maxLength: 90, + expOut: "bbbbbbbbbbbbbc-d1b69a0803d97526b868335f95a8bc6fcf02e8e08644264c470faded0ca42033", + }, + { + in: randomString64, + maxLength: 64, + expOut: randomString64, + }, + } + + for i, test := range tests { + test := test + t.Run(fmt.Sprintf("test-%d", i), func(t *testing.T) { + out, err := ComputeSecureUniqueDeterministicNameFromData(test.in, test.maxLength) + if (err != nil) != test.expErr { + t.Errorf("expected err %v, got %v", test.expErr, err) + } + if len(out) > test.maxLength { + t.Errorf("expected output to be at most %d characters, got %d", test.maxLength, len(out)) + } + if out != test.expOut { + t.Errorf("expected %q, got %q", test.expOut, out) + } + }) + } + + aString70 := "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa" + randomString70 := cmutil.RandStringRunes(70) + + // Test that the output is unique for different inputs + inputs := []string{ + aString70, + aString70 + "a", + aString70 + "b", + aString70 + ".", + "." + aString70, + "...................." + aString70, + "...................a" + aString70, + "a..................." + aString70, + randomString70, + randomString70 + "a", + randomString70 + "b", + randomString70 + "c", + } + + outputs := make(map[string]struct{}) + for _, in := range inputs { + out, err := ComputeSecureUniqueDeterministicNameFromData(in, 80) + if err != nil { + t.Errorf("unexpected error: %v", err) + } + if _, ok := outputs[out]; ok { + t.Errorf("output %q already seen", out) + } + outputs[out] = struct{}{} + } +} diff --git a/pkg/controller/certificates/requestmanager/requestmanager_controller.go b/pkg/controller/certificates/requestmanager/requestmanager_controller.go index e864691a9ed..a552f085927 100644 --- a/pkg/controller/certificates/requestmanager/requestmanager_controller.go +++ b/pkg/controller/certificates/requestmanager/requestmanager_controller.go @@ -377,7 +377,10 @@ func (c *controller) createNewCertificateRequest(ctx context.Context, crt *cmapi cr := &cmapi.CertificateRequest{ ObjectMeta: metav1.ObjectMeta{ - Namespace: crt.Namespace, + Namespace: crt.Namespace, + // We limit the GenerateName to 52 + 1 characters to stay within the 63 - 5 character limit that + // is used in Kubernetes when generating names. + // see https://github.com/kubernetes/apiserver/blob/696768606f546f71a1e90546613be37d1aa37f64/pkg/storage/names/generate.go GenerateName: apiutil.DNSSafeShortenTo52Characters(crt.Name) + "-", Annotations: annotations, Labels: crt.Labels, @@ -394,7 +397,20 @@ func (c *controller) createNewCertificateRequest(ctx context.Context, crt *cmapi if utilfeature.DefaultFeatureGate.Enabled(feature.StableCertificateRequestName) { cr.ObjectMeta.GenerateName = "" - cr.ObjectMeta.Name = apiutil.DNSSafeShortenTo52Characters(crt.Name) + "-" + fmt.Sprintf("%d", nextRevision) + + // The CertificateRequest name is limited to 253 characters, assuming the nextRevision and hyphen + // can be represented using 20 characters, we can directly accept certificate names up to 233 + // characters. Certificate names that are longer than this will be hashed to a shorter name. We want + // to make crafting two Certificates with the same truncated name as difficult as possible, so we + // use a cryptographic hash function to hash the full certificate name to 64 characters. + // Finally, for Certificates with a name longer than 233 characters, we build the CertificateRequest + // name as follows: -<64-char-hash>-<19-char-nextRevision> + crName, err := apiutil.ComputeSecureUniqueDeterministicNameFromData(crt.Name, 233) + if err != nil { + return err + } + + cr.ObjectMeta.Name = fmt.Sprintf("%s-%d", crName, nextRevision) } cr, err = c.client.CertmanagerV1().CertificateRequests(cr.Namespace).Create(ctx, cr, metav1.CreateOptions{FieldManager: c.fieldManager}) diff --git a/pkg/controller/certificates/requestmanager/requestmanager_controller_test.go b/pkg/controller/certificates/requestmanager/requestmanager_controller_test.go index f2434f6bbca..8645d48f0c6 100644 --- a/pkg/controller/certificates/requestmanager/requestmanager_controller_test.go +++ b/pkg/controller/certificates/requestmanager/requestmanager_controller_test.go @@ -20,6 +20,7 @@ import ( "context" "fmt" "reflect" + "strings" "testing" "time" @@ -90,6 +91,14 @@ func TestProcessItem(t *testing.T) { }, Spec: cmapi.CertificateSpec{CommonName: "test-bundle-3"}}, ) + bundle4 := mustCreateCryptoBundle(t, &cmapi.Certificate{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: "testns", + Name: strings.Repeat("a", 167) + "b" + strings.Repeat("c", 85), + UID: "test", + }, + Spec: cmapi.CertificateSpec{CommonName: "test-bundle-4"}}, + ) fixedNow := metav1.NewTime(time.Now()) fixedClock := fakeclock.NewFakeClock(fixedNow.Time) failedCRConditionPreviousIssuance := cmapi.CertificateRequestCondition{ @@ -228,6 +237,58 @@ func TestProcessItem(t *testing.T) { )), relaxedCertificateRequestMatcher), }, }, + "create a CertificateRequest if none exists (with long name)": { + secrets: []runtime.Object{ + &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{Namespace: bundle3.certificate.Namespace, Name: "exists"}, + Data: map[string][]byte{corev1.TLSPrivateKeyKey: bundle3.privateKeyBytes}, + }, + }, + certificate: gen.CertificateFrom(bundle4.certificate, + gen.SetCertificateNextPrivateKeySecretName("exists"), + gen.SetCertificateStatusCondition(cmapi.CertificateCondition{Type: cmapi.CertificateConditionIssuing, Status: cmmeta.ConditionTrue}), + gen.SetCertificateRevision(19), + ), + expectedEvents: []string{ + fmt.Sprintf(`Normal Requested Created new CertificateRequest resource "%s"`, strings.Repeat("a", 167)+"b-d3f4fc40a686edfd404adf1d3fb1530653988c878e6c9c07b2e2fa4001a21269-20"), + }, + expectedActions: []testpkg.Action{ + testpkg.NewCustomMatch(coretesting.NewCreateAction(cmapi.SchemeGroupVersion.WithResource("certificaterequests"), "testns", + gen.CertificateRequestFrom(bundle4.certificateRequest, + gen.SetCertificateRequestName(strings.Repeat("a", 167)+"b-d3f4fc40a686edfd404adf1d3fb1530653988c878e6c9c07b2e2fa4001a21269-20"), + gen.SetCertificateRequestAnnotations(map[string]string{ + cmapi.CertificateRequestPrivateKeyAnnotationKey: "exists", + cmapi.CertificateRequestRevisionAnnotationKey: "20", + }), + )), relaxedCertificateRequestMatcher), + }, + }, + "create a CertificateRequest if none exists (with long name and very large revision)": { + secrets: []runtime.Object{ + &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{Namespace: bundle3.certificate.Namespace, Name: "exists"}, + Data: map[string][]byte{corev1.TLSPrivateKeyKey: bundle3.privateKeyBytes}, + }, + }, + certificate: gen.CertificateFrom(bundle4.certificate, + gen.SetCertificateNextPrivateKeySecretName("exists"), + gen.SetCertificateStatusCondition(cmapi.CertificateCondition{Type: cmapi.CertificateConditionIssuing, Status: cmmeta.ConditionTrue}), + gen.SetCertificateRevision(999999999), + ), + expectedEvents: []string{ + fmt.Sprintf(`Normal Requested Created new CertificateRequest resource "%s"`, strings.Repeat("a", 167)+"b-d3f4fc40a686edfd404adf1d3fb1530653988c878e6c9c07b2e2fa4001a21269-1000000000"), + }, + expectedActions: []testpkg.Action{ + testpkg.NewCustomMatch(coretesting.NewCreateAction(cmapi.SchemeGroupVersion.WithResource("certificaterequests"), "testns", + gen.CertificateRequestFrom(bundle4.certificateRequest, + gen.SetCertificateRequestName(strings.Repeat("a", 167)+"b-d3f4fc40a686edfd404adf1d3fb1530653988c878e6c9c07b2e2fa4001a21269-1000000000"), + gen.SetCertificateRequestAnnotations(map[string]string{ + cmapi.CertificateRequestPrivateKeyAnnotationKey: "exists", + cmapi.CertificateRequestRevisionAnnotationKey: "1000000000", + }), + )), relaxedCertificateRequestMatcher), + }, + }, "delete the owned CertificateRequest and create a new one if existing one does not have the annotation": { secrets: []runtime.Object{ &corev1.Secret{