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 {
- 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

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)