From 8fac6cb9a400f2ef6da763b57710565a018a08d7 Mon Sep 17 00:00:00 2001 From: Monis Khan Date: Thu, 10 Mar 2022 15:26:22 -0500 Subject: [PATCH 1/5] Rework or remove tests that rely on the http port Signed-off-by: Monis Khan --- test/integration/supervisor_discovery_test.go | 154 +++++++++--------- test/integration/supervisor_healthz_test.go | 21 --- 2 files changed, 80 insertions(+), 95 deletions(-) diff --git a/test/integration/supervisor_discovery_test.go b/test/integration/supervisor_discovery_test.go index a8099296..4274d880 100644 --- a/test/integration/supervisor_discovery_test.go +++ b/test/integration/supervisor_discovery_test.go @@ -32,116 +32,122 @@ import ( "go.pinniped.dev/test/testlib" ) -// This test is intended to exercise the supervisor's HTTP port 8080. It can either access it directly via -// the env.SupervisorHTTPAddress setting, or it can access it indirectly through a TLS-enabled Ingress which -// uses the supervisor's port 8080 as its backend via the env.SupervisorHTTPSIngressAddress and -// env.SupervisorHTTPSIngressCABundle settings, or it can exercise it both ways when all of those -// env settings are present. -// -// Testing talking to the supervisor's port 8443 where the supervisor is terminating TLS itself is -// handled by the others tests in this file. +// TestSupervisorOIDCDiscovery_Disruptive is intended to exercise the supervisor's HTTPS port. +// It can either access it directly via the env.SupervisorHTTPSAddress setting, or it can access +// it indirectly through a TLS-enabled Ingress which uses the supervisor's HTTPS port as its backend +// (via the env.SupervisorHTTPSIngressAddress and env.SupervisorHTTPSIngressCABundle settings), +// or it can exercise it both ways when all of those env settings are present. // Never run this test in parallel since deleting all federation domains is disruptive, see main_test.go. func TestSupervisorOIDCDiscovery_Disruptive(t *testing.T) { env := testlib.IntegrationEnv(t) client := testlib.NewSupervisorClientset(t) + kubeClient := testlib.NewKubernetesClientset(t) ns := env.SupervisorNamespace ctx, cancel := context.WithTimeout(context.Background(), 10*time.Minute) defer cancel() + httpsAddress := env.SupervisorHTTPSAddress + var ips []net.IP + if host, _, err := net.SplitHostPort(httpsAddress); err == nil { + httpsAddress = host + } + if ip := net.ParseIP(httpsAddress); ip != nil { + ips = append(ips, ip) + } + temporarilyRemoveAllFederationDomainsAndDefaultTLSCertSecret(ctx, t, ns, defaultTLSCertSecretName(env), client, testlib.NewKubernetesClientset(t)) + defaultCA := createTLSCertificateSecret(ctx, t, ns, httpsAddress, ips, defaultTLSCertSecretName(env), kubeClient) tests := []struct { + Name string Scheme string Address string CABundle string }{ - {Scheme: "http", Address: env.SupervisorHTTPAddress}, - {Scheme: "https", Address: env.SupervisorHTTPSIngressAddress, CABundle: env.SupervisorHTTPSIngressCABundle}, + {Name: "direct https", Scheme: "https", Address: env.SupervisorHTTPSAddress, CABundle: string(defaultCA.Bundle())}, + {Name: "ingress https", Scheme: "https", Address: env.SupervisorHTTPSIngressAddress, CABundle: env.SupervisorHTTPSIngressCABundle}, } for _, test := range tests { - scheme := test.Scheme - addr := test.Address - caBundle := test.CABundle + test := test + t.Run(test.Name, func(t *testing.T) { + scheme := test.Scheme + addr := test.Address + caBundle := test.CABundle - if addr == "" { - // Both cases are not required, so when one is empty skip it. - continue - } + if addr == "" { + // Both cases are not required, so when one is empty skip it. + t.Skip("no address defined") + } - // Test that there is no default discovery endpoint available when there are no FederationDomains. - requireDiscoveryEndpointsAreNotFound(t, scheme, addr, caBundle, fmt.Sprintf("%s://%s", scheme, addr)) + // Test that there is no default discovery endpoint available when there are no FederationDomains. + requireDiscoveryEndpointsAreNotFound(t, scheme, addr, caBundle, fmt.Sprintf("%s://%s", scheme, addr)) - // Define several unique issuer strings. Always use https in the issuer name even when we are accessing the http port. - issuer1 := fmt.Sprintf("https://%s/nested/issuer1", addr) - issuer2 := fmt.Sprintf("https://%s/nested/issuer2", addr) - issuer3 := fmt.Sprintf("https://%s/issuer3", addr) - issuer4 := fmt.Sprintf("https://%s/issuer4", addr) - issuer5 := fmt.Sprintf("https://%s/issuer5", addr) - issuer6 := fmt.Sprintf("https://%s/issuer6", addr) - badIssuer := fmt.Sprintf("https://%s/badIssuer?cannot-use=queries", addr) + // Define several unique issuer strings. Always use https in the issuer name even when we are accessing the http port. + issuer1 := fmt.Sprintf("https://%s/nested/issuer1", addr) + issuer2 := fmt.Sprintf("https://%s/nested/issuer2", addr) + issuer3 := fmt.Sprintf("https://%s/issuer3", addr) + issuer4 := fmt.Sprintf("https://%s/issuer4", addr) + issuer5 := fmt.Sprintf("https://%s/issuer5", addr) + issuer6 := fmt.Sprintf("https://%s/issuer6", addr) + badIssuer := fmt.Sprintf("https://%s/badIssuer?cannot-use=queries", addr) - // When FederationDomain are created in sequence they each cause a discovery endpoint to appear only for as long as the FederationDomain exists. - config1, jwks1 := requireCreatingFederationDomainCausesDiscoveryEndpointsToAppear(ctx, t, scheme, addr, caBundle, issuer1, client) - requireDeletingFederationDomainCausesDiscoveryEndpointsToDisappear(t, config1, client, ns, scheme, addr, caBundle, issuer1) - config2, jwks2 := requireCreatingFederationDomainCausesDiscoveryEndpointsToAppear(ctx, t, scheme, addr, caBundle, issuer2, client) - requireDeletingFederationDomainCausesDiscoveryEndpointsToDisappear(t, config2, client, ns, scheme, addr, caBundle, issuer2) - // The auto-created JWK's were different from each other. - require.NotEqual(t, jwks1.Keys[0]["x"], jwks2.Keys[0]["x"]) - require.NotEqual(t, jwks1.Keys[0]["y"], jwks2.Keys[0]["y"]) + // When FederationDomain are created in sequence they each cause a discovery endpoint to appear only for as long as the FederationDomain exists. + config1, jwks1 := requireCreatingFederationDomainCausesDiscoveryEndpointsToAppear(ctx, t, scheme, addr, caBundle, issuer1, client) + requireDeletingFederationDomainCausesDiscoveryEndpointsToDisappear(t, config1, client, ns, scheme, addr, caBundle, issuer1) + config2, jwks2 := requireCreatingFederationDomainCausesDiscoveryEndpointsToAppear(ctx, t, scheme, addr, caBundle, issuer2, client) + requireDeletingFederationDomainCausesDiscoveryEndpointsToDisappear(t, config2, client, ns, scheme, addr, caBundle, issuer2) + // The auto-created JWK's were different from each other. + require.NotEqual(t, jwks1.Keys[0]["x"], jwks2.Keys[0]["x"]) + require.NotEqual(t, jwks1.Keys[0]["y"], jwks2.Keys[0]["y"]) - // When multiple FederationDomains exist at the same time they each serve a unique discovery endpoint. - config3, jwks3 := requireCreatingFederationDomainCausesDiscoveryEndpointsToAppear(ctx, t, scheme, addr, caBundle, issuer3, client) - config4, jwks4 := requireCreatingFederationDomainCausesDiscoveryEndpointsToAppear(ctx, t, scheme, addr, caBundle, issuer4, client) - requireDiscoveryEndpointsAreWorking(t, scheme, addr, caBundle, issuer3, nil) // discovery for issuer3 is still working after issuer4 started working - // The auto-created JWK's were different from each other. - require.NotEqual(t, jwks3.Keys[0]["x"], jwks4.Keys[0]["x"]) - require.NotEqual(t, jwks3.Keys[0]["y"], jwks4.Keys[0]["y"]) + // When multiple FederationDomains exist at the same time they each serve a unique discovery endpoint. + config3, jwks3 := requireCreatingFederationDomainCausesDiscoveryEndpointsToAppear(ctx, t, scheme, addr, caBundle, issuer3, client) + config4, jwks4 := requireCreatingFederationDomainCausesDiscoveryEndpointsToAppear(ctx, t, scheme, addr, caBundle, issuer4, client) + requireDiscoveryEndpointsAreWorking(t, scheme, addr, caBundle, issuer3, nil) // discovery for issuer3 is still working after issuer4 started working + // The auto-created JWK's were different from each other. + require.NotEqual(t, jwks3.Keys[0]["x"], jwks4.Keys[0]["x"]) + require.NotEqual(t, jwks3.Keys[0]["y"], jwks4.Keys[0]["y"]) - // Editing a provider to change the issuer name updates the endpoints that are being served. - updatedConfig4 := editFederationDomainIssuerName(t, config4, client, ns, issuer5) - requireDiscoveryEndpointsAreNotFound(t, scheme, addr, caBundle, issuer4) - jwks5 := requireDiscoveryEndpointsAreWorking(t, scheme, addr, caBundle, issuer5, nil) - // The JWK did not change when the issuer name was updated. - require.Equal(t, jwks4.Keys[0], jwks5.Keys[0]) + // Editing a provider to change the issuer name updates the endpoints that are being served. + updatedConfig4 := editFederationDomainIssuerName(t, config4, client, ns, issuer5) + requireDiscoveryEndpointsAreNotFound(t, scheme, addr, caBundle, issuer4) + jwks5 := requireDiscoveryEndpointsAreWorking(t, scheme, addr, caBundle, issuer5, nil) + // The JWK did not change when the issuer name was updated. + require.Equal(t, jwks4.Keys[0], jwks5.Keys[0]) - // When they are deleted they stop serving discovery endpoints. - requireDeletingFederationDomainCausesDiscoveryEndpointsToDisappear(t, config3, client, ns, scheme, addr, caBundle, issuer3) - requireDeletingFederationDomainCausesDiscoveryEndpointsToDisappear(t, updatedConfig4, client, ns, scheme, addr, caBundle, issuer5) + // When they are deleted they stop serving discovery endpoints. + requireDeletingFederationDomainCausesDiscoveryEndpointsToDisappear(t, config3, client, ns, scheme, addr, caBundle, issuer3) + requireDeletingFederationDomainCausesDiscoveryEndpointsToDisappear(t, updatedConfig4, client, ns, scheme, addr, caBundle, issuer5) - // When the same issuer is added twice, both issuers are marked as duplicates, and neither provider is serving. - config6Duplicate1, _ := requireCreatingFederationDomainCausesDiscoveryEndpointsToAppear(ctx, t, scheme, addr, caBundle, issuer6, client) - config6Duplicate2 := testlib.CreateTestFederationDomain(ctx, t, issuer6, "", "") - requireStatus(t, client, ns, config6Duplicate1.Name, v1alpha1.DuplicateFederationDomainStatusCondition) - requireStatus(t, client, ns, config6Duplicate2.Name, v1alpha1.DuplicateFederationDomainStatusCondition) - requireDiscoveryEndpointsAreNotFound(t, scheme, addr, caBundle, issuer6) + // When the same issuer is added twice, both issuers are marked as duplicates, and neither provider is serving. + config6Duplicate1, _ := requireCreatingFederationDomainCausesDiscoveryEndpointsToAppear(ctx, t, scheme, addr, caBundle, issuer6, client) + config6Duplicate2 := testlib.CreateTestFederationDomain(ctx, t, issuer6, "", "") + requireStatus(t, client, ns, config6Duplicate1.Name, v1alpha1.DuplicateFederationDomainStatusCondition) + requireStatus(t, client, ns, config6Duplicate2.Name, v1alpha1.DuplicateFederationDomainStatusCondition) + requireDiscoveryEndpointsAreNotFound(t, scheme, addr, caBundle, issuer6) - // If we delete the first duplicate issuer, the second duplicate issuer starts serving. - requireDelete(t, client, ns, config6Duplicate1.Name) - requireWellKnownEndpointIsWorking(t, scheme, addr, caBundle, issuer6, nil) - requireStatus(t, client, ns, config6Duplicate2.Name, v1alpha1.SuccessFederationDomainStatusCondition) + // If we delete the first duplicate issuer, the second duplicate issuer starts serving. + requireDelete(t, client, ns, config6Duplicate1.Name) + requireWellKnownEndpointIsWorking(t, scheme, addr, caBundle, issuer6, nil) + requireStatus(t, client, ns, config6Duplicate2.Name, v1alpha1.SuccessFederationDomainStatusCondition) - // When we finally delete all issuers, the endpoint should be down. - requireDeletingFederationDomainCausesDiscoveryEndpointsToDisappear(t, config6Duplicate2, client, ns, scheme, addr, caBundle, issuer6) + // When we finally delete all issuers, the endpoint should be down. + requireDeletingFederationDomainCausesDiscoveryEndpointsToDisappear(t, config6Duplicate2, client, ns, scheme, addr, caBundle, issuer6) - // Only test this for http endpoints because https endpoints are going through an Ingress, - // and while it is possible to configure an Ingress to serve multiple hostnames with matching TLS certs - // for each hostname, that it not something that we felt like doing on all of our clusters that we - // run tests against. :) - if scheme == "http" { // "Host" headers can be used to send requests to discovery endpoints when the public address is different from the issuer name. issuer7 := "https://some-issuer-host-and-port-that-doesnt-match-public-supervisor-address.com:2684/issuer7" config7, _ := requireCreatingFederationDomainCausesDiscoveryEndpointsToAppear(ctx, t, scheme, addr, caBundle, issuer7, client) requireDeletingFederationDomainCausesDiscoveryEndpointsToDisappear(t, config7, client, ns, scheme, addr, caBundle, issuer7) - } - // When we create a provider with an invalid issuer, the status is set to invalid. - badConfig := testlib.CreateTestFederationDomain(ctx, t, badIssuer, "", "") - requireStatus(t, client, ns, badConfig.Name, v1alpha1.InvalidFederationDomainStatusCondition) - requireDiscoveryEndpointsAreNotFound(t, scheme, addr, caBundle, badIssuer) - requireDeletingFederationDomainCausesDiscoveryEndpointsToDisappear(t, badConfig, client, ns, scheme, addr, caBundle, badIssuer) + // When we create a provider with an invalid issuer, the status is set to invalid. + badConfig := testlib.CreateTestFederationDomain(ctx, t, badIssuer, "", "") + requireStatus(t, client, ns, badConfig.Name, v1alpha1.InvalidFederationDomainStatusCondition) + requireDiscoveryEndpointsAreNotFound(t, scheme, addr, caBundle, badIssuer) + requireDeletingFederationDomainCausesDiscoveryEndpointsToDisappear(t, badConfig, client, ns, scheme, addr, caBundle, badIssuer) + }) } } diff --git a/test/integration/supervisor_healthz_test.go b/test/integration/supervisor_healthz_test.go index 637dd635..8e4519af 100644 --- a/test/integration/supervisor_healthz_test.go +++ b/test/integration/supervisor_healthz_test.go @@ -17,27 +17,6 @@ import ( "go.pinniped.dev/test/testlib" ) -// The Supervisor health endpoint is public because that makes it easier -// for users to create an Ingress for the supervisor on platforms like -// GKE where the Ingress wants to perform a health check. It's somewhere -// between inconvenient and impossible to make that Ingress health check -// happen on a private container port at this time. -// This test checks that it is working and that it is public. -func TestSupervisorHealthz(t *testing.T) { - env := testlib.IntegrationEnv(t) - - if env.SupervisorHTTPAddress == "" { - t.Skip("PINNIPED_TEST_SUPERVISOR_HTTP_ADDRESS not defined") - } - - ctx, cancel := context.WithTimeout(context.Background(), 2*time.Minute) - defer cancel() - - httpClient := &http.Client{} - - httpGet(ctx, t, httpClient, fmt.Sprintf("http://%s/healthz", env.SupervisorHTTPAddress), http.StatusOK, "ok") -} - // Never run this test in parallel since deleting all federation domains and the default TLS secret is disruptive, see main_test.go. func TestSupervisorHealthzBootstrap_Disruptive(t *testing.T) { env := testlib.IntegrationEnv(t) From 16c4c67af1bbd0bab91ef32ac22a5f925a1aed15 Mon Sep 17 00:00:00 2001 From: Ryan Richard Date: Tue, 15 Mar 2022 09:37:27 -0700 Subject: [PATCH 2/5] Use kubeadm.k8s.io/v1beta3 instead of v1beta2 for kind config It appears that kind completely ignores kubeadm.k8s.io/v1beta2 config starting in Kind v0.12.0. You can observe the config being ignored or used by adding `-v 10` to the command-line arguments of `kind create cluster` in kind-up.sh. --- hack/lib/kind-config/single-node.yaml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/hack/lib/kind-config/single-node.yaml b/hack/lib/kind-config/single-node.yaml index 0031fbd7..2b56ffa3 100644 --- a/hack/lib/kind-config/single-node.yaml +++ b/hack/lib/kind-config/single-node.yaml @@ -26,7 +26,7 @@ nodes: listenAddress: 127.0.0.1 kubeadmConfigPatches: - | - apiVersion: kubeadm.k8s.io/v1beta2 + apiVersion: kubeadm.k8s.io/v1beta3 kind: ClusterConfiguration apiServer: extraArgs: From e465056943e4f0def5111b9ea2fac79c329e136e Mon Sep 17 00:00:00 2001 From: Ryan Richard Date: Tue, 15 Mar 2022 15:09:39 -0700 Subject: [PATCH 3/5] Use both kubeadm.k8s.io/v1beta2 and v1beta3 to allow old versions of K8s You can use an older version of K8s on your development workstation by temporarily editing kind-up.sh to add the `--image` flag. By defining both v1beta2 and v1beta3 you should continue to be able to use old versions of K8s in this way with Kind v0.12.0. --- hack/kind-up.sh | 3 ++- hack/lib/kind-config/single-node.yaml | 14 +++++++++++++- 2 files changed, 15 insertions(+), 2 deletions(-) diff --git a/hack/kind-up.sh b/hack/kind-up.sh index 64fdacc6..c60eb4c7 100755 --- a/hack/kind-up.sh +++ b/hack/kind-up.sh @@ -1,6 +1,6 @@ #!/usr/bin/env bash -# Copyright 2020 the Pinniped contributors. All Rights Reserved. +# Copyright 2020-2022 the Pinniped contributors. All Rights Reserved. # SPDX-License-Identifier: Apache-2.0 set -euo pipefail @@ -9,4 +9,5 @@ ROOT="$( cd "$( dirname "${BASH_SOURCE[0]}" )/.." && pwd )" cd "${ROOT}" # To choose a specific version of kube, add this option to the command below: `--image kindest/node:v1.18.8`. +# To debug the kind config, add this option to the command below: `-v 10` kind create cluster --config "hack/lib/kind-config/single-node.yaml" --name pinniped diff --git a/hack/lib/kind-config/single-node.yaml b/hack/lib/kind-config/single-node.yaml index 2b56ffa3..ac90bfcb 100644 --- a/hack/lib/kind-config/single-node.yaml +++ b/hack/lib/kind-config/single-node.yaml @@ -24,9 +24,14 @@ nodes: containerPort: 31235 hostPort: 12346 listenAddress: 127.0.0.1 +# Kind v0.12.0 ignores kubeadm.k8s.io/v1beta2 for Kube v1.23+ but uses it for older versions of Kube. +# Previous versions of Kind would use kubeadm.k8s.io/v1beta2 for all versions of Kube including 1.23. +# To try to maximize compatibility with various versions of Kind and Kube, define this +# ClusterConfiguration twice and hope that Kind will use the one that it likes for the given version +# of Kube, and ignore the one that it doesn't like. This seems to work, at least for Kind v0.12.0. kubeadmConfigPatches: - | - apiVersion: kubeadm.k8s.io/v1beta3 + apiVersion: kubeadm.k8s.io/v1beta2 kind: ClusterConfiguration apiServer: extraArgs: @@ -39,3 +44,10 @@ kubeadmConfigPatches: # are exercised. For whatever reason, leaving this as false (i.e. use kube-proxy) appears to # hide some network misconfigurations when used internally by the API server aggregation layer. enable-aggregator-routing: "true" +- | + apiVersion: kubeadm.k8s.io/v1beta3 + kind: ClusterConfiguration + apiServer: + extraArgs: + # See comment above. + enable-aggregator-routing: "true" From 305276302074abae6b30e0f7e8d155b43af7c468 Mon Sep 17 00:00:00 2001 From: anjalitelang <49958114+anjaltelang@users.noreply.github.com> Date: Thu, 17 Mar 2022 09:36:24 -0400 Subject: [PATCH 4/5] Update ROADMAP.md Updated roadmap with current priorities --- ROADMAP.md | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/ROADMAP.md b/ROADMAP.md index 1a270133..9861f9ab 100644 --- a/ROADMAP.md +++ b/ROADMAP.md @@ -36,12 +36,11 @@ The following table includes the current roadmap for Pinniped. If you have any q Last Updated: Jan 2022 |Theme|Description|Timeline| |--|--|--| -|Improving Security Posture|Support for refreshing LDAP/AD Group information |March 2022| |Improving Security Posture|Support FIPS compliant Boring crypto libraries |March/April 2022| -|Multiple IDP support|Support multiple IDPs configured on a single Supervisor|April 2022| |Improving Security Posture|TLS hardening |March/April 2022| |Improving Security Posture|Support Audit logging of security events related to Authentication |April/May 2022| |Improving Usability|Support for integrating with UI/Dashboards |June/July 2022| +|Multiple IDP support|Support multiple IDPs configured on a single Supervisor|Exploring/Ongoing| |Improving Security Posture|mTLS for Supervisor sessions |Exploring/Ongoing| |Improving Security Posture|Key management/rotation for Pinniped components with minimal downtime |Exploring/Ongoing| |Improving Security Posture|Support for Session Logout |Exploring/Ongoing| From c710cfbc701193549f88890d3fee1cd287b50fa5 Mon Sep 17 00:00:00 2001 From: anjalitelang <49958114+anjaltelang@users.noreply.github.com> Date: Fri, 18 Mar 2022 10:12:50 -0400 Subject: [PATCH 5/5] Update ROADMAP.md Changed last updated field for March --- ROADMAP.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ROADMAP.md b/ROADMAP.md index 9861f9ab..8f0e5f33 100644 --- a/ROADMAP.md +++ b/ROADMAP.md @@ -33,7 +33,7 @@ The following table includes the current roadmap for Pinniped. If you have any q -Last Updated: Jan 2022 +Last Updated: March 2022 |Theme|Description|Timeline| |--|--|--| |Improving Security Posture|Support FIPS compliant Boring crypto libraries |March/April 2022|