diff --git a/internal/controller/impersonatorconfig/impersonator_config.go b/internal/controller/impersonatorconfig/impersonator_config.go index 5497d805..3626a6c9 100644 --- a/internal/controller/impersonatorconfig/impersonator_config.go +++ b/internal/controller/impersonatorconfig/impersonator_config.go @@ -359,10 +359,10 @@ func (c *impersonatorConfigController) deleteTLSCertificateWithWrongName(ctx con // The certPEM is not valid. return secret, nil // TODO what should we do? } - parsed, _ := x509.ParseCertificate(block.Bytes) + actualCertFromSecret, _ := x509.ParseCertificate(block.Bytes) // TODO handle err - desiredIPs, _, nameIsReady, err := c.findTLSCertificateName(config) // TODO check this for hostnames too, not just ips + desiredIPs, desiredHostnames, nameIsReady, err := c.findTLSCertificateName(config) // TODO check this for hostnames too, not just ips //nolint:staticcheck // TODO remove this nolint when we fix the TODO below if err != nil { // TODO return err @@ -377,12 +377,24 @@ func (c *impersonatorConfigController) deleteTLSCertificateWithWrongName(ctx con return nil, nil } - actualIPs := parsed.IPAddresses - // TODO handle multiple IPs, and handle when there is no IP - if desiredIPs[0].Equal(actualIPs[0]) { + actualIPs := actualCertFromSecret.IPAddresses + actualHostnames := actualCertFromSecret.DNSNames + plog.Info("Checking TLS certificate names", + "desiredIPs", desiredIPs, + "desiredHostnames", desiredHostnames, + "actualIPs", actualIPs, + "actualHostnames", actualHostnames, + "secret", c.tlsSecretName, + "namespace", c.namespace) + + // TODO handle multiple IPs + if len(desiredIPs) == len(actualIPs) && len(desiredIPs) == 1 && desiredIPs[0].Equal(actualIPs[0]) { //nolint:gocritic // The cert matches the desired state, so we do not need to delete it. return secret, nil } + if len(desiredHostnames) == len(actualHostnames) && len(desiredHostnames) == 1 && desiredHostnames[0] == actualHostnames[0] { //nolint:gocritic + return secret, nil + } err = c.ensureTLSSecretIsRemoved(ctx) if err != nil { @@ -430,38 +442,47 @@ func (c *impersonatorConfigController) ensureTLSSecretIsCreatedAndLoaded(ctx con } func (c *impersonatorConfigController) findTLSCertificateName(config *impersonator.Config) ([]net.IP, []string, bool, error) { + if config.Endpoint != "" { + return c.findTLSCertificateNameFromEndpointConfig(config) + } + return c.findTLSCertificateNameFromLoadBalancer() +} + +func (c *impersonatorConfigController) findTLSCertificateNameFromEndpointConfig(config *impersonator.Config) ([]net.IP, []string, bool, error) { var ips []net.IP var hostnames []string - if config.Endpoint != "" { - parsedAsIP := net.ParseIP(config.Endpoint) - if parsedAsIP != nil { - ips = []net.IP{parsedAsIP} - } else { - hostnames = []string{config.Endpoint} - } - // TODO Endpoint could be a hostname - // TODO Endpoint could have a port number in it, which we should parse out and ignore for this purpose + parsedAsIP := net.ParseIP(config.Endpoint) + if parsedAsIP != nil { + ips = []net.IP{parsedAsIP} } else { - lb, err := c.servicesInformer.Lister().Services(c.namespace).Get(c.generatedLoadBalancerServiceName) - notFound := k8serrors.IsNotFound(err) - if notFound { - // TODO is this an error? we should have already created the load balancer, so why would it not exist here? - return nil, nil, false, nil - } - if err != nil { - return nil, nil, false, err - } - ingresses := lb.Status.LoadBalancer.Ingress - if len(ingresses) == 0 { - plog.Info("load balancer for impersonation proxy does not have an ingress yet, so skipping tls cert generation while we wait", - "service", c.generatedLoadBalancerServiceName, - "namespace", c.namespace) - return nil, nil, false, nil - } - // TODO get all IPs and all hostnames from ingresses and put them in the cert - ip := ingresses[0].IP - ips = []net.IP{net.ParseIP(ip)} + hostnames = []string{config.Endpoint} } + // TODO Endpoint could have a port number in it, which we should parse out and ignore for this purpose + return ips, hostnames, true, nil +} + +func (c *impersonatorConfigController) findTLSCertificateNameFromLoadBalancer() ([]net.IP, []string, bool, error) { + var ips []net.IP + var hostnames []string + lb, err := c.servicesInformer.Lister().Services(c.namespace).Get(c.generatedLoadBalancerServiceName) + notFound := k8serrors.IsNotFound(err) + if notFound { + // TODO is this an error? we should have already created the load balancer, so why would it not exist here? + return nil, nil, false, nil + } + if err != nil { + return nil, nil, false, err + } + ingresses := lb.Status.LoadBalancer.Ingress + if len(ingresses) == 0 { + plog.Info("load balancer for impersonation proxy does not have an ingress yet, so skipping tls cert generation while we wait", + "service", c.generatedLoadBalancerServiceName, + "namespace", c.namespace) + return nil, nil, false, nil + } + // TODO get all IPs and all hostnames from ingresses and put them in the cert + ip := ingresses[0].IP + ips = []net.IP{net.ParseIP(ip)} return ips, hostnames, true, nil } @@ -487,6 +508,11 @@ func (c *impersonatorConfigController) createNewTLSSecret(ctx context.Context, c v1.TLSCertKey: certPEM, }, } + plog.Info("Creating TLS certificates for impersonation proxy", + "ips", ips, + "hostnames", hostnames, + "secret", c.tlsSecretName, + "namespace", c.namespace) _, _ = c.k8sClient.CoreV1().Secrets(c.namespace).Create(ctx, newTLSSecret, metav1.CreateOptions{}) // TODO handle error on create return newTLSSecret, nil @@ -505,6 +531,10 @@ func (c *impersonatorConfigController) loadTLSCertFromSecret(tlsSecret *v1.Secre // TODO clear the secret if it was already set previously... c.setTLSCert(nil) return fmt.Errorf("could not parse TLS cert PEM data from Secret: %w", err) } + plog.Info("Loading TLS certificates for impersonation proxy", + "certPEM", certPEM, + "secret", c.tlsSecretName, + "namespace", c.namespace) c.setTLSCert(&tlsCert) return nil } @@ -518,7 +548,7 @@ func (c *impersonatorConfigController) ensureTLSSecretIsRemoved(ctx context.Cont return nil } plog.Info("Deleting TLS certificates for impersonation proxy", - "service", c.tlsSecretName, + "secret", c.tlsSecretName, "namespace", c.namespace) err = c.k8sClient.CoreV1().Secrets(c.namespace).Delete(ctx, c.tlsSecretName, metav1.DeleteOptions{}) if err != nil { diff --git a/internal/controller/impersonatorconfig/impersonator_config_test.go b/internal/controller/impersonatorconfig/impersonator_config_test.go index c61026da..d8d31f1c 100644 --- a/internal/controller/impersonatorconfig/impersonator_config_test.go +++ b/internal/controller/impersonatorconfig/impersonator_config_test.go @@ -1017,10 +1017,62 @@ func TestImpersonatorConfigControllerSync(t *testing.T) { r.Len(kubeAPIClient.Actions(), 2) requireNodesListed(kubeAPIClient.Actions()[0]) ca := requireTLSSecretWasCreated(kubeAPIClient.Actions()[1]) - // Check that the server is running and that TLS certs that are being served are are for fakeIP. + // Check that the server is running and that TLS certs that are being served are are for fakeHostname. requireTLSServerIsRunning(ca, fakeHostname, map[string]string{fakeHostname + ":443": startedTLSListener.Addr().String()}) }) }) + + when("switching from ip address endpoint to hostname endpoint and back to ip address", func() { + const fakeHostname = "fake.example.com" + const fakeIP = "127.0.0.42" + var hostnameYAML = fmt.Sprintf("{mode: enabled, endpoint: %s}", fakeHostname) + var ipAddressYAML = fmt.Sprintf("{mode: enabled, endpoint: %s}", fakeIP) + it.Before(func() { + addImpersonatorConfigMapToTracker(configMapResourceName, ipAddressYAML) + addNodeWithRoleToTracker("worker") + }) + + it("regenerates the cert for the hostname, then regenerates it for the IP again", func() { + startInformersAndController() + r.NoError(controllerlib.TestSync(t, subject, *syncContext)) + r.Len(kubeAPIClient.Actions(), 2) + requireNodesListed(kubeAPIClient.Actions()[0]) + ca := requireTLSSecretWasCreated(kubeAPIClient.Actions()[1]) + // Check that the server is running and that TLS certs that are being served are are for fakeIP. + requireTLSServerIsRunning(ca, fakeIP, map[string]string{fakeIP + ":443": startedTLSListener.Addr().String()}) + + // update manually because the kubeAPIClient isn't connected to the informer in the tests + addSecretFromCreateActionToTracker(kubeAPIClient.Actions()[1], kubeInformerClient, "1") + waitForInformerCacheToSeeResourceVersion(kubeInformers.Core().V1().Secrets().Informer(), "1") + + // Switch the endpoint config to a hostname. + updateImpersonatorConfigMapInTracker(configMapResourceName, hostnameYAML, "1") + waitForInformerCacheToSeeResourceVersion(kubeInformers.Core().V1().ConfigMaps().Informer(), "1") + + r.NoError(controllerlib.TestSync(t, subject, *syncContext)) + r.Len(kubeAPIClient.Actions(), 4) + requireTLSSecretDeleted(kubeAPIClient.Actions()[2]) + ca = requireTLSSecretWasCreated(kubeAPIClient.Actions()[3]) + // Check that the server is running and that TLS certs that are being served are are for fakeHostname. + requireTLSServerIsRunning(ca, fakeHostname, map[string]string{fakeHostname + ":443": startedTLSListener.Addr().String()}) + + // update manually because the kubeAPIClient isn't connected to the informer in the tests + deleteTLSCertSecretFromTracker(tlsSecretName, kubeInformerClient) + addSecretFromCreateActionToTracker(kubeAPIClient.Actions()[3], kubeInformerClient, "2") + waitForInformerCacheToSeeResourceVersion(kubeInformers.Core().V1().Secrets().Informer(), "2") + + // Switch the endpoint config back to an IP. + updateImpersonatorConfigMapInTracker(configMapResourceName, ipAddressYAML, "2") + waitForInformerCacheToSeeResourceVersion(kubeInformers.Core().V1().ConfigMaps().Informer(), "2") + + r.NoError(controllerlib.TestSync(t, subject, *syncContext)) + r.Len(kubeAPIClient.Actions(), 6) + requireTLSSecretDeleted(kubeAPIClient.Actions()[4]) + ca = requireTLSSecretWasCreated(kubeAPIClient.Actions()[5]) + // Check that the server is running and that TLS certs that are being served are are for fakeIP. + requireTLSServerIsRunning(ca, fakeIP, map[string]string{fakeIP + ":443": startedTLSListener.Addr().String()}) + }) + }) }) when("the configuration switches from enabled to disabled mode", func() { diff --git a/test/integration/concierge_impersonation_proxy_test.go b/test/integration/concierge_impersonation_proxy_test.go index 1ec2fc08..b59fe91d 100644 --- a/test/integration/concierge_impersonation_proxy_test.go +++ b/test/integration/concierge_impersonation_proxy_test.go @@ -29,8 +29,11 @@ import ( "go.pinniped.dev/test/library" ) -// TODO don't hard code "pinniped-concierge-" in this string. It should be constructed from the env app name. -const impersonationProxyConfigMapName = "pinniped-concierge-impersonation-proxy-config" +const ( + // TODO don't hard code "pinniped-concierge-" in these strings. It should be constructed from the env app name. + impersonationProxyConfigMapName = "pinniped-concierge-impersonation-proxy-config" + impersonationProxyTLSSecretName = "pinniped-concierge-impersonation-proxy-tls-serving-certificate" //nolint:gosec // this is not a credential +) func TestImpersonationProxy(t *testing.T) { env := library.IntegrationEnv(t) @@ -49,23 +52,27 @@ func TestImpersonationProxy(t *testing.T) { authenticator := library.CreateTestWebhookAuthenticator(ctx, t) // The address of the ClusterIP service that points at the impersonation proxy's port - proxyServiceURL := fmt.Sprintf("https://%s-proxy.%s.svc.cluster.local", env.ConciergeAppName, env.ConciergeNamespace) + proxyServiceEndpoint := fmt.Sprintf("%s-proxy.%s.svc.cluster.local", env.ConciergeAppName, env.ConciergeNamespace) + proxyServiceURL := fmt.Sprintf("https://%s", proxyServiceEndpoint) t.Logf("making kubeconfig that points to %q", proxyServiceURL) - kubeconfig := &rest.Config{ - Host: proxyServiceURL, - TLSClientConfig: rest.TLSClientConfig{Insecure: true}, - BearerToken: impersonationtoken.Make(t, env.TestUser.Token, &authenticator, env.APIGroupSuffix), - Proxy: func(req *http.Request) (*url.URL, error) { - proxyURL, err := url.Parse(env.Proxy) - require.NoError(t, err) - t.Logf("passing request for %s through proxy %s", req.URL, proxyURL.String()) - return proxyURL, nil - }, - } + getImpersonationProxyClient := func(caData []byte) *kubernetes.Clientset { + kubeconfig := &rest.Config{ + Host: proxyServiceURL, + TLSClientConfig: rest.TLSClientConfig{Insecure: caData == nil, CAData: caData}, + BearerToken: impersonationtoken.Make(t, env.TestUser.Token, &authenticator, env.APIGroupSuffix), + Proxy: func(req *http.Request) (*url.URL, error) { + proxyURL, err := url.Parse(env.Proxy) + require.NoError(t, err) + t.Logf("passing request for %s through proxy %s", req.URL, proxyURL.String()) + return proxyURL, nil + }, + } - impersonationProxyClient, err := kubernetes.NewForConfig(kubeconfig) - require.NoError(t, err, "unexpected failure from kubernetes.NewForConfig()") + impersonationProxyClient, err := kubernetes.NewForConfig(kubeconfig) + require.NoError(t, err, "unexpected failure from kubernetes.NewForConfig()") + return impersonationProxyClient + } oldConfigMap, err := adminClient.CoreV1().ConfigMaps(env.ConciergeNamespace).Get(ctx, impersonationProxyConfigMapName, metav1.GetOptions{}) if oldConfigMap.Data != nil { @@ -74,6 +81,8 @@ func TestImpersonationProxy(t *testing.T) { } serviceUnavailableError := fmt.Sprintf(`Get "%s/api/v1/namespaces": Service Unavailable`, proxyServiceURL) + insecureImpersonationProxyClient := getImpersonationProxyClient(nil) + if env.HasCapability(library.HasExternalLoadBalancerProvider) { // Check that load balancer has been created require.Eventually(t, func() bool { @@ -86,13 +95,13 @@ func TestImpersonationProxy(t *testing.T) { }, 10*time.Second, 500*time.Millisecond) // Check that we can't use the impersonation proxy to execute kubectl commands yet - _, err = impersonationProxyClient.CoreV1().Namespaces().List(ctx, metav1.ListOptions{}) + _, err = insecureImpersonationProxyClient.CoreV1().Namespaces().List(ctx, metav1.ListOptions{}) require.EqualError(t, err, serviceUnavailableError) // Create configuration to make the impersonation proxy turn on with a hard coded endpoint (without a LoadBalancer) configMap := configMapForConfig(t, impersonator.Config{ Mode: impersonator.ModeEnabled, - Endpoint: proxyServiceURL, + Endpoint: proxyServiceEndpoint, TLS: nil, }) t.Logf("creating configmap %s", configMap.Name) @@ -117,6 +126,16 @@ func TestImpersonationProxy(t *testing.T) { }) } + // Wait for ca data to be available at the secret location. + var caSecret *corev1.Secret + require.Eventually(t, + func() bool { + caSecret, err = adminClient.CoreV1().Secrets(env.ConciergeNamespace).Get(ctx, impersonationProxyTLSSecretName, metav1.GetOptions{}) + return caSecret != nil && caSecret.Data["ca.crt"] != nil + }, 5*time.Minute, 250*time.Millisecond) + + // Create an impersonation proxy client with that ca data. + impersonationProxyClient := getImpersonationProxyClient(caSecret.Data["ca.crt"]) t.Run( "access as user", library.AccessAsUserTest(ctx, env.TestUser.ExpectedUsername, impersonationProxyClient), @@ -296,9 +315,9 @@ func TestImpersonationProxy(t *testing.T) { require.Eventually(t, func() bool { // It's okay if this returns RBAC errors because this user has no role bindings. // What we want to see is that the proxy eventually shuts down entirely. - _, err = impersonationProxyClient.CoreV1().Namespaces().List(ctx, metav1.ListOptions{}) + _, err = insecureImpersonationProxyClient.CoreV1().Namespaces().List(ctx, metav1.ListOptions{}) return err.Error() == serviceUnavailableError - }, 10*time.Second, 500*time.Millisecond) + }, 20*time.Second, 500*time.Millisecond) if env.HasCapability(library.HasExternalLoadBalancerProvider) { // The load balancer should not exist after we disable the impersonation proxy. @@ -307,6 +326,11 @@ func TestImpersonationProxy(t *testing.T) { return !hasLoadBalancerService(ctx, t, adminClient, env.ConciergeNamespace) }, time.Minute, 500*time.Millisecond) } + + require.Eventually(t, func() bool { + caSecret, err = adminClient.CoreV1().Secrets(env.ConciergeNamespace).Get(ctx, impersonationProxyTLSSecretName, metav1.GetOptions{}) + return k8serrors.IsNotFound(err) + }, 10*time.Second, 250*time.Millisecond) } func configMapForConfig(t *testing.T, config impersonator.Config) corev1.ConfigMap {