Merge pull request #1615 from vmware-tanzu/jtc/fix-double-decoding-of-ca-crt

Fix #1582 by not double-decoding the ca.crt field in external TLS secrets for the impersonation proxy

internal/controller/impersonatorconfig/impersonator_config.go

@@ -730,24 +730,18 @@ func (c *impersonatorConfigController) readExternalTLSSecret(externalTLSSecretNa
 		return nil, err
 	}
 
-	base64EncodedCaCert := secretFromInformer.Data[caCrtKey]
+	if caCertPEM, ok := secretFromInformer.Data[caCrtKey]; ok && len(caCertPEM) > 0 {
+		plog.Info(fmt.Sprintf("found a %s field in the externally provided TLS secret for the impersonation proxy", caCrtKey),
+			"secretName", externalTLSSecretName,
+			"caCertPEM", caCertPEM)
 
-	if len(base64EncodedCaCert) > 0 {
+		block, _ := pem.Decode(caCertPEM)
-		var decodedCaCert []byte
-		decodedCaCert, err = base64.StdEncoding.DecodeString(string(secretFromInformer.Data[caCrtKey]))
-		if err != nil {
-			err = fmt.Errorf("unable to read provided ca.crt: %w", err)
-			plog.Error("error loading cert from externally provided TLS secret for the impersonation proxy", err)
-			return nil, err
-		}
-
-		block, _ := pem.Decode(decodedCaCert)
 		if block == nil {
 			plog.Warning("error loading cert from externally provided TLS secret for the impersonation proxy: data is not a certificate")
 			return nil, fmt.Errorf("unable to read provided ca.crt: data is not a certificate")
 		}
 
-		return decodedCaCert, nil
+		return caCertPEM, nil
 	}
 
 	return nil, nil

internal/controller/impersonatorconfig/impersonator_config_test.go

@@ -1356,7 +1356,7 @@ func TestImpersonatorConfigControllerSync(t *testing.T) {
 				when("the externally provided TLS secret has a ca.crt field", func() {
 					it.Before(func() {
 						addSecretToTrackers(mTLSClientCertCASecret, kubeInformerClient)
-						externalTLSSecret.Data["ca.crt"] = []byte(base64.StdEncoding.EncodeToString(externalCA.Bundle()))
+						externalTLSSecret.Data["ca.crt"] = externalCA.Bundle()
 						addSecretToTrackers(externalTLSSecret, kubeInformerClient)
 						addCredentialIssuerToTrackers(v1alpha1.CredentialIssuer{
 							ObjectMeta: metav1.ObjectMeta{Name: credentialIssuerResourceName},
@@ -1386,42 +1386,10 @@ func TestImpersonatorConfigControllerSync(t *testing.T) {
 					})
 				})
 
-				when("the externally provided TLS secret has a ca.crt field that is not base64-encoded", func() {
-					it.Before(func() {
-						addSecretToTrackers(mTLSClientCertCASecret, kubeInformerClient)
-						externalTLSSecret.Data["ca.crt"] = []byte("hello")
-						addSecretToTrackers(externalTLSSecret, kubeInformerClient)
-						addCredentialIssuerToTrackers(v1alpha1.CredentialIssuer{
-							ObjectMeta: metav1.ObjectMeta{Name: credentialIssuerResourceName},
-							Spec: v1alpha1.CredentialIssuerSpec{
-								ImpersonationProxy: &v1alpha1.ImpersonationProxySpec{
-									Mode:             v1alpha1.ImpersonationProxyModeAuto,
-									ExternalEndpoint: localhostIP,
-									Service: v1alpha1.ImpersonationProxyServiceSpec{
-										Type: v1alpha1.ImpersonationProxyServiceTypeNone,
-									},
-									TLS: &v1alpha1.ImpersonationProxyTLSSpec{
-										SecretName: externallyProvidedTLSSecretName,
-									},
-								},
-							},
-						}, pinnipedInformerClient, pinnipedAPIClient)
-					})
-
-					it("returns an error", func() {
-						startInformersAndController()
-						r.Error(runControllerSync(), "could not load the externally provided TLS secret for the impersonation proxy: unable to read provided ca.crt: illegal base64 data at input byte 4")
-						r.Len(kubeAPIClient.Actions(), 1)
-						requireNodesListed(kubeAPIClient.Actions()[0])
-						requireCredentialIssuer(newErrorStrategy("could not load the externally provided TLS secret for the impersonation proxy: unable to read provided ca.crt: illegal base64 data at input byte 4"))
-						requireMTLSClientCertProviderHasLoadedCerts([]byte{}, []byte{})
-					})
-				})
-
 				when("the externally provided TLS secret has a ca.crt field that is not a valid cert", func() {
 					it.Before(func() {
 						addSecretToTrackers(mTLSClientCertCASecret, kubeInformerClient)
-						externalTLSSecret.Data["ca.crt"] = []byte(base64.StdEncoding.EncodeToString([]byte("hello")))
+						externalTLSSecret.Data["ca.crt"] = []byte("hello")
 						addSecretToTrackers(externalTLSSecret, kubeInformerClient)
 						addCredentialIssuerToTrackers(v1alpha1.CredentialIssuer{
 							ObjectMeta: metav1.ObjectMeta{Name: credentialIssuerResourceName},

test/integration/concierge_impersonation_proxy_test.go

@@ -1778,7 +1778,7 @@ func TestImpersonationProxy(t *testing.T) { //nolint:gocyclo // yeah, it's compl
 		)
 	})
 
-	t.Run("using externally provided TLS serving cert", func(t *testing.T) {
+	t.Run("using externally provided TLS serving cert with stringData", func(t *testing.T) {
 		var externallyProvidedCA *certauthority.CA
 		externallyProvidedCA, err = certauthority.New("Impersonation Proxy Integration Test CA", 1*time.Hour)
 		require.NoError(t, err)
@@ -1787,12 +1787,15 @@ func TestImpersonationProxy(t *testing.T) { //nolint:gocyclo // yeah, it's compl
 		externallyProvidedTLSServingCertPEM, externallyProvidedTLSServingKeyPEM, err = externallyProvidedCA.IssueServerCertPEM([]string{proxyServiceEndpoint}, nil, 1*time.Hour)
 		require.NoError(t, err)
 
+		// Specifically use corev1.Secret.StringData
+		// https://kubernetes.io/docs/tasks/configmap-secret/managing-secret-using-config-file/#create-the-config-file
 		externallyProvidedTLSServingCertSecret := testlib.CreateTestSecret(
 			t,
 			env.ConciergeNamespace,
 			"external-tls-cert-secret-name",
 			corev1.SecretTypeTLS,
 			map[string]string{
+				"ca.crt":            string(externallyProvidedCA.Bundle()),
 				v1.TLSCertKey:       string(externallyProvidedTLSServingCertPEM),
 				v1.TLSPrivateKeyKey: string(externallyProvidedTLSServingKeyPEM),
 			})
@@ -1847,6 +1850,78 @@ func TestImpersonationProxy(t *testing.T) { //nolint:gocyclo // yeah, it's compl
 		}, 2*time.Minute, 500*time.Millisecond)
 	})
 
+	t.Run("using externally provided TLS serving cert with data []byte arrays", func(t *testing.T) {
+		var externallyProvidedCA *certauthority.CA
+		externallyProvidedCA, err = certauthority.New("Impersonation Proxy Integration Test CA", 1*time.Hour)
+		require.NoError(t, err)
+
+		var externallyProvidedTLSServingCertPEM, externallyProvidedTLSServingKeyPEM []byte
+		externallyProvidedTLSServingCertPEM, externallyProvidedTLSServingKeyPEM, err = externallyProvidedCA.IssueServerCertPEM([]string{proxyServiceEndpoint}, nil, 1*time.Hour)
+		require.NoError(t, err)
+
+		// Specifically use corev1.Secret.Data
+		// https://kubernetes.io/docs/tasks/configmap-secret/managing-secret-using-config-file/#create-the-config-file
+		externallyProvidedTLSServingCertSecret := testlib.CreateTestSecretBytes(
+			t,
+			env.ConciergeNamespace,
+			"external-tls-cert-secret-name-integration-tests",
+			corev1.SecretTypeTLS,
+			map[string][]byte{
+				"ca.crt":            externallyProvidedCA.Bundle(),
+				v1.TLSCertKey:       externallyProvidedTLSServingCertPEM,
+				v1.TLSPrivateKeyKey: externallyProvidedTLSServingKeyPEM,
+			})
+
+		_, originalInternallyGeneratedCAPEM := performImpersonatorDiscoveryURL(ctx, t, env, adminConciergeClient)
+
+		t.Cleanup(func() {
+			// Remove the TLS block from the CredentialIssuer, which should revert the ImpersonationProxy to using an
+			// internally generated TLS serving cert derived from the original CA.
+			updateCredentialIssuer(ctx, t, env, adminConciergeClient, conciergev1alpha.CredentialIssuerSpec{
+				ImpersonationProxy: &conciergev1alpha.ImpersonationProxySpec{
+					Mode:             conciergev1alpha.ImpersonationProxyModeEnabled,
+					ExternalEndpoint: proxyServiceEndpoint,
+					Service: conciergev1alpha.ImpersonationProxyServiceSpec{
+						Type: conciergev1alpha.ImpersonationProxyServiceTypeClusterIP,
+					},
+				},
+			})
+
+			// Wait for the CredentialIssuer's impersonation proxy frontend strategy to be updated to the original CA bundle
+			testlib.RequireEventuallyWithoutError(t, func() (bool, error) {
+				_, impersonationProxyCACertPEM = performImpersonatorDiscoveryURL(ctx, t, env, adminConciergeClient)
+
+				return bytes.Equal(impersonationProxyCACertPEM, originalInternallyGeneratedCAPEM), nil
+			}, 2*time.Minute, 500*time.Millisecond)
+		})
+
+		updateCredentialIssuer(ctx, t, env, adminConciergeClient, conciergev1alpha.CredentialIssuerSpec{
+			ImpersonationProxy: &conciergev1alpha.ImpersonationProxySpec{
+				Mode:             conciergev1alpha.ImpersonationProxyModeEnabled,
+				ExternalEndpoint: proxyServiceEndpoint,
+				Service: conciergev1alpha.ImpersonationProxyServiceSpec{
+					Type: conciergev1alpha.ImpersonationProxyServiceTypeClusterIP,
+				},
+				TLS: &conciergev1alpha.ImpersonationProxyTLSSpec{
+					CertificateAuthorityData: base64.StdEncoding.EncodeToString(externallyProvidedCA.Bundle()),
+					SecretName:               externallyProvidedTLSServingCertSecret.Name,
+				},
+			},
+		})
+
+		// Wait for the CredentialIssuer's impersonation proxy frontend strategy to be updated with the right CA bundle
+		testlib.RequireEventuallyWithoutError(t, func() (bool, error) {
+			_, impersonationProxyCACertPEM = performImpersonatorDiscoveryURL(ctx, t, env, adminConciergeClient)
+			return bytes.Equal(impersonationProxyCACertPEM, externallyProvidedCA.Bundle()), nil
+		}, 2*time.Minute, 500*time.Millisecond)
+
+		// Do a login via performImpersonatorDiscovery
+		testlib.RequireEventuallyWithoutError(t, func() (bool, error) {
+			_, newImpersonationProxyCACertPEM := performImpersonatorDiscovery(ctx, t, env, adminClient, adminConciergeClient, refreshCredential)
+			return bytes.Equal(newImpersonationProxyCACertPEM, externallyProvidedCA.Bundle()), err
+		}, 2*time.Minute, 500*time.Millisecond)
+	})
+
 	t.Run("manually disabling the impersonation proxy feature", func(t *testing.T) {
 		// Update configuration to force the proxy to disabled mode
 		updateCredentialIssuer(ctx, t, env, adminConciergeClient, conciergev1alpha.CredentialIssuerSpec{

test/testlib/client.go

@@ -364,6 +364,28 @@ func CreateTestSecret(t *testing.T, namespace string, baseName string, secretTyp
 	return created
 }
 
+func CreateTestSecretBytes(t *testing.T, namespace string, baseName string, secretType corev1.SecretType, data map[string][]byte) *corev1.Secret {
+	t.Helper()
+	client := NewKubernetesClientset(t)
+	ctx, cancel := context.WithTimeout(context.Background(), time.Minute)
+	defer cancel()
+
+	created, err := client.CoreV1().Secrets(namespace).Create(ctx, &corev1.Secret{
+		ObjectMeta: testObjectMeta(t, baseName),
+		Type:       secretType,
+		Data:       data,
+	}, metav1.CreateOptions{})
+	require.NoError(t, err)
+
+	t.Cleanup(func() {
+		t.Logf("cleaning up test Secret %s/%s", created.Namespace, created.Name)
+		err := client.CoreV1().Secrets(namespace).Delete(context.Background(), created.Name, metav1.DeleteOptions{})
+		require.NoError(t, err)
+	})
+	t.Logf("created test Secret %s", created.Name)
+	return created
+}
+
 func CreateClientCredsSecret(t *testing.T, clientID string, clientSecret string) *corev1.Secret {
 	t.Helper()
 	env := IntegrationEnv(t)