From 2c4b4df01a55b88d603702177693640db1e61bec Mon Sep 17 00:00:00 2001 From: Marton Soos Date: Fri, 3 Sep 2021 15:41:46 +0300 Subject: [PATCH 1/6] Do not make secret names lowercase on normalization --- pkg/kubernetessecrets/kubernetes_secrets_builder.go | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/pkg/kubernetessecrets/kubernetes_secrets_builder.go b/pkg/kubernetessecrets/kubernetes_secrets_builder.go index fba54fb..f2b706b 100644 --- a/pkg/kubernetessecrets/kubernetes_secrets_builder.go +++ b/pkg/kubernetessecrets/kubernetes_secrets_builder.go @@ -97,11 +97,10 @@ func formatSecretName(value string) string { return createValidSecretName(value) } -var invalidDNS1123Chars = regexp.MustCompile("[^a-z0-9-]+") +var invalidDNS1123Chars = regexp.MustCompile("[^a-zA-Z0-9-]+") func createValidSecretName(value string) string { - result := strings.ToLower(value) - result = invalidDNS1123Chars.ReplaceAllString(result, "-") + result := invalidDNS1123Chars.ReplaceAllString(value, "-") if len(result) > kubeValidate.DNS1123SubdomainMaxLength { result = result[0:kubeValidate.DNS1123SubdomainMaxLength] From e365ebfdfa9234873a366a0718cd4b95b146c281 Mon Sep 17 00:00:00 2001 From: Marton Soos Date: Fri, 3 Sep 2021 15:42:02 +0300 Subject: [PATCH 2/6] Fix tests --- pkg/controller/onepassworditem/onepassworditem_test.go | 6 +++--- pkg/kubernetessecrets/kubernetes_secrets_builder_test.go | 4 ++-- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/pkg/controller/onepassworditem/onepassworditem_test.go b/pkg/controller/onepassworditem/onepassworditem_test.go index 09d5b3b..db383cb 100644 --- a/pkg/controller/onepassworditem/onepassworditem_test.go +++ b/pkg/controller/onepassworditem/onepassworditem_test.go @@ -232,7 +232,7 @@ var tests = []testReconcileItem{ expectedError: nil, expectedResultSecret: &corev1.Secret{ ObjectMeta: metav1.ObjectMeta{ - Name: "my-secret-it3m", + Name: "my-sECReT-it3m", Namespace: namespace, Annotations: map[string]string{ op.VersionAnnotation: fmt.Sprint(version), @@ -264,7 +264,7 @@ var tests = []testReconcileItem{ expectedError: nil, expectedResultSecret: &corev1.Secret{ ObjectMeta: metav1.ObjectMeta{ - Name: "my-secret-it3m", + Name: "my-sECReT-it3m", Namespace: namespace, Annotations: map[string]string{ op.VersionAnnotation: fmt.Sprint(version), @@ -274,7 +274,7 @@ var tests = []testReconcileItem{ "password": []byte(password), "username": []byte(username), "first-host": []byte(firstHost), - "aws-access-key": []byte(awsKey), + "AWS-Access-Key": []byte(awsKey), "ice-cream-type": []byte(iceCream), }, }, diff --git a/pkg/kubernetessecrets/kubernetes_secrets_builder_test.go b/pkg/kubernetessecrets/kubernetes_secrets_builder_test.go index 3c9484d..3ce27ee 100644 --- a/pkg/kubernetessecrets/kubernetes_secrets_builder_test.go +++ b/pkg/kubernetessecrets/kubernetes_secrets_builder_test.go @@ -102,7 +102,7 @@ func TestBuildKubernetesSecretFromOnePasswordItem(t *testing.T) { item.Fields = generateFields(5) kubeSecret := BuildKubernetesSecretFromOnePasswordItem(name, namespace, annotations, item) - if kubeSecret.Name != strings.ToLower(name) { + if kubeSecret.Name != name { t.Errorf("Expected name value: %v but got: %v", name, kubeSecret.Name) } if kubeSecret.Namespace != namespace { @@ -116,7 +116,7 @@ func TestBuildKubernetesSecretFromOnePasswordItem(t *testing.T) { func TestBuildKubernetesSecretFixesInvalidLabels(t *testing.T) { name := "inV@l1d k8s secret%name" - expectedName := "inv-l1d-k8s-secret-name" + expectedName := "inV-l1d-k8s-secret-name" namespace := "someNamespace" annotations := map[string]string{ "annotationKey": "annotationValue", From 88728909ff28fca2a6f46da2be64628826aae3ce Mon Sep 17 00:00:00 2001 From: Eddy Filip Date: Wed, 8 Sep 2021 13:49:32 +0300 Subject: [PATCH 3/6] Adjust regex to support `_` and `.` and trim them Now secret names can also contain `_` and `.` and they will be trimmed from start and end of string to be DNS1123 compliant --- pkg/kubernetessecrets/kubernetes_secrets_builder.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/pkg/kubernetessecrets/kubernetes_secrets_builder.go b/pkg/kubernetessecrets/kubernetes_secrets_builder.go index f2b706b..49a7522 100644 --- a/pkg/kubernetessecrets/kubernetes_secrets_builder.go +++ b/pkg/kubernetessecrets/kubernetes_secrets_builder.go @@ -97,7 +97,7 @@ func formatSecretName(value string) string { return createValidSecretName(value) } -var invalidDNS1123Chars = regexp.MustCompile("[^a-zA-Z0-9-]+") +var invalidDNS1123Chars = regexp.MustCompile("[^a-zA-Z0-9-_.]+") func createValidSecretName(value string) string { result := invalidDNS1123Chars.ReplaceAllString(value, "-") @@ -107,5 +107,5 @@ func createValidSecretName(value string) string { } // first and last character MUST be alphanumeric - return strings.Trim(result, "-") + return strings.Trim(result, "-._") } From d80e8dd799d5f769ff72763caaae4aa8ee634276 Mon Sep 17 00:00:00 2001 From: Eddy Filip Date: Wed, 8 Sep 2021 13:58:48 +0300 Subject: [PATCH 4/6] Add tests with names that contain `.` and `_` --- .../onepassworditem/onepassworditem_test.go | 41 +++++++++++++++++++ 1 file changed, 41 insertions(+) diff --git a/pkg/controller/onepassworditem/onepassworditem_test.go b/pkg/controller/onepassworditem/onepassworditem_test.go index db383cb..858fe74 100644 --- a/pkg/controller/onepassworditem/onepassworditem_test.go +++ b/pkg/controller/onepassworditem/onepassworditem_test.go @@ -286,6 +286,47 @@ var tests = []testReconcileItem{ "😄 ice-cream type": iceCream, }, }, + { + testName: "Secret from 1Password item with `_` and `.`", + customResource: &onepasswordv1.OnePasswordItem{ + TypeMeta: metav1.TypeMeta{ + Kind: onePasswordItemKind, + APIVersion: onePasswordItemAPIVersion, + }, + ObjectMeta: metav1.ObjectMeta{ + Name: "!.my_sECReT.it3m%-_", + Namespace: namespace, + }, + Spec: onepasswordv1.OnePasswordItemSpec{ + ItemPath: itemPath, + }, + }, + existingSecret: nil, + expectedError: nil, + expectedResultSecret: &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: "my_sECReT.it3m", + Namespace: namespace, + Annotations: map[string]string{ + op.VersionAnnotation: fmt.Sprint(version), + }, + }, + Data: map[string][]byte{ + "password": []byte(password), + "username": []byte(username), + "first-host": []byte(firstHost), + "AWS-Access-Key": []byte(awsKey), + "ice_cream.type": []byte(iceCream), + }, + }, + opItem: map[string]string{ + userKey: username, + passKey: password, + "first host": firstHost, + "AWS Access Key": awsKey, + "😄 -_ice_cream.type.": iceCream, + }, + }, } func TestReconcileOnePasswordItem(t *testing.T) { From a45a310611dc7638cdcd77057b7b57a8600f9e12 Mon Sep 17 00:00:00 2001 From: Eddy Filip Date: Wed, 8 Sep 2021 15:36:40 +0300 Subject: [PATCH 5/6] Make secret names DNS1123 Subdomain compiant This is done while ensuring that secret keys are compliant (contain alphanumeric characters, `-`, `_` and `.`) --- .../onepassworditem/onepassworditem_test.go | 18 +++++----- .../kubernetes_secrets_builder.go | 34 +++++++++++++++---- .../kubernetes_secrets_builder_test.go | 10 +++--- 3 files changed, 42 insertions(+), 20 deletions(-) diff --git a/pkg/controller/onepassworditem/onepassworditem_test.go b/pkg/controller/onepassworditem/onepassworditem_test.go index 858fe74..228cf7f 100644 --- a/pkg/controller/onepassworditem/onepassworditem_test.go +++ b/pkg/controller/onepassworditem/onepassworditem_test.go @@ -232,7 +232,7 @@ var tests = []testReconcileItem{ expectedError: nil, expectedResultSecret: &corev1.Secret{ ObjectMeta: metav1.ObjectMeta{ - Name: "my-sECReT-it3m", + Name: "my-secret-it3m", Namespace: namespace, Annotations: map[string]string{ op.VersionAnnotation: fmt.Sprint(version), @@ -264,7 +264,7 @@ var tests = []testReconcileItem{ expectedError: nil, expectedResultSecret: &corev1.Secret{ ObjectMeta: metav1.ObjectMeta{ - Name: "my-sECReT-it3m", + Name: "my-secret-it3m", Namespace: namespace, Annotations: map[string]string{ op.VersionAnnotation: fmt.Sprint(version), @@ -287,7 +287,7 @@ var tests = []testReconcileItem{ }, }, { - testName: "Secret from 1Password item with `_` and `.`", + testName: "Secret from 1Password item with `-`, `_` and `.`", customResource: &onepasswordv1.OnePasswordItem{ TypeMeta: metav1.TypeMeta{ Kind: onePasswordItemKind, @@ -305,18 +305,18 @@ var tests = []testReconcileItem{ expectedError: nil, expectedResultSecret: &corev1.Secret{ ObjectMeta: metav1.ObjectMeta{ - Name: "my_sECReT.it3m", + Name: "my-secret.it3m", Namespace: namespace, Annotations: map[string]string{ op.VersionAnnotation: fmt.Sprint(version), }, }, Data: map[string][]byte{ - "password": []byte(password), - "username": []byte(username), - "first-host": []byte(firstHost), - "AWS-Access-Key": []byte(awsKey), - "ice_cream.type": []byte(iceCream), + "password": []byte(password), + "username": []byte(username), + "first-host": []byte(firstHost), + "AWS-Access-Key": []byte(awsKey), + "-_ice_cream.type.": []byte(iceCream), }, }, opItem: map[string]string{ diff --git a/pkg/kubernetessecrets/kubernetes_secrets_builder.go b/pkg/kubernetessecrets/kubernetes_secrets_builder.go index 49a7522..571e25b 100644 --- a/pkg/kubernetessecrets/kubernetes_secrets_builder.go +++ b/pkg/kubernetessecrets/kubernetes_secrets_builder.go @@ -80,16 +80,17 @@ func BuildKubernetesSecretData(fields []*onepassword.ItemField) map[string][]byt secretData := map[string][]byte{} for i := 0; i < len(fields); i++ { if fields[i].Value != "" { - key := formatSecretName(fields[i].Label) + key := formatSecretDataName(fields[i].Label) secretData[key] = []byte(fields[i].Value) } } return secretData } -// formatSecretName rewrites a value to be a valid Secret name or Secret data key. +// formatSecretName rewrites a value to be a valid Secret name. // -// The Secret meta.name and data keys must be valid DNS subdomain names (https://kubernetes.io/docs/concepts/configuration/secret/#overview-of-secrets) +// The Secret meta.name and data keys must be valid DNS subdomain names +// (https://kubernetes.io/docs/concepts/configuration/secret/#overview-of-secrets) func formatSecretName(value string) string { if errs := kubeValidate.IsDNS1123Subdomain(value); len(errs) == 0 { return value @@ -97,15 +98,36 @@ func formatSecretName(value string) string { return createValidSecretName(value) } -var invalidDNS1123Chars = regexp.MustCompile("[^a-zA-Z0-9-_.]+") +// formatSecretDataName rewrites a value to be a valid Secret data key. +// +// The Secret data keys must consist of alphanumeric numbers, `-`, `_` or `.` +// (https://kubernetes.io/docs/concepts/configuration/secret/#overview-of-secrets) +func formatSecretDataName(value string) string { + if errs := kubeValidate.IsConfigMapKey(value); len(errs) == 0 { + return value + } + return createValidSecretDataName(value) +} + +var invalidDNS1123Chars = regexp.MustCompile("[^a-z0-9-.]+") func createValidSecretName(value string) string { - result := invalidDNS1123Chars.ReplaceAllString(value, "-") + result := strings.ToLower(value) + result = invalidDNS1123Chars.ReplaceAllString(result, "-") if len(result) > kubeValidate.DNS1123SubdomainMaxLength { result = result[0:kubeValidate.DNS1123SubdomainMaxLength] } // first and last character MUST be alphanumeric - return strings.Trim(result, "-._") + return strings.Trim(result, "-.") +} + +var invalidDataChars = regexp.MustCompile("[^a-zA-Z0-9-._]+") +var invalidStartEndChars = regexp.MustCompile("(^[^a-zA-Z0-9-._]+|[^a-zA-Z0-9-._]+$)") + +func createValidSecretDataName(value string) string { + result := invalidStartEndChars.ReplaceAllString(value, "") + result = invalidDataChars.ReplaceAllString(result, "-") + return result } diff --git a/pkg/kubernetessecrets/kubernetes_secrets_builder_test.go b/pkg/kubernetessecrets/kubernetes_secrets_builder_test.go index 3ce27ee..ccd86d3 100644 --- a/pkg/kubernetessecrets/kubernetes_secrets_builder_test.go +++ b/pkg/kubernetessecrets/kubernetes_secrets_builder_test.go @@ -3,13 +3,13 @@ package kubernetessecrets import ( "context" "fmt" - kubeValidate "k8s.io/apimachinery/pkg/util/validation" "strings" "testing" "github.com/1Password/connect-sdk-go/onepassword" corev1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/types" + kubeValidate "k8s.io/apimachinery/pkg/util/validation" "k8s.io/client-go/kubernetes" "sigs.k8s.io/controller-runtime/pkg/client/fake" ) @@ -102,7 +102,7 @@ func TestBuildKubernetesSecretFromOnePasswordItem(t *testing.T) { item.Fields = generateFields(5) kubeSecret := BuildKubernetesSecretFromOnePasswordItem(name, namespace, annotations, item) - if kubeSecret.Name != name { + if kubeSecret.Name != strings.ToLower(name) { t.Errorf("Expected name value: %v but got: %v", name, kubeSecret.Name) } if kubeSecret.Namespace != namespace { @@ -116,7 +116,7 @@ func TestBuildKubernetesSecretFromOnePasswordItem(t *testing.T) { func TestBuildKubernetesSecretFixesInvalidLabels(t *testing.T) { name := "inV@l1d k8s secret%name" - expectedName := "inV-l1d-k8s-secret-name" + expectedName := "inv-l1d-k8s-secret-name" namespace := "someNamespace" annotations := map[string]string{ "annotationKey": "annotationValue", @@ -129,7 +129,7 @@ func TestBuildKubernetesSecretFixesInvalidLabels(t *testing.T) { Value: "value1", }, { - Label: strings.Repeat("x", kubeValidate.DNS1123SubdomainMaxLength+1), + Label: strings.Repeat("x", kubeValidate.LabelValueMaxLength+1), Value: "name exceeds max length", }, } @@ -205,7 +205,7 @@ func ParseVaultIdAndItemIdFromPath(path string) (string, string, error) { } func validLabel(v string) bool { - if err := kubeValidate.IsDNS1123Subdomain(v); len(err) > 0 { + if err := kubeValidate.IsConfigMapKey(v); len(err) > 0 { return false } return true From 670040477e8e1d7baf1842ac21af7cc6bb8d01f2 Mon Sep 17 00:00:00 2001 From: Eddy Filip Date: Wed, 8 Sep 2021 16:02:08 +0300 Subject: [PATCH 6/6] Add max length for secret key names Max length for secret key names must be DNS1123 compliant (253) --- pkg/kubernetessecrets/kubernetes_secrets_builder.go | 5 +++++ pkg/kubernetessecrets/kubernetes_secrets_builder_test.go | 2 +- 2 files changed, 6 insertions(+), 1 deletion(-) diff --git a/pkg/kubernetessecrets/kubernetes_secrets_builder.go b/pkg/kubernetessecrets/kubernetes_secrets_builder.go index 571e25b..f07d86e 100644 --- a/pkg/kubernetessecrets/kubernetes_secrets_builder.go +++ b/pkg/kubernetessecrets/kubernetes_secrets_builder.go @@ -129,5 +129,10 @@ var invalidStartEndChars = regexp.MustCompile("(^[^a-zA-Z0-9-._]+|[^a-zA-Z0-9-._ func createValidSecretDataName(value string) string { result := invalidStartEndChars.ReplaceAllString(value, "") result = invalidDataChars.ReplaceAllString(result, "-") + + if len(result) > kubeValidate.DNS1123SubdomainMaxLength { + result = result[0:kubeValidate.DNS1123SubdomainMaxLength] + } + return result } diff --git a/pkg/kubernetessecrets/kubernetes_secrets_builder_test.go b/pkg/kubernetessecrets/kubernetes_secrets_builder_test.go index ccd86d3..0d7d22b 100644 --- a/pkg/kubernetessecrets/kubernetes_secrets_builder_test.go +++ b/pkg/kubernetessecrets/kubernetes_secrets_builder_test.go @@ -129,7 +129,7 @@ func TestBuildKubernetesSecretFixesInvalidLabels(t *testing.T) { Value: "value1", }, { - Label: strings.Repeat("x", kubeValidate.LabelValueMaxLength+1), + Label: strings.Repeat("x", kubeValidate.DNS1123SubdomainMaxLength+1), Value: "name exceeds max length", }, }