BUGFIX: CertificateRequest short names must be unique.

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,65 @@ 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
+	}
+
+	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 52 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)
 }

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,182 @@ func TestComputeName(t *testing.T) {
 		})
 	}
 }
+
+func TestDNSSafeShortenToNCharacters(t *testing.T) {
+	type testcase struct {
+		in        string
+		maxLength int
+		expOut    string
+	}
+
+	tests := []testcase{
+		{
+			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{}{}
+	}
+}

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: <first-168-chars-of-certificate-name>-<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})

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{