From cd686ffdf31ae2a5f7241cd8d3ddf304c0684a19 Mon Sep 17 00:00:00 2001 From: Monis Khan Date: Wed, 20 Oct 2021 07:59:24 -0400 Subject: [PATCH] Force the use of secure TLS config This change updates the TLS config used by all pinniped components. There are no configuration knobs associated with this change. Thus this change tightens our static defaults. There are four TLS config levels: 1. Secure (TLS 1.3 only) 2. Default (TLS 1.2+ best ciphers that are well supported) 3. Default LDAP (TLS 1.2+ with less good ciphers) 4. Legacy (currently unused, TLS 1.2+ with all non-broken ciphers) Highlights per component: 1. pinniped CLI - uses "secure" config against KAS - uses "default" for all other connections 2. concierge - uses "secure" config as an aggregated API server - uses "default" config as a impersonation proxy API server - uses "secure" config against KAS - uses "default" config for JWT authenticater (mostly, see code) - no changes to webhook authenticater (see code) 3. supervisor - uses "default" config as a server - uses "secure" config against KAS - uses "default" config against OIDC IDPs - uses "default LDAP" config against LDAP IDPs Signed-off-by: Monis Khan --- CONTRIBUTING.md | 3 +- cmd/pinniped/cmd/kubeconfig.go | 31 +- cmd/pinniped/cmd/login_oidc.go | 20 +- hack/prepare-for-integration-tests.sh | 7 + .../concierge/impersonator/impersonator.go | 88 ++++- .../impersonator/impersonator_test.go | 70 ++-- internal/concierge/server/server.go | 11 +- .../controller/authenticator/authenticator.go | 15 +- .../jwtcachefiller/jwtcachefiller.go | 44 ++- .../jwtcachefiller/jwtcachefiller_test.go | 15 +- .../webhookcachefiller/webhookcachefiller.go | 4 +- .../webhookcachefiller_test.go | 2 +- .../kubecertagent/pod_command_executor.go | 2 + .../pod_command_executor_test.go | 44 +++ .../oidc_upstream_watcher.go | 33 +- .../oidc_upstream_watcher_test.go | 20 +- internal/crypto/ptls/old.go | 15 + internal/crypto/ptls/ptls.go | 215 +++++++++++ internal/crypto/ptls/ptls_test.go | 253 +++++++++++++ internal/dynamiccert/provider_test.go | 8 +- .../httputil/roundtripper/roundtripper.go | 25 +- internal/kubeclient/kubeclient.go | 157 +++++++- internal/kubeclient/kubeclient_test.go | 317 ++++++++++++++-- internal/kubeclient/roundtrip.go | 5 +- .../localuserauthenticator.go | 18 +- .../localuserauthenticator_test.go | 29 +- internal/net/phttp/debug.go | 118 ++++++ internal/net/phttp/debug_test.go | 340 ++++++++++++++++++ internal/net/phttp/phttp.go | 48 +++ internal/net/phttp/phttp_test.go | 116 ++++++ .../downstreamsession/downstream_session.go | 6 +- internal/plog/klog.go | 30 +- internal/plog/level.go | 3 +- internal/plog/level_test.go | 12 + internal/plog/plog.go | 62 ++-- internal/supervisor/server/server.go | 32 +- internal/testutil/fakekubeapi/fakekubeapi.go | 11 +- internal/testutil/tlsserver.go | 27 +- internal/testutil/tlsserver/tlsserver.go | 110 ++++++ internal/upstreamldap/upstreamldap.go | 3 +- internal/upstreamldap/upstreamldap_test.go | 33 +- pkg/oidcclient/login.go | 3 +- test/integration/cli_test.go | 9 +- .../concierge_credentialrequest_test.go | 6 +- .../concierge_impersonation_proxy_test.go | 67 ++-- test/integration/ldap_client_test.go | 2 - test/integration/securetls_test.go | 257 +++++++++++++ test/testlib/client.go | 2 +- 48 files changed, 2431 insertions(+), 317 deletions(-) create mode 100644 internal/controller/kubecertagent/pod_command_executor_test.go create mode 100644 internal/crypto/ptls/old.go create mode 100644 internal/crypto/ptls/ptls.go create mode 100644 internal/crypto/ptls/ptls_test.go create mode 100644 internal/net/phttp/debug.go create mode 100644 internal/net/phttp/debug_test.go create mode 100644 internal/net/phttp/phttp.go create mode 100644 internal/net/phttp/phttp_test.go create mode 100644 internal/testutil/tlsserver/tlsserver.go create mode 100644 test/integration/securetls_test.go diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 8633c6ec..39bd6e94 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -114,11 +114,12 @@ docker build . - [`kind`](https://kind.sigs.k8s.io/docs/user/quick-start) - [`kubectl`](https://kubernetes.io/docs/tasks/tools/install-kubectl/) - [`ytt`](https://carvel.dev/#getting-started) + - [`nmap`](https://nmap.org/download.html) On macOS, these tools can be installed with [Homebrew](https://brew.sh/) (assuming you have Chrome installed already): ```bash - brew install kind k14s/tap/ytt k14s/tap/kapp kubectl chromedriver && brew cask install docker + brew install kind k14s/tap/ytt k14s/tap/kapp kubectl chromedriver nmap && brew cask install docker ``` 1. Create a kind cluster, compile, create container images, and install Pinniped and supporting dependencies using: diff --git a/cmd/pinniped/cmd/kubeconfig.go b/cmd/pinniped/cmd/kubeconfig.go index 7f04e6e5..013c0166 100644 --- a/cmd/pinniped/cmd/kubeconfig.go +++ b/cmd/pinniped/cmd/kubeconfig.go @@ -5,7 +5,6 @@ package cmd import ( "context" - "crypto/tls" "crypto/x509" "encoding/base64" "encoding/json" @@ -28,13 +27,13 @@ import ( _ "k8s.io/client-go/plugin/pkg/client/auth" // Adds handlers for various dynamic auth plugins in client-go "k8s.io/client-go/tools/clientcmd" clientcmdapi "k8s.io/client-go/tools/clientcmd/api" - "k8s.io/client-go/transport" conciergev1alpha1 "go.pinniped.dev/generated/latest/apis/concierge/authentication/v1alpha1" configv1alpha1 "go.pinniped.dev/generated/latest/apis/concierge/config/v1alpha1" idpdiscoveryv1alpha1 "go.pinniped.dev/generated/latest/apis/supervisor/idpdiscovery/v1alpha1" conciergeclientset "go.pinniped.dev/generated/latest/client/concierge/clientset/versioned" "go.pinniped.dev/internal/groupsuffix" + "go.pinniped.dev/internal/net/phttp" ) type kubeconfigDeps struct { @@ -664,17 +663,8 @@ func validateKubeconfig(ctx context.Context, flags getKubeconfigParams, kubeconf return fmt.Errorf("invalid kubeconfig (no certificateAuthorityData)") } - httpClient := &http.Client{ - Transport: &http.Transport{ - TLSClientConfig: &tls.Config{ - MinVersion: tls.VersionTLS12, - RootCAs: kubeconfigCA, - }, - Proxy: http.ProxyFromEnvironment, - TLSHandshakeTimeout: 10 * time.Second, - }, - Timeout: 10 * time.Second, - } + httpClient := phttp.Default(kubeconfigCA) + httpClient.Timeout = 10 * time.Second ticker := time.NewTicker(2 * time.Second) defer ticker.Stop() @@ -781,21 +771,14 @@ func discoverSupervisorUpstreamIDP(ctx context.Context, flags *getKubeconfigPara } func newDiscoveryHTTPClient(caBundleFlag caBundleFlag) (*http.Client, error) { - t := &http.Transport{ - TLSClientConfig: &tls.Config{MinVersion: tls.VersionTLS12}, - Proxy: http.ProxyFromEnvironment, - } - httpClient := &http.Client{Transport: t} + var rootCAs *x509.CertPool if caBundleFlag != nil { - rootCAs := x509.NewCertPool() - ok := rootCAs.AppendCertsFromPEM(caBundleFlag) - if !ok { + rootCAs = x509.NewCertPool() + if ok := rootCAs.AppendCertsFromPEM(caBundleFlag); !ok { return nil, fmt.Errorf("unable to fetch OIDC discovery data from issuer: could not parse CA bundle") } - t.TLSClientConfig.RootCAs = rootCAs } - httpClient.Transport = transport.DebugWrappers(httpClient.Transport) - return httpClient, nil + return phttp.Default(rootCAs), nil } func discoverIDPsDiscoveryEndpointURL(ctx context.Context, issuer string, httpClient *http.Client) (string, error) { diff --git a/cmd/pinniped/cmd/login_oidc.go b/cmd/pinniped/cmd/login_oidc.go index 9a050f59..c8b2b0cc 100644 --- a/cmd/pinniped/cmd/login_oidc.go +++ b/cmd/pinniped/cmd/login_oidc.go @@ -5,7 +5,6 @@ package cmd import ( "context" - "crypto/tls" "crypto/x509" "encoding/base64" "encoding/json" @@ -21,12 +20,12 @@ import ( "github.com/spf13/cobra" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" clientauthv1beta1 "k8s.io/client-go/pkg/apis/clientauthentication/v1beta1" - "k8s.io/client-go/transport" "k8s.io/klog/v2/klogr" idpdiscoveryv1alpha1 "go.pinniped.dev/generated/latest/apis/supervisor/idpdiscovery/v1alpha1" "go.pinniped.dev/internal/execcredcache" "go.pinniped.dev/internal/groupsuffix" + "go.pinniped.dev/internal/net/phttp" "go.pinniped.dev/internal/plog" "go.pinniped.dev/pkg/conciergeclient" "go.pinniped.dev/pkg/oidcclient" @@ -308,18 +307,7 @@ func makeClient(caBundlePaths []string, caBundleData []string) (*http.Client, er } pool.AppendCertsFromPEM(pem) } - client := &http.Client{ - Transport: &http.Transport{ - Proxy: http.ProxyFromEnvironment, - TLSClientConfig: &tls.Config{ - RootCAs: pool, - MinVersion: tls.VersionTLS12, - }, - }, - } - - client.Transport = transport.DebugWrappers(client.Transport) - return client, nil + return phttp.Default(pool), nil } func tokenCredential(token *oidctypes.Token) *clientauthv1beta1.ExecCredential { @@ -338,7 +326,7 @@ func tokenCredential(token *oidctypes.Token) *clientauthv1beta1.ExecCredential { return &cred } -func SetLogLevel(lookupEnv func(string) (string, bool)) (*plog.PLogger, error) { +func SetLogLevel(lookupEnv func(string) (string, bool)) (plog.Logger, error) { debug, _ := lookupEnv("PINNIPED_DEBUG") if debug == "true" { err := plog.ValidateAndSetLogLevelGlobally(plog.LevelDebug) @@ -347,7 +335,7 @@ func SetLogLevel(lookupEnv func(string) (string, bool)) (*plog.PLogger, error) { } } logger := plog.New("Pinniped login: ") - return &logger, nil + return logger, nil } // mustGetConfigDir returns a directory that follows the XDG base directory convention: diff --git a/hack/prepare-for-integration-tests.sh b/hack/prepare-for-integration-tests.sh index 8258e4d2..c54bdad6 100755 --- a/hack/prepare-for-integration-tests.sh +++ b/hack/prepare-for-integration-tests.sh @@ -132,6 +132,7 @@ check_dependency kubectl "Please install kubectl. e.g. 'brew install kubectl' fo check_dependency htpasswd "Please install htpasswd. Should be pre-installed on MacOS. Usually found in 'apache2-utils' package for linux." check_dependency openssl "Please install openssl. Should be pre-installed on MacOS." check_dependency chromedriver "Please install chromedriver. e.g. 'brew install chromedriver' for MacOS" +check_dependency nmap "Please install nmap. e.g. 'brew install nmap' for MacOS" # Check that Chrome and chromedriver versions match. If chromedriver falls a couple versions behind # then usually tests start to fail with strange error messages. @@ -158,6 +159,12 @@ if [ "$(kubectl version --client=true --short | cut -d '.' -f 2)" -lt 18 ]; then exit 1 fi +# Require nmap >= 7.92.x +if [ "$(nmap -V | grep 'Nmap version' | cut -d ' ' -f 3 | cut -d '.' -f 2)" -lt 92 ]; then + log_error "nmap >= 7.92.x is required, you have $(nmap -V | grep 'Nmap version' | cut -d ' ' -f 3)" + exit 1 +fi + if [[ "$clean_kind" == "yes" ]]; then log_note "Deleting running kind cluster to prepare from a clean slate..." ./hack/kind-down.sh diff --git a/internal/concierge/impersonator/impersonator.go b/internal/concierge/impersonator/impersonator.go index c482511e..4e47d880 100644 --- a/internal/concierge/impersonator/impersonator.go +++ b/internal/concierge/impersonator/impersonator.go @@ -7,6 +7,7 @@ import ( "context" "encoding/json" "fmt" + "io" "net" "net/http" "net/http/httputil" @@ -14,6 +15,7 @@ import ( "os" "regexp" "strings" + "sync" "time" authenticationv1 "k8s.io/api/authentication/v1" @@ -47,6 +49,7 @@ import ( "k8s.io/client-go/transport" "go.pinniped.dev/internal/constable" + "go.pinniped.dev/internal/crypto/ptls" "go.pinniped.dev/internal/dynamiccert" "go.pinniped.dev/internal/httputil/securityheader" "go.pinniped.dev/internal/kubeclient" @@ -70,13 +73,14 @@ func New( dynamicCertProvider dynamiccert.Private, impersonationProxySignerCA dynamiccert.Public, ) (func(stopCh <-chan struct{}) error, error) { - return newInternal(port, dynamicCertProvider, impersonationProxySignerCA, nil, nil, nil) + return newInternal(port, dynamicCertProvider, impersonationProxySignerCA, kubeclient.Secure, nil, nil, nil) } func newInternal( //nolint:funlen // yeah, it's kind of long. port int, dynamicCertProvider dynamiccert.Private, impersonationProxySignerCA dynamiccert.Public, + restConfigFunc ptls.RestConfigFunc, // for unit testing, should always be kubeclient.Secure in production clientOpts []kubeclient.Option, // for unit testing, should always be nil in production recOpts func(*genericoptions.RecommendedOptions), // for unit testing, should always be nil in production recConfig func(*genericapiserver.RecommendedConfig), // for unit testing, should always be nil in production @@ -100,6 +104,13 @@ func newInternal( //nolint:funlen // yeah, it's kind of long. recommendedOptions.SecureServing.ServerCert.GeneratedCert = dynamicCertProvider // serving certs (end user facing) recommendedOptions.SecureServing.BindPort = port + // secure TLS for connections coming from external clients and going to the Kube API server + // this is best effort because not all options provide the right hooks to override TLS config + // since any client could connect to the impersonation proxy, this uses the default TLS config + if err := ptls.DefaultRecommendedOptions(recommendedOptions, restConfigFunc); err != nil { + return nil, fmt.Errorf("failed to secure recommended options: %w", err) + } + // Wire up the impersonation proxy signer CA as another valid authenticator for client cert auth, // along with the Kube API server's CA. // Note: any changes to the Authentication stack need to be kept in sync with any assumptions made @@ -258,7 +269,7 @@ func newInternal( //nolint:funlen // yeah, it's kind of long. delegatingAuthorizer := serverConfig.Authorization.Authorizer customReasonAuthorizer := &comparableAuthorizer{ - authorizerFunc: func(ctx context.Context, a authorizer.Attributes) (authorizer.Decision, string, error) { + AuthorizerFunc: func(ctx context.Context, a authorizer.Attributes) (authorizer.Decision, string, error) { const baseReason = "decision made by impersonation-proxy.concierge.pinniped.dev" switch a.GetVerb() { case "": @@ -347,14 +358,14 @@ func getReverseProxyClient(clientOpts []kubeclient.Option) (*kubeclient.Client, if err != nil { return nil, err } - impersonationProxyRestConfig = rest.AnonymousClientConfig(impersonationProxyRestConfig) + impersonationProxyRestConfig = kubeclient.SecureAnonymousClientConfig(impersonationProxyRestConfig) impersonationProxyRestConfig.BearerTokenFile = tokenFile return kubeclient.New(kubeclient.WithConfig(impersonationProxyRestConfig)) } func isAnonymousAuthEnabled(config *rest.Config) (bool, error) { - anonymousConfig := rest.AnonymousClientConfig(config) + anonymousConfig := kubeclient.SecureAnonymousClientConfig(config) // we do not need either of these but RESTClientFor complains if they are not set anonymousConfig.GroupVersion = &schema.GroupVersion{} @@ -413,14 +424,7 @@ type comparableAuthenticator struct { // No-op wrapping around AuthorizerFunc to allow for comparisons. type comparableAuthorizer struct { - authorizerFunc -} - -// TODO: delete when we pick up https://github.com/kubernetes/kubernetes/pull/100963 -type authorizerFunc func(ctx context.Context, a authorizer.Attributes) (authorizer.Decision, string, error) - -func (f authorizerFunc) Authorize(ctx context.Context, a authorizer.Attributes) (authorizer.Decision, string, error) { - return f(ctx, a) + authorizer.AuthorizerFunc } func withBearerTokenPreservation(delegate http.Handler) http.Handler { @@ -462,16 +466,16 @@ func newImpersonationReverseProxyFunc(restConfig *rest.Config) (func(*genericapi if err != nil { return nil, fmt.Errorf("could not get http/1.1 round tripper: %w", err) } - http1RoundTripperAnonymous, err := getTransportForProtocol(rest.AnonymousClientConfig(restConfig), "http/1.1") + http1RoundTripperAnonymous, err := getTransportForProtocol(kubeclient.SecureAnonymousClientConfig(restConfig), "http/1.1") if err != nil { return nil, fmt.Errorf("could not get http/1.1 anonymous round tripper: %w", err) } - http2RoundTripper, err := getTransportForProtocol(restConfig, "h2") + http2RoundTripper, err := getTransportForProtocol(restConfig, "h2") // TODO figure out why this leads to still supporting http1 if err != nil { return nil, fmt.Errorf("could not get http/2.0 round tripper: %w", err) } - http2RoundTripperAnonymous, err := getTransportForProtocol(rest.AnonymousClientConfig(restConfig), "h2") + http2RoundTripperAnonymous, err := getTransportForProtocol(kubeclient.SecureAnonymousClientConfig(restConfig), "h2") if err != nil { return nil, fmt.Errorf("could not get http/2.0 anonymous round tripper: %w", err) } @@ -565,6 +569,12 @@ func newImpersonationReverseProxyFunc(restConfig *rest.Config) (func(*genericapi r.Header.Del("X-Forwarded-For") } + // the http2 code seems to call Close concurrently which can lead to data races + if r.Body != nil { + r = utilnet.CloneRequest(r) + r.Body = &safeReadWriteCloser{rc: r.Body} + } + reverseProxy := httputil.NewSingleHostReverseProxy(serverURL) reverseProxy.Transport = rt reverseProxy.FlushInterval = 200 * time.Millisecond // the "watch" verb will not work without this line @@ -573,6 +583,43 @@ func newImpersonationReverseProxyFunc(restConfig *rest.Config) (func(*genericapi }, nil } +var _ io.ReadWriteCloser = &safeReadWriteCloser{} + +type safeReadWriteCloser struct { + m sync.Mutex // all methods allowed concurrently, this only guards concurrent calls to Close + + rc io.ReadCloser + + once sync.Once // set up rwc and writeErr + rwc io.ReadWriteCloser + writeErr error +} + +func (r *safeReadWriteCloser) Read(p []byte) (int, error) { + return r.rc.Read(p) +} + +func (r *safeReadWriteCloser) Write(p []byte) (int, error) { + r.once.Do(func() { + var ok bool + r.rwc, ok = r.rc.(io.ReadWriteCloser) + if !ok { // this method should only be caused during flows that switch protocols + r.writeErr = fmt.Errorf("switching protocols failed: io.ReadCloser %T is not io.ReadWriteCloser", r.rc) + } + }) + if r.writeErr != nil { + return 0, r.writeErr + } + return r.rwc.Write(p) +} + +func (r *safeReadWriteCloser) Close() error { + r.m.Lock() + defer r.m.Unlock() + + return r.rc.Close() +} + func ensureNoImpersonationHeaders(r *http.Request) error { for key := range r.Header { // even though we have unit tests that try to cover this case, it is hard to tell if Go does @@ -759,5 +806,14 @@ func getTransportForProtocol(restConfig *rest.Config, protocol string) (http.Rou } transportConfig.TLS.NextProtos = []string{protocol} - return transport.New(transportConfig) + rt, err := transport.New(transportConfig) + if err != nil { + return nil, fmt.Errorf("could not build transport: %w", err) + } + + if err := kubeclient.AssertSecureTransport(rt); err != nil { + return nil, err // make sure we only use a secure TLS config + } + + return rt, nil } diff --git a/internal/concierge/impersonator/impersonator_test.go b/internal/concierge/impersonator/impersonator_test.go index b67b3733..e29d5359 100644 --- a/internal/concierge/impersonator/impersonator_test.go +++ b/internal/concierge/impersonator/impersonator_test.go @@ -36,6 +36,7 @@ import ( genericapiserver "k8s.io/apiserver/pkg/server" genericoptions "k8s.io/apiserver/pkg/server/options" utilfeature "k8s.io/apiserver/pkg/util/feature" + "k8s.io/client-go/kubernetes" "k8s.io/client-go/rest" "k8s.io/client-go/tools/clientcmd/api" featuregatetesting "k8s.io/component-base/featuregate/testing" @@ -44,12 +45,13 @@ import ( loginv1alpha1 "go.pinniped.dev/generated/latest/apis/concierge/login/v1alpha1" "go.pinniped.dev/internal/certauthority" "go.pinniped.dev/internal/constable" + "go.pinniped.dev/internal/crypto/ptls" "go.pinniped.dev/internal/dynamiccert" "go.pinniped.dev/internal/groupsuffix" "go.pinniped.dev/internal/here" "go.pinniped.dev/internal/httputil/roundtripper" "go.pinniped.dev/internal/kubeclient" - "go.pinniped.dev/internal/testutil" + "go.pinniped.dev/internal/testutil/tlsserver" ) func TestImpersonator(t *testing.T) { @@ -697,7 +699,9 @@ func TestImpersonator(t *testing.T) { // will proxy incoming calls to this fake server. testKubeAPIServerWasCalled := false var testKubeAPIServerSawHeaders http.Header - testKubeAPIServerCA, testKubeAPIServerURL := testutil.TLSTestServer(t, func(w http.ResponseWriter, r *http.Request) { + testKubeAPIServer := tlsserver.TLSTestServer(t, http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + tlsserver.AssertTLS(t, r, ptls.Secure) + switch r.URL.Path { case "/api/v1/namespaces/kube-system/configmaps": require.Equal(t, http.MethodGet, r.Method) @@ -708,6 +712,13 @@ func TestImpersonator(t *testing.T) { http.NotFound(w, r) return + case "/apis/flowcontrol.apiserver.k8s.io/v1beta1/prioritylevelconfigurations", + "/apis/flowcontrol.apiserver.k8s.io/v1beta1/flowschemas": + // ignore requests related to priority and fairness logic + require.Equal(t, http.MethodGet, r.Method) + http.NotFound(w, r) + return + case "/api/v1/namespaces": require.Equal(t, http.MethodGet, r.Method) @@ -785,13 +796,13 @@ func TestImpersonator(t *testing.T) { require.Fail(t, "fake Kube API server got an unexpected request", "path: %s", r.URL.Path) return } - }) + }), tlsserver.RecordTLSHello) // Create the client config that the impersonation server should use to talk to the Kube API server. testKubeAPIServerKubeconfig := rest.Config{ - Host: testKubeAPIServerURL, + Host: testKubeAPIServer.URL, BearerToken: "some-service-account-token", - TLSClientConfig: rest.TLSClientConfig{CAData: []byte(testKubeAPIServerCA)}, + TLSClientConfig: rest.TLSClientConfig{CAData: tlsserver.TLSTestServerCA(testKubeAPIServer)}, BearerTokenFile: tt.kubeAPIServerClientBearerTokenFile, } clientOpts := []kubeclient.Option{kubeclient.WithConfig(&testKubeAPIServerKubeconfig)} @@ -800,7 +811,6 @@ func TestImpersonator(t *testing.T) { recOpts := func(options *genericoptions.RecommendedOptions) { options.Authentication.RemoteKubeConfigFileOptional = true options.Authorization.RemoteKubeConfigFileOptional = true - options.CoreAPI = nil options.Admission = nil options.SecureServing.Listener = listener // use our listener with the dynamic port } @@ -814,8 +824,8 @@ func TestImpersonator(t *testing.T) { // Allow standard REST verbs to be authorized so that tests pass without invasive changes recConfig := func(config *genericapiserver.RecommendedConfig) { authz := config.Authorization.Authorizer.(*comparableAuthorizer) - delegate := authz.authorizerFunc - authz.authorizerFunc = func(ctx context.Context, a authorizer.Attributes) (authorizer.Decision, string, error) { + delegate := authz.AuthorizerFunc + authz.AuthorizerFunc = func(ctx context.Context, a authorizer.Attributes) (authorizer.Decision, string, error) { recorder.record(a) switch a.GetVerb() { case "create", "get", "list": @@ -826,8 +836,15 @@ func TestImpersonator(t *testing.T) { } } + restConfigFunc := func(config *rest.Config) (kubernetes.Interface, *rest.Config, error) { + if config == nil { + config = &testKubeAPIServerKubeconfig + } + return kubeclient.Secure(config) + } + // Create an impersonator. Use an invalid port number to make sure our listener override works. - runner, constructionErr := newInternal(-1000, certKeyContent, caContent, clientOpts, recOpts, recConfig) + runner, constructionErr := newInternal(-1000, certKeyContent, caContent, restConfigFunc, clientOpts, recOpts, recConfig) if len(tt.wantConstructionError) > 0 { require.EqualError(t, constructionErr, tt.wantConstructionError) require.Nil(t, runner) @@ -864,7 +881,7 @@ func TestImpersonator(t *testing.T) { return rt } - return roundtripper.Func(func(req *http.Request) (*http.Response, error) { + return roundtripper.WrapFunc(rt, func(req *http.Request) (*http.Response, error) { req = req.Clone(req.Context()) tt.clientMutateHeaders(req.Header) return rt.RoundTrip(req) @@ -928,10 +945,10 @@ func TestImpersonator(t *testing.T) { // anonymous TCR should always work - tcrRegGroup, err := kubeclient.New(kubeclient.WithConfig(rest.AnonymousClientConfig(clientKubeconfig))) + tcrRegGroup, err := kubeclient.New(kubeclient.WithConfig(kubeclient.SecureAnonymousClientConfig(clientKubeconfig))) require.NoError(t, err) - tcrOtherGroup, err := kubeclient.New(kubeclient.WithConfig(rest.AnonymousClientConfig(clientKubeconfig)), + tcrOtherGroup, err := kubeclient.New(kubeclient.WithConfig(kubeclient.SecureAnonymousClientConfig(clientKubeconfig)), kubeclient.WithMiddleware(groupsuffix.New("walrus.tld"))) require.NoError(t, err) @@ -950,7 +967,7 @@ func TestImpersonator(t *testing.T) { // these calls should only work when anonymous auth is enabled - anonymousConfig := rest.AnonymousClientConfig(clientKubeconfig) + anonymousConfig := kubeclient.SecureAnonymousClientConfig(clientKubeconfig) anonymousConfig.GroupVersion = &schema.GroupVersion{ Group: "not-concierge.walrus.tld", Version: "v1", @@ -989,7 +1006,7 @@ func TestImpersonator(t *testing.T) { // this should always fail as unauthorized (even for TCR) because the cert is not valid - badCertConfig := rest.AnonymousClientConfig(clientKubeconfig) + badCertConfig := kubeclient.SecureAnonymousClientConfig(clientKubeconfig) badCert := newClientCert(t, unrelatedCA, "bad-user", []string{"bad-group"}) badCertConfig.TLSClientConfig.CertData = badCert.certPEM badCertConfig.TLSClientConfig.KeyData = badCert.keyPEM @@ -1041,7 +1058,7 @@ func TestImpersonatorHTTPHandler(t *testing.T) { ExecProvider: &api.ExecConfig{}, AuthProvider: &api.AuthProviderConfig{}, }, - wantCreationErr: "could not get http/1.1 round tripper: could not get in-cluster transport config: execProvider and authProvider cannot be used in combination", + wantCreationErr: "could not create secure client config: execProvider and authProvider cannot be used in combination", }, { name: "fail to get transport from config", @@ -1051,7 +1068,7 @@ func TestImpersonatorHTTPHandler(t *testing.T) { Transport: http.DefaultTransport, TLSClientConfig: rest.TLSClientConfig{Insecure: true}, }, - wantCreationErr: "could not get http/1.1 round tripper: using a custom transport with TLS certificate options or the insecure flag is not allowed", + wantCreationErr: "could not create secure client config: failed to build transport: using a custom transport with TLS certificate options or the insecure flag is not allowed", }, { name: "Impersonate-User header already in request", @@ -1761,7 +1778,9 @@ func TestImpersonatorHTTPHandler(t *testing.T) { testKubeAPIServerWasCalled := false testKubeAPIServerSawHeaders := http.Header{} - testKubeAPIServerCA, testKubeAPIServerURL := testutil.TLSTestServer(t, func(w http.ResponseWriter, r *http.Request) { + testKubeAPIServer := tlsserver.TLSTestServer(t, http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + tlsserver.AssertTLS(t, r, ptls.Secure) + testKubeAPIServerWasCalled = true testKubeAPIServerSawHeaders = r.Header if tt.kubeAPIServerStatusCode != http.StatusOK { @@ -1769,17 +1788,26 @@ func TestImpersonatorHTTPHandler(t *testing.T) { } else { _, _ = w.Write([]byte("successful proxied response")) } - }) + }), tlsserver.RecordTLSHello) + testKubeAPIServerKubeconfig := rest.Config{ - Host: testKubeAPIServerURL, + Host: testKubeAPIServer.URL, BearerToken: "some-service-account-token", - TLSClientConfig: rest.TLSClientConfig{CAData: []byte(testKubeAPIServerCA)}, + TLSClientConfig: rest.TLSClientConfig{CAData: tlsserver.TLSTestServerCA(testKubeAPIServer)}, } if tt.restConfig == nil { tt.restConfig = &testKubeAPIServerKubeconfig } - impersonatorHTTPHandlerFunc, err := newImpersonationReverseProxyFunc(tt.restConfig) + // mimic how newInternal would call newImpersonationReverseProxyFunc + impersonatorHTTPHandlerFunc, err := func() (func(*genericapiserver.Config) http.Handler, error) { + kubeClientForProxy, err := kubeclient.New(kubeclient.WithConfig(tt.restConfig)) + if err != nil { + return nil, err + } + return newImpersonationReverseProxyFunc(rest.CopyConfig(kubeClientForProxy.ProtoConfig)) + }() + if tt.wantCreationErr != "" { require.EqualError(t, err, tt.wantCreationErr) require.Nil(t, impersonatorHTTPHandlerFunc) diff --git a/internal/concierge/server/server.go b/internal/concierge/server/server.go index 71e583cc..f9e9210b 100644 --- a/internal/concierge/server/server.go +++ b/internal/concierge/server/server.go @@ -29,10 +29,12 @@ import ( "go.pinniped.dev/internal/controller/authenticator/authncache" "go.pinniped.dev/internal/controllerinit" "go.pinniped.dev/internal/controllermanager" + "go.pinniped.dev/internal/crypto/ptls" "go.pinniped.dev/internal/downward" "go.pinniped.dev/internal/dynamiccert" "go.pinniped.dev/internal/here" "go.pinniped.dev/internal/issuer" + "go.pinniped.dev/internal/kubeclient" "go.pinniped.dev/internal/plog" "go.pinniped.dev/internal/registry/credentialrequest" ) @@ -209,6 +211,13 @@ func getAggregatedAPIServerConfig( recommendedOptions.SecureServing.ServerCert.GeneratedCert = dynamicCertProvider recommendedOptions.SecureServing.BindPort = 8443 // Don't run on default 443 because that requires root + // secure TLS for connections coming from and going to the Kube API server + // this is best effort because not all options provide the right hooks to override TLS config + // since our only client is the Kube API server, this uses the most secure TLS config + if err := ptls.SecureRecommendedOptions(recommendedOptions, kubeclient.Secure); err != nil { + return nil, fmt.Errorf("failed to secure recommended options: %w", err) + } + serverConfig := genericapiserver.NewRecommendedConfig(codecs) // Note that among other things, this ApplyTo() function copies // `recommendedOptions.SecureServing.ServerCert.GeneratedCert` into @@ -220,7 +229,7 @@ func getAggregatedAPIServerConfig( // If the provider later starts returning certs, then the API server // will use them to handle the incoming requests successfully. if err := recommendedOptions.ApplyTo(serverConfig); err != nil { - return nil, err + return nil, fmt.Errorf("failed to apply recommended options: %w", err) } apiServerConfig := &apiserver.Config{ diff --git a/internal/controller/authenticator/authenticator.go b/internal/controller/authenticator/authenticator.go index 2ac68500..7623aecd 100644 --- a/internal/controller/authenticator/authenticator.go +++ b/internal/controller/authenticator/authenticator.go @@ -9,6 +9,8 @@ import ( "encoding/base64" "fmt" + "k8s.io/client-go/util/cert" + auth1alpha1 "go.pinniped.dev/generated/latest/apis/concierge/authentication/v1alpha1" ) @@ -23,19 +25,20 @@ type Closer interface { // CABundle returns a PEM-encoded CA bundle from the provided spec. If the provided spec is nil, a // nil CA bundle will be returned. If the provided spec contains a CA bundle that is not properly // encoded, an error will be returned. -func CABundle(spec *auth1alpha1.TLSSpec) ([]byte, error) { +func CABundle(spec *auth1alpha1.TLSSpec) (*x509.CertPool, []byte, error) { if spec == nil || len(spec.CertificateAuthorityData) == 0 { - return nil, nil + return nil, nil, nil } pem, err := base64.StdEncoding.DecodeString(spec.CertificateAuthorityData) if err != nil { - return nil, err + return nil, nil, err } - if ok := x509.NewCertPool().AppendCertsFromPEM(pem); !ok { - return nil, fmt.Errorf("certificateAuthorityData is not valid PEM") + rootCAs, err := cert.NewPoolFromBytes(pem) + if err != nil { + return nil, nil, fmt.Errorf("certificateAuthorityData is not valid PEM: %w", err) } - return pem, nil + return rootCAs, pem, nil } diff --git a/internal/controller/authenticator/jwtcachefiller/jwtcachefiller.go b/internal/controller/authenticator/jwtcachefiller/jwtcachefiller.go index 66c96a06..f45cd058 100644 --- a/internal/controller/authenticator/jwtcachefiller/jwtcachefiller.go +++ b/internal/controller/authenticator/jwtcachefiller/jwtcachefiller.go @@ -6,9 +6,13 @@ package jwtcachefiller import ( + "context" "fmt" + "net/url" "reflect" + "time" + coreosoidc "github.com/coreos/go-oidc/v3/oidc" "github.com/go-logr/logr" "gopkg.in/square/go-jose.v2" "k8s.io/apimachinery/pkg/api/errors" @@ -23,6 +27,7 @@ import ( pinnipedauthenticator "go.pinniped.dev/internal/controller/authenticator" "go.pinniped.dev/internal/controller/authenticator/authncache" "go.pinniped.dev/internal/controllerlib" + "go.pinniped.dev/internal/net/phttp" ) // These default values come from the way that the Supervisor issues and signs tokens. We make these @@ -145,7 +150,7 @@ func (c *controller) extractValueAsJWTAuthenticator(value authncache.Value) *jwt // newJWTAuthenticator creates a jwt authenticator from the provided spec. func newJWTAuthenticator(spec *auth1alpha1.JWTAuthenticatorSpec) (*jwtAuthenticator, error) { - caBundle, err := pinnipedauthenticator.CABundle(spec.TLS) + rootCAs, caBundle, err := pinnipedauthenticator.CABundle(spec.TLS) if err != nil { return nil, fmt.Errorf("invalid TLS configuration: %w", err) } @@ -167,20 +172,51 @@ func newJWTAuthenticator(spec *auth1alpha1.JWTAuthenticatorSpec) (*jwtAuthentica groupsClaim = defaultGroupsClaim } - authenticator, err := oidc.New(oidc.Options{ + // copied from Kube OIDC code + issuerURL, err := url.Parse(spec.Issuer) + if err != nil { + return nil, err + } + if issuerURL.Scheme != "https" { + return nil, fmt.Errorf("issuer (%q) has invalid scheme (%q), require 'https'", spec.Issuer, issuerURL.Scheme) + } + + client := phttp.Default(rootCAs) + client.Timeout = 30 * time.Second // copied from Kube OIDC code + + ctx := coreosoidc.ClientContext(context.Background(), client) + + provider, err := coreosoidc.NewProvider(ctx, spec.Issuer) + if err != nil { + return nil, fmt.Errorf("could not initialize provider: %w", err) + } + providerJSON := &struct { + JWKSURL string `json:"jwks_uri"` + }{} + if err := provider.Claims(providerJSON); err != nil { + return nil, fmt.Errorf("could not get provider jwks_uri: %w", err) // should be impossible because coreosoidc.NewProvider validates this + } + if len(providerJSON.JWKSURL) == 0 { + return nil, fmt.Errorf("issuer %q does not have jwks_uri set", spec.Issuer) + } + + oidcAuthenticator, err := oidc.New(oidc.Options{ IssuerURL: spec.Issuer, + KeySet: coreosoidc.NewRemoteKeySet(ctx, providerJSON.JWKSURL), ClientID: spec.Audience, UsernameClaim: usernameClaim, GroupsClaim: groupsClaim, SupportedSigningAlgs: defaultSupportedSigningAlgos(), - CAContentProvider: caContentProvider, + // this is still needed for distributed claim resolution, meaning this uses a http client that does not honor our TLS config + // TODO fix when we pick up https://github.com/kubernetes/kubernetes/pull/106141 + CAContentProvider: caContentProvider, }) if err != nil { return nil, fmt.Errorf("could not initialize authenticator: %w", err) } return &jwtAuthenticator{ - tokenAuthenticatorCloser: authenticator, + tokenAuthenticatorCloser: oidcAuthenticator, spec: spec, }, nil } diff --git a/internal/controller/authenticator/jwtcachefiller/jwtcachefiller_test.go b/internal/controller/authenticator/jwtcachefiller/jwtcachefiller_test.go index 19eb1731..0541a5b9 100644 --- a/internal/controller/authenticator/jwtcachefiller/jwtcachefiller_test.go +++ b/internal/controller/authenticator/jwtcachefiller/jwtcachefiller_test.go @@ -15,7 +15,6 @@ import ( "encoding/pem" "fmt" "net/http" - "net/http/httptest" "strings" "testing" "time" @@ -35,8 +34,10 @@ import ( pinnipedinformers "go.pinniped.dev/generated/latest/client/concierge/informers/externalversions" "go.pinniped.dev/internal/controller/authenticator/authncache" "go.pinniped.dev/internal/controllerlib" + "go.pinniped.dev/internal/crypto/ptls" "go.pinniped.dev/internal/mocks/mocktokenauthenticatorcloser" "go.pinniped.dev/internal/testutil/testlogger" + "go.pinniped.dev/internal/testutil/tlsserver" ) func TestController(t *testing.T) { @@ -57,8 +58,10 @@ func TestController(t *testing.T) { goodRSASigningAlgo := jose.RS256 mux := http.NewServeMux() - server := httptest.NewTLSServer(mux) - t.Cleanup(server.Close) + server := tlsserver.TLSTestServer(t, http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + tlsserver.AssertTLS(t, r, ptls.Default) + mux.ServeHTTP(w, r) + }), tlsserver.RecordTLSHello) mux.Handle("/.well-known/openid-configuration", http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { w.Header().Set("Content-Type", "application/json") @@ -290,11 +293,7 @@ func TestController(t *testing.T) { Spec: *missingTLSJWTAuthenticatorSpec, }, }, - wantLogs: []string{ - `jwtcachefiller-controller "level"=0 "msg"="added new jwt authenticator" "issuer"="` + goodIssuer + `" "jwtAuthenticator"={"name":"test-name"}`, - }, - wantCacheEntries: 1, - runTestsOnResultingAuthenticator: false, // skip the tests because the authenticator left in the cache doesn't have the CA for our test discovery server + wantErr: `failed to build jwt authenticator: could not initialize provider: Get "` + goodIssuer + `/.well-known/openid-configuration": x509: certificate signed by unknown authority`, }, { name: "invalid jwt authenticator CA", diff --git a/internal/controller/authenticator/webhookcachefiller/webhookcachefiller.go b/internal/controller/authenticator/webhookcachefiller/webhookcachefiller.go index d8a897c7..d394c913 100644 --- a/internal/controller/authenticator/webhookcachefiller/webhookcachefiller.go +++ b/internal/controller/authenticator/webhookcachefiller/webhookcachefiller.go @@ -91,7 +91,7 @@ func newWebhookAuthenticator( defer func() { _ = os.Remove(temp.Name()) }() cluster := &clientcmdapi.Cluster{Server: spec.Endpoint} - cluster.CertificateAuthorityData, err = pinnipedauthenticator.CABundle(spec.TLS) + _, cluster.CertificateAuthorityData, err = pinnipedauthenticator.CABundle(spec.TLS) if err != nil { return nil, fmt.Errorf("invalid TLS configuration: %w", err) } @@ -118,5 +118,7 @@ func newWebhookAuthenticator( // custom proxy stuff used by the API server. var customDial net.DialFunc + // this uses a http client that does not honor our TLS config + // TODO fix when we pick up https://github.com/kubernetes/kubernetes/pull/106155 return webhook.New(temp.Name(), version, implicitAuds, *webhook.DefaultRetryBackoff(), customDial) } diff --git a/internal/controller/authenticator/webhookcachefiller/webhookcachefiller_test.go b/internal/controller/authenticator/webhookcachefiller/webhookcachefiller_test.go index aae172d9..abe3c46d 100644 --- a/internal/controller/authenticator/webhookcachefiller/webhookcachefiller_test.go +++ b/internal/controller/authenticator/webhookcachefiller/webhookcachefiller_test.go @@ -141,7 +141,7 @@ func TestNewWebhookAuthenticator(t *testing.T) { TLS: &auth1alpha1.TLSSpec{CertificateAuthorityData: base64.StdEncoding.EncodeToString([]byte("bad data"))}, }, ioutil.TempFile, clientcmd.WriteToFile) require.Nil(t, res) - require.EqualError(t, err, "invalid TLS configuration: certificateAuthorityData is not valid PEM") + require.EqualError(t, err, "invalid TLS configuration: certificateAuthorityData is not valid PEM: data does not contain any valid RSA or ECDSA certificates") }) t.Run("valid config with no TLS spec", func(t *testing.T) { diff --git a/internal/controller/kubecertagent/pod_command_executor.go b/internal/controller/kubecertagent/pod_command_executor.go index ff9aeb31..242f6435 100644 --- a/internal/controller/kubecertagent/pod_command_executor.go +++ b/internal/controller/kubecertagent/pod_command_executor.go @@ -26,6 +26,8 @@ type kubeClientPodCommandExecutor struct { // NewPodCommandExecutor returns a PodCommandExecutor that will interact with a pod via the provided // kubeConfig and corresponding kubeClient. func NewPodCommandExecutor(kubeConfig *restclient.Config, kubeClient kubernetes.Interface) PodCommandExecutor { + kubeConfig = restclient.CopyConfig(kubeConfig) + kubeConfig.NextProtos = []string{"http/1.1"} // we explicitly need to upgrade from http1 to spdy, exec cannot use http2 return &kubeClientPodCommandExecutor{kubeConfig: kubeConfig, kubeClient: kubeClient} } diff --git a/internal/controller/kubecertagent/pod_command_executor_test.go b/internal/controller/kubecertagent/pod_command_executor_test.go new file mode 100644 index 00000000..934f102d --- /dev/null +++ b/internal/controller/kubecertagent/pod_command_executor_test.go @@ -0,0 +1,44 @@ +// Copyright 2021 the Pinniped contributors. All Rights Reserved. +// SPDX-License-Identifier: Apache-2.0 + +package kubecertagent + +import ( + "net/http" + "testing" + + "github.com/stretchr/testify/require" + "k8s.io/apimachinery/pkg/api/errors" + "k8s.io/client-go/rest" + + "go.pinniped.dev/internal/crypto/ptls" + "go.pinniped.dev/internal/kubeclient" + "go.pinniped.dev/internal/testutil/tlsserver" +) + +func TestSecureTLS(t *testing.T) { + var sawRequest bool + server := tlsserver.TLSTestServer(t, http.HandlerFunc(func(_ http.ResponseWriter, r *http.Request) { + tlsserver.AssertTLS(t, r, ptls.Secure) + sawRequest = true + }), tlsserver.RecordTLSHello) + + config := &rest.Config{ + Host: server.URL, + TLSClientConfig: rest.TLSClientConfig{ + CAData: tlsserver.TLSTestServerCA(server), + }, + } + + client, err := kubeclient.New(kubeclient.WithConfig(config)) + require.NoError(t, err) + + // build this exactly like our production could does + podCommandExecutor := NewPodCommandExecutor(client.JSONConfig, client.Kubernetes) + + got, err := podCommandExecutor.Exec("podNamespace", "podName", "command", "arg1", "arg2") + require.Equal(t, &errors.StatusError{}, err) + require.Empty(t, got) + + require.True(t, sawRequest) +} diff --git a/internal/controller/supervisorconfig/oidcupstreamwatcher/oidc_upstream_watcher.go b/internal/controller/supervisorconfig/oidcupstreamwatcher/oidc_upstream_watcher.go index 0e4399ea..1b2c4e23 100644 --- a/internal/controller/supervisorconfig/oidcupstreamwatcher/oidc_upstream_watcher.go +++ b/internal/controller/supervisorconfig/oidcupstreamwatcher/oidc_upstream_watcher.go @@ -6,7 +6,6 @@ package oidcupstreamwatcher import ( "context" - "crypto/tls" "crypto/x509" "encoding/base64" "fmt" @@ -35,6 +34,7 @@ import ( "go.pinniped.dev/internal/controller/conditionsutil" "go.pinniped.dev/internal/controller/supervisorconfig/upstreamwatchers" "go.pinniped.dev/internal/controllerlib" + "go.pinniped.dev/internal/net/phttp" "go.pinniped.dev/internal/oidc/provider" "go.pinniped.dev/internal/upstreamoidc" ) @@ -313,7 +313,8 @@ func (c *oidcWatcherController) validateIssuer(ctx context.Context, upstream *v1 // If the provider does not exist in the cache, do a fresh discovery lookup and save to the cache. if discoveredProvider == nil { - tlsConfig, err := getTLSConfig(upstream) + var err error + httpClient, err = getClient(upstream) if err != nil { return &v1alpha1.Condition{ Type: typeOIDCDiscoverySucceeded, @@ -323,14 +324,6 @@ func (c *oidcWatcherController) validateIssuer(ctx context.Context, upstream *v1 } } - httpClient = &http.Client{ - Timeout: time.Minute, - Transport: &http.Transport{ - Proxy: http.ProxyFromEnvironment, - TLSClientConfig: tlsConfig, - }, - } - discoveredProvider, err = oidc.NewProvider(oidc.ClientContext(ctx, httpClient), upstream.Spec.Issuer) if err != nil { const klogLevelTrace = 6 @@ -406,13 +399,9 @@ func (c *oidcWatcherController) updateStatus(ctx context.Context, upstream *v1al } } -func getTLSConfig(upstream *v1alpha1.OIDCIdentityProvider) (*tls.Config, error) { - result := tls.Config{ - MinVersion: tls.VersionTLS12, - } - +func getClient(upstream *v1alpha1.OIDCIdentityProvider) (*http.Client, error) { if upstream.Spec.TLS == nil || upstream.Spec.TLS.CertificateAuthorityData == "" { - return &result, nil + return defaultClientShortTimeout(nil), nil } bundle, err := base64.StdEncoding.DecodeString(upstream.Spec.TLS.CertificateAuthorityData) @@ -420,12 +409,18 @@ func getTLSConfig(upstream *v1alpha1.OIDCIdentityProvider) (*tls.Config, error) return nil, fmt.Errorf("spec.certificateAuthorityData is invalid: %w", err) } - result.RootCAs = x509.NewCertPool() - if !result.RootCAs.AppendCertsFromPEM(bundle) { + rootCAs := x509.NewCertPool() + if !rootCAs.AppendCertsFromPEM(bundle) { return nil, fmt.Errorf("spec.certificateAuthorityData is invalid: %w", upstreamwatchers.ErrNoCertificates) } - return &result, nil + return defaultClientShortTimeout(rootCAs), nil +} + +func defaultClientShortTimeout(rootCAs *x509.CertPool) *http.Client { + c := phttp.Default(rootCAs) + c.Timeout = time.Minute + return c } func computeScopes(additionalScopes []string) []string { diff --git a/internal/controller/supervisorconfig/oidcupstreamwatcher/oidc_upstream_watcher_test.go b/internal/controller/supervisorconfig/oidcupstreamwatcher/oidc_upstream_watcher_test.go index 57942c02..977ed35f 100644 --- a/internal/controller/supervisorconfig/oidcupstreamwatcher/oidc_upstream_watcher_test.go +++ b/internal/controller/supervisorconfig/oidcupstreamwatcher/oidc_upstream_watcher_test.go @@ -19,6 +19,7 @@ import ( metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/types" + "k8s.io/apimachinery/pkg/util/net" "k8s.io/client-go/informers" "k8s.io/client-go/kubernetes/fake" @@ -1014,8 +1015,7 @@ oidc: issuer did not match the issuer returned by provider, expected "` + testIs // We always want to use the proxy from env on these clients, so although the following assertions // are a little hacky, this is a cheap way to test that we are using it. - actualTransport, ok := actualIDP.Client.Transport.(*http.Transport) - require.True(t, ok, "expected cached provider to have client with Transport of type *http.Transport") + actualTransport := unwrapTransport(t, actualIDP.Client.Transport) httpProxyFromEnvFunction := reflect.ValueOf(http.ProxyFromEnvironment).Pointer() actualTransportProxyFunction := reflect.ValueOf(actualTransport.Proxy).Pointer() require.Equal(t, httpProxyFromEnvFunction, actualTransportProxyFunction, @@ -1041,6 +1041,22 @@ oidc: issuer did not match the issuer returned by provider, expected "` + testIs } } +func unwrapTransport(t *testing.T, rt http.RoundTripper) *http.Transport { + t.Helper() + + switch baseRT := rt.(type) { + case *http.Transport: + return baseRT + + case net.RoundTripperWrapper: + return unwrapTransport(t, baseRT.WrappedRoundTripper()) + + default: + t.Fatalf("expected cached provider to have client with Transport of type *http.Transport, got: %T", baseRT) + return nil // unreachable + } +} + func normalizeOIDCUpstreams(upstreams []v1alpha1.OIDCIdentityProvider, now metav1.Time) []v1alpha1.OIDCIdentityProvider { result := make([]v1alpha1.OIDCIdentityProvider, 0, len(upstreams)) for _, u := range upstreams { diff --git a/internal/crypto/ptls/old.go b/internal/crypto/ptls/old.go new file mode 100644 index 00000000..a8987673 --- /dev/null +++ b/internal/crypto/ptls/old.go @@ -0,0 +1,15 @@ +// Copyright 2021 the Pinniped contributors. All Rights Reserved. +// SPDX-License-Identifier: Apache-2.0 + +//go:build !go1.17 +// +build !go1.17 + +package ptls + +func init() { + // cause compile time failure if an older version of Go is used + `Pinniped's TLS configuration makes assumptions about how the Go standard library implementation of TLS works. +It particular, we rely on the server controlling cipher suite selection. For these assumptions to hold, Pinniped +must be compiled with Go 1.17+. If you are seeing this error message, your attempt to compile Pinniped with an +older Go compiler was explicitly failed to prevent an unsafe configuration.` +} diff --git a/internal/crypto/ptls/ptls.go b/internal/crypto/ptls/ptls.go new file mode 100644 index 00000000..5c64978c --- /dev/null +++ b/internal/crypto/ptls/ptls.go @@ -0,0 +1,215 @@ +// Copyright 2021 the Pinniped contributors. All Rights Reserved. +// SPDX-License-Identifier: Apache-2.0 + +package ptls + +import ( + "crypto/tls" + "crypto/x509" + "fmt" + "net/http" + "sync" + + "k8s.io/apiserver/pkg/admission" + genericapiserver "k8s.io/apiserver/pkg/server" + "k8s.io/apiserver/pkg/server/options" + kubeinformers "k8s.io/client-go/informers" + "k8s.io/client-go/kubernetes" + "k8s.io/client-go/rest" + "k8s.io/client-go/transport" +) + +// TODO decide if we need to expose the four TLS levels (secure, default, default-ldap, legacy) as config. + +type ConfigFunc func(*x509.CertPool) *tls.Config + +func Default(rootCAs *x509.CertPool) *tls.Config { + return &tls.Config{ + // Can't use SSLv3 because of POODLE and BEAST + // Can't use TLSv1.0 because of POODLE and BEAST using CBC cipher + // Can't use TLSv1.1 because of RC4 cipher usage + // + // The Kubernetes API Server must use TLS 1.2, at a minimum, + // to protect the confidentiality of sensitive data during electronic dissemination. + // https://stigviewer.com/stig/kubernetes/2021-06-17/finding/V-242378 + MinVersion: tls.VersionTLS12, + + // the order does not matter in go 1.17+ https://go.dev/blog/tls-cipher-suites + // we match crypto/tls.cipherSuitesPreferenceOrder because it makes unit tests easier to write + // this list is ignored when TLS 1.3 is used + // + // as of 2021-10-19, Mozilla Guideline v5.6, Go 1.17.2, intermediate configuration, supports: + // - Firefox 27 + // - Android 4.4.2 + // - Chrome 31 + // - Edge + // - IE 11 on Windows 7 + // - Java 8u31 + // - OpenSSL 1.0.1 + // - Opera 20 + // - Safari 9 + // https://ssl-config.mozilla.org/#server=go&version=1.17.2&config=intermediate&guideline=5.6 + // + // The Kubernetes API server must use approved cipher suites. + // https://stigviewer.com/stig/kubernetes/2021-06-17/finding/V-242418 + CipherSuites: []uint16{ + // these are all AEADs with ECDHE, some use ChaCha20Poly1305 while others use AES-GCM + // this provides forward secrecy, confidentiality and authenticity of data + tls.TLS_ECDHE_ECDSA_WITH_AES_128_GCM_SHA256, tls.TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256, + tls.TLS_ECDHE_ECDSA_WITH_AES_256_GCM_SHA384, tls.TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384, + tls.TLS_ECDHE_ECDSA_WITH_CHACHA20_POLY1305, tls.TLS_ECDHE_RSA_WITH_CHACHA20_POLY1305, + }, + + // enable HTTP2 for go's 1.7 HTTP Server + // setting this explicitly is only required in very specific circumstances + // it is simpler to just set it here than to try and determine if we need to + NextProtos: []string{"h2", "http/1.1"}, + + // optional root CAs, nil means use the host's root CA set + RootCAs: rootCAs, + } +} + +func Secure(rootCAs *x509.CertPool) *tls.Config { + // as of 2021-10-19, Mozilla Guideline v5.6, Go 1.17.2, modern configuration, supports: + // - Firefox 63 + // - Android 10.0 + // - Chrome 70 + // - Edge 75 + // - Java 11 + // - OpenSSL 1.1.1 + // - Opera 57 + // - Safari 12.1 + // https://ssl-config.mozilla.org/#server=go&version=1.17.2&config=modern&guideline=5.6 + c := Default(rootCAs) + c.MinVersion = tls.VersionTLS13 // max out the security + c.CipherSuites = []uint16{ + // TLS 1.3 ciphers are not configurable, but we need to explicitly set them here to make our client hello behave correctly + // See https://github.com/golang/go/pull/49293 + tls.TLS_AES_128_GCM_SHA256, + tls.TLS_AES_256_GCM_SHA384, + tls.TLS_CHACHA20_POLY1305_SHA256, + } + return c +} + +func DefaultLDAP(rootCAs *x509.CertPool) *tls.Config { + c := Default(rootCAs) + // add less secure ciphers to support the default AWS Active Directory config + c.CipherSuites = append(c.CipherSuites, + // CBC with ECDHE + // this provides forward secrecy and confidentiality of data but not authenticity + // MAC-then-Encrypt CBC ciphers are susceptible to padding oracle attacks + // See https://crypto.stackexchange.com/a/205 and https://crypto.stackexchange.com/a/224 + tls.TLS_ECDHE_ECDSA_WITH_AES_128_CBC_SHA, tls.TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA, + tls.TLS_ECDHE_ECDSA_WITH_AES_256_CBC_SHA, tls.TLS_ECDHE_RSA_WITH_AES_256_CBC_SHA, + ) + return c +} + +func Legacy(rootCAs *x509.CertPool) *tls.Config { + c := Default(rootCAs) + // add all the ciphers (even the crappy ones) except the ones that Go considers to be outright broken like 3DES + c.CipherSuites = suitesToIDs(tls.CipherSuites()) + return c +} + +func suitesToIDs(suites []*tls.CipherSuite) []uint16 { + out := make([]uint16, 0, len(suites)) + for _, suite := range suites { + suite := suite + out = append(out, suite.ID) + } + return out +} + +func Merge(tlsConfigFunc ConfigFunc, tlsConfig *tls.Config) { + secureTLSConfig := tlsConfigFunc(nil) + + // override the core security knobs of the TLS config + // note that these have to be kept in sync with Default / Secure above + tlsConfig.MinVersion = secureTLSConfig.MinVersion + tlsConfig.CipherSuites = secureTLSConfig.CipherSuites + + // if the TLS config already states what protocols it wants to use, honor that instead of overriding + if len(tlsConfig.NextProtos) == 0 { + tlsConfig.NextProtos = secureTLSConfig.NextProtos + } +} + +// RestConfigFunc allows this package to not depend on the kubeclient package. +type RestConfigFunc func(*rest.Config) (kubernetes.Interface, *rest.Config, error) + +func DefaultRecommendedOptions(opts *options.RecommendedOptions, f RestConfigFunc) error { + defaultServing(opts.SecureServing) + return secureClient(opts, f) +} + +func SecureRecommendedOptions(opts *options.RecommendedOptions, f RestConfigFunc) error { + secureServing(opts.SecureServing) + return secureClient(opts, f) +} + +func defaultServing(opts *options.SecureServingOptionsWithLoopback) { + c := Default(nil) + cipherSuites := make([]string, 0, len(c.CipherSuites)) + for _, id := range c.CipherSuites { + cipherSuites = append(cipherSuites, tls.CipherSuiteName(id)) + } + opts.CipherSuites = cipherSuites + + opts.MinTLSVersion = "VersionTLS12" +} + +func secureServing(opts *options.SecureServingOptionsWithLoopback) { + opts.MinTLSVersion = "VersionTLS13" + opts.CipherSuites = nil +} + +func secureClient(opts *options.RecommendedOptions, f RestConfigFunc) error { + inClusterClient, inClusterConfig, err := f(nil) + if err != nil { + return fmt.Errorf("failed to build in cluster client: %w", err) + } + + if n, z := opts.Authentication.RemoteKubeConfigFile, opts.Authorization.RemoteKubeConfigFile; len(n) > 0 || len(z) > 0 { + return fmt.Errorf("delgating auth is not using in-cluster config:\nauthentication=%s\nauthorization=%s", n, z) + } + + // delegated authn and authz provide easy hooks for us to set the TLS config. + // however, the underlying clients use client-go's global TLS cache with an + // in-cluster config. to make this safe, we simply do the mutation once. + wrapperFunc := wrapTransportOnce(inClusterConfig.WrapTransport) + opts.Authentication.CustomRoundTripperFn = wrapperFunc + opts.Authorization.CustomRoundTripperFn = wrapperFunc + + opts.CoreAPI = nil // set this to nil to make sure our ExtraAdmissionInitializers is used + baseExtraAdmissionInitializers := opts.ExtraAdmissionInitializers + opts.ExtraAdmissionInitializers = func(c *genericapiserver.RecommendedConfig) ([]admission.PluginInitializer, error) { + // abuse this closure to rewrite how we load admission plugins + c.ClientConfig = inClusterConfig + c.SharedInformerFactory = kubeinformers.NewSharedInformerFactory(inClusterClient, 0) + + // abuse this closure to rewrite our loopback config + // this is mostly future proofing for post start hooks + _, loopbackConfig, err := f(c.LoopbackClientConfig) + if err != nil { + return nil, fmt.Errorf("failed to build loopback config: %w", err) + } + c.LoopbackClientConfig = loopbackConfig + + return baseExtraAdmissionInitializers(c) + } + + return nil +} + +func wrapTransportOnce(f transport.WrapperFunc) transport.WrapperFunc { + var once sync.Once + return func(rt http.RoundTripper) http.RoundTripper { + once.Do(func() { + _ = f(rt) // assume in-place mutation + }) + return rt + } +} diff --git a/internal/crypto/ptls/ptls_test.go b/internal/crypto/ptls/ptls_test.go new file mode 100644 index 00000000..500e1885 --- /dev/null +++ b/internal/crypto/ptls/ptls_test.go @@ -0,0 +1,253 @@ +// Copyright 2021 the Pinniped contributors. All Rights Reserved. +// SPDX-License-Identifier: Apache-2.0 + +package ptls + +import ( + "crypto/tls" + "testing" + + "github.com/stretchr/testify/require" + "k8s.io/apiserver/pkg/server/options" +) + +func TestDefaultServing(t *testing.T) { + t.Parallel() + + opts := &options.SecureServingOptionsWithLoopback{SecureServingOptions: &options.SecureServingOptions{}} + defaultServing(opts) + require.Equal(t, options.SecureServingOptionsWithLoopback{ + SecureServingOptions: &options.SecureServingOptions{ + CipherSuites: []string{ + "TLS_ECDHE_ECDSA_WITH_AES_128_GCM_SHA256", + "TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256", + "TLS_ECDHE_ECDSA_WITH_AES_256_GCM_SHA384", + "TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384", + "TLS_ECDHE_ECDSA_WITH_CHACHA20_POLY1305_SHA256", + "TLS_ECDHE_RSA_WITH_CHACHA20_POLY1305_SHA256", + }, + MinTLSVersion: "VersionTLS12", + }, + }, *opts) +} + +func TestSecureServing(t *testing.T) { + t.Parallel() + + opts := &options.SecureServingOptionsWithLoopback{SecureServingOptions: &options.SecureServingOptions{}} + secureServing(opts) + require.Equal(t, options.SecureServingOptionsWithLoopback{ + SecureServingOptions: &options.SecureServingOptions{ + MinTLSVersion: "VersionTLS13", + }, + }, *opts) +} + +func TestMerge(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + tlsConfigFunc ConfigFunc + tlsConfig *tls.Config + want *tls.Config + }{ + { + name: "default no protos", + tlsConfigFunc: Default, + tlsConfig: &tls.Config{ + ServerName: "something-to-check-passthrough", + }, + want: &tls.Config{ + ServerName: "something-to-check-passthrough", + MinVersion: tls.VersionTLS12, + CipherSuites: []uint16{ + tls.TLS_ECDHE_ECDSA_WITH_AES_128_GCM_SHA256, + tls.TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256, + tls.TLS_ECDHE_ECDSA_WITH_AES_256_GCM_SHA384, + tls.TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384, + tls.TLS_ECDHE_ECDSA_WITH_CHACHA20_POLY1305_SHA256, + tls.TLS_ECDHE_RSA_WITH_CHACHA20_POLY1305_SHA256, + }, + NextProtos: []string{"h2", "http/1.1"}, + }, + }, + { + name: "default with protos", + tlsConfigFunc: Default, + tlsConfig: &tls.Config{ + ServerName: "a different thing for passthrough", + NextProtos: []string{"panda"}, + }, + want: &tls.Config{ + ServerName: "a different thing for passthrough", + MinVersion: tls.VersionTLS12, + CipherSuites: []uint16{ + tls.TLS_ECDHE_ECDSA_WITH_AES_128_GCM_SHA256, + tls.TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256, + tls.TLS_ECDHE_ECDSA_WITH_AES_256_GCM_SHA384, + tls.TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384, + tls.TLS_ECDHE_ECDSA_WITH_CHACHA20_POLY1305_SHA256, + tls.TLS_ECDHE_RSA_WITH_CHACHA20_POLY1305_SHA256, + }, + NextProtos: []string{"panda"}, + }, + }, + { + name: "secure no protos", + tlsConfigFunc: Secure, + tlsConfig: &tls.Config{ + ServerName: "something-to-check-passthrough", + }, + want: &tls.Config{ + ServerName: "something-to-check-passthrough", + MinVersion: tls.VersionTLS13, + CipherSuites: []uint16{ + tls.TLS_AES_128_GCM_SHA256, + tls.TLS_AES_256_GCM_SHA384, + tls.TLS_CHACHA20_POLY1305_SHA256, + }, + NextProtos: []string{"h2", "http/1.1"}, + }, + }, + { + name: "secure with protos", + tlsConfigFunc: Secure, + tlsConfig: &tls.Config{ + ServerName: "a different thing for passthrough", + NextProtos: []string{"panda"}, + }, + want: &tls.Config{ + ServerName: "a different thing for passthrough", + MinVersion: tls.VersionTLS13, + CipherSuites: []uint16{ + tls.TLS_AES_128_GCM_SHA256, + tls.TLS_AES_256_GCM_SHA384, + tls.TLS_CHACHA20_POLY1305_SHA256, + }, + NextProtos: []string{"panda"}, + }, + }, + { + name: "default ldap no protos", + tlsConfigFunc: DefaultLDAP, + tlsConfig: &tls.Config{ + ServerName: "something-to-check-passthrough", + }, + want: &tls.Config{ + ServerName: "something-to-check-passthrough", + MinVersion: tls.VersionTLS12, + CipherSuites: []uint16{ + tls.TLS_ECDHE_ECDSA_WITH_AES_128_GCM_SHA256, + tls.TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256, + tls.TLS_ECDHE_ECDSA_WITH_AES_256_GCM_SHA384, + tls.TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384, + tls.TLS_ECDHE_ECDSA_WITH_CHACHA20_POLY1305_SHA256, + tls.TLS_ECDHE_RSA_WITH_CHACHA20_POLY1305_SHA256, + tls.TLS_ECDHE_ECDSA_WITH_AES_128_CBC_SHA, //nolint: gosec // yeah, I know it is a bad cipher, but AD sucks + tls.TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA, + tls.TLS_ECDHE_ECDSA_WITH_AES_256_CBC_SHA, + tls.TLS_ECDHE_RSA_WITH_AES_256_CBC_SHA, + }, + NextProtos: []string{"h2", "http/1.1"}, + }, + }, + { + name: "default ldap with protos", + tlsConfigFunc: DefaultLDAP, + tlsConfig: &tls.Config{ + ServerName: "a different thing for passthrough", + NextProtos: []string{"panda"}, + }, + want: &tls.Config{ + ServerName: "a different thing for passthrough", + MinVersion: tls.VersionTLS12, + CipherSuites: []uint16{ + tls.TLS_ECDHE_ECDSA_WITH_AES_128_GCM_SHA256, + tls.TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256, + tls.TLS_ECDHE_ECDSA_WITH_AES_256_GCM_SHA384, + tls.TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384, + tls.TLS_ECDHE_ECDSA_WITH_CHACHA20_POLY1305_SHA256, + tls.TLS_ECDHE_RSA_WITH_CHACHA20_POLY1305_SHA256, + tls.TLS_ECDHE_ECDSA_WITH_AES_128_CBC_SHA, //nolint: gosec // yeah, I know it is a bad cipher, but AD sucks + tls.TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA, + tls.TLS_ECDHE_ECDSA_WITH_AES_256_CBC_SHA, + tls.TLS_ECDHE_RSA_WITH_AES_256_CBC_SHA, + }, + NextProtos: []string{"panda"}, + }, + }, + { + name: "legacy no protos", + tlsConfigFunc: Legacy, + tlsConfig: &tls.Config{ + ServerName: "something-to-check-passthrough", + }, + want: &tls.Config{ + ServerName: "something-to-check-passthrough", + MinVersion: tls.VersionTLS12, + CipherSuites: []uint16{ + tls.TLS_RSA_WITH_AES_128_CBC_SHA, //nolint: gosec // yeah, I know it is a bad cipher, this is the legacy config + tls.TLS_RSA_WITH_AES_256_CBC_SHA, + tls.TLS_RSA_WITH_AES_128_GCM_SHA256, + tls.TLS_RSA_WITH_AES_256_GCM_SHA384, + tls.TLS_AES_128_GCM_SHA256, + tls.TLS_AES_256_GCM_SHA384, + tls.TLS_CHACHA20_POLY1305_SHA256, + tls.TLS_ECDHE_ECDSA_WITH_AES_128_CBC_SHA, + tls.TLS_ECDHE_ECDSA_WITH_AES_256_CBC_SHA, + tls.TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA, + tls.TLS_ECDHE_RSA_WITH_AES_256_CBC_SHA, + tls.TLS_ECDHE_ECDSA_WITH_AES_128_GCM_SHA256, + tls.TLS_ECDHE_ECDSA_WITH_AES_256_GCM_SHA384, + tls.TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256, + tls.TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384, + tls.TLS_ECDHE_RSA_WITH_CHACHA20_POLY1305_SHA256, + tls.TLS_ECDHE_ECDSA_WITH_CHACHA20_POLY1305_SHA256, + }, + NextProtos: []string{"h2", "http/1.1"}, + }, + }, + { + name: "legacy with protos", + tlsConfigFunc: Legacy, + tlsConfig: &tls.Config{ + ServerName: "a different thing for passthrough", + NextProtos: []string{"panda"}, + }, + want: &tls.Config{ + ServerName: "a different thing for passthrough", + MinVersion: tls.VersionTLS12, + CipherSuites: []uint16{ + tls.TLS_RSA_WITH_AES_128_CBC_SHA, //nolint: gosec // yeah, I know it is a bad cipher, this is the legacy config + tls.TLS_RSA_WITH_AES_256_CBC_SHA, + tls.TLS_RSA_WITH_AES_128_GCM_SHA256, + tls.TLS_RSA_WITH_AES_256_GCM_SHA384, + tls.TLS_AES_128_GCM_SHA256, + tls.TLS_AES_256_GCM_SHA384, + tls.TLS_CHACHA20_POLY1305_SHA256, + tls.TLS_ECDHE_ECDSA_WITH_AES_128_CBC_SHA, + tls.TLS_ECDHE_ECDSA_WITH_AES_256_CBC_SHA, + tls.TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA, + tls.TLS_ECDHE_RSA_WITH_AES_256_CBC_SHA, + tls.TLS_ECDHE_ECDSA_WITH_AES_128_GCM_SHA256, + tls.TLS_ECDHE_ECDSA_WITH_AES_256_GCM_SHA384, + tls.TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256, + tls.TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384, + tls.TLS_ECDHE_RSA_WITH_CHACHA20_POLY1305_SHA256, + tls.TLS_ECDHE_ECDSA_WITH_CHACHA20_POLY1305_SHA256, + }, + NextProtos: []string{"panda"}, + }, + }, + } + for _, tt := range tests { + tt := tt + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + Merge(tt.tlsConfigFunc, tt.tlsConfig) + require.Equal(t, tt.want, tt.tlsConfig) + }) + } +} diff --git a/internal/dynamiccert/provider_test.go b/internal/dynamiccert/provider_test.go index fce2cd39..9a3758ae 100644 --- a/internal/dynamiccert/provider_test.go +++ b/internal/dynamiccert/provider_test.go @@ -19,6 +19,7 @@ import ( "k8s.io/apiserver/pkg/storage/names" "go.pinniped.dev/internal/certauthority" + "go.pinniped.dev/internal/crypto/ptls" "go.pinniped.dev/test/testlib" ) @@ -160,11 +161,8 @@ func TestProviderWithDynamicServingCertificateController(t *testing.T) { err = certKeyContent.SetCertKeyContent(cert, key) require.NoError(t, err) - tlsConfig := &tls.Config{ - MinVersion: tls.VersionTLS12, - NextProtos: []string{"h2", "http/1.1"}, - ClientAuth: tls.RequestClientCert, - } + tlsConfig := ptls.Default(nil) + tlsConfig.ClientAuth = tls.RequestClientCert dynamicCertificateController := dynamiccertificates.NewDynamicServingCertificateController( tlsConfig, diff --git a/internal/httputil/roundtripper/roundtripper.go b/internal/httputil/roundtripper/roundtripper.go index d0094b8a..0bd05234 100644 --- a/internal/httputil/roundtripper/roundtripper.go +++ b/internal/httputil/roundtripper/roundtripper.go @@ -3,7 +3,11 @@ package roundtripper -import "net/http" +import ( + "net/http" + + "k8s.io/apimachinery/pkg/util/net" +) var _ http.RoundTripper = Func(nil) @@ -12,3 +16,22 @@ type Func func(*http.Request) (*http.Response, error) func (f Func) RoundTrip(req *http.Request) (*http.Response, error) { return f(req) } + +var _ net.RoundTripperWrapper = &wrapper{} + +type wrapper struct { + delegate http.RoundTripper + f Func +} + +func (w *wrapper) RoundTrip(req *http.Request) (*http.Response, error) { + return w.f.RoundTrip(req) +} + +func (w *wrapper) WrappedRoundTripper() http.RoundTripper { + return w.delegate +} + +func WrapFunc(delegate http.RoundTripper, f Func) net.RoundTripperWrapper { + return &wrapper{delegate: delegate, f: f} +} diff --git a/internal/kubeclient/kubeclient.go b/internal/kubeclient/kubeclient.go index c093a2c1..c9b3106f 100644 --- a/internal/kubeclient/kubeclient.go +++ b/internal/kubeclient/kubeclient.go @@ -4,9 +4,17 @@ package kubeclient import ( + "crypto/tls" + "crypto/x509" "fmt" + "net/http" + "reflect" + "unsafe" + "github.com/google/go-cmp/cmp" + "github.com/google/go-cmp/cmp/cmpopts" "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/util/net" "k8s.io/client-go/kubernetes" kubescheme "k8s.io/client-go/kubernetes/scheme" restclient "k8s.io/client-go/rest" @@ -17,6 +25,7 @@ import ( pinnipedconciergeclientsetscheme "go.pinniped.dev/generated/latest/client/concierge/clientset/versioned/scheme" pinnipedsupervisorclientset "go.pinniped.dev/generated/latest/client/supervisor/clientset/versioned" pinnipedsupervisorclientsetscheme "go.pinniped.dev/generated/latest/client/supervisor/clientset/versioned/scheme" + "go.pinniped.dev/internal/crypto/ptls" ) type Client struct { @@ -44,11 +53,16 @@ func New(opts ...Option) (*Client, error) { WithConfig(inClusterConfig)(c) // make sure all writes to clientConfig flow through one code path } + secureKubeConfig, err := createSecureKubeConfig(c.config) + if err != nil { + return nil, fmt.Errorf("could not create secure client config: %w", err) + } + // explicitly use json when talking to CRD APIs - jsonKubeConfig := createJSONKubeConfig(c.config) + jsonKubeConfig := createJSONKubeConfig(secureKubeConfig) // explicitly use protobuf when talking to built-in kube APIs - protoKubeConfig := createProtoKubeConfig(c.config) + protoKubeConfig := createProtoKubeConfig(secureKubeConfig) // Connect to the core Kubernetes API. k8sClient, err := kubernetes.NewForConfig(configWithWrapper(protoKubeConfig, kubescheme.Scheme, kubescheme.Codecs, c.middlewares, c.transportWrapper)) @@ -107,3 +121,142 @@ func createProtoKubeConfig(kubeConfig *restclient.Config) *restclient.Config { protoKubeConfig.ContentType = runtime.ContentTypeProtobuf return protoKubeConfig } + +// createSecureKubeConfig returns a copy of the input config with the WrapTransport +// enhanced to use the secure TLS configuration of the ptls / phttp packages. +func createSecureKubeConfig(kubeConfig *restclient.Config) (*restclient.Config, error) { + secureKubeConfig := restclient.CopyConfig(kubeConfig) + + // by setting proxy to always be non-nil, we bust the client-go global TLS config cache. + // this is required to make our wrapper function work without data races. the unit tests + // associated with this code run in parallel to assert that we are not using the cache. + // see k8s.io/client-go/transport.tlsConfigKey + if secureKubeConfig.Proxy == nil { + secureKubeConfig.Proxy = net.NewProxierWithNoProxyCIDR(http.ProxyFromEnvironment) + } + + // make sure restclient.TLSConfigFor always returns a non-nil TLS config + if len(secureKubeConfig.NextProtos) == 0 { + secureKubeConfig.NextProtos = ptls.Secure(nil).NextProtos + } + + tlsConfigTest, err := restclient.TLSConfigFor(secureKubeConfig) + if err != nil { + return nil, err // should never happen because our input config should always be valid + } + if tlsConfigTest == nil { + return nil, fmt.Errorf("unexpected empty TLS config") // should never happen because we set NextProtos above + } + + secureKubeConfig.Wrap(func(rt http.RoundTripper) http.RoundTripper { + defer func() { + if err := AssertSecureTransport(rt); err != nil { + panic(err) // not sure what the point of this function would be if it failed to make the config secure + } + }() + + tlsConfig, err := netTLSClientConfig(rt) + if err != nil { + // this assumes none of our production code calls Wrap or messes with WrapTransport. + // this is a reasonable assumption because all such code should live in this package + // and all such code should run after this function is called, not before. the kube + // codebase uses transport wrappers that can be unwrapped to access the underlying + // TLS config. + panic(err) + } + if tlsConfig == nil { + panic("unexpected empty TLS config") // we validate this case above via tlsConfigTest + } + + // mutate the TLS config into our desired state before it is used + ptls.Merge(ptls.Secure, tlsConfig) + + return rt // return the input transport since we mutated it in-place + }) + + if err := AssertSecureConfig(secureKubeConfig); err != nil { + return nil, err // not sure what the point of this function would be if it failed to make the config secure + } + + return secureKubeConfig, nil +} + +// SecureAnonymousClientConfig has the same properties as restclient.AnonymousClientConfig +// while still enforcing the secure TLS configuration of the ptls / phttp packages. +func SecureAnonymousClientConfig(kubeConfig *restclient.Config) *restclient.Config { + kubeConfig = restclient.AnonymousClientConfig(kubeConfig) + secureKubeConfig, err := createSecureKubeConfig(kubeConfig) + if err != nil { + panic(err) // should never happen as this would only fail on invalid CA data, which would never work anyway + } + if err := AssertSecureConfig(secureKubeConfig); err != nil { + panic(err) // not sure what the point of this function would be if it failed to make the config secure + } + return secureKubeConfig +} + +func AssertSecureConfig(kubeConfig *restclient.Config) error { + rt, err := restclient.TransportFor(kubeConfig) + if err != nil { + return fmt.Errorf("failed to build transport: %w", err) + } + + return AssertSecureTransport(rt) +} + +func AssertSecureTransport(rt http.RoundTripper) error { + tlsConfig, err := netTLSClientConfig(rt) + if err != nil { + return fmt.Errorf("failed to get TLS config: %w", err) + } + + tlsConfigCopy := tlsConfig.Clone() + ptls.Merge(ptls.Secure, tlsConfigCopy) // only mutate the copy + + //nolint: gosec // the empty TLS config here is not used + if diff := cmp.Diff(tlsConfigCopy, tlsConfig, + cmpopts.IgnoreUnexported(tls.Config{}, x509.CertPool{}), + cmpopts.IgnoreFields(tls.Config{}, "GetClientCertificate"), + ); len(diff) != 0 { + return fmt.Errorf("tls config is not secure:\n%s", diff) + } + + return nil +} + +func netTLSClientConfig(rt http.RoundTripper) (*tls.Config, error) { + tlsConfig, err := net.TLSClientConfig(rt) + if err == nil { + return tlsConfig, nil + } + + // TODO fix when we pick up https://github.com/kubernetes/kubernetes/pull/106014 + if err.Error() == "unknown transport type: *exec.roundTripper" { + return net.TLSClientConfig(extractRTUnsafe(rt)) + } + + return nil, err +} + +func extractRTUnsafe(rt http.RoundTripper) (out http.RoundTripper) { + for wrapper, ok := rt.(net.RoundTripperWrapper); ok; wrapper, ok = rt.(net.RoundTripperWrapper) { + // keep peeling the wrappers until we get to the exec.roundTripper + rt = wrapper.WrappedRoundTripper() + } + + // this is some dark magic to read a private field + baseField := reflect.ValueOf(rt).Elem().FieldByName("base") + basePointer := (*http.RoundTripper)(unsafe.Pointer(baseField.UnsafeAddr())) + + return *basePointer +} + +func Secure(config *restclient.Config) (kubernetes.Interface, *restclient.Config, error) { + // our middleware does not apply to the returned restclient.Config, therefore, this + // client not having a leader election lock is irrelevant since it would not be enforced + secureClient, err := New(WithConfig(config)) // handles nil config correctly + if err != nil { + return nil, nil, fmt.Errorf("failed to build secure client: %w", err) + } + return secureClient.Kubernetes, secureClient.ProtoConfig, nil +} diff --git a/internal/kubeclient/kubeclient_test.go b/internal/kubeclient/kubeclient_test.go index 07288238..2821760f 100644 --- a/internal/kubeclient/kubeclient_test.go +++ b/internal/kubeclient/kubeclient_test.go @@ -7,22 +7,31 @@ import ( "context" "fmt" "io" - "net" "net/http" + "net/url" "runtime" "strings" + "sync" "testing" + "time" "github.com/stretchr/testify/require" corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime/schema" + clientauthenticationv1 "k8s.io/client-go/pkg/apis/clientauthentication/v1" "k8s.io/client-go/rest" + clientcmdapi "k8s.io/client-go/tools/clientcmd/api" "k8s.io/client-go/transport" apiregistrationv1 "k8s.io/kube-aggregator/pkg/apis/apiregistration/v1" + // register all client-go auth plugins. + _ "k8s.io/client-go/plugin/pkg/client/auth" + conciergeconfigv1alpha1 "go.pinniped.dev/generated/latest/apis/concierge/config/v1alpha1" supervisorconfigv1alpha1 "go.pinniped.dev/generated/latest/apis/supervisor/config/v1alpha1" + "go.pinniped.dev/internal/crypto/ptls" + "go.pinniped.dev/internal/httputil/roundtripper" "go.pinniped.dev/internal/testutil/fakekubeapi" ) @@ -514,9 +523,12 @@ func TestKubeclient(t *testing.T) { return []*spyMiddleware{newSimpleMiddleware(t, true, false, false)} }, editRestConfig: func(t *testing.T, restConfig *rest.Config) { - restConfig.Dial = func(_ context.Context, _, _ string) (net.Conn, error) { - return nil, fmt.Errorf("some fake connection error") - } + // avoid messing with restConfig.Dial since it breaks client-go TLS cache logic + restConfig.Wrap(func(rt http.RoundTripper) http.RoundTripper { + return roundtripper.WrapFunc(rt, func(_ *http.Request) (*http.Response, error) { + return nil, fmt.Errorf("some fake connection error") + }) + }) }, reallyRunTest: func(t *testing.T, c *Client) { _, err := c.PinnipedSupervisor. @@ -591,8 +603,7 @@ func TestKubeclient(t *testing.T) { for _, test := range tests { test := test t.Run(test.name, func(t *testing.T) { - server, restConfig := fakekubeapi.Start(t, nil) - defer server.Close() + _, restConfig := fakekubeapi.Start(t, nil) if test.editRestConfig != nil { test.editRestConfig(t, restConfig) @@ -754,21 +765,45 @@ func newFailingMiddleware(t *testing.T, name string, mutateReqFails, mutateRespF } type wantCloser struct { - io.ReadCloser - closeCount int - closeCalls []string - couldReadBytesJustBeforeClosing bool + m sync.Mutex + + _rc io.ReadCloser + _closeCalls []string + _couldReadBytesJustBeforeClosing bool } -func (wc *wantCloser) Close() error { - wc.closeCount++ - wc.closeCalls = append(wc.closeCalls, getCaller()) - n, _ := wc.ReadCloser.Read([]byte{0}) +func (w *wantCloser) Close() error { + w.m.Lock() + defer w.m.Unlock() + + w._closeCalls = append(w._closeCalls, getCaller()) + n, _ := w._rc.Read([]byte{0}) if n > 0 { // there were still bytes left to be read - wc.couldReadBytesJustBeforeClosing = true + w._couldReadBytesJustBeforeClosing = true } - return wc.ReadCloser.Close() + return w._rc.Close() +} + +func (w *wantCloser) Read(p []byte) (int, error) { + w.m.Lock() + defer w.m.Unlock() + + return w._rc.Read(p) +} + +func (w *wantCloser) couldRead() bool { + w.m.Lock() + defer w.m.Unlock() + + return w._couldReadBytesJustBeforeClosing +} + +func (w *wantCloser) calls() []string { + w.m.Lock() + defer w.m.Unlock() + + return w._closeCalls } func getCaller() string { @@ -785,11 +820,14 @@ func getCaller() string { func wantCloseReqWrapper(t *testing.T) transport.WrapperFunc { caller := getCaller() return func(rt http.RoundTripper) http.RoundTripper { - return roundTripperFunc(func(req *http.Request) (bool, *http.Response, error) { + return roundtripper.WrapFunc(rt, roundTripperFunc(func(req *http.Request) (bool, *http.Response, error) { if req.Body != nil { - wc := &wantCloser{ReadCloser: req.Body} + wc := &wantCloser{_rc: req.Body} t.Cleanup(func() { - require.Equalf(t, wc.closeCount, 1, "did not close req body expected number of times at %s for req %#v; actual calls = %s", caller, req, wc.closeCalls) + require.Eventuallyf(t, func() bool { + return 1 == len(wc.calls()) + }, 5*time.Second, 100*time.Millisecond, + "did not close req body expected number of times at %s for req %#v; actual calls = %s", caller, req, wc.calls()) }) req.Body = wc } @@ -800,9 +838,12 @@ func wantCloseReqWrapper(t *testing.T) transport.WrapperFunc { if originalErr != nil { return nil, originalErr } - wc := &wantCloser{ReadCloser: originalBodyCopy} + wc := &wantCloser{_rc: originalBodyCopy} t.Cleanup(func() { - require.Equalf(t, wc.closeCount, 1, "did not close req body copy expected number of times at %s for req %#v; actual calls = %s", caller, req, wc.closeCalls) + require.Eventuallyf(t, func() bool { + return 1 == len(wc.calls()) + }, 5*time.Second, 100*time.Millisecond, + "did not close req body copy expected number of times at %s for req %#v; actual calls = %s", caller, req, wc.calls()) }) return wc, nil } @@ -810,7 +851,7 @@ func wantCloseReqWrapper(t *testing.T) transport.WrapperFunc { resp, err := rt.RoundTrip(req) return false, resp, err - }) + }).RoundTrip) } } @@ -819,20 +860,24 @@ func wantCloseReqWrapper(t *testing.T) transport.WrapperFunc { func wantCloseRespWrapper(t *testing.T) transport.WrapperFunc { caller := getCaller() return func(rt http.RoundTripper) http.RoundTripper { - return roundTripperFunc(func(req *http.Request) (bool, *http.Response, error) { + return roundtripper.WrapFunc(rt, roundTripperFunc(func(req *http.Request) (bool, *http.Response, error) { resp, err := rt.RoundTrip(req) if err != nil { // request failed, so there is no response body to watch for Close() calls on return false, resp, err } - wc := &wantCloser{ReadCloser: resp.Body} + wc := &wantCloser{_rc: resp.Body} t.Cleanup(func() { - require.False(t, wc.couldReadBytesJustBeforeClosing, "did not consume all response body bytes before closing %s", caller) - require.Equalf(t, wc.closeCount, 1, "did not close resp body expected number of times at %s for req %#v; actual calls = %s", caller, req, wc.closeCalls) + require.Eventuallyf(t, func() bool { + return wc.couldRead() == false && + 1 == len(wc.calls()) + }, 5*time.Second, 10*time.Millisecond, + `did not close resp body expected number of times at %s for req %#v; actual calls = %s +did not consume all response body bytes before closing %s, couldRead=%v`, caller, req, wc.calls(), caller, wc.couldRead()) }) resp.Body = wc return false, resp, err - }) + }).RoundTrip) } } @@ -895,3 +940,221 @@ func createGetFederationDomainTest(t *testing.T, client *Client) { require.NoError(t, err) require.Equal(t, goodFederationDomain, federationDomain) } + +// TestUnwrap ensures that the Client struct returned by this package only contains +// transports that can be fully unwrapped to get access to the underlying TLS config. +func TestUnwrap(t *testing.T) { + t.Parallel() // make sure to run in parallel to confirm that our client-go TLS cache busting works (i.e. assert no data races) + + server, restConfig := fakekubeapi.Start(t, nil) + + serverSubjects := server.Client().Transport.(*http.Transport).TLSClientConfig.RootCAs.Subjects() + + t.Run("regular client", func(t *testing.T) { + t.Parallel() // make sure to run in parallel to confirm that our client-go TLS cache busting works (i.e. assert no data races) + + regularClient := makeClient(t, restConfig, func(_ *rest.Config) {}) + + testUnwrap(t, regularClient, serverSubjects) + }) + + t.Run("exec client", func(t *testing.T) { + t.Parallel() // make sure to run in parallel to confirm that our client-go TLS cache busting works (i.e. assert no data races) + + execClient := makeClient(t, restConfig, func(config *rest.Config) { + config.ExecProvider = &clientcmdapi.ExecConfig{ + Command: "echo", + Args: []string{"pandas are awesome"}, + APIVersion: clientauthenticationv1.SchemeGroupVersion.String(), + InteractiveMode: clientcmdapi.NeverExecInteractiveMode, + } + }) + + testUnwrap(t, execClient, serverSubjects) + }) + + t.Run("gcp client", func(t *testing.T) { + t.Parallel() // make sure to run in parallel to confirm that our client-go TLS cache busting works (i.e. assert no data races) + + gcpClient := makeClient(t, restConfig, func(config *rest.Config) { + config.AuthProvider = &clientcmdapi.AuthProviderConfig{ + Name: "gcp", + } + }) + + testUnwrap(t, gcpClient, serverSubjects) + }) + + t.Run("oidc client", func(t *testing.T) { + t.Parallel() // make sure to run in parallel to confirm that our client-go TLS cache busting works (i.e. assert no data races) + + oidcClient := makeClient(t, restConfig, func(config *rest.Config) { + config.AuthProvider = &clientcmdapi.AuthProviderConfig{ + Name: "oidc", + Config: map[string]string{ + "idp-issuer-url": "https://pandas.local", + "client-id": "walrus", + }, + } + }) + + testUnwrap(t, oidcClient, serverSubjects) + }) + + t.Run("azure client", func(t *testing.T) { + t.Parallel() // make sure to run in parallel to confirm that our client-go TLS cache busting works (i.e. assert no data races) + + azureClient := makeClient(t, restConfig, func(config *rest.Config) { + config.AuthProvider = &clientcmdapi.AuthProviderConfig{ + Name: "azure", + Config: map[string]string{ + "client-id": "pinny", + "tenant-id": "danger", + "apiserver-id": "1234", + }, + } + }) + + testUnwrap(t, azureClient, serverSubjects) + }) +} + +func testUnwrap(t *testing.T, client *Client, serverSubjects [][]byte) { + tests := []struct { + name string + rt http.RoundTripper + }{ + { + name: "core v1", + rt: extractTransport(client.Kubernetes.CoreV1()), + }, + { + name: "coordination v1", + rt: extractTransport(client.Kubernetes.CoordinationV1()), + }, + { + name: "api registration v1", + rt: extractTransport(client.Aggregation.ApiregistrationV1()), + }, + { + name: "concierge login", + rt: extractTransport(client.PinnipedConcierge.LoginV1alpha1()), + }, + { + name: "concierge config", + rt: extractTransport(client.PinnipedConcierge.ConfigV1alpha1()), + }, + { + name: "supervisor idp", + rt: extractTransport(client.PinnipedSupervisor.IDPV1alpha1()), + }, + { + name: "supervisor config", + rt: extractTransport(client.PinnipedSupervisor.ConfigV1alpha1()), + }, + { + name: "json config", + rt: configToTransport(t, client.JSONConfig), + }, + { + name: "proto config", + rt: configToTransport(t, client.ProtoConfig), + }, + { + name: "anonymous json config", + rt: configToTransport(t, SecureAnonymousClientConfig(client.JSONConfig)), + }, + { + name: "anonymous proto config", + rt: configToTransport(t, SecureAnonymousClientConfig(client.ProtoConfig)), + }, + { + name: "json config - no cache", + rt: configToTransport(t, bustTLSCache(client.JSONConfig)), + }, + { + name: "proto config - no cache", + rt: configToTransport(t, bustTLSCache(client.ProtoConfig)), + }, + { + name: "anonymous json config - no cache, inner bust", + rt: configToTransport(t, SecureAnonymousClientConfig(bustTLSCache(client.JSONConfig))), + }, + { + name: "anonymous proto config - no cache, inner bust", + rt: configToTransport(t, SecureAnonymousClientConfig(bustTLSCache(client.ProtoConfig))), + }, + { + name: "anonymous json config - no cache, double bust", + rt: configToTransport(t, bustTLSCache(SecureAnonymousClientConfig(bustTLSCache(client.JSONConfig)))), + }, + { + name: "anonymous proto config - no cache, double bust", + rt: configToTransport(t, bustTLSCache(SecureAnonymousClientConfig(bustTLSCache(client.ProtoConfig)))), + }, + { + name: "anonymous json config - no cache, outer bust", + rt: configToTransport(t, bustTLSCache(SecureAnonymousClientConfig(client.JSONConfig))), + }, + { + name: "anonymous proto config - no cache, outer bust", + rt: configToTransport(t, bustTLSCache(SecureAnonymousClientConfig(client.ProtoConfig))), + }, + } + for _, tt := range tests { + tt := tt + t.Run(tt.name, func(t *testing.T) { + t.Parallel() // make sure to run in parallel to confirm that our client-go TLS cache busting works (i.e. assert no data races) + + tlsConfig, err := netTLSClientConfig(tt.rt) + require.NoError(t, err) + require.NotNil(t, tlsConfig) + + secureTLSConfig := ptls.Secure(nil) + + require.Equal(t, secureTLSConfig.MinVersion, tlsConfig.MinVersion) + require.Equal(t, secureTLSConfig.CipherSuites, tlsConfig.CipherSuites) + require.Equal(t, secureTLSConfig.NextProtos, tlsConfig.NextProtos) + + // x509.CertPool has some embedded functions that make it hard to compare so just look at the subjects + require.Equal(t, serverSubjects, tlsConfig.RootCAs.Subjects()) + }) + } +} + +type restClientGetter interface { + RESTClient() rest.Interface +} + +func extractTransport(getter restClientGetter) http.RoundTripper { + return getter.RESTClient().(*rest.RESTClient).Client.Transport +} + +func configToTransport(t *testing.T, config *rest.Config) http.RoundTripper { + t.Helper() + + rt, err := rest.TransportFor(config) + require.NoError(t, err) + return rt +} + +func bustTLSCache(config *rest.Config) *rest.Config { + c := rest.CopyConfig(config) + c.Proxy = func(h *http.Request) (*url.URL, error) { + return nil, nil // having a non-nil proxy func makes client-go not cache the TLS config + } + return c +} + +func makeClient(t *testing.T, restConfig *rest.Config, f func(*rest.Config)) *Client { + t.Helper() + + restConfig = rest.CopyConfig(restConfig) + + f(restConfig) + + client, err := New(WithConfig(restConfig)) + require.NoError(t, err) + + return client +} diff --git a/internal/kubeclient/roundtrip.go b/internal/kubeclient/roundtrip.go index 21b63eaa..57c8714d 100644 --- a/internal/kubeclient/roundtrip.go +++ b/internal/kubeclient/roundtrip.go @@ -20,6 +20,7 @@ import ( restclient "k8s.io/client-go/rest" "k8s.io/client-go/transport" + "go.pinniped.dev/internal/httputil/roundtripper" "go.pinniped.dev/internal/plog" ) @@ -78,7 +79,7 @@ func newWrapper( middlewares []Middleware, ) transport.WrapperFunc { return func(rt http.RoundTripper) http.RoundTripper { - return roundTripperFunc(func(req *http.Request) (bool, *http.Response, error) { + return roundtripper.WrapFunc(rt, roundTripperFunc(func(req *http.Request) (bool, *http.Response, error) { reqInfo, err := resolver.NewRequestInfo(reqWithoutPrefix(req, hostURL, apiPathPrefix)) if err != nil || !reqInfo.IsResourceRequest { resp, err := rt.RoundTrip(req) // we only handle kube resource requests @@ -120,7 +121,7 @@ func newWrapper( resp, err := rt.RoundTrip(req) // we only handle certain verbs return false, resp, err } - }) + }).RoundTrip) } } diff --git a/internal/localuserauthenticator/localuserauthenticator.go b/internal/localuserauthenticator/localuserauthenticator.go index 426530dc..3cc2f8d2 100644 --- a/internal/localuserauthenticator/localuserauthenticator.go +++ b/internal/localuserauthenticator/localuserauthenticator.go @@ -36,6 +36,7 @@ import ( "go.pinniped.dev/internal/constable" "go.pinniped.dev/internal/controller/apicerts" "go.pinniped.dev/internal/controllerlib" + "go.pinniped.dev/internal/crypto/ptls" "go.pinniped.dev/internal/dynamiccert" "go.pinniped.dev/internal/kubeclient" "go.pinniped.dev/internal/plog" @@ -71,16 +72,15 @@ func newWebhook( // start runs the webhook in a separate goroutine and returns whether or not the // webhook was started successfully. func (w *webhook) start(ctx context.Context, l net.Listener) error { + c := ptls.Secure(nil) + c.GetCertificate = func(_ *tls.ClientHelloInfo) (*tls.Certificate, error) { + certPEM, keyPEM := w.certProvider.CurrentCertKeyContent() + cert, err := tls.X509KeyPair(certPEM, keyPEM) + return &cert, err + } server := http.Server{ - Handler: w, - TLSConfig: &tls.Config{ - MinVersion: tls.VersionTLS13, - GetCertificate: func(_ *tls.ClientHelloInfo) (*tls.Certificate, error) { - certPEM, keyPEM := w.certProvider.CurrentCertKeyContent() - cert, err := tls.X509KeyPair(certPEM, keyPEM) - return &cert, err - }, - }, + Handler: w, + TLSConfig: c, } errCh := make(chan error) diff --git a/internal/localuserauthenticator/localuserauthenticator_test.go b/internal/localuserauthenticator/localuserauthenticator_test.go index 6a24e002..6e188ff5 100644 --- a/internal/localuserauthenticator/localuserauthenticator_test.go +++ b/internal/localuserauthenticator/localuserauthenticator_test.go @@ -6,7 +6,6 @@ package localuserauthenticator import ( "bytes" "context" - "crypto/tls" "crypto/x509" "encoding/json" "fmt" @@ -25,6 +24,7 @@ import ( corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime/schema" + utilnet "k8s.io/apimachinery/pkg/util/net" kubeinformers "k8s.io/client-go/informers" corev1informers "k8s.io/client-go/informers/core/v1" "k8s.io/client-go/kubernetes" @@ -32,6 +32,7 @@ import ( "go.pinniped.dev/internal/certauthority" "go.pinniped.dev/internal/dynamiccert" + "go.pinniped.dev/internal/net/phttp" ) func TestWebhook(t *testing.T) { @@ -102,7 +103,7 @@ func TestWebhook(t *testing.T) { defer func() { _ = l.Close() }() require.NoError(t, w.start(ctx, l)) - client := newClient(caBundle, serverName) + client := newClient(t, caBundle, serverName) goodURL := fmt.Sprintf("https://%s/authenticate", l.Addr().String()) goodRequestHeaders := map[string][]string{ @@ -475,18 +476,20 @@ func newCertProvider(t *testing.T) (dynamiccert.Private, []byte, string) { // newClient creates an http.Client that can be used to make an HTTPS call to a // service whose serving certs can be verified by the provided CA bundle. -func newClient(caBundle []byte, serverName string) *http.Client { +func newClient(t *testing.T, caBundle []byte, serverName string) *http.Client { + t.Helper() + rootCAs := x509.NewCertPool() - rootCAs.AppendCertsFromPEM(caBundle) - return &http.Client{ - Transport: &http.Transport{ - TLSClientConfig: &tls.Config{ - MinVersion: tls.VersionTLS13, - RootCAs: rootCAs, - ServerName: serverName, - }, - }, - } + ok := rootCAs.AppendCertsFromPEM(caBundle) + require.True(t, ok) + + c := phttp.Secure(rootCAs) + + tlsConfig, err := utilnet.TLSClientConfig(c.Transport) + require.NoError(t, err) + tlsConfig.ServerName = serverName + + return c } // newTokenReviewBody creates an io.ReadCloser that contains a JSON-encodeed diff --git a/internal/net/phttp/debug.go b/internal/net/phttp/debug.go new file mode 100644 index 00000000..32100195 --- /dev/null +++ b/internal/net/phttp/debug.go @@ -0,0 +1,118 @@ +// Copyright 2021 the Pinniped contributors. All Rights Reserved. +// SPDX-License-Identifier: Apache-2.0 + +package phttp + +import ( + "net/http" + "net/url" + + "k8s.io/client-go/transport" + + "go.pinniped.dev/internal/httputil/roundtripper" +) + +func safeDebugWrappers(rt http.RoundTripper, f transport.WrapperFunc, shouldLog func() bool) http.RoundTripper { + return roundtripper.WrapFunc(rt, func(req *http.Request) (*http.Response, error) { + // minor optimization to avoid the cleaning logic when the debug wrappers are unused + // note: do not make this entire wrapper conditional on shouldLog() - the output is allowed to change at runtime + if !shouldLog() { + return rt.RoundTrip(req) + } + + var ( + resp *http.Response + err error + ) + debugRT := f(roundtripper.Func(func(_ *http.Request) (*http.Response, error) { + // this call needs to be inside this closure so that the debug wrappers can time it + // note also that it takes the original (real) request + resp, err = rt.RoundTrip(req) + + cleanedResp := cleanResp(resp) // do not leak the user's password during the password grant + + return cleanedResp, err + })) + + // run the debug wrappers for their side effects (i.e. logging) + // the output is ignored because the input is not the real request + cleanedReq := cleanReq(req) // do not leak the user's password during the password grant + _, _ = debugRT.RoundTrip(cleanedReq) + + return resp, err + }) +} + +func cleanReq(req *http.Request) *http.Request { + // only pass back things we know to be safe to log + return &http.Request{ + Method: req.Method, + URL: cleanURL(req.URL), + Header: cleanHeader(req.Header), + } +} + +func cleanResp(resp *http.Response) *http.Response { + if resp == nil { + return nil + } + + // only pass back things we know to be safe to log + return &http.Response{ + Status: resp.Status, + Header: cleanHeader(resp.Header), + } +} + +func cleanURL(u *url.URL) *url.URL { + var user *url.Userinfo + if len(u.User.Username()) > 0 { + user = url.User("masked_username") + } + + var opaque string + if len(u.Opaque) > 0 { + opaque = "masked_opaque_data" + } + + var fragment string + if len(u.Fragment) > 0 || len(u.RawFragment) > 0 { + fragment = "masked_fragment" + } + + // only pass back things we know to be safe to log + return &url.URL{ + Scheme: u.Scheme, + Opaque: opaque, + User: user, + Host: u.Host, + Path: u.Path, + RawPath: u.RawPath, + ForceQuery: u.ForceQuery, + RawQuery: cleanQuery(u.Query()), + Fragment: fragment, + } +} + +func cleanQuery(query url.Values) string { + if len(query) == 0 { + return "" + } + + out := url.Values(cleanHeader(http.Header(query))) // cast so we can re-use logic + return out.Encode() +} + +func cleanHeader(header http.Header) http.Header { + if len(header) == 0 { + return nil + } + + mask := []string{"masked_value"} + out := make(http.Header, len(header)) + for key := range header { + out[key] = mask // only copy the keys + } + + return out +} diff --git a/internal/net/phttp/debug_test.go b/internal/net/phttp/debug_test.go new file mode 100644 index 00000000..e3faf899 --- /dev/null +++ b/internal/net/phttp/debug_test.go @@ -0,0 +1,340 @@ +// Copyright 2021 the Pinniped contributors. All Rights Reserved. +// SPDX-License-Identifier: Apache-2.0 + +package phttp + +import ( + "bytes" + "context" + "io" + "net/http" + "net/url" + "testing" + + "github.com/stretchr/testify/require" + + "go.pinniped.dev/internal/constable" + "go.pinniped.dev/internal/httputil/roundtripper" +) + +func Test_safeDebugWrappers_shouldLog(t *testing.T) { + t.Parallel() + + var rtFuncCalled, wrapFuncCalled, innerRTCalled int + var shouldLog, skipInnerRT bool + + shouldLogFunc := func() bool { return shouldLog } + + rtFunc := roundtripper.Func(func(_ *http.Request) (*http.Response, error) { + rtFuncCalled++ + return nil, nil + }) + + wrapFunc := func(rt http.RoundTripper) http.RoundTripper { + wrapFuncCalled++ + return roundtripper.Func(func(r *http.Request) (*http.Response, error) { + innerRTCalled++ + if skipInnerRT { + return nil, nil + } + return rt.RoundTrip(r) + }) + } + + r := testReq(t, nil, nil) + + out := safeDebugWrappers(rtFunc, wrapFunc, shouldLogFunc) + + // assert that shouldLogFunc is dynamically honored + + _, _ = out.RoundTrip(r) //nolint:bodyclose + + require.Equal(t, 1, rtFuncCalled) + require.Equal(t, 0, wrapFuncCalled) + require.Equal(t, 0, innerRTCalled) + + shouldLog = true + + _, _ = out.RoundTrip(r) //nolint:bodyclose + + require.Equal(t, 2, rtFuncCalled) + require.Equal(t, 1, wrapFuncCalled) + require.Equal(t, 1, innerRTCalled) + + shouldLog = false + + _, _ = out.RoundTrip(r) //nolint:bodyclose + + require.Equal(t, 3, rtFuncCalled) + require.Equal(t, 1, wrapFuncCalled) + require.Equal(t, 1, innerRTCalled) + + shouldLog = true + + _, _ = out.RoundTrip(r) //nolint:bodyclose + + require.Equal(t, 4, rtFuncCalled) + require.Equal(t, 2, wrapFuncCalled) + require.Equal(t, 2, innerRTCalled) + + // assert that wrapFunc controls rtFunc being called + + skipInnerRT = true + + _, _ = out.RoundTrip(r) //nolint:bodyclose + + require.Equal(t, 4, rtFuncCalled) + require.Equal(t, 3, wrapFuncCalled) + require.Equal(t, 3, innerRTCalled) + + skipInnerRT = false + + _, _ = out.RoundTrip(r) //nolint:bodyclose + + require.Equal(t, 5, rtFuncCalled) + require.Equal(t, 4, wrapFuncCalled) + require.Equal(t, 4, innerRTCalled) +} + +func Test_safeDebugWrappers_clean(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + shouldLog bool + inReq, wantReq *http.Request + inResp, wantResp *http.Response + inErr error + }{ + { + name: "header cleaned", + shouldLog: true, + inReq: testReq(t, http.Header{"hello": {"from", "earth"}}, nil), + wantReq: testCleanReq(http.Header{"hello": {"masked_value"}}, nil), + inResp: testResp(t, http.Header{"bye": {"for", "now"}}), //nolint:bodyclose + wantResp: testCleanResp(http.Header{"bye": {"masked_value"}}), //nolint:bodyclose + inErr: nil, + }, + { + name: "header cleaned error", + shouldLog: true, + inReq: testReq(t, http.Header{"see": {"from", "mars"}}, nil), + wantReq: testCleanReq(http.Header{"see": {"masked_value"}}, nil), + inResp: testResp(t, http.Header{"bear": {"is", "a"}}), //nolint:bodyclose + wantResp: testCleanResp(http.Header{"bear": {"masked_value"}}), //nolint:bodyclose + inErr: constable.Error("some error"), + }, + { + name: "header cleaned error nil resp", + shouldLog: true, + inReq: testReq(t, http.Header{"see": {"from", "mars"}}, nil), + wantReq: testCleanReq(http.Header{"see": {"masked_value"}}, nil), + inResp: nil, + wantResp: nil, + inErr: constable.Error("some other error"), + }, + { + name: "header cleaned no log", + shouldLog: false, + inReq: testReq(t, http.Header{"sky": {"is", "blue"}}, nil), + wantReq: nil, + inResp: testResp(t, http.Header{"night": {"is", "dark"}}), //nolint:bodyclose + wantResp: nil, + inErr: nil, + }, + { + name: "url cleaned, all fields", + shouldLog: true, + inReq: testReq(t, nil, &url.URL{ + Scheme: "sc", + Opaque: "op", + User: url.UserPassword("us", "pa"), + Host: "ho", + Path: "pa", + RawPath: "rap", + ForceQuery: true, + RawQuery: "key1=val1&key2=val2", + Fragment: "fra", + RawFragment: "rawf", + }), + wantReq: testCleanReq(nil, &url.URL{ + Scheme: "sc", + Opaque: "masked_opaque_data", + User: url.User("masked_username"), + Host: "ho", + Path: "pa", + RawPath: "rap", + ForceQuery: true, + RawQuery: "key1=masked_value&key2=masked_value", + Fragment: "masked_fragment", + RawFragment: "", + }), + inResp: testResp(t, http.Header{"sun": {"yellow"}}), //nolint:bodyclose + wantResp: testCleanResp(http.Header{"sun": {"masked_value"}}), //nolint:bodyclose + inErr: nil, + }, + { + name: "url cleaned, some fields", + shouldLog: true, + inReq: testReq(t, nil, &url.URL{ + Scheme: "sc", + Opaque: "", + User: nil, + Host: "ho", + Path: "pa", + RawPath: "rap", + ForceQuery: false, + RawQuery: "key3=val3&key4=val4", + Fragment: "", + RawFragment: "", + }), + wantReq: testCleanReq(nil, &url.URL{ + Scheme: "sc", + Opaque: "", + User: nil, + Host: "ho", + Path: "pa", + RawPath: "rap", + ForceQuery: false, + RawQuery: "key3=masked_value&key4=masked_value", + Fragment: "", + RawFragment: "", + }), + inResp: testResp(t, http.Header{"sun": {"yellow"}}), //nolint:bodyclose + wantResp: testCleanResp(http.Header{"sun": {"masked_value"}}), //nolint:bodyclose + inErr: nil, + }, + { + name: "header and url cleaned, all fields with error", + shouldLog: true, + inReq: testReq(t, http.Header{"zone": {"of", "the", "enders"}, "welcome": {"home"}}, &url.URL{ + Scheme: "sc2", + Opaque: "op2", + User: url.UserPassword("us2", "pa2"), + Host: "ho2", + Path: "pa2", + RawPath: "rap2", + ForceQuery: true, + RawQuery: "a=b&c=d&e=f&a=1&a=2", + Fragment: "fra2", + RawFragment: "rawf2", + }), + wantReq: testCleanReq(http.Header{"zone": {"masked_value"}, "welcome": {"masked_value"}}, &url.URL{ + Scheme: "sc2", + Opaque: "masked_opaque_data", + User: url.User("masked_username"), + Host: "ho2", + Path: "pa2", + RawPath: "rap2", + ForceQuery: true, + RawQuery: "a=masked_value&c=masked_value&e=masked_value", + Fragment: "masked_fragment", + RawFragment: "", + }), + inResp: testResp(t, http.Header{"moon": {"white"}}), //nolint:bodyclose + wantResp: testCleanResp(http.Header{"moon": {"masked_value"}}), //nolint:bodyclose + inErr: constable.Error("yay pandas"), + }, + } + for _, tt := range tests { + tt := tt + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + var rtCalled, wrapCalled, innerCalled bool + + rtFunc := roundtripper.Func(func(r *http.Request) (*http.Response, error) { + rtCalled = true + require.Equal(t, tt.inReq, r) + return tt.inResp, tt.inErr + }) + + var gotReq *http.Request + var gotResp *http.Response + var gotErr error + + wrapFunc := func(rt http.RoundTripper) http.RoundTripper { + wrapCalled = true + return roundtripper.Func(func(r *http.Request) (*http.Response, error) { + innerCalled = true + + gotReq = r + + resp, err := rt.RoundTrip(r) //nolint:bodyclose + + gotResp = resp + gotErr = err + + return resp, err + }) + } + + out := safeDebugWrappers(rtFunc, wrapFunc, func() bool { return tt.shouldLog }) + + resp, err := out.RoundTrip(tt.inReq) //nolint:bodyclose + + require.Equal(t, tt.inResp, resp) + require.Equal(t, tt.inErr, err) + require.True(t, rtCalled) + require.Equal(t, tt.shouldLog, wrapCalled) + require.Equal(t, tt.shouldLog, innerCalled) + + require.Equal(t, tt.wantReq, gotReq) + require.Equal(t, tt.wantResp, gotResp) + require.Equal(t, tt.inErr, gotErr) + }) + } +} + +func testReq(t *testing.T, header http.Header, u *url.URL) *http.Request { + t.Helper() + + r, err := http.NewRequestWithContext(context.Background(), http.MethodGet, "https://overwritten.com", nil) + require.NoError(t, err) + + if u == nil { + u = &url.URL{} + } + + r.URL = u + r.Header = header + + // something non-nil for testing + r.Body = io.NopCloser(&bytes.Buffer{}) + r.Form = url.Values{"a": {"b"}} + r.PostForm = url.Values{"c": {"d"}} + + return r +} + +func testResp(t *testing.T, header http.Header) *http.Response { + t.Helper() + + return &http.Response{ + Status: "pandas are the best", + Header: header, + + // something non-nil for testing + Body: io.NopCloser(&bytes.Buffer{}), + Request: testReq(t, header, nil), + } +} + +func testCleanReq(header http.Header, u *url.URL) *http.Request { + if u == nil { + u = &url.URL{} + } + + return &http.Request{ + Method: http.MethodGet, + URL: u, + Header: header, + } +} + +func testCleanResp(header http.Header) *http.Response { + return &http.Response{ + Status: "pandas are the best", + Header: header, + } +} diff --git a/internal/net/phttp/phttp.go b/internal/net/phttp/phttp.go new file mode 100644 index 00000000..2a75ddda --- /dev/null +++ b/internal/net/phttp/phttp.go @@ -0,0 +1,48 @@ +// Copyright 2021 the Pinniped contributors. All Rights Reserved. +// SPDX-License-Identifier: Apache-2.0 + +package phttp + +import ( + "crypto/x509" + "net/http" + "time" + + "k8s.io/apimachinery/pkg/util/net" + "k8s.io/client-go/rest" + "k8s.io/client-go/transport" + + "go.pinniped.dev/internal/crypto/ptls" + "go.pinniped.dev/internal/plog" +) + +func Default(rootCAs *x509.CertPool) *http.Client { + return buildClient(ptls.Default, rootCAs) +} + +func Secure(rootCAs *x509.CertPool) *http.Client { + return buildClient(ptls.Secure, rootCAs) +} + +func buildClient(tlsConfigFunc ptls.ConfigFunc, rootCAs *x509.CertPool) *http.Client { + baseRT := defaultTransport() + baseRT.TLSClientConfig = tlsConfigFunc(rootCAs) + + return &http.Client{ + Transport: defaultWrap(baseRT), + Timeout: 3 * time.Hour, // make it impossible for requests to hang indefinitely + } +} + +func defaultTransport() *http.Transport { + baseRT := http.DefaultTransport.(*http.Transport).Clone() + net.SetTransportDefaults(baseRT) + baseRT.MaxIdleConnsPerHost = 25 // copied from client-go + return baseRT +} + +func defaultWrap(rt http.RoundTripper) http.RoundTripper { + rt = safeDebugWrappers(rt, transport.DebugWrappers, func() bool { return plog.Enabled(plog.LevelTrace) }) + rt = transport.NewUserAgentRoundTripper(rest.DefaultKubernetesUserAgent(), rt) + return rt +} diff --git a/internal/net/phttp/phttp_test.go b/internal/net/phttp/phttp_test.go new file mode 100644 index 00000000..83792df1 --- /dev/null +++ b/internal/net/phttp/phttp_test.go @@ -0,0 +1,116 @@ +// Copyright 2021 the Pinniped contributors. All Rights Reserved. +// SPDX-License-Identifier: Apache-2.0 + +package phttp + +import ( + "context" + "crypto/tls" + "crypto/x509" + "net/http" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "k8s.io/apimachinery/pkg/util/net" + "k8s.io/client-go/util/cert" + + "go.pinniped.dev/internal/crypto/ptls" + "go.pinniped.dev/internal/testutil/tlsserver" +) + +// TestUnwrap ensures that the http.Client structs returned by this package contain +// a transport that can be fully unwrapped to get access to the underlying TLS config. +func TestUnwrap(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + f func(*x509.CertPool) *http.Client + }{ + { + name: "default", + f: Default, + }, + { + name: "secure", + f: Secure, + }, + } + for _, tt := range tests { + tt := tt + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + p, err := x509.SystemCertPool() + require.NoError(t, err) + + c := tt.f(p) + + tlsConfig, err := net.TLSClientConfig(c.Transport) + require.NoError(t, err) + require.NotNil(t, tlsConfig) + + require.NotEmpty(t, tlsConfig.NextProtos) + require.GreaterOrEqual(t, tlsConfig.MinVersion, uint16(tls.VersionTLS12)) + require.Equal(t, p, tlsConfig.RootCAs) + }) + } +} + +func TestClient(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + clientFunc func(*x509.CertPool) *http.Client + configFunc ptls.ConfigFunc + }{ + { + name: "default", + clientFunc: Default, + configFunc: ptls.Default, + }, + { + name: "secure", + clientFunc: Secure, + configFunc: ptls.Secure, + }, + } + for _, tt := range tests { + tt := tt + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + var sawRequest bool + server := tlsserver.TLSTestServer(t, http.HandlerFunc(func(_ http.ResponseWriter, r *http.Request) { + tlsserver.AssertTLS(t, r, tt.configFunc) + assertUserAgent(t, r) + sawRequest = true + }), tlsserver.RecordTLSHello) + + rootCAs, err := cert.NewPoolFromBytes(tlsserver.TLSTestServerCA(server)) + require.NoError(t, err) + + c := tt.clientFunc(rootCAs) + + req, err := http.NewRequestWithContext(context.Background(), http.MethodGet, server.URL, nil) + require.NoError(t, err) + + resp, err := c.Do(req) + require.NoError(t, err) + require.NoError(t, resp.Body.Close()) + + require.True(t, sawRequest) + }) + } +} + +func assertUserAgent(t *testing.T, r *http.Request) { + t.Helper() + + ua := r.Header.Get("user-agent") + + // use assert instead of require to not break the http.Handler with a panic + assert.Contains(t, ua, ") kubernetes/") +} diff --git a/internal/oidc/downstreamsession/downstream_session.go b/internal/oidc/downstreamsession/downstream_session.go index 8618ab57..5f0cf5f0 100644 --- a/internal/oidc/downstreamsession/downstream_session.go +++ b/internal/oidc/downstreamsession/downstream_session.go @@ -9,7 +9,7 @@ import ( "net/url" "time" - oidc2 "github.com/coreos/go-oidc/v3/oidc" + coreosoidc "github.com/coreos/go-oidc/v3/oidc" "github.com/ory/fosite" "github.com/ory/fosite/handler/openid" "github.com/ory/fosite/token/jwt" @@ -60,8 +60,8 @@ func MakeDownstreamSession(subject string, username string, groups []string, cus // GrantScopesIfRequested auto-grants the scopes for which we do not require end-user approval, if they were requested. func GrantScopesIfRequested(authorizeRequester fosite.AuthorizeRequester) { - oidc.GrantScopeIfRequested(authorizeRequester, oidc2.ScopeOpenID) - oidc.GrantScopeIfRequested(authorizeRequester, oidc2.ScopeOfflineAccess) + oidc.GrantScopeIfRequested(authorizeRequester, coreosoidc.ScopeOpenID) + oidc.GrantScopeIfRequested(authorizeRequester, coreosoidc.ScopeOfflineAccess) oidc.GrantScopeIfRequested(authorizeRequester, "pinniped:request-audience") } diff --git a/internal/plog/klog.go b/internal/plog/klog.go index 9544d886..7669508e 100644 --- a/internal/plog/klog.go +++ b/internal/plog/klog.go @@ -33,7 +33,7 @@ func RemoveKlogGlobalFlags() { } } -// KRef is (mostly) copied from klog - it is a standard way to represent a a metav1.Object in logs +// KRef is (mostly) copied from klog - it is a standard way to represent a metav1.Object in logs // when you only have access to the namespace and name of the object. func KRef(namespace, name string) string { return fmt.Sprintf("%s/%s", namespace, name) @@ -44,33 +44,19 @@ func KObj(obj klog.KMetadata) string { return fmt.Sprintf("%s/%s", obj.GetNamespace(), obj.GetName()) } -func klogLevelForPlogLevel(plogLevel LogLevel) (klogLevel klog.Level) { +func klogLevelForPlogLevel(plogLevel LogLevel) klog.Level { switch plogLevel { case LevelWarning: - klogLevel = klogLevelWarning // unset means minimal logs (Error and Warning) + return klogLevelWarning // unset means minimal logs (Error and Warning) case LevelInfo: - klogLevel = klogLevelInfo + return klogLevelInfo case LevelDebug: - klogLevel = klogLevelDebug + return klogLevelDebug case LevelTrace: - klogLevel = klogLevelTrace + return klogLevelTrace case LevelAll: - klogLevel = klogLevelAll + 100 // make all really mean all + return klogLevelAll + 100 // make all really mean all default: - klogLevel = -1 + return -1 } - - return -} - -func getKlogLevel() klog.Level { - // hack around klog not exposing a Get method - for i := klog.Level(0); i < 256; i++ { - if klog.V(i).Enabled() { - continue - } - return i - 1 - } - - return -1 } diff --git a/internal/plog/level.go b/internal/plog/level.go index 20083d68..0f8401d6 100644 --- a/internal/plog/level.go +++ b/internal/plog/level.go @@ -7,6 +7,7 @@ import ( "strconv" "k8s.io/component-base/logs" + "k8s.io/klog/v2" "go.pinniped.dev/internal/constable" ) @@ -54,5 +55,5 @@ func ValidateAndSetLogLevelGlobally(level LogLevel) error { // Enabled returns whether the provided plog level is enabled, i.e., whether print statements at the // provided level will show up. func Enabled(level LogLevel) bool { - return getKlogLevel() >= klogLevelForPlogLevel(level) + return klog.V(klogLevelForPlogLevel(level)).Enabled() } diff --git a/internal/plog/level_test.go b/internal/plog/level_test.go index a2fde0b6..9da066f4 100644 --- a/internal/plog/level_test.go +++ b/internal/plog/level_test.go @@ -114,3 +114,15 @@ func undoGlobalLogLevelChanges(t *testing.T, originalLogLevel klog.Level) { _, err := logs.GlogSetter(strconv.Itoa(int(originalLogLevel))) require.NoError(t, err) } + +func getKlogLevel() klog.Level { + // hack around klog not exposing a Get method + for i := klog.Level(0); i < 256; i++ { + if klog.V(i).Enabled() { + continue + } + return i - 1 + } + + return -1 +} diff --git a/internal/plog/plog.go b/internal/plog/plog.go index cf6bc80b..5fbfb512 100644 --- a/internal/plog/plog.go +++ b/internal/plog/plog.go @@ -26,13 +26,11 @@ // act of desperation to determine why the system is broken. package plog -import ( - "k8s.io/klog/v2" -) +import "k8s.io/klog/v2" const errorKey = "error" -type _ interface { +type Logger interface { Error(msg string, err error, keysAndValues ...interface{}) Warning(msg string, keysAndValues ...interface{}) WarningErr(msg string, err error, keysAndValues ...interface{}) @@ -45,23 +43,23 @@ type _ interface { All(msg string, keysAndValues ...interface{}) } -type PLogger struct { +type pLogger struct { prefix string depth int } -func New(prefix string) PLogger { - return PLogger{ +func New(prefix string) Logger { + return &pLogger{ depth: 0, prefix: prefix, } } -func (p *PLogger) Error(msg string, err error, keysAndValues ...interface{}) { +func (p *pLogger) Error(msg string, err error, keysAndValues ...interface{}) { klog.ErrorSDepth(p.depth+1, err, p.prefix+msg, keysAndValues...) } -func (p *PLogger) warningDepth(msg string, depth int, keysAndValues ...interface{}) { +func (p *pLogger) warningDepth(msg string, depth int, keysAndValues ...interface{}) { // klog's structured logging has no concept of a warning (i.e. no WarningS function) // Thus we use info at log level zero as a proxy // klog's info logs have an I prefix and its warning logs have a W prefix @@ -72,111 +70,111 @@ func (p *PLogger) warningDepth(msg string, depth int, keysAndValues ...interface } } -func (p *PLogger) Warning(msg string, keysAndValues ...interface{}) { +func (p *pLogger) Warning(msg string, keysAndValues ...interface{}) { p.warningDepth(msg, p.depth+1, keysAndValues...) } // Use WarningErr to issue a Warning message with an error object as part of the message. -func (p *PLogger) WarningErr(msg string, err error, keysAndValues ...interface{}) { +func (p *pLogger) WarningErr(msg string, err error, keysAndValues ...interface{}) { p.warningDepth(msg, p.depth+1, append([]interface{}{errorKey, err}, keysAndValues...)...) } -func (p *PLogger) infoDepth(msg string, depth int, keysAndValues ...interface{}) { +func (p *pLogger) infoDepth(msg string, depth int, keysAndValues ...interface{}) { if klog.V(klogLevelInfo).Enabled() { klog.InfoSDepth(depth+1, p.prefix+msg, keysAndValues...) } } -func (p *PLogger) Info(msg string, keysAndValues ...interface{}) { +func (p *pLogger) Info(msg string, keysAndValues ...interface{}) { p.infoDepth(msg, p.depth+1, keysAndValues...) } // Use InfoErr to log an expected error, e.g. validation failure of an http parameter. -func (p *PLogger) InfoErr(msg string, err error, keysAndValues ...interface{}) { +func (p *pLogger) InfoErr(msg string, err error, keysAndValues ...interface{}) { p.infoDepth(msg, p.depth+1, append([]interface{}{errorKey, err}, keysAndValues...)...) } -func (p *PLogger) debugDepth(msg string, depth int, keysAndValues ...interface{}) { +func (p *pLogger) debugDepth(msg string, depth int, keysAndValues ...interface{}) { if klog.V(klogLevelDebug).Enabled() { klog.InfoSDepth(depth+1, p.prefix+msg, keysAndValues...) } } -func (p *PLogger) Debug(msg string, keysAndValues ...interface{}) { +func (p *pLogger) Debug(msg string, keysAndValues ...interface{}) { p.debugDepth(msg, p.depth+1, keysAndValues...) } // Use DebugErr to issue a Debug message with an error object as part of the message. -func (p *PLogger) DebugErr(msg string, err error, keysAndValues ...interface{}) { +func (p *pLogger) DebugErr(msg string, err error, keysAndValues ...interface{}) { p.debugDepth(msg, p.depth+1, append([]interface{}{errorKey, err}, keysAndValues...)...) } -func (p *PLogger) traceDepth(msg string, depth int, keysAndValues ...interface{}) { +func (p *pLogger) traceDepth(msg string, depth int, keysAndValues ...interface{}) { if klog.V(klogLevelTrace).Enabled() { klog.InfoSDepth(depth+1, p.prefix+msg, keysAndValues...) } } -func (p *PLogger) Trace(msg string, keysAndValues ...interface{}) { +func (p *pLogger) Trace(msg string, keysAndValues ...interface{}) { p.traceDepth(msg, p.depth+1, keysAndValues...) } // Use TraceErr to issue a Trace message with an error object as part of the message. -func (p *PLogger) TraceErr(msg string, err error, keysAndValues ...interface{}) { +func (p *pLogger) TraceErr(msg string, err error, keysAndValues ...interface{}) { p.traceDepth(msg, p.depth+1, append([]interface{}{errorKey, err}, keysAndValues...)...) } -func (p *PLogger) All(msg string, keysAndValues ...interface{}) { +func (p *pLogger) All(msg string, keysAndValues ...interface{}) { if klog.V(klogLevelAll).Enabled() { klog.InfoSDepth(p.depth+1, p.prefix+msg, keysAndValues...) } } -var pLogger = PLogger{ //nolint:gochecknoglobals +var logger Logger = &pLogger{ //nolint:gochecknoglobals depth: 1, } // Use Error to log an unexpected system error. func Error(msg string, err error, keysAndValues ...interface{}) { - pLogger.Error(msg, err, keysAndValues...) + logger.Error(msg, err, keysAndValues...) } func Warning(msg string, keysAndValues ...interface{}) { - pLogger.Warning(msg, keysAndValues...) + logger.Warning(msg, keysAndValues...) } // Use WarningErr to issue a Warning message with an error object as part of the message. func WarningErr(msg string, err error, keysAndValues ...interface{}) { - pLogger.WarningErr(msg, err, keysAndValues...) + logger.WarningErr(msg, err, keysAndValues...) } func Info(msg string, keysAndValues ...interface{}) { - pLogger.Info(msg, keysAndValues...) + logger.Info(msg, keysAndValues...) } // Use InfoErr to log an expected error, e.g. validation failure of an http parameter. func InfoErr(msg string, err error, keysAndValues ...interface{}) { - pLogger.InfoErr(msg, err, keysAndValues...) + logger.InfoErr(msg, err, keysAndValues...) } func Debug(msg string, keysAndValues ...interface{}) { - pLogger.Debug(msg, keysAndValues...) + logger.Debug(msg, keysAndValues...) } // Use DebugErr to issue a Debug message with an error object as part of the message. func DebugErr(msg string, err error, keysAndValues ...interface{}) { - pLogger.DebugErr(msg, err, keysAndValues...) + logger.DebugErr(msg, err, keysAndValues...) } func Trace(msg string, keysAndValues ...interface{}) { - pLogger.Trace(msg, keysAndValues...) + logger.Trace(msg, keysAndValues...) } // Use TraceErr to issue a Trace message with an error object as part of the message. func TraceErr(msg string, err error, keysAndValues ...interface{}) { - pLogger.TraceErr(msg, err, keysAndValues...) + logger.TraceErr(msg, err, keysAndValues...) } func All(msg string, keysAndValues ...interface{}) { - pLogger.All(msg, keysAndValues...) + logger.All(msg, keysAndValues...) } diff --git a/internal/supervisor/server/server.go b/internal/supervisor/server/server.go index 805c4d3e..85211f9a 100644 --- a/internal/supervisor/server/server.go +++ b/internal/supervisor/server/server.go @@ -41,6 +41,7 @@ import ( "go.pinniped.dev/internal/controller/supervisorstorage" "go.pinniped.dev/internal/controllerinit" "go.pinniped.dev/internal/controllerlib" + "go.pinniped.dev/internal/crypto/ptls" "go.pinniped.dev/internal/deploymentref" "go.pinniped.dev/internal/downward" "go.pinniped.dev/internal/groupsuffix" @@ -392,23 +393,22 @@ func runSupervisor(podInfo *downward.PodInfo, cfg *supervisor.Config) error { defer func() { _ = httpListener.Close() }() startServer(ctx, shutdown, httpListener, oidProvidersManager) + c := ptls.Default(nil) + c.GetCertificate = func(info *tls.ClientHelloInfo) (*tls.Certificate, error) { + cert := dynamicTLSCertProvider.GetTLSCert(strings.ToLower(info.ServerName)) + defaultCert := dynamicTLSCertProvider.GetDefaultTLSCert() + plog.Debug("GetCertificate called for port 8443", + "info.ServerName", info.ServerName, + "foundSNICert", cert != nil, + "foundDefaultCert", defaultCert != nil, + ) + if cert == nil { + cert = defaultCert + } + return cert, nil + } //nolint: gosec // Intentionally binding to all network interfaces. - httpsListener, err := tls.Listen("tcp", ":8443", &tls.Config{ - MinVersion: tls.VersionTLS12, // Allow v1.2 because clients like the default `curl` on MacOS don't support 1.3 yet. - GetCertificate: func(info *tls.ClientHelloInfo) (*tls.Certificate, error) { - cert := dynamicTLSCertProvider.GetTLSCert(strings.ToLower(info.ServerName)) - defaultCert := dynamicTLSCertProvider.GetDefaultTLSCert() - plog.Debug("GetCertificate called for port 8443", - "info.ServerName", info.ServerName, - "foundSNICert", cert != nil, - "foundDefaultCert", defaultCert != nil, - ) - if cert == nil { - cert = defaultCert - } - return cert, nil - }, - }) + httpsListener, err := tls.Listen("tcp", ":8443", c) if err != nil { return fmt.Errorf("cannot create listener: %w", err) } diff --git a/internal/testutil/fakekubeapi/fakekubeapi.go b/internal/testutil/fakekubeapi/fakekubeapi.go index b0ccf995..ec15de05 100644 --- a/internal/testutil/fakekubeapi/fakekubeapi.go +++ b/internal/testutil/fakekubeapi/fakekubeapi.go @@ -18,7 +18,6 @@ package fakekubeapi import ( - "encoding/pem" "fmt" "io/ioutil" "mime" @@ -37,7 +36,9 @@ import ( pinnipedconciergeclientsetscheme "go.pinniped.dev/generated/latest/client/concierge/clientset/versioned/scheme" pinnipedsupervisorclientsetscheme "go.pinniped.dev/generated/latest/client/supervisor/clientset/versioned/scheme" + "go.pinniped.dev/internal/crypto/ptls" "go.pinniped.dev/internal/httputil/httperr" + "go.pinniped.dev/internal/testutil/tlsserver" ) // Start starts an httptest.Server (with TLS) that pretends to be a Kube API server. @@ -54,7 +55,9 @@ func Start(t *testing.T, resources map[string]runtime.Object) (*httptest.Server, resources = make(map[string]runtime.Object) } - server := httptest.NewTLSServer(httperr.HandlerFunc(func(w http.ResponseWriter, r *http.Request) (err error) { + server := tlsserver.TLSTestServer(t, httperr.HandlerFunc(func(w http.ResponseWriter, r *http.Request) error { + tlsserver.AssertTLS(t, r, ptls.Secure) + obj, err := decodeObj(r) if err != nil { return err @@ -74,11 +77,11 @@ func Start(t *testing.T, resources map[string]runtime.Object) (*httptest.Server, } return nil - })) + }), tlsserver.RecordTLSHello) restConfig := &restclient.Config{ Host: server.URL, TLSClientConfig: restclient.TLSClientConfig{ - CAData: pem.EncodeToMemory(&pem.Block{Bytes: server.Certificate().Raw, Type: "CERTIFICATE"}), + CAData: tlsserver.TLSTestServerCA(server), }, } return server, restConfig diff --git a/internal/testutil/tlsserver.go b/internal/testutil/tlsserver.go index 13e39324..b2d3eb46 100644 --- a/internal/testutil/tlsserver.go +++ b/internal/testutil/tlsserver.go @@ -5,39 +5,36 @@ package testutil import ( "crypto/tls" - "encoding/pem" "errors" "net" "net/http" - "net/http/httptest" "testing" "github.com/stretchr/testify/require" + + "go.pinniped.dev/internal/crypto/ptls" + "go.pinniped.dev/internal/testutil/tlsserver" ) // TLSTestServer starts a test server listening on a local port using a test CA. It returns the PEM CA bundle and the // URL of the listening server. The lifetime of the server is bound to the provided *testing.T. -func TLSTestServer(t *testing.T, handler http.HandlerFunc) (caBundlePEM string, url string) { +func TLSTestServer(t *testing.T, handler http.HandlerFunc) (caBundlePEM, url string) { t.Helper() - server := httptest.NewTLSServer(handler) - t.Cleanup(server.Close) - caBundle := string(pem.EncodeToMemory(&pem.Block{ - Type: "CERTIFICATE", - Bytes: server.TLS.Certificates[0].Certificate[0], - })) - return caBundle, server.URL + server := tlsserver.TLSTestServer(t, handler, nil) + + return string(tlsserver.TLSTestServerCA(server)), server.URL } func TLSTestServerWithCert(t *testing.T, handler http.HandlerFunc, certificate *tls.Certificate) (url string) { t.Helper() + c := ptls.Default(nil) // mimic API server config + c.Certificates = []tls.Certificate{*certificate} + server := http.Server{ - TLSConfig: &tls.Config{ - Certificates: []tls.Certificate{*certificate}, - MinVersion: tls.VersionTLS12, - }, - Handler: handler, + TLSConfig: c, + Handler: handler, } l, err := net.Listen("tcp", "127.0.0.1:0") diff --git a/internal/testutil/tlsserver/tlsserver.go b/internal/testutil/tlsserver/tlsserver.go new file mode 100644 index 00000000..425c43c9 --- /dev/null +++ b/internal/testutil/tlsserver/tlsserver.go @@ -0,0 +1,110 @@ +// Copyright 2021 the Pinniped contributors. All Rights Reserved. +// SPDX-License-Identifier: Apache-2.0 + +package tlsserver + +import ( + "context" + "crypto/tls" + "encoding/pem" + "fmt" + "net" + "net/http" + "net/http/httptest" + "reflect" + "sync" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "k8s.io/apimachinery/pkg/util/httpstream" + + "go.pinniped.dev/internal/crypto/ptls" +) + +type ctxKey int + +const ( + mapKey ctxKey = iota + 1 + helloKey +) + +func TLSTestServer(t *testing.T, handler http.Handler, f func(*httptest.Server)) *httptest.Server { + t.Helper() + + server := httptest.NewUnstartedServer(handler) + server.TLS = ptls.Default(nil) // mimic API server config + if f != nil { + f(server) + } + server.StartTLS() + t.Cleanup(server.Close) + return server +} + +func TLSTestServerCA(server *httptest.Server) []byte { + return pem.EncodeToMemory(&pem.Block{ + Type: "CERTIFICATE", + Bytes: server.Certificate().Raw, + }) +} + +func RecordTLSHello(server *httptest.Server) { + server.Config.ConnContext = func(ctx context.Context, _ net.Conn) context.Context { + return context.WithValue(ctx, mapKey, &sync.Map{}) + } + + server.TLS.GetConfigForClient = func(info *tls.ClientHelloInfo) (*tls.Config, error) { + m, ok := getCtxMap(info.Context()) + if !ok { + return nil, fmt.Errorf("could not find ctx map") + } + if actual, loaded := m.LoadOrStore(helloKey, info); loaded && !reflect.DeepEqual(info, actual) { + return nil, fmt.Errorf("different client hello seen") + } + return nil, nil + } +} + +func AssertTLS(t *testing.T, r *http.Request, tlsConfigFunc ptls.ConfigFunc) { + t.Helper() + + m, ok := getCtxMap(r.Context()) + require.True(t, ok) + + h, ok := m.Load(helloKey) + require.True(t, ok) + + info, ok := h.(*tls.ClientHelloInfo) + require.True(t, ok) + + tlsConfig := tlsConfigFunc(nil) + + supportedVersions := []uint16{tlsConfig.MinVersion} + ciphers := tlsConfig.CipherSuites + + if secureTLSConfig := ptls.Secure(nil); tlsConfig.MinVersion != secureTLSConfig.MinVersion { + supportedVersions = append([]uint16{secureTLSConfig.MinVersion}, supportedVersions...) + ciphers = append(ciphers, secureTLSConfig.CipherSuites...) + } + + protos := tlsConfig.NextProtos + if httpstream.IsUpgradeRequest(r) { + protos = tlsConfig.NextProtos[1:] + } + + // use assert instead of require to not break the http.Handler with a panic + ok1 := assert.Equal(t, supportedVersions, info.SupportedVersions) + ok2 := assert.Equal(t, ciphers, info.CipherSuites) + ok3 := assert.Equal(t, protos, info.SupportedProtos) + + if all := ok1 && ok2 && ok3; !all { + t.Errorf("insecure TLS detected for %q %q %q upgrade=%v supportedVersions=%v ciphers=%v protos=%v", + r.Proto, r.Method, r.URL.String(), httpstream.IsUpgradeRequest(r), ok1, ok2, ok3) + } +} + +func getCtxMap(ctx context.Context) (*sync.Map, bool) { + m, ok := ctx.Value(mapKey).(*sync.Map) + return m, ok +} diff --git a/internal/upstreamldap/upstreamldap.go b/internal/upstreamldap/upstreamldap.go index 1f0587de..b69b37354 100644 --- a/internal/upstreamldap/upstreamldap.go +++ b/internal/upstreamldap/upstreamldap.go @@ -25,6 +25,7 @@ import ( "k8s.io/utils/trace" "go.pinniped.dev/internal/authenticators" + "go.pinniped.dev/internal/crypto/ptls" "go.pinniped.dev/internal/endpointaddr" "go.pinniped.dev/internal/oidc/downstreamsession" "go.pinniped.dev/internal/oidc/provider" @@ -328,7 +329,7 @@ func (p *Provider) tlsConfig() (*tls.Config, error) { return nil, fmt.Errorf("could not parse CA bundle") } } - return &tls.Config{MinVersion: tls.VersionTLS12, RootCAs: rootCAs}, nil + return ptls.DefaultLDAP(rootCAs), nil } // A name for this upstream provider. diff --git a/internal/upstreamldap/upstreamldap_test.go b/internal/upstreamldap/upstreamldap_test.go index 6a9eca34..ca2f3e6b 100644 --- a/internal/upstreamldap/upstreamldap_test.go +++ b/internal/upstreamldap/upstreamldap_test.go @@ -11,6 +11,7 @@ import ( "fmt" "net" "net/http" + "net/http/httptest" "net/url" "testing" "time" @@ -22,9 +23,11 @@ import ( "go.pinniped.dev/internal/authenticators" "go.pinniped.dev/internal/certauthority" + "go.pinniped.dev/internal/crypto/ptls" "go.pinniped.dev/internal/endpointaddr" "go.pinniped.dev/internal/mocks/mockldapconn" "go.pinniped.dev/internal/testutil" + "go.pinniped.dev/internal/testutil/tlsserver" ) const ( @@ -1689,10 +1692,22 @@ func TestGetURL(t *testing.T) { // Testing of host parsing, TLS negotiation, and CA bundle, etc. for the production code's dialer. func TestRealTLSDialing(t *testing.T) { - testServerCABundle, testServerURL := testutil.TLSTestServer(t, func(w http.ResponseWriter, r *http.Request) {}) - parsedURL, err := url.Parse(testServerURL) + testServer := tlsserver.TLSTestServer(t, http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {}), + func(server *httptest.Server) { + tlsserver.RecordTLSHello(server) + recordFunc := server.TLS.GetConfigForClient + server.TLS.GetConfigForClient = func(info *tls.ClientHelloInfo) (*tls.Config, error) { + _, _ = recordFunc(info) + r, err := http.NewRequestWithContext(info.Context(), http.MethodGet, "/this-is-ldap", nil) + require.NoError(t, err) + tlsserver.AssertTLS(t, r, ptls.DefaultLDAP) + return nil, nil + } + }) + parsedURL, err := url.Parse(testServer.URL) require.NoError(t, err) testServerHostAndPort := parsedURL.Host + testServerCABundle := tlsserver.TLSTestServerCA(testServer) caForTestServerWithBadCertName, err := certauthority.New("Test CA", time.Hour) require.NoError(t, err) @@ -1720,7 +1735,7 @@ func TestRealTLSDialing(t *testing.T) { { name: "happy path", host: testServerHostAndPort, - caBundle: []byte(testServerCABundle), + caBundle: testServerCABundle, connProto: TLS, context: context.Background(), }, @@ -1751,7 +1766,7 @@ func TestRealTLSDialing(t *testing.T) { { name: "invalid host with TLS", host: "this:is:not:a:valid:hostname", - caBundle: []byte(testServerCABundle), + caBundle: testServerCABundle, connProto: TLS, context: context.Background(), wantError: `LDAP Result Code 200 "Network Error": host "this:is:not:a:valid:hostname" is not a valid hostname or IP address`, @@ -1759,7 +1774,7 @@ func TestRealTLSDialing(t *testing.T) { { name: "invalid host with StartTLS", host: "this:is:not:a:valid:hostname", - caBundle: []byte(testServerCABundle), + caBundle: testServerCABundle, connProto: StartTLS, context: context.Background(), wantError: `LDAP Result Code 200 "Network Error": host "this:is:not:a:valid:hostname" is not a valid hostname or IP address`, @@ -1776,7 +1791,7 @@ func TestRealTLSDialing(t *testing.T) { name: "cannot connect to host", // This is assuming that this port was not reclaimed by another app since the test setup ran. Seems safe enough. host: recentlyClaimedHostAndPort, - caBundle: []byte(testServerCABundle), + caBundle: testServerCABundle, connProto: TLS, context: context.Background(), wantError: fmt.Sprintf(`LDAP Result Code 200 "Network Error": dial tcp %s: connect: connection refused`, recentlyClaimedHostAndPort), @@ -1784,7 +1799,7 @@ func TestRealTLSDialing(t *testing.T) { { name: "pays attention to the passed context", host: testServerHostAndPort, - caBundle: []byte(testServerCABundle), + caBundle: testServerCABundle, connProto: TLS, context: alreadyCancelledContext, wantError: fmt.Sprintf(`LDAP Result Code 200 "Network Error": dial tcp %s: operation was canceled`, testServerHostAndPort), @@ -1792,7 +1807,7 @@ func TestRealTLSDialing(t *testing.T) { { name: "unsupported connection protocol", host: testServerHostAndPort, - caBundle: []byte(testServerCABundle), + caBundle: testServerCABundle, connProto: "bad usage of this type", context: alreadyCancelledContext, wantError: `LDAP Result Code 200 "Network Error": did not specify valid ConnectionProtocol`, @@ -1824,7 +1839,7 @@ func TestRealTLSDialing(t *testing.T) { // Indirectly checking that the Dialer method constructed the ldap.Conn with isTLS set to true, // since this is always the correct behavior unless/until we want to support StartTLS. - err := conn.(*ldap.Conn).StartTLS(&tls.Config{}) + err := conn.(*ldap.Conn).StartTLS(ptls.DefaultLDAP(nil)) require.EqualError(t, err, `LDAP Result Code 200 "Network Error": ldap: already encrypted`) } }) diff --git a/pkg/oidcclient/login.go b/pkg/oidcclient/login.go index b01e5238..2046d047 100644 --- a/pkg/oidcclient/login.go +++ b/pkg/oidcclient/login.go @@ -31,6 +31,7 @@ import ( supervisoroidc "go.pinniped.dev/generated/latest/apis/supervisor/oidc" "go.pinniped.dev/internal/httputil/httperr" "go.pinniped.dev/internal/httputil/securityheader" + "go.pinniped.dev/internal/net/phttp" "go.pinniped.dev/internal/oidc/provider" "go.pinniped.dev/internal/upstreamoidc" "go.pinniped.dev/pkg/oidcclient/nonce" @@ -274,7 +275,7 @@ func Login(issuer string, clientID string, opts ...Option) (*oidctypes.Token, er ctx: context.Background(), logger: logr.Discard(), // discard logs unless a logger is specified callbacks: make(chan callbackResult, 2), - httpClient: http.DefaultClient, + httpClient: phttp.Default(nil), // Default implementations of external dependencies (to be mocked in tests). generateState: state.Generate, diff --git a/test/integration/cli_test.go b/test/integration/cli_test.go index 26ba36bd..dd833414 100644 --- a/test/integration/cli_test.go +++ b/test/integration/cli_test.go @@ -108,7 +108,14 @@ func TestCLIGetKubeconfigStaticToken_Parallel(t *testing.T) { }) } -func runPinnipedCLI(t *testing.T, envVars []string, pinnipedExe string, args ...string) (string, string) { +type testingT interface { + Helper() + Errorf(format string, args ...interface{}) + FailNow() + Logf(format string, args ...interface{}) +} + +func runPinnipedCLI(t testingT, envVars []string, pinnipedExe string, args ...string) (string, string) { t.Helper() start := time.Now() var stdout, stderr bytes.Buffer diff --git a/test/integration/concierge_credentialrequest_test.go b/test/integration/concierge_credentialrequest_test.go index 30a771e7..d403a8d0 100644 --- a/test/integration/concierge_credentialrequest_test.go +++ b/test/integration/concierge_credentialrequest_test.go @@ -45,8 +45,10 @@ func TestUnsuccessfulCredentialRequest_Parallel(t *testing.T) { require.Equal(t, "authentication failed", *response.Status.Message) } -// TCRs are non-mutating and safe to run in parallel with serial tests, see main_test.go. -func TestSuccessfulCredentialRequest_Parallel(t *testing.T) { +// TestSuccessfulCredentialRequest cannot run in parallel because runPinnipedLoginOIDC uses a fixed port +// for its localhost listener via --listen-port=env.CLIUpstreamOIDC.CallbackURL.Port() per oidcLoginCommand. +// Since ports are global to the process, tests using oidcLoginCommand must be run serially. +func TestSuccessfulCredentialRequest(t *testing.T) { env := testlib.IntegrationEnv(t).WithCapability(testlib.ClusterSigningKeyIsAvailable) ctx, cancel := context.WithTimeout(context.Background(), 6*time.Minute) diff --git a/test/integration/concierge_impersonation_proxy_test.go b/test/integration/concierge_impersonation_proxy_test.go index ce451a65..97a32f75 100644 --- a/test/integration/concierge_impersonation_proxy_test.go +++ b/test/integration/concierge_impersonation_proxy_test.go @@ -65,6 +65,7 @@ import ( identityv1alpha1 "go.pinniped.dev/generated/latest/apis/concierge/identity/v1alpha1" loginv1alpha1 "go.pinniped.dev/generated/latest/apis/concierge/login/v1alpha1" pinnipedconciergeclientset "go.pinniped.dev/generated/latest/client/concierge/clientset/versioned" + "go.pinniped.dev/internal/crypto/ptls" "go.pinniped.dev/internal/httputil/roundtripper" "go.pinniped.dev/internal/kubeclient" "go.pinniped.dev/internal/testutil" @@ -305,20 +306,18 @@ func TestImpersonationProxy(t *testing.T) { //nolint:gocyclo // yeah, it's compl Verb: "get", Group: "", Version: "v1", Resource: "namespaces", }) - // Get pods in concierge namespace and pick one. - // this is for tests that require performing actions against a running pod. We use the concierge pod because we already have it handy. - // We want to make sure it's a concierge pod (not cert agent), because we need to be able to port-forward a running port. - pods, err := adminClient.CoreV1().Pods(env.ConciergeNamespace).List(ctx, metav1.ListOptions{}) + // Get pods in supervisor namespace and pick one. + // this is for tests that require performing actions against a running pod. + // We use the supervisor pod because we already have it handy and need to port-forward a running port. + // We avoid using the concierge for this because it requires TLS 1.3 which is not support by older versions of curl. + supervisorPods, err := adminClient.CoreV1().Pods(env.SupervisorNamespace).List(ctx, + metav1.ListOptions{LabelSelector: "deployment.pinniped.dev=supervisor"}) require.NoError(t, err) - require.Greater(t, len(pods.Items), 0) - var conciergePod *corev1.Pod - for _, pod := range pods.Items { - pod := pod - if !strings.Contains(pod.Name, "kube-cert-agent") { - conciergePod = &pod - } - } - require.NotNil(t, conciergePod, "could not find a concierge pod") + require.NotEmpty(t, supervisorPods.Items, "could not find supervisor pods") + supervisorPod := supervisorPods.Items[0] + + // make sure the supervisor has a default TLS cert during this test so that it can handle a TLS connection + _ = createTLSCertificateSecret(ctx, t, env.SupervisorNamespace, "cert-hostname-doesnt-matter", nil, defaultTLSCertSecretName(env), adminClient) // Test that the user can perform basic actions through the client with their username and group membership // influencing RBAC checks correctly. @@ -346,7 +345,7 @@ func TestImpersonationProxy(t *testing.T) { //nolint:gocyclo // yeah, it's compl // Run the kubectl port-forward command. timeout, cancelFunc := context.WithTimeout(ctx, 2*time.Minute) defer cancelFunc() - portForwardCmd, _, portForwardStderr := kubectlCommand(timeout, t, kubeconfigPath, envVarsWithProxy, "port-forward", "--namespace", env.ConciergeNamespace, conciergePod.Name, "10443:8443") + portForwardCmd, _, portForwardStderr := kubectlCommand(timeout, t, kubeconfigPath, envVarsWithProxy, "port-forward", "--namespace", supervisorPod.Namespace, supervisorPod.Name, "10443:8443") portForwardCmd.Env = envVarsWithProxy // Start, but don't wait for the command to finish. @@ -366,7 +365,7 @@ func TestImpersonationProxy(t *testing.T) { //nolint:gocyclo // yeah, it's compl defer cancelFunc() startTime := time.Now() for time.Now().Before(startTime.Add(70 * time.Second)) { - curlCmd := exec.CommandContext(timeout, "curl", "-k", "-sS", "https://127.0.0.1:10443") // -sS turns off the progressbar but still prints errors + curlCmd := exec.CommandContext(timeout, "curl", "-k", "-sS", "https://127.0.0.1:10443/healthz") // -sS turns off the progressbar but still prints errors curlCmd.Stdout = &curlStdOut curlCmd.Stderr = &curlStdErr curlErr := curlCmd.Run() @@ -382,7 +381,7 @@ func TestImpersonationProxy(t *testing.T) { //nolint:gocyclo // yeah, it's compl // curl the endpoint once more, once 70 seconds has elapsed, to make sure the connection is still open. timeout, cancelFunc = context.WithTimeout(ctx, 30*time.Second) defer cancelFunc() - curlCmd := exec.CommandContext(timeout, "curl", "-k", "-sS", "https://127.0.0.1:10443") // -sS turns off the progressbar but still prints errors + curlCmd := exec.CommandContext(timeout, "curl", "-k", "-sS", "https://127.0.0.1:10443/healthz") // -sS turns off the progressbar but still prints errors curlCmd.Stdout = &curlStdOut curlCmd.Stderr = &curlStdErr curlErr := curlCmd.Run() @@ -392,9 +391,8 @@ func TestImpersonationProxy(t *testing.T) { //nolint:gocyclo // yeah, it's compl t.Log("curlStdErr: " + curlStdErr.String()) t.Log("stdout: " + curlStdOut.String()) } - // We expect this to 403, but all we care is that it gets through. require.NoError(t, curlErr) - require.Contains(t, curlStdOut.String(), `"forbidden: User \"system:anonymous\" cannot get path \"/\""`) + require.Contains(t, curlStdOut.String(), "okokokokok") // a few successful healthz responses }) t.Run("kubectl port-forward and keeping the connection open for over a minute (idle)", func(t *testing.T) { @@ -404,7 +402,7 @@ func TestImpersonationProxy(t *testing.T) { //nolint:gocyclo // yeah, it's compl // Run the kubectl port-forward command. timeout, cancelFunc := context.WithTimeout(ctx, 2*time.Minute) defer cancelFunc() - portForwardCmd, _, portForwardStderr := kubectlCommand(timeout, t, kubeconfigPath, envVarsWithProxy, "port-forward", "--namespace", env.ConciergeNamespace, conciergePod.Name, "10444:8443") + portForwardCmd, _, portForwardStderr := kubectlCommand(timeout, t, kubeconfigPath, envVarsWithProxy, "port-forward", "--namespace", supervisorPod.Namespace, supervisorPod.Name, "10444:8443") portForwardCmd.Env = envVarsWithProxy // Start, but don't wait for the command to finish. @@ -414,13 +412,13 @@ func TestImpersonationProxy(t *testing.T) { //nolint:gocyclo // yeah, it's compl assert.EqualErrorf(t, portForwardCmd.Wait(), "signal: killed", `wanted "kubectl port-forward" to get signaled because context was cancelled (stderr: %q)`, portForwardStderr.String()) }() - // Wait to see if we time out. The default timeout is 60 seconds, but the server should recognize this this + // Wait to see if we time out. The default timeout is 60 seconds, but the server should recognize that this // is going to be a long-running command and keep the connection open as long as the client stays connected. time.Sleep(70 * time.Second) timeout, cancelFunc = context.WithTimeout(ctx, 2*time.Minute) defer cancelFunc() - curlCmd := exec.CommandContext(timeout, "curl", "-k", "-sS", "https://127.0.0.1:10444") // -sS turns off the progressbar but still prints errors + curlCmd := exec.CommandContext(timeout, "curl", "-k", "-sS", "https://127.0.0.1:10444/healthz") // -sS turns off the progressbar but still prints errors var curlStdOut, curlStdErr bytes.Buffer curlCmd.Stdout = &curlStdOut curlCmd.Stderr = &curlStdErr @@ -430,9 +428,8 @@ func TestImpersonationProxy(t *testing.T) { //nolint:gocyclo // yeah, it's compl t.Log("curlStdErr: " + curlStdErr.String()) t.Log("stdout: " + curlStdOut.String()) } - // We expect this to 403, but all we care is that it gets through. require.NoError(t, err) - require.Contains(t, curlStdOut.String(), `"forbidden: User \"system:anonymous\" cannot get path \"/\""`) + require.Equal(t, curlStdOut.String(), "ok") }) t.Run("using and watching all the basic verbs", func(t *testing.T) { @@ -760,7 +757,7 @@ func TestImpersonationProxy(t *testing.T) { //nolint:gocyclo // yeah, it's compl clusterAdminCredentials, impersonationProxyURL, impersonationProxyCACertPEM, nil, ) nestedImpersonationUIDOnly.Wrap(func(rt http.RoundTripper) http.RoundTripper { - return roundtripper.Func(func(r *http.Request) (*http.Response, error) { + return roundtripper.WrapFunc(rt, func(r *http.Request) (*http.Response, error) { r.Header.Set("iMperSONATE-uid", "some-awesome-uid") return rt.RoundTrip(r) }) @@ -798,7 +795,7 @@ func TestImpersonationProxy(t *testing.T) { //nolint:gocyclo // yeah, it's compl }, ) nestedImpersonationUID.Wrap(func(rt http.RoundTripper) http.RoundTripper { - return roundtripper.Func(func(r *http.Request) (*http.Response, error) { + return roundtripper.WrapFunc(rt, func(r *http.Request) (*http.Response, error) { r.Header.Set("imperSONate-uiD", "some-fancy-uid") return rt.RoundTrip(r) }) @@ -1115,7 +1112,7 @@ func TestImpersonationProxy(t *testing.T) { //nolint:gocyclo // yeah, it's compl // run the kubectl logs command logLinesCount := 10 - stdout, err = runKubectl(t, kubeconfigPath, envVarsWithProxy, "logs", "--namespace", conciergePod.Namespace, conciergePod.Name, fmt.Sprintf("--tail=%d", logLinesCount)) + stdout, err = runKubectl(t, kubeconfigPath, envVarsWithProxy, "logs", "--namespace", supervisorPod.Namespace, supervisorPod.Name, fmt.Sprintf("--tail=%d", logLinesCount)) require.NoError(t, err, `"kubectl logs" failed`) // Expect _approximately_ logLinesCount lines in the output // (we can't match 100% exactly due to https://github.com/kubernetes/kubernetes/issues/72628). @@ -1500,6 +1497,20 @@ func TestImpersonationProxy(t *testing.T) { //nolint:gocyclo // yeah, it's compl }) }) }) + + t.Run("assert impersonator runs with secure TLS config", func(t *testing.T) { + parallelIfNotEKS(t) + + cancelCtx, cancel := context.WithCancel(ctx) + t.Cleanup(cancel) + + startKubectlPortForward(cancelCtx, t, "10445", "443", env.ConciergeAppName+"-proxy", env.ConciergeNamespace) + + stdout, stderr := runNmapSSLEnum(t, "127.0.0.1", 10445) + + require.Empty(t, stderr) + require.Contains(t, stdout, getExpectedCiphers(ptls.Default), "stdout:\n%s", stdout) + }) }) t.Run("assert correct impersonator service account is being used", func(t *testing.T) { @@ -1956,7 +1967,7 @@ func performImpersonatorDiscovery(ctx context.Context, t *testing.T, env *testli // probe each pod directly for readiness since the concierge status is a lie - it just means a single pod is ready testlib.RequireEventually(t, func(requireEventually *require.Assertions) { pods, err := adminClient.CoreV1().Pods(env.ConciergeNamespace).List(ctx, - metav1.ListOptions{LabelSelector: "app=" + env.ConciergeAppName + ",!kube-cert-agent.pinniped.dev"}) // TODO replace with deployment.pinniped.dev=concierge + metav1.ListOptions{LabelSelector: "deployment.pinniped.dev=concierge"}) requireEventually.NoError(err) requireEventually.Len(pods.Items, 2) // has to stay in sync with the defaults in our YAML @@ -2373,7 +2384,7 @@ func getCredForConfig(t *testing.T, config *rest.Config) *loginv1alpha1.ClusterC config = rest.CopyConfig(config) config.Wrap(func(rt http.RoundTripper) http.RoundTripper { - return roundtripper.Func(func(req *http.Request) (*http.Response, error) { + return roundtripper.WrapFunc(rt, func(req *http.Request) (*http.Response, error) { resp, err := rt.RoundTrip(req) r := req diff --git a/test/integration/ldap_client_test.go b/test/integration/ldap_client_test.go index 4bc268e2..1d913f8a 100644 --- a/test/integration/ldap_client_test.go +++ b/test/integration/ldap_client_test.go @@ -759,8 +759,6 @@ func startLongRunningCommandAndWaitForInitialOutput( cmd := exec.CommandContext(ctx, command, args...) var stdoutBuf, stderrBuf syncBuffer - cmd.Stdout = &stdoutBuf - cmd.Stderr = &stderrBuf cmd.Stdout = io.MultiWriter(os.Stdout, &stdoutBuf) cmd.Stderr = io.MultiWriter(os.Stderr, &stderrBuf) diff --git a/test/integration/securetls_test.go b/test/integration/securetls_test.go new file mode 100644 index 00000000..f39feab3 --- /dev/null +++ b/test/integration/securetls_test.go @@ -0,0 +1,257 @@ +// Copyright 2021 the Pinniped contributors. All Rights Reserved. +// SPDX-License-Identifier: Apache-2.0 + +package integration + +import ( + "bytes" + "context" + "crypto/tls" + "crypto/x509" + "encoding/base64" + "fmt" + "net/http" + "os/exec" + "regexp" + "sort" + "strconv" + "strings" + "testing" + "time" + + "github.com/stretchr/testify/require" + + "go.pinniped.dev/internal/crypto/ptls" + "go.pinniped.dev/internal/testutil/tlsserver" + "go.pinniped.dev/test/testlib" +) + +// TLS checks safe to run in parallel with serial tests, see main_test.go. +func TestSecureTLSPinnipedCLIToKAS_Parallel(t *testing.T) { + _ = testlib.IntegrationEnv(t) + + server := tlsserver.TLSTestServer(t, http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + tlsserver.AssertTLS(t, r, ptls.Secure) // pinniped CLI uses ptls.Secure when talking to KAS + w.Header().Set("content-type", "application/json") + fmt.Fprint(w, `{"kind":"TokenCredentialRequest","apiVersion":"login.concierge.pinniped.dev/v1alpha1",`+ + `"status":{"credential":{"token":"some-fancy-token"}}}`) + }), tlsserver.RecordTLSHello) + + ca := tlsserver.TLSTestServerCA(server) + + pinnipedExe := testlib.PinnipedCLIPath(t) + + stdout, stderr := runPinnipedCLI(t, nil, pinnipedExe, "login", "static", + "--token", "does-not-matter", + "--concierge-authenticator-type", "webhook", + "--concierge-authenticator-name", "does-not-matter", + "--concierge-ca-bundle-data", base64.StdEncoding.EncodeToString(ca), + "--concierge-endpoint", server.URL, + "--enable-concierge", + "--credential-cache", "", + ) + + require.Empty(t, stderr) + require.Equal(t, `{"kind":"ExecCredential","apiVersion":"client.authentication.k8s.io/v1beta1",`+ + `"spec":{"interactive":false},"status":{"expirationTimestamp":null,"token":"some-fancy-token"}} +`, stdout) +} + +// TLS checks safe to run in parallel with serial tests, see main_test.go. +func TestSecureTLSPinnipedCLIToSupervisor_Parallel(t *testing.T) { + _ = testlib.IntegrationEnv(t) + + server := tlsserver.TLSTestServer(t, http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + tlsserver.AssertTLS(t, r, ptls.Default) // pinniped CLI uses ptls.Default when talking to supervisor + w.Header().Set("content-type", "application/json") + fmt.Fprint(w, `{"issuer":"https://not-a-good-issuer"}`) + }), tlsserver.RecordTLSHello) + + ca := tlsserver.TLSTestServerCA(server) + + pinnipedExe := testlib.PinnipedCLIPath(t) + + stdout, stderr := runPinnipedCLI(&fakeT{T: t}, nil, pinnipedExe, "login", "oidc", + "--ca-bundle-data", base64.StdEncoding.EncodeToString(ca), + "--issuer", server.URL, + "--credential-cache", "", + "--upstream-identity-provider-flow", "cli_password", + "--upstream-identity-provider-name", "does-not-matter", + "--upstream-identity-provider-type", "oidc", + ) + + require.Equal(t, `Error: could not complete Pinniped login: could not perform OIDC discovery for "`+ + server.URL+`": oidc: issuer did not match the issuer returned by provider, expected "`+ + server.URL+`" got "https://not-a-good-issuer" +`, stderr) + require.Empty(t, stdout) +} + +// TLS checks safe to run in parallel with serial tests, see main_test.go. +func TestSecureTLSConciergeAggregatedAPI_Parallel(t *testing.T) { + env := testlib.IntegrationEnv(t) + + cancelCtx, cancel := context.WithCancel(context.Background()) + t.Cleanup(cancel) + + startKubectlPortForward(cancelCtx, t, "10446", "443", env.ConciergeAppName+"-api", env.ConciergeNamespace) + + stdout, stderr := runNmapSSLEnum(t, "127.0.0.1", 10446) + + require.Empty(t, stderr) + require.Contains(t, stdout, getExpectedCiphers(ptls.Secure), "stdout:\n%s", stdout) +} + +func TestSecureTLSSupervisor(t *testing.T) { // does not run in parallel because of the createTLSCertificateSecret call + env := testlib.IntegrationEnv(t) + + ctx, cancel := context.WithCancel(context.Background()) + t.Cleanup(cancel) + + adminClient := testlib.NewKubernetesClientset(t) + // make sure the supervisor has a default TLS cert during this test so that it can handle a TLS connection + _ = createTLSCertificateSecret(ctx, t, env.SupervisorNamespace, "cert-hostname-doesnt-matter", nil, defaultTLSCertSecretName(env), adminClient) + + startKubectlPortForward(ctx, t, "10447", "443", env.SupervisorAppName+"-clusterip", env.SupervisorNamespace) + + stdout, stderr := runNmapSSLEnum(t, "127.0.0.1", 10447) + + // supervisor's cert is ECDSA + defaultECDSAOnly := func(rootCAs *x509.CertPool) *tls.Config { + c := ptls.Default(rootCAs) + ciphers := make([]uint16, 0, len(c.CipherSuites)/2) + for _, id := range c.CipherSuites { + id := id + if !strings.Contains(tls.CipherSuiteName(id), "_ECDSA_") { + continue + } + ciphers = append(ciphers, id) + } + c.CipherSuites = ciphers + return c + } + + require.Empty(t, stderr) + require.Contains(t, stdout, getExpectedCiphers(defaultECDSAOnly), "stdout:\n%s", stdout) +} + +type fakeT struct { + *testing.T +} + +func (t *fakeT) FailNow() { + t.Errorf("fakeT ignored FailNow") +} + +func (t *fakeT) Errorf(format string, args ...interface{}) { + t.Cleanup(func() { + if !t.Failed() { + return + } + t.Logf("reporting previously ignored errors since main test failed:\n"+format, args...) + }) +} + +func runNmapSSLEnum(t *testing.T, host string, port uint16) (string, string) { + t.Helper() + + ctx, cancel := context.WithTimeout(context.Background(), time.Minute) + defer cancel() + + version, err := exec.CommandContext(ctx, "nmap", "-V").CombinedOutput() + require.NoError(t, err) + + versionMatches := regexp.MustCompile(`Nmap version 7\.(?P\d+)`).FindStringSubmatch(string(version)) + require.Len(t, versionMatches, 2) + minorVersion, err := strconv.Atoi(versionMatches[1]) + require.NoError(t, err) + require.GreaterOrEqual(t, minorVersion, 92, "nmap >= 7.92.x is required") + + var stdout, stderr bytes.Buffer + //nolint:gosec // we are not performing malicious argument injection against ourselves + cmd := exec.CommandContext(ctx, "nmap", "--script", "ssl-enum-ciphers", + "-p", strconv.FormatUint(uint64(port), 10), + host, + ) + cmd.Stdout = &stdout + cmd.Stderr = &stderr + + require.NoErrorf(t, cmd.Run(), "stderr:\n%s\n\nstdout:\n%s\n\n", stderr.String(), stdout.String()) + + return stdout.String(), stderr.String() +} + +func getExpectedCiphers(configFunc ptls.ConfigFunc) string { + config := configFunc(nil) + secureConfig := ptls.Secure(nil) + + skip12 := config.MinVersion == secureConfig.MinVersion + + var tls12Bit, tls13Bit string + + if !skip12 { + sort.SliceStable(config.CipherSuites, func(i, j int) bool { + a := tls.CipherSuiteName(config.CipherSuites[i]) + b := tls.CipherSuiteName(config.CipherSuites[j]) + + ok1 := strings.Contains(a, "_ECDSA_") + ok2 := strings.Contains(b, "_ECDSA_") + + if ok1 && ok2 { + return false + } + + return ok1 + }) + + var s strings.Builder + for i, id := range config.CipherSuites { + s.WriteString(fmt.Sprintf(tls12Item, tls.CipherSuiteName(id))) + if i == len(config.CipherSuites)-1 { + break + } + s.WriteString("\n") + } + tls12Bit = fmt.Sprintf(tls12Base, s.String()) + } + + var s strings.Builder + for i, id := range secureConfig.CipherSuites { + s.WriteString(fmt.Sprintf(tls13Item, strings.Replace(tls.CipherSuiteName(id), "TLS_", "TLS_AKE_WITH_", 1))) + if i == len(secureConfig.CipherSuites)-1 { + break + } + s.WriteString("\n") + } + tls13Bit = fmt.Sprintf(tls13Base, s.String()) + + return fmt.Sprintf(baseItem, tls12Bit, tls13Bit) +} + +const ( + // this surrounds the tls 1.2 and 1.3 text in a way that guarantees that other TLS versions are not supported. + baseItem = `/tcp open unknown +| ssl-enum-ciphers: %s%s +|_ least strength: A + +Nmap done: 1 IP address (1 host up) scanned in` + + // the "cipher preference: client" bit a bug in nmap. + // https://github.com/nmap/nmap/issues/1691#issuecomment-536919978 + tls12Base = ` +| TLSv1.2: +| ciphers: +%s +| compressors: +| NULL +| cipher preference: client` + + tls13Base = ` +| TLSv1.3: +| ciphers: +%s +| cipher preference: server` + + tls12Item = `| %s (secp256r1) - A` + tls13Item = `| %s (ecdh_x25519) - A` +) diff --git a/test/testlib/client.go b/test/testlib/client.go index 02acc86f..c5e96339 100644 --- a/test/testlib/client.go +++ b/test/testlib/client.go @@ -132,7 +132,7 @@ func newClientsetWithConfig(t *testing.T, config *rest.Config) kubernetes.Interf func NewAnonymousClientRestConfig(t *testing.T) *rest.Config { t.Helper() - return rest.AnonymousClientConfig(NewClientConfig(t)) + return kubeclient.SecureAnonymousClientConfig(NewClientConfig(t)) } // Starting with an anonymous client config, add a cert and key to use for authentication in the API server.