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.