From 170799537872c9f17d1d978378ac278b48c0590d Mon Sep 17 00:00:00 2001 From: Joshua Casey Date: Tue, 8 Aug 2023 20:17:21 -0500 Subject: [PATCH] Fix #1582 by not double-decoding the ca.crt field in external TLS secrets for the impersonation proxy --- .../impersonatorconfig/impersonator_config.go | 18 ++--- .../impersonator_config_test.go | 36 +--------- .../concierge_impersonation_proxy_test.go | 71 +++++++++++++++++++ test/testlib/client.go | 22 ++++++ 4 files changed, 101 insertions(+), 46 deletions(-) diff --git a/internal/controller/impersonatorconfig/impersonator_config.go b/internal/controller/impersonatorconfig/impersonator_config.go index 401abf0f..99f99c3c 100644 --- a/internal/controller/impersonatorconfig/impersonator_config.go +++ b/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 { - 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) + block, _ := pem.Decode(caCertPEM) 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 diff --git a/internal/controller/impersonatorconfig/impersonator_config_test.go b/internal/controller/impersonatorconfig/impersonator_config_test.go index b25dadcf..25252afc 100644 --- a/internal/controller/impersonatorconfig/impersonator_config_test.go +++ b/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}, diff --git a/test/integration/concierge_impersonation_proxy_test.go b/test/integration/concierge_impersonation_proxy_test.go index 42ed3c9c..fa5ff137 100644 --- a/test/integration/concierge_impersonation_proxy_test.go +++ b/test/integration/concierge_impersonation_proxy_test.go @@ -1793,6 +1793,7 @@ func TestImpersonationProxy(t *testing.T) { //nolint:gocyclo // yeah, it's compl "external-tls-cert-secret-name", corev1.SecretTypeTLS, map[string]string{ + "ca.crt": string(externallyProvidedTLSServingCertPEM), v1.TLSCertKey: string(externallyProvidedTLSServingCertPEM), v1.TLSPrivateKeyKey: string(externallyProvidedTLSServingKeyPEM), }) @@ -1847,6 +1848,76 @@ 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 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) + + 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{ diff --git a/test/testlib/client.go b/test/testlib/client.go index 06756ce1..55f208c1 100644 --- a/test/testlib/client.go +++ b/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)