From e65817ad5b8290a9f85737a145f05c860a7acfb1 Mon Sep 17 00:00:00 2001 From: Monis Khan Date: Mon, 20 Sep 2021 15:34:05 -0400 Subject: [PATCH 1/3] token credential request: fix trace log kind Signed-off-by: Monis Khan --- internal/registry/credentialrequest/rest.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/registry/credentialrequest/rest.go b/internal/registry/credentialrequest/rest.go index 4c258bdd2..4a5d1e0bf 100644 --- a/internal/registry/credentialrequest/rest.go +++ b/internal/registry/credentialrequest/rest.go @@ -87,7 +87,7 @@ func (*REST) Categories() []string { func (r *REST) Create(ctx context.Context, obj runtime.Object, createValidation rest.ValidateObjectFunc, options *metav1.CreateOptions) (runtime.Object, error) { t := trace.FromContext(ctx).Nest("create", trace.Field{ Key: "kind", - Value: obj.GetObjectKind().GroupVersionKind().Kind, + Value: "TokenCredentialRequest", }) defer t.Log() From 4e98c1bbdbf9a2b63126063946dca5e51b18ce16 Mon Sep 17 00:00:00 2001 From: Ryan Richard Date: Mon, 20 Sep 2021 17:14:58 -0700 Subject: [PATCH 2/3] Tests use CertificatesV1 when available, otherwise use CertificatesV1beta1 CertificatesV1beta1 was removed in Kube 1.22, so the tests cannot blindly rely on it anymore. Use CertificatesV1 whenever the server reports that is available, and otherwise use the old CertificatesV1beta1. Note that CertificatesV1 was introduced in Kube 1.19. --- .../testutil/kube_server_compatibility.go | 30 ++++++ .../concierge_impersonation_proxy_test.go | 96 +++++++++++++------ test/integration/whoami_test.go | 60 ++++++++---- 3 files changed, 142 insertions(+), 44 deletions(-) create mode 100644 internal/testutil/kube_server_compatibility.go diff --git a/internal/testutil/kube_server_compatibility.go b/internal/testutil/kube_server_compatibility.go new file mode 100644 index 000000000..89cf15b44 --- /dev/null +++ b/internal/testutil/kube_server_compatibility.go @@ -0,0 +1,30 @@ +// Copyright 2021 the Pinniped contributors. All Rights Reserved. +// SPDX-License-Identifier: Apache-2.0 + +package testutil + +import ( + "testing" + + "github.com/stretchr/testify/require" + certificatesv1 "k8s.io/api/certificates/v1" + "k8s.io/client-go/discovery" +) + +func KubeServerSupportsCertificatesV1API(t *testing.T, discoveryClient discovery.DiscoveryInterface) bool { + t.Helper() + groupList, err := discoveryClient.ServerGroups() + require.NoError(t, err) + for _, group := range groupList.Groups { + if group.Name == certificatesv1.GroupName { + for _, version := range group.Versions { + if version.Version == "v1" { + // Note: v1 should exist in Kubernetes 1.19 and above + return true + } + } + } + continue + } + return false +} diff --git a/test/integration/concierge_impersonation_proxy_test.go b/test/integration/concierge_impersonation_proxy_test.go index 5c3b09941..ce451a653 100644 --- a/test/integration/concierge_impersonation_proxy_test.go +++ b/test/integration/concierge_impersonation_proxy_test.go @@ -938,6 +938,9 @@ func TestImpersonationProxy(t *testing.T) { //nolint:gocyclo // yeah, it's compl namespaceName := testlib.CreateNamespace(ctx, t, "impersonation").Name kubeClient := adminClient.CoreV1() saName, _, saUID := createServiceAccountToken(ctx, t, adminClient, namespaceName) + expectedUsername := serviceaccount.MakeUsername(namespaceName, saName) + expectedUID := string(saUID) + expectedGroups := []string{"system:serviceaccounts", "system:serviceaccounts:" + namespaceName, "system:authenticated"} _, tokenRequestProbeErr := kubeClient.ServiceAccounts(namespaceName).CreateToken(ctx, saName, &authenticationv1.TokenRequest{}, metav1.CreateOptions{}) if k8serrors.IsNotFound(tokenRequestProbeErr) && tokenRequestProbeErr.Error() == "the server could not find the requested resource" { @@ -1002,8 +1005,8 @@ func TestImpersonationProxy(t *testing.T) { //nolint:gocyclo // yeah, it's compl // new service account tokens include the pod info in the extra fields require.Equal(t, expectedWhoAmIRequestResponse( - serviceaccount.MakeUsername(namespaceName, saName), - []string{"system:serviceaccounts", "system:serviceaccounts:" + namespaceName, "system:authenticated"}, + expectedUsername, + expectedGroups, map[string]identityv1alpha1.ExtraValue{ "authentication.kubernetes.io/pod-name": {pod.Name}, "authentication.kubernetes.io/pod-uid": {string(pod.UID)}, @@ -1017,7 +1020,7 @@ func TestImpersonationProxy(t *testing.T) { //nolint:gocyclo // yeah, it's compl rbacv1.Subject{Kind: rbacv1.ServiceAccountKind, Name: saName, Namespace: namespaceName}, rbacv1.RoleRef{Kind: "ClusterRole", APIGroup: rbacv1.GroupName, Name: "system:node-bootstrapper"}, ) - testlib.WaitForUserToHaveAccess(t, serviceaccount.MakeUsername(namespaceName, saName), []string{}, &authorizationv1.ResourceAttributes{ + testlib.WaitForUserToHaveAccess(t, expectedUsername, []string{}, &authorizationv1.ResourceAttributes{ Verb: "create", Group: certificatesv1.GroupName, Version: "*", Resource: "certificatesigningrequests", }) @@ -1041,20 +1044,34 @@ func TestImpersonationProxy(t *testing.T) { //nolint:gocyclo // yeah, it's compl ) require.NoError(t, err) - saCSR, err := impersonationProxySAClient.Kubernetes.CertificatesV1beta1().CertificateSigningRequests().Get(ctx, csrName, metav1.GetOptions{}) - require.NoError(t, err) - - err = adminClient.CertificatesV1beta1().CertificateSigningRequests().Delete(ctx, csrName, metav1.DeleteOptions{}) - require.NoError(t, err) - - // make sure the user info that the CSR captured matches the SA, including the UID - require.Equal(t, serviceaccount.MakeUsername(namespaceName, saName), saCSR.Spec.Username) - require.Equal(t, string(saUID), saCSR.Spec.UID) - require.Equal(t, []string{"system:serviceaccounts", "system:serviceaccounts:" + namespaceName, "system:authenticated"}, saCSR.Spec.Groups) - require.Equal(t, map[string]certificatesv1beta1.ExtraValue{ - "authentication.kubernetes.io/pod-name": {pod.Name}, - "authentication.kubernetes.io/pod-uid": {string(pod.UID)}, - }, saCSR.Spec.Extra) + if testutil.KubeServerSupportsCertificatesV1API(t, adminClient.Discovery()) { + saCSR, err := impersonationProxySAClient.Kubernetes.CertificatesV1().CertificateSigningRequests().Get(ctx, csrName, metav1.GetOptions{}) + require.NoError(t, err) + err = adminClient.CertificatesV1().CertificateSigningRequests().Delete(ctx, csrName, metav1.DeleteOptions{}) + require.NoError(t, err) + // make sure the user info that the CSR captured matches the SA, including the UID + require.Equal(t, expectedUsername, saCSR.Spec.Username) + require.Equal(t, expectedUID, saCSR.Spec.UID) + require.Equal(t, expectedGroups, saCSR.Spec.Groups) + require.Equal(t, map[string]certificatesv1.ExtraValue{ + "authentication.kubernetes.io/pod-name": {pod.Name}, + "authentication.kubernetes.io/pod-uid": {string(pod.UID)}, + }, saCSR.Spec.Extra) + } else { + // On old Kubernetes clusters use CertificatesV1beta1 + saCSR, err := impersonationProxySAClient.Kubernetes.CertificatesV1beta1().CertificateSigningRequests().Get(ctx, csrName, metav1.GetOptions{}) + require.NoError(t, err) + err = adminClient.CertificatesV1beta1().CertificateSigningRequests().Delete(ctx, csrName, metav1.DeleteOptions{}) + require.NoError(t, err) + // make sure the user info that the CSR captured matches the SA, including the UID + require.Equal(t, expectedUsername, saCSR.Spec.Username) + require.Equal(t, expectedUID, saCSR.Spec.UID) + require.Equal(t, expectedGroups, saCSR.Spec.Groups) + require.Equal(t, map[string]certificatesv1beta1.ExtraValue{ + "authentication.kubernetes.io/pod-name": {pod.Name}, + "authentication.kubernetes.io/pod-uid": {string(pod.UID)}, + }, saCSR.Spec.Extra) + } }) t.Run("kubectl as a client", func(t *testing.T) { @@ -2416,7 +2433,7 @@ func getCredForConfig(t *testing.T, config *rest.Config) *loginv1alpha1.ClusterC return out } -func getUIDAndExtraViaCSR(ctx context.Context, t *testing.T, uid string, client kubernetes.Interface) (string, map[string]certificatesv1beta1.ExtraValue) { +func getUIDAndExtraViaCSR(ctx context.Context, t *testing.T, uid string, client kubernetes.Interface) (string, map[string][]string) { t.Helper() privateKey, err := ecdsa.GenerateKey(elliptic.P256(), rand.Reader) @@ -2439,18 +2456,43 @@ func getUIDAndExtraViaCSR(ctx context.Context, t *testing.T, uid string, client ) require.NoError(t, err) - csReq, err := client.CertificatesV1beta1().CertificateSigningRequests().Get(ctx, csrName, metav1.GetOptions{}) - require.NoError(t, err) - - err = client.CertificatesV1beta1().CertificateSigningRequests().Delete(ctx, csrName, metav1.DeleteOptions{}) - require.NoError(t, err) - outUID := uid // in the future this may not be empty on some clusters - if len(outUID) == 0 { - outUID = csReq.Spec.UID + extrasAsStrings := map[string][]string{} + + if testutil.KubeServerSupportsCertificatesV1API(t, client.Discovery()) { + csReq, err := client.CertificatesV1().CertificateSigningRequests().Get(ctx, csrName, metav1.GetOptions{}) + require.NoError(t, err) + + err = client.CertificatesV1().CertificateSigningRequests().Delete(ctx, csrName, metav1.DeleteOptions{}) + require.NoError(t, err) + + if len(outUID) == 0 { + outUID = csReq.Spec.UID + } + + // Convert each `ExtraValue` to `[]string` to return, so we don't have to deal with v1beta1 types versus v1 types + for k, v := range csReq.Spec.Extra { + extrasAsStrings[k] = v + } + } else { + // On old Kubernetes clusters use CertificatesV1beta1 + csReq, err := client.CertificatesV1beta1().CertificateSigningRequests().Get(ctx, csrName, metav1.GetOptions{}) + require.NoError(t, err) + + err = client.CertificatesV1beta1().CertificateSigningRequests().Delete(ctx, csrName, metav1.DeleteOptions{}) + require.NoError(t, err) + + if len(outUID) == 0 { + outUID = csReq.Spec.UID + } + + // Convert each `ExtraValue` to `[]string` to return, so we don't have to deal with v1beta1 types versus v1 types + for k, v := range csReq.Spec.Extra { + extrasAsStrings[k] = v + } } - return outUID, csReq.Spec.Extra + return outUID, extrasAsStrings } func parallelIfNotEKS(t *testing.T) { diff --git a/test/integration/whoami_test.go b/test/integration/whoami_test.go index 17bae3219..f4aef611b 100644 --- a/test/integration/whoami_test.go +++ b/test/integration/whoami_test.go @@ -26,6 +26,7 @@ import ( "k8s.io/client-go/util/keyutil" identityv1alpha1 "go.pinniped.dev/generated/latest/apis/concierge/identity/v1alpha1" + "go.pinniped.dev/internal/testutil" "go.pinniped.dev/test/testlib" ) @@ -281,28 +282,53 @@ func TestWhoAmI_CSR_Parallel(t *testing.T) { ) require.NoError(t, err) + useCertificatesV1API := testutil.KubeServerSupportsCertificatesV1API(t, kubeClient.Discovery()) + t.Cleanup(func() { - require.NoError(t, kubeClient.CertificatesV1beta1().CertificateSigningRequests(). - Delete(context.Background(), csrName, metav1.DeleteOptions{})) + if useCertificatesV1API { + require.NoError(t, kubeClient.CertificatesV1().CertificateSigningRequests(). + Delete(context.Background(), csrName, metav1.DeleteOptions{})) + } else { + // On old clusters use v1beta1 + require.NoError(t, kubeClient.CertificatesV1beta1().CertificateSigningRequests(). + Delete(context.Background(), csrName, metav1.DeleteOptions{})) + } }) - // this is a blind update with no resource version checks, which is only safe during tests - // use the beta CSR API to support older clusters - _, err = kubeClient.CertificatesV1beta1().CertificateSigningRequests().UpdateApproval(ctx, &certificatesv1beta1.CertificateSigningRequest{ - ObjectMeta: metav1.ObjectMeta{ - Name: csrName, - }, - Status: certificatesv1beta1.CertificateSigningRequestStatus{ - Conditions: []certificatesv1beta1.CertificateSigningRequestCondition{ - { - Type: certificatesv1beta1.CertificateApproved, - Status: corev1.ConditionTrue, - Reason: "WhoAmICSRTest", + if useCertificatesV1API { + _, err = kubeClient.CertificatesV1().CertificateSigningRequests().UpdateApproval(ctx, csrName, &certificatesv1.CertificateSigningRequest{ + ObjectMeta: metav1.ObjectMeta{ + Name: csrName, + }, + Status: certificatesv1.CertificateSigningRequestStatus{ + Conditions: []certificatesv1.CertificateSigningRequestCondition{ + { + Type: certificatesv1.CertificateApproved, + Status: corev1.ConditionTrue, + Reason: "WhoAmICSRTest", + }, }, }, - }, - }, metav1.UpdateOptions{}) - require.NoError(t, err) + }, metav1.UpdateOptions{}) + require.NoError(t, err) + } else { + // On old Kubernetes clusters use CertificatesV1beta1 + _, err = kubeClient.CertificatesV1beta1().CertificateSigningRequests().UpdateApproval(ctx, &certificatesv1beta1.CertificateSigningRequest{ + ObjectMeta: metav1.ObjectMeta{ + Name: csrName, + }, + Status: certificatesv1beta1.CertificateSigningRequestStatus{ + Conditions: []certificatesv1beta1.CertificateSigningRequestCondition{ + { + Type: certificatesv1beta1.CertificateApproved, + Status: corev1.ConditionTrue, + Reason: "WhoAmICSRTest", + }, + }, + }, + }, metav1.UpdateOptions{}) + require.NoError(t, err) + } crtPEM, err := csr.WaitForCertificate(ctx, kubeClient, csrName, csrUID) require.NoError(t, err) From 91c8f747f4909adfec3a57e9ce7e4a14fcf04dc6 Mon Sep 17 00:00:00 2001 From: Monis Khan Date: Tue, 21 Sep 2021 09:19:50 -0400 Subject: [PATCH 3/3] certauthority: tolerate larger clock skew between API server and pinniped This change updates our certificate code to use the same 5 minute backdate that is used by the Kubernetes controller manager. This helps to account for clock skews between the API servers and the kubelets that are running the pinniped pods. While this backdating reflects a large percentage of the lifetime of our short lived certificates (100% for the 5 minute client certificates), even a 10 minute irrevocable client certificate is within our limits. When we move to the CSR based short lived certificates, they will always have at least a 15 minute lifetime (5 minute backdating plus 10 minute minimum valid duration). Signed-off-by: Monis Khan --- internal/certauthority/certauthority.go | 10 ++++------ internal/certauthority/certauthority_test.go | 4 ++-- .../impersonatorconfig/impersonator_config_test.go | 4 ++-- 3 files changed, 8 insertions(+), 10 deletions(-) diff --git a/internal/certauthority/certauthority.go b/internal/certauthority/certauthority.go index 60aed89fc..9fcfdcfbf 100644 --- a/internal/certauthority/certauthority.go +++ b/internal/certauthority/certauthority.go @@ -24,12 +24,10 @@ import ( ) // certBackdate is the amount of time before time.Now() that will be used to set -// a certificate's NotBefore field. -// -// This could certainly be made configurable by an installer of pinniped, but we -// will see if we can save adding a configuration knob with a reasonable default -// here. -const certBackdate = 10 * time.Second +// a certificate's NotBefore field. We use the same hard coded and unconfigurable +// backdate value as used by the Kubernetes controller manager certificate signer: +// https://github.com/kubernetes/kubernetes/blob/68d646a101005e95379d84160adf01d146bdd149/pkg/controller/certificates/signer/signer.go#L199 +const certBackdate = 5 * time.Minute type env struct { // secure random number generators for various steps (usually crypto/rand.Reader, but broken out here for tests). diff --git a/internal/certauthority/certauthority_test.go b/internal/certauthority/certauthority_test.go index 016268be0..4d55f6ff6 100644 --- a/internal/certauthority/certauthority_test.go +++ b/internal/certauthority/certauthority_test.go @@ -96,7 +96,7 @@ func TestNew(t *testing.T) { caCert, err := x509.ParseCertificate(ca.caCertBytes) require.NoError(t, err) require.Equal(t, "Test CA", caCert.Subject.CommonName) - require.WithinDuration(t, now.Add(-10*time.Second), caCert.NotBefore, 10*time.Second) + require.WithinDuration(t, now.Add(-5*time.Minute), caCert.NotBefore, 10*time.Second) require.WithinDuration(t, now.Add(time.Minute), caCert.NotAfter, 10*time.Second) require.NotNil(t, ca.privateKey) @@ -153,7 +153,7 @@ func TestNewInternal(t *testing.T) { }, wantCommonName: "Test CA", wantNotAfter: now.Add(time.Minute), - wantNotBefore: now.Add(-10 * time.Second), + wantNotBefore: now.Add(-5 * time.Minute), }, } for _, tt := range tests { diff --git a/internal/controller/impersonatorconfig/impersonator_config_test.go b/internal/controller/impersonatorconfig/impersonator_config_test.go index 00aeadd1f..968298673 100644 --- a/internal/controller/impersonatorconfig/impersonator_config_test.go +++ b/internal/controller/impersonatorconfig/impersonator_config_test.go @@ -1056,7 +1056,7 @@ func TestImpersonatorConfigControllerSync(t *testing.T) { caCert, err := x509.ParseCertificate(block.Bytes) require.NoError(t, err) require.Equal(t, "Pinniped Impersonation Proxy CA", caCert.Subject.CommonName) - require.WithinDuration(t, time.Now().Add(-10*time.Second), caCert.NotBefore, 10*time.Second) + require.WithinDuration(t, time.Now().Add(-5*time.Minute), caCert.NotBefore, 10*time.Second) require.WithinDuration(t, time.Now().Add(100*time.Hour*24*365), caCert.NotAfter, 10*time.Second) return createdCertPEM } @@ -1077,7 +1077,7 @@ func TestImpersonatorConfigControllerSync(t *testing.T) { r.NotNil(createdCertPEM) validCert := testutil.ValidateServerCertificate(t, string(caCert), string(createdCertPEM)) validCert.RequireMatchesPrivateKey(string(createdKeyPEM)) - validCert.RequireLifetime(time.Now().Add(-10*time.Second), time.Now().Add(100*time.Hour*24*365), 10*time.Second) + validCert.RequireLifetime(time.Now().Add(-5*time.Minute), time.Now().Add(100*time.Hour*24*365), 10*time.Second) } var requireSigningCertProviderHasLoadedCerts = func(certPEM, keyPEM []byte) {