Merge pull request #6358 from jetstack-bot/cherry-pick-6354-to-releas…

…e-1.13

[release-1.13] 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,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.-<hash>). 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)
 }

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{}{}
+	}
+}

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})