Skip to content

Commit

Permalink
fix feedback: make hash secure
Browse files Browse the repository at this point in the history 
Signed-off-by: Tim Ramlot <42113979+inteon@users.noreply.github.com>
  • Loading branch information
@inteon
inteon committed Sep 21, 2023
1 parent 6006182 commit 0f35a61
Show file tree
Hide file tree
Showing 3 changed files with 187 additions and 48 deletions.
55 changes: 41 additions & 14 deletions pkg/api/util/names.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ limitations under the License.
package util

import (
"crypto/sha256"
"encoding/json"
"fmt"
"hash/fnv"
Expand All @@ -43,39 +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
}

// ComputeUniqueDeterministicNameFromData returns a short unique name for the given object.
func ComputeUniqueDeterministicNameFromData(fullName string) (string, error) {
const maxNameLength = 61 // 52 + 1 + 8
// 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
}

hashF := fnv.New32()
hash := sha256.New()

_, err := hashF.Write([]byte(fullName))
_, err := hash.Write([]byte(fullName))
if err != nil {
return "", err
}

prefix := DNSSafeShortenTo52Characters(fullName)
prefix := DNSSafeShortenToNCharacters(fullName, maxNameLength-hashLength-1)
hashResult := hash.Sum(nil)

if len(prefix) == 0 {
return fmt.Sprintf("%08x", hashF.Sum32()), nil
return fmt.Sprintf("%08x", hashResult), nil
}

return fmt.Sprintf("%s-%08x", prefix, hashF.Sum32()), nil
return fmt.Sprintf("%s-%08x", prefix, hashResult), 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]]
// 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 ""
}

return in
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)
}
163 changes: 134 additions & 29 deletions pkg/api/util/names_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -114,70 +114,175 @@ func TestComputeName(t *testing.T) {
}
}

func TestComputeUniqueDeterministicNameFromData(t *testing.T) {
func TestDNSSafeShortenToNCharacters(t *testing.T) {
type testcase struct {
in string
expOut string
in string
maxLength int
expOut string
}

randomString61 := cmutil.RandStringRunes(61)
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: "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa",
expOut: "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa-014e7a7f",
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: "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa",
expOut: "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa",
in: "bb" + aString64,
maxLength: 65,
expOut: "824cc1084d15d9bff4dda12c92066ff5d15ef2f9847c47347836cee174138ca0",
},
{
in: "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa.",
expOut: "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa.",
in: "bbb" + aString64,
maxLength: 66,
expOut: "b-9a956f515497faf6c2e733e5c2a0e35700ff0b9457e6fd163f30bfe5ec81d13c",
},
{
in: "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa.aaaaaaaaa",
expOut: "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa.aaaaaaaaa",
in: ".bb" + aString64,
maxLength: 66,
expOut: "efd1f8e9b2f02af94b0d00c03eaddbde3a510b626eb92022f1f25bcc74eedb5b",
},
{
in: "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa.aaaaaaaaaa",
expOut: "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa-72314ca8",
in: "b.b" + aString64,
maxLength: 66,
expOut: "b-f0673c1af88891be1ecfe74876e460de28e073a0bb78d3308fb41617db4c2ca5",
},
{
in: randomString61,
expOut: randomString61,
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 := ComputeUniqueDeterministicNameFromData(test.in)
if err != nil {
t.Errorf("unexpected error: %v", err)
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{
"aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa",
"aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa",
"aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa.",
"aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa.aaaaaaaaa",
"aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa.aaaaaaaaaa",
randomString61,
randomString61 + "a",
randomString61 + "b",
randomString61 + "c",
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 := ComputeUniqueDeterministicNameFromData(in)
out, err := ComputeSecureUniqueDeterministicNameFromData(in, 80)
if err != nil {
t.Errorf("unexpected error: %v", err)
}
Expand Down
17 changes: 12 additions & 5 deletions pkg/controller/certificates/requestmanager/requestmanager_controller.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -395,10 +398,14 @@ func (c *controller) createNewCertificateRequest(ctx context.Context, crt *cmapi
if utilfeature.DefaultFeatureGate.Enabled(feature.StableCertificateRequestName) {
cr.ObjectMeta.GenerateName = ""

// limit the name to 54 chars to leave room for 8 characters that represent the revision
// number and a hyphen. This way, we stay within the 63 character limit for pod names (which is not
// a hard limit since we are working with CertificateRequest resources).
crName, err := apiutil.ComputeUniqueDeterministicNameFromData(crt.Name)
// 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
}
Expand Down

0 comments on commit 0f35a61

Please sign in to comment.