diff --git a/internal/certauthority/certauthority.go b/internal/certauthority/certauthority.go index 806d6498..ad81e38a 100644 --- a/internal/certauthority/certauthority.go +++ b/internal/certauthority/certauthority.go @@ -220,8 +220,8 @@ func (c *CA) Issue(subject pkix.Name, dnsNames []string, ips []net.IP, ttl time. // IssuePEM issues a new server certificate for the given identity and duration, returning it as a pair of // PEM-formatted byte slices for the certificate and private key. -func (c *CA) IssuePEM(subject pkix.Name, dnsNames []string, ttl time.Duration) ([]byte, []byte, error) { - return toPEM(c.Issue(subject, dnsNames, nil, ttl)) +func (c *CA) IssuePEM(subject pkix.Name, dnsNames []string, ips []net.IP, ttl time.Duration) ([]byte, []byte, error) { + return toPEM(c.Issue(subject, dnsNames, ips, ttl)) } func toPEM(cert *tls.Certificate, err error) ([]byte, []byte, error) { diff --git a/internal/certauthority/certauthority_test.go b/internal/certauthority/certauthority_test.go index 4193990b..7f9e3571 100644 --- a/internal/certauthority/certauthority_test.go +++ b/internal/certauthority/certauthority_test.go @@ -325,7 +325,7 @@ func TestIssuePEM(t *testing.T) { realCA, err := loadFromFiles(t, "./testdata/test.crt", "./testdata/test.key") require.NoError(t, err) - certPEM, keyPEM, err := realCA.IssuePEM(pkix.Name{CommonName: "Test Server"}, []string{"example.com"}, 10*time.Minute) + certPEM, keyPEM, err := realCA.IssuePEM(pkix.Name{CommonName: "Test Server"}, []string{"example.com"}, nil, 10*time.Minute) require.NoError(t, err) require.NotEmpty(t, certPEM) require.NotEmpty(t, keyPEM) diff --git a/internal/certauthority/dynamiccertauthority/dynamiccertauthority.go b/internal/certauthority/dynamiccertauthority/dynamiccertauthority.go index c15e4cc0..3c0c4b72 100644 --- a/internal/certauthority/dynamiccertauthority/dynamiccertauthority.go +++ b/internal/certauthority/dynamiccertauthority/dynamiccertauthority.go @@ -36,5 +36,5 @@ func (c *CA) IssuePEM(subject pkix.Name, dnsNames []string, ttl time.Duration) ( return nil, nil, err } - return ca.IssuePEM(subject, dnsNames, ttl) + return ca.IssuePEM(subject, dnsNames, nil, ttl) } diff --git a/internal/concierge/impersonator/impersonator_test.go b/internal/concierge/impersonator/impersonator_test.go index 66cbd3d3..3a7911de 100644 --- a/internal/concierge/impersonator/impersonator_test.go +++ b/internal/concierge/impersonator/impersonator_test.go @@ -15,6 +15,8 @@ import ( "time" "github.com/stretchr/testify/require" + v1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apiserver/pkg/authentication/user" "k8s.io/apiserver/pkg/endpoints/request" "k8s.io/apiserver/pkg/features" @@ -26,12 +28,13 @@ import ( "go.pinniped.dev/internal/certauthority" "go.pinniped.dev/internal/dynamiccert" + "go.pinniped.dev/internal/here" "go.pinniped.dev/internal/kubeclient" "go.pinniped.dev/internal/testutil" ) -func TestNew(t *testing.T) { - const port = 8444 +func TestImpersonator(t *testing.T) { + const port = 9444 ca, err := certauthority.New(pkix.Name{CommonName: "ca"}, time.Hour) require.NoError(t, err) @@ -41,7 +44,7 @@ func TestNew(t *testing.T) { err = caContent.SetCertKeyContent(ca.Bundle(), caKey) require.NoError(t, err) - cert, key, err := ca.IssuePEM(pkix.Name{CommonName: "example.com"}, []string{"example.com"}, time.Hour) + cert, key, err := ca.IssuePEM(pkix.Name{}, nil, []net.IP{net.ParseIP("127.0.0.1")}, time.Hour) require.NoError(t, err) certKeyContent := dynamiccert.New("cert-key") err = certKeyContent.SetCertKeyContent(cert, key) @@ -57,77 +60,195 @@ func TestNew(t *testing.T) { defer featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.APIPriorityAndFairness, false)() tests := []struct { - name string - clientOpts []kubeclient.Option - wantErr string + name string + clientUsername string + clientGroups []string + clientImpersonateUser rest.ImpersonationConfig + kubeAPIServerClientBearerTokenFile string + kubeAPIServerStatusCode int + wantError string + wantConstructionError string }{ { - name: "happy path", - clientOpts: []kubeclient.Option{ - kubeclient.WithConfig(&rest.Config{ - BearerToken: "should-be-ignored", - BearerTokenFile: "required-to-be-set", - }), - }, + name: "happy path", + clientUsername: "test-username", + clientGroups: []string{"test-group1", "test-group2"}, + kubeAPIServerClientBearerTokenFile: "required-to-be-set", }, { - name: "no bearer token file", - clientOpts: []kubeclient.Option{ - kubeclient.WithConfig(&rest.Config{ - BearerToken: "should-be-ignored", - }), - }, - wantErr: "invalid impersonator loopback rest config has wrong bearer token semantics", + name: "no bearer token file in Kube API server client config", + wantConstructionError: "invalid impersonator loopback rest config has wrong bearer token semantics", + }, + { + name: "user is authenticated but the kube API request returns an error", + kubeAPIServerStatusCode: http.StatusNotFound, + clientUsername: "test-username", + clientGroups: []string{"test-group1", "test-group2"}, + kubeAPIServerClientBearerTokenFile: "required-to-be-set", + wantError: `the server could not find the requested resource (get namespaces)`, + }, + { + name: "double impersonation is not allowed by regular users", + clientUsername: "test-username", + clientGroups: []string{"test-group1", "test-group2"}, + clientImpersonateUser: rest.ImpersonationConfig{UserName: "some-other-username"}, + kubeAPIServerClientBearerTokenFile: "required-to-be-set", + wantError: `users "some-other-username" is forbidden: User "test-username" ` + + `cannot impersonate resource "users" in API group "" at the cluster scope: impersonation is not allowed or invalid verb`, + }, + { + name: "double impersonation is not allowed by admin users", + clientUsername: "test-admin", + clientGroups: []string{"system:masters", "test-group2"}, + clientImpersonateUser: rest.ImpersonationConfig{UserName: "some-other-username"}, + kubeAPIServerClientBearerTokenFile: "required-to-be-set", + wantError: `users "some-other-username" is forbidden: User "test-admin" ` + + `cannot impersonate resource "users" in API group "" at the cluster scope: impersonation is not allowed or invalid verb`, }, } for _, tt := range tests { tt := tt + // This is a serial test because the production code binds to the port. t.Run(tt.name, func(t *testing.T) { - // This is a serial test because the production code binds to the port. - runner, constructionErr := newInternal(port, certKeyContent, caContent, tt.clientOpts, recOpts) - - if len(tt.wantErr) != 0 { - require.EqualError(t, constructionErr, tt.wantErr) - require.Nil(t, runner) - } else { - require.NoError(t, constructionErr) - require.NotNil(t, runner) - - stopCh := make(chan struct{}) - errCh := make(chan error) - go func() { - stopErr := runner(stopCh) - errCh <- stopErr - }() - - select { - case unexpectedExit := <-errCh: - t.Errorf("unexpected exit, err=%v (even nil error is failure)", unexpectedExit) - case <-time.After(10 * time.Second): - } - - close(stopCh) - exitErr := <-errCh - require.NoError(t, exitErr) + if tt.kubeAPIServerStatusCode == 0 { + tt.kubeAPIServerStatusCode = http.StatusOK } - // assert listener is closed is both cases above by trying to make another one on the same port - ln, _, listenErr := genericoptions.CreateListener("", "0.0.0.0:"+strconv.Itoa(port), net.ListenConfig{}) - defer func() { - if ln == nil { + // Set up a fake Kube API server which will stand in for the real one. The impersonator + // will proxy incoming calls to this fake server. + testKubeAPIServerWasCalled := false + testKubeAPIServerSawHeaders := http.Header{} + testKubeAPIServerCA, testKubeAPIServerURL := testutil.TLSTestServer(t, func(w http.ResponseWriter, r *http.Request) { + require.Equal(t, http.MethodGet, r.Method) + switch r.URL.Path { + case "/api/v1/namespaces/kube-system/configmaps": + // The production code uses NewDynamicCAFromConfigMapController which fetches a ConfigMap, + // so treat that differently. It wants to read the Kube API server CA from that ConfigMap + // to use it to validate client certs. We don't need it for this test, so return NotFound. + http.NotFound(w, r) return + case "/api/v1/namespaces": + testKubeAPIServerWasCalled = true + testKubeAPIServerSawHeaders = r.Header + if tt.kubeAPIServerStatusCode != http.StatusOK { + w.WriteHeader(tt.kubeAPIServerStatusCode) + } else { + w.Header().Add("Content-Type", "application/json; charset=UTF-8") + _, _ = w.Write([]byte(here.Doc(` + { + "kind": "NamespaceList", + "apiVersion":"v1", + "items": [ + {"metadata":{"name": "namespace1"}}, + {"metadata":{"name": "namespace2"}} + ] + } + `))) + } + default: + require.Fail(t, "fake Kube API server got an unexpected request") } - require.NoError(t, ln.Close()) - }() - require.NoError(t, listenErr) + }) + testKubeAPIServerKubeconfig := rest.Config{ + Host: testKubeAPIServerURL, + BearerToken: "some-service-account-token", + TLSClientConfig: rest.TLSClientConfig{CAData: []byte(testKubeAPIServerCA)}, + BearerTokenFile: tt.kubeAPIServerClientBearerTokenFile, + } + clientOpts := []kubeclient.Option{kubeclient.WithConfig(&testKubeAPIServerKubeconfig)} - // TODO: create some client certs and assert the authorizer works correctly with system:masters - // and nested impersonation - we could also try to test what headers are sent to KAS + // Create an impersonator. + runner, constructionErr := newInternal(port, certKeyContent, caContent, clientOpts, recOpts) + if len(tt.wantConstructionError) > 0 { + require.EqualError(t, constructionErr, tt.wantConstructionError) + require.Nil(t, runner) + // After failing to start, the impersonator port should be available again. + requireCanBindToPort(t, port) + // The rest of the test doesn't make sense when you expect a construction error, so stop here. + return + } + require.NoError(t, constructionErr) + require.NotNil(t, runner) + + // Start the impersonator. + stopCh := make(chan struct{}) + errCh := make(chan error) + go func() { + stopErr := runner(stopCh) + errCh <- stopErr + }() + + // Create client certs for authentication to the impersonator. + clientCertPEM, clientKeyPEM, err := ca.IssuePEM( + pkix.Name{CommonName: tt.clientUsername, Organization: tt.clientGroups}, nil, nil, time.Hour, + ) + require.NoError(t, err) + + // Create a kubeconfig to talk to the impersonator as a client. + clientKubeconfig := &rest.Config{ + Host: "https://127.0.0.1:" + strconv.Itoa(port), + TLSClientConfig: rest.TLSClientConfig{ + CAData: ca.Bundle(), + CertData: clientCertPEM, + KeyData: clientKeyPEM, + }, + UserAgent: "test-agent", + Impersonate: tt.clientImpersonateUser, + } + + // Create a real Kube client to make API requests to the impersonator. + client, err := kubeclient.New(kubeclient.WithConfig(clientKubeconfig)) + require.NoError(t, err) + + // The fake Kube API server knows how to to list namespaces, so make that request using the client + // through the impersonator. + listResponse, err := client.Kubernetes.CoreV1().Namespaces().List(context.Background(), metav1.ListOptions{}) + if len(tt.wantError) > 0 { + require.EqualError(t, err, tt.wantError) + } else { + require.NoError(t, err) + require.Equal(t, &v1.NamespaceList{ + Items: []v1.Namespace{ + {ObjectMeta: metav1.ObjectMeta{Name: "namespace1"}}, + {ObjectMeta: metav1.ObjectMeta{Name: "namespace2"}}, + }, + }, listResponse) + + // The impersonator should have proxied the request to the fake Kube API server, which should have seen + // the headers of the original request mutated by the impersonator. + require.True(t, testKubeAPIServerWasCalled) + require.Equal(t, + http.Header{ + "Impersonate-User": {tt.clientUsername}, + "Impersonate-Group": append(tt.clientGroups, "system:authenticated"), + "Authorization": {"Bearer some-service-account-token"}, + "User-Agent": {"test-agent"}, + "Accept": {"application/vnd.kubernetes.protobuf,application/json"}, + "Accept-Encoding": {"gzip"}, + "X-Forwarded-For": {"127.0.0.1"}, + }, + testKubeAPIServerSawHeaders, + ) + } + + // Stop the impersonator server. + close(stopCh) + exitErr := <-errCh + require.NoError(t, exitErr) + + // After shutdown, the impersonator port should be available again. + requireCanBindToPort(t, port) }) } } -func TestImpersonator(t *testing.T) { +func requireCanBindToPort(t *testing.T, port int) { + ln, _, listenErr := genericoptions.CreateListener("", "0.0.0.0:"+strconv.Itoa(port), net.ListenConfig{}) + require.NoError(t, listenErr) + require.NoError(t, ln.Close()) +} + +func TestImpersonatorHTTPHandler(t *testing.T) { const testUser = "test-user" testGroups := []string{"test-group-1", "test-group-2"} diff --git a/internal/controllermanager/prepare_controllers.go b/internal/controllermanager/prepare_controllers.go index 5af528e4..aef64f28 100644 --- a/internal/controllermanager/prepare_controllers.go +++ b/internal/controllermanager/prepare_controllers.go @@ -64,14 +64,22 @@ type Config struct { // DynamicServingCertProvider provides a setter and a getter to the Pinniped API's serving cert. DynamicServingCertProvider dynamiccert.Provider - // DynamicSigningCertProvider provides a setter and a getter to the Pinniped API's // TODO fix comment + + // DynamicSigningCertProvider provides a setter and a getter to the Pinniped API's // signing cert, i.e., the cert that it uses to sign certs for Pinniped clients wishing to login. + // This is filled with the Kube API server's signing cert by a controller, if it can be found. DynamicSigningCertProvider dynamiccert.Provider - // TODO fix comment + + // ImpersonationSigningCertProvider provides a setter and a getter to the CA cert that should be + // used to sign client certs for authentication to the impersonation proxy. This CA is used by + // the TokenCredentialRequest to sign certs and by the impersonation proxy to check certs. + // When the impersonation proxy is not running, the getter will return nil cert and nil key. + // (Note that the impersonation proxy also accepts client certs signed by the Kube API server's cert.) ImpersonationSigningCertProvider dynamiccert.Provider // ServingCertDuration is the validity period, in seconds, of the API serving certificate. ServingCertDuration time.Duration + // ServingCertRenewBefore is the period of time, in seconds, that pinniped will wait before // rotating the serving certificate. This period of time starts upon issuance of the serving // certificate. diff --git a/test/integration/concierge_impersonation_proxy_test.go b/test/integration/concierge_impersonation_proxy_test.go index 095f8bb0..6c3efeb0 100644 --- a/test/integration/concierge_impersonation_proxy_test.go +++ b/test/integration/concierge_impersonation_proxy_test.go @@ -427,11 +427,13 @@ func TestImpersonationProxy(t *testing.T) { //nolint:gocyclo // yeah, it's compl _, err = doubleImpersonationKubeClient.CoreV1().Secrets(env.ConciergeNamespace).Get(ctx, impersonationProxyTLSSecretName(env), metav1.GetOptions{}) // Double impersonation is not supported yet, so we should get an error. - require.EqualError(t, err, fmt.Sprintf( + require.Error(t, err) + require.Regexp(t, `users "other-user-to-impersonate" is forbidden: `+ - `User "%s" cannot impersonate resource "users" in API group "" at the cluster scope: `+ + `User ".*" cannot impersonate resource "users" in API group "" at the cluster scope: `+ `impersonation is not allowed or invalid verb`, - "kubernetes-admin")) + err.Error(), + ) }) t.Run("WhoAmIRequests and different kinds of authentication through the impersonation proxy", func(t *testing.T) {