From 1ee7c9aeabe4687de86b5a61e9cb49c30e2a0f0c Mon Sep 17 00:00:00 2001 From: Darren Lau Date: Wed, 15 Jun 2022 20:06:59 +0800 Subject: [PATCH] Fix common name length limitation issue (#822) * Hash and restrict the length of common names when they exceed the length limitation --- pkg/pki/certmanagerpki/certmanager_pki.go | 2 +- pkg/util/cert/certutil.go | 4 +-- pkg/util/pki/common.go | 28 ++++++++++++---- pkg/util/pki/pki_common_test.go | 39 +++++++++++++++++++++++ 4 files changed, 63 insertions(+), 10 deletions(-) diff --git a/pkg/pki/certmanagerpki/certmanager_pki.go b/pkg/pki/certmanagerpki/certmanager_pki.go index 2a05405d5..e8b8fcbe4 100644 --- a/pkg/pki/certmanagerpki/certmanager_pki.go +++ b/pkg/pki/certmanagerpki/certmanager_pki.go @@ -219,7 +219,7 @@ func caCertForCluster(cluster *v1beta1.KafkaCluster) *certv1.Certificate { }, Spec: certv1.CertificateSpec{ SecretName: fmt.Sprintf(pkicommon.BrokerCACertTemplate, cluster.Name), - CommonName: fmt.Sprintf(pkicommon.CAFQDNTemplate, cluster.Name, cluster.Namespace), + CommonName: pkicommon.EnsureValidCommonNameLen(fmt.Sprintf(pkicommon.CAFQDNTemplate, cluster.Name, cluster.Namespace)), IsCA: true, IssuerRef: certmeta.ObjectReference{ Name: fmt.Sprintf(pkicommon.BrokerSelfSignerTemplate, cluster.Name), diff --git a/pkg/util/cert/certutil.go b/pkg/util/cert/certutil.go index 0ec2cbd50..87e61c615 100644 --- a/pkg/util/cert/certutil.go +++ b/pkg/util/cert/certutil.go @@ -178,12 +178,12 @@ func GenerateJKSFromByte(certByte []byte, privateKey []byte, caCert []byte) (out func GenerateJKS(certs []*x509.Certificate, privateKey []byte) (out, passw []byte, err error) { pKeyRaw, err := DecodePrivateKeyBytes(privateKey) if err != nil { - return + return nil, nil, err } pKeyPKCS8, err := x509.MarshalPKCS8PrivateKey(pKeyRaw) if err != nil { - return + return nil, nil, err } certCABundle := make([]keystore.Certificate, 0, len(certs)) diff --git a/pkg/util/pki/common.go b/pkg/util/pki/common.go index 17d120983..2168abcc6 100644 --- a/pkg/util/pki/common.go +++ b/pkg/util/pki/common.go @@ -16,19 +16,20 @@ package pki import ( "context" + "crypto/sha256" "crypto/tls" "fmt" "sort" "strings" - "github.com/banzaicloud/koperator/pkg/errorfactory" - "github.com/banzaicloud/koperator/pkg/k8sutil" - corev1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/runtime" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" + "github.com/banzaicloud/koperator/pkg/errorfactory" + "github.com/banzaicloud/koperator/pkg/k8sutil" + "github.com/banzaicloud/koperator/api/v1alpha1" "github.com/banzaicloud/koperator/api/v1beta1" "github.com/banzaicloud/koperator/pkg/resources/templates" @@ -57,6 +58,8 @@ const ( // KafkaUserAnnotationName used in case of PKIbackend is k8s-csr to find the appropriate kafkauser in case of // signing request event KafkaUserAnnotationName = "banzaicloud.io/owner" + // MaxCNLen specifies the number of chars that the longest common name can have + MaxCNLen = 64 ) // Manager is the main interface for objects performing PKI operations @@ -184,7 +187,7 @@ func BrokerUserForCluster(cluster *v1beta1.KafkaCluster, extListenerStatuses map } additionalHosts = sortAndDedupe(additionalHosts) return &v1alpha1.KafkaUser{ - ObjectMeta: templates.ObjectMeta(GetCommonName(cluster), LabelsForKafkaPKI(cluster.Name, cluster.Namespace), cluster), + ObjectMeta: templates.ObjectMeta(EnsureValidCommonNameLen(GetCommonName(cluster)), LabelsForKafkaPKI(cluster.Name, cluster.Namespace), cluster), Spec: v1alpha1.KafkaUserSpec{ SecretName: fmt.Sprintf(BrokerServerCertTemplate, cluster.Name), DNSNames: append(GetInternalDNSNames(cluster), additionalHosts...), @@ -215,9 +218,9 @@ func sortAndDedupe(hosts []string) []string { func ControllerUserForCluster(cluster *v1beta1.KafkaCluster) *v1alpha1.KafkaUser { return &v1alpha1.KafkaUser{ ObjectMeta: templates.ObjectMeta( - fmt.Sprintf(BrokerControllerFQDNTemplate, - fmt.Sprintf(BrokerControllerTemplate, cluster.Name), cluster.Namespace, cluster.Spec.GetKubernetesClusterDomain()), - LabelsForKafkaPKI(cluster.Name, cluster.Namespace), cluster, + EnsureValidCommonNameLen(fmt.Sprintf(BrokerControllerFQDNTemplate, fmt.Sprintf(BrokerControllerTemplate, cluster.Name), cluster.Namespace, cluster.Spec.GetKubernetesClusterDomain())), + LabelsForKafkaPKI(cluster.Name, cluster.Namespace), + cluster, ), Spec: v1alpha1.KafkaUserSpec{ SecretName: fmt.Sprintf(BrokerControllerTemplate, cluster.Name), @@ -243,3 +246,14 @@ func EnsureControllerReference(ctx context.Context, user *v1alpha1.KafkaUser, } return nil } + +// EnsureValidCommonNameLen ensures that the passed-in common name doesn't exceed the longest supported length +func EnsureValidCommonNameLen(s string) string { + if len(s) > MaxCNLen { + first := sha256.New() + first.Write([]byte(s)) + // encode the hash to make sure it only consists of lower case alphanumeric characters to enforce RFC 1123 + return fmt.Sprintf("%x", first.Sum(nil)) + } + return s +} diff --git a/pkg/util/pki/pki_common_test.go b/pkg/util/pki/pki_common_test.go index fb6a4391b..0bd1b75e2 100644 --- a/pkg/util/pki/pki_common_test.go +++ b/pkg/util/pki/pki_common_test.go @@ -175,3 +175,42 @@ func TestControllerUserForCluster(t *testing.T) { t.Errorf("Expected %+v\nGot %+v", expected, user) } } + +func TestTruncatedCommonName(t *testing.T) { + testCases := []struct { + testName string + commonName string + expectedHashedString string + expectedLen int + }{ + { + testName: "Common name with length exceeded limitation", + commonName: "kafka-test-controller-exceeded-length.test-namespace.mgt.cluster.local", + expectedLen: MaxCNLen, + }, + { + testName: "Common name with length within limitation", + commonName: "kafka-test-controller.test-namespace.mgt.cluster.local", + expectedHashedString: "kafka-test-controller.test-namespace.mgt.cluster.local", + }, + } + + t.Parallel() + + for _, test := range testCases { + test := test + t.Run(test.testName, func(t *testing.T) { + get := EnsureValidCommonNameLen(test.commonName) + + if test.expectedHashedString == "" { + if len(get) != test.expectedLen { + t.Errorf("Common name with exceeded lenghth after hashing doesnt match expected length, got:%d, expected:%d", len(get), test.expectedLen) + } + } else { + if get != test.expectedHashedString { + t.Errorf("Common name with valid length shouldn't be changed, got:%s, expected:%s", get, test.expectedHashedString) + } + } + }) + } +}