From b80cbb8cc55edc41a5d427f88a65144d25535d1e Mon Sep 17 00:00:00 2001 From: Matt Moyer Date: Mon, 3 May 2021 16:20:13 -0500 Subject: [PATCH 1/2] Run kube-cert-agent pod as Concierge ServiceAccount. Since 0dfb3e95c55b0f38d899b5efc40cd0643e1e0f86, we no longer directly create the kube-cert-agent Pod, so our "use" permission on PodSecurityPolicies no longer has the intended effect. Since the deployments controller is now the one creating pods for us, we need to get the permission on the PodSpec of the target pod instead, which we do somewhat simply by using the same service account as the main Concierge pods. We still set `automountServiceAccountToken: false`, so this should not actually give any useful permissions to the agent pod when running. Signed-off-by: Matt Moyer --- deploy/concierge/deployment.yaml | 1 + internal/config/concierge/config.go | 3 +++ internal/config/concierge/config_test.go | 15 ++++++++++++++- internal/config/concierge/types.go | 1 + .../controller/kubecertagent/kubecertagent.go | 4 ++++ .../kubecertagent/kubecertagent_test.go | 2 ++ internal/controllermanager/prepare_controllers.go | 1 + 7 files changed, 26 insertions(+), 1 deletion(-) diff --git a/deploy/concierge/deployment.yaml b/deploy/concierge/deployment.yaml index 223b7fa8..ad12e915 100644 --- a/deploy/concierge/deployment.yaml +++ b/deploy/concierge/deployment.yaml @@ -47,6 +47,7 @@ data: impersonationTLSCertificateSecret: (@= defaultResourceNameWithSuffix("impersonation-proxy-tls-serving-certificate") @) impersonationCACertificateSecret: (@= defaultResourceNameWithSuffix("impersonation-proxy-ca-certificate") @) impersonationSignerSecret: (@= defaultResourceNameWithSuffix("impersonation-proxy-signer-ca-certificate") @) + serviceAccount: (@= defaultResourceName() @) labels: (@= json.encode(labels()).rstrip() @) kubeCertAgent: namePrefix: (@= defaultResourceNameWithSuffix("kube-cert-agent-") @) diff --git a/internal/config/concierge/config.go b/internal/config/concierge/config.go index b4d4c9a5..4c080a48 100644 --- a/internal/config/concierge/config.go +++ b/internal/config/concierge/config.go @@ -122,6 +122,9 @@ func validateNames(names *NamesConfigSpec) error { if names.ImpersonationSignerSecret == "" { missingNames = append(missingNames, "impersonationSignerSecret") } + if names.ServiceAccount == "" { + missingNames = append(missingNames, "serviceAccount") + } if len(missingNames) > 0 { return constable.Error("missing required names: " + strings.Join(missingNames, ", ")) } diff --git a/internal/config/concierge/config_test.go b/internal/config/concierge/config_test.go index 0034d06b..da1172e7 100644 --- a/internal/config/concierge/config_test.go +++ b/internal/config/concierge/config_test.go @@ -43,6 +43,7 @@ func TestFromPath(t *testing.T) { impersonationCACertificateSecret: impersonationCACertificateSecret-value impersonationSignerSecret: impersonationSignerSecret-value impersonationSignerSecret: impersonationSignerSecret-value + serviceAccount: serviceAccount-value labels: myLabelKey1: myLabelValue1 myLabelKey2: myLabelValue2 @@ -72,6 +73,7 @@ func TestFromPath(t *testing.T) { ImpersonationTLSCertificateSecret: "impersonationTLSCertificateSecret-value", ImpersonationCACertificateSecret: "impersonationCACertificateSecret-value", ImpersonationSignerSecret: "impersonationSignerSecret-value", + ServiceAccount: "serviceAccount-value", }, Labels: map[string]string{ "myLabelKey1": "myLabelValue1", @@ -98,6 +100,7 @@ func TestFromPath(t *testing.T) { impersonationTLSCertificateSecret: impersonationTLSCertificateSecret-value impersonationCACertificateSecret: impersonationCACertificateSecret-value impersonationSignerSecret: impersonationSignerSecret-value + serviceAccount: serviceAccount-value `), wantConfig: &Config{ DiscoveryInfo: DiscoveryInfoSpec{ @@ -119,6 +122,7 @@ func TestFromPath(t *testing.T) { ImpersonationTLSCertificateSecret: "impersonationTLSCertificateSecret-value", ImpersonationCACertificateSecret: "impersonationCACertificateSecret-value", ImpersonationSignerSecret: "impersonationSignerSecret-value", + ServiceAccount: "serviceAccount-value", }, Labels: map[string]string{}, KubeCertAgentConfig: KubeCertAgentSpec{ @@ -133,7 +137,7 @@ func TestFromPath(t *testing.T) { wantError: "validate names: missing required names: servingCertificateSecret, credentialIssuer, " + "apiService, impersonationConfigMap, impersonationLoadBalancerService, " + "impersonationTLSCertificateSecret, impersonationCACertificateSecret, " + - "impersonationSignerSecret", + "impersonationSignerSecret, serviceAccount", }, { name: "Missing apiService name", @@ -147,6 +151,7 @@ func TestFromPath(t *testing.T) { impersonationTLSCertificateSecret: impersonationTLSCertificateSecret-value impersonationCACertificateSecret: impersonationCACertificateSecret-value impersonationSignerSecret: impersonationSignerSecret-value + serviceAccount: serviceAccount-value `), wantError: "validate names: missing required names: apiService", }, @@ -162,6 +167,7 @@ func TestFromPath(t *testing.T) { impersonationTLSCertificateSecret: impersonationTLSCertificateSecret-value impersonationCACertificateSecret: impersonationCACertificateSecret-value impersonationSignerSecret: impersonationSignerSecret-value + serviceAccount: serviceAccount-value `), wantError: "validate names: missing required names: credentialIssuer", }, @@ -177,6 +183,7 @@ func TestFromPath(t *testing.T) { impersonationTLSCertificateSecret: impersonationTLSCertificateSecret-value impersonationCACertificateSecret: impersonationCACertificateSecret-value impersonationSignerSecret: impersonationSignerSecret-value + serviceAccount: serviceAccount-value `), wantError: "validate names: missing required names: servingCertificateSecret", }, @@ -192,6 +199,7 @@ func TestFromPath(t *testing.T) { impersonationTLSCertificateSecret: impersonationTLSCertificateSecret-value impersonationCACertificateSecret: impersonationCACertificateSecret-value impersonationSignerSecret: impersonationSignerSecret-value + serviceAccount: serviceAccount-value `), wantError: "validate names: missing required names: impersonationConfigMap", }, @@ -207,6 +215,7 @@ func TestFromPath(t *testing.T) { impersonationTLSCertificateSecret: impersonationTLSCertificateSecret-value impersonationCACertificateSecret: impersonationCACertificateSecret-value impersonationSignerSecret: impersonationSignerSecret-value + serviceAccount: serviceAccount-value `), wantError: "validate names: missing required names: impersonationLoadBalancerService", }, @@ -222,6 +231,7 @@ func TestFromPath(t *testing.T) { impersonationLoadBalancerService: impersonationLoadBalancerService-value impersonationCACertificateSecret: impersonationCACertificateSecret-value impersonationSignerSecret: impersonationSignerSecret-value + serviceAccount: serviceAccount-value `), wantError: "validate names: missing required names: impersonationTLSCertificateSecret", }, @@ -237,6 +247,7 @@ func TestFromPath(t *testing.T) { impersonationLoadBalancerService: impersonationLoadBalancerService-value impersonationTLSCertificateSecret: impersonationTLSCertificateSecret-value impersonationSignerSecret: impersonationSignerSecret-value + serviceAccount: serviceAccount-value `), wantError: "validate names: missing required names: impersonationCACertificateSecret", }, @@ -252,6 +263,7 @@ func TestFromPath(t *testing.T) { impersonationLoadBalancerService: impersonationLoadBalancerService-value impersonationTLSCertificateSecret: impersonationTLSCertificateSecret-value impersonationCACertificateSecret: impersonationCACertificateSecret-value + serviceAccount: serviceAccount-value `), wantError: "validate names: missing required names: impersonationSignerSecret", }, @@ -265,6 +277,7 @@ func TestFromPath(t *testing.T) { apiService: pinniped-api impersonationLoadBalancerService: impersonationLoadBalancerService-value impersonationSignerSecret: impersonationSignerSecret-value + serviceAccount: serviceAccount-value `), wantError: "validate names: missing required names: impersonationConfigMap, " + "impersonationTLSCertificateSecret, impersonationCACertificateSecret", diff --git a/internal/config/concierge/types.go b/internal/config/concierge/types.go index cfec621e..b2fed37b 100644 --- a/internal/config/concierge/types.go +++ b/internal/config/concierge/types.go @@ -41,6 +41,7 @@ type NamesConfigSpec struct { ImpersonationTLSCertificateSecret string `json:"impersonationTLSCertificateSecret"` ImpersonationCACertificateSecret string `json:"impersonationCACertificateSecret"` ImpersonationSignerSecret string `json:"impersonationSignerSecret"` + ServiceAccount string `json:"serviceAccount"` } // ServingCertificateConfigSpec contains the configuration knobs for the API's diff --git a/internal/controller/kubecertagent/kubecertagent.go b/internal/controller/kubecertagent/kubecertagent.go index 478d27fe..e45bf850 100644 --- a/internal/controller/kubecertagent/kubecertagent.go +++ b/internal/controller/kubecertagent/kubecertagent.go @@ -64,6 +64,9 @@ type AgentConfig struct { // NamePrefix will be prefixed to all agent pod names. NamePrefix string + // ServiceAccountName is the service account under which to run the agent pods. + ServiceAccountName string + // ContainerImagePullSecrets is a list of names of Kubernetes Secret objects that will be used as // ImagePullSecrets on the kube-cert-agent pods. ContainerImagePullSecrets []string @@ -472,6 +475,7 @@ func (c *agentController) newAgentDeployment(controllerManagerPod *corev1.Pod) * RestartPolicy: corev1.RestartPolicyAlways, NodeSelector: controllerManagerPod.Spec.NodeSelector, AutomountServiceAccountToken: pointer.BoolPtr(false), + ServiceAccountName: c.cfg.ServiceAccountName, NodeName: controllerManagerPod.Spec.NodeName, Tolerations: controllerManagerPod.Spec.Tolerations, // We need to run the agent pod as root since the file permissions diff --git a/internal/controller/kubecertagent/kubecertagent_test.go b/internal/controller/kubecertagent/kubecertagent_test.go index 4a1a17f9..73d0a749 100644 --- a/internal/controller/kubecertagent/kubecertagent_test.go +++ b/internal/controller/kubecertagent/kubecertagent_test.go @@ -123,6 +123,7 @@ func TestAgentController(t *testing.T) { }}, RestartPolicy: corev1.RestartPolicyAlways, TerminationGracePeriodSeconds: pointer.Int64Ptr(0), + ServiceAccountName: "test-service-account-name", AutomountServiceAccountToken: pointer.BoolPtr(false), SecurityContext: &corev1.PodSecurityContext{ RunAsUser: pointer.Int64Ptr(0), @@ -672,6 +673,7 @@ func TestAgentController(t *testing.T) { AgentConfig{ Namespace: "concierge", ContainerImage: "pinniped-server-image", + ServiceAccountName: "test-service-account-name", NamePrefix: "pinniped-concierge-kube-cert-agent-", ContainerImagePullSecrets: []string{"pinniped-image-pull-secret"}, CredentialIssuerName: "pinniped-concierge-config", diff --git a/internal/controllermanager/prepare_controllers.go b/internal/controllermanager/prepare_controllers.go index 8990a4ec..ed74c4ac 100644 --- a/internal/controllermanager/prepare_controllers.go +++ b/internal/controllermanager/prepare_controllers.go @@ -121,6 +121,7 @@ func PrepareControllers(c *Config) (func(ctx context.Context), error) { agentConfig := kubecertagent.AgentConfig{ Namespace: c.ServerInstallationInfo.Namespace, + ServiceAccountName: c.NamesConfig.ServiceAccount, ContainerImage: *c.KubeCertAgentConfig.Image, NamePrefix: *c.KubeCertAgentConfig.NamePrefix, ContainerImagePullSecrets: c.KubeCertAgentConfig.ImagePullSecrets, From 165bef78092c1002e13e58bb7354a175159f6400 Mon Sep 17 00:00:00 2001 From: Matt Moyer Date: Mon, 3 May 2021 16:31:48 -0500 Subject: [PATCH 2/2] Split out kube-cert-agent service account and bindings. Followup on the previous comment to split apart the ServiceAccount of the kube-cert-agent and the main concierge pod. This is a bit cleaner and ensures that in testing our main Concierge pod never requires any privileged permissions. Signed-off-by: Matt Moyer --- deploy/concierge/deployment.yaml | 9 +++++- deploy/concierge/rbac.yaml | 31 +++++++++++++++++-- internal/config/concierge/config.go | 4 +-- internal/config/concierge/config_test.go | 28 ++++++++--------- internal/config/concierge/types.go | 2 +- .../controllermanager/prepare_controllers.go | 2 +- 6 files changed, 54 insertions(+), 22 deletions(-) diff --git a/deploy/concierge/deployment.yaml b/deploy/concierge/deployment.yaml index ad12e915..525257a7 100644 --- a/deploy/concierge/deployment.yaml +++ b/deploy/concierge/deployment.yaml @@ -22,6 +22,13 @@ metadata: labels: #@ labels() --- apiVersion: v1 +kind: ServiceAccount +metadata: + name: #@ defaultResourceNameWithSuffix("kube-cert-agent") + namespace: #@ namespace() + labels: #@ labels() +--- +apiVersion: v1 kind: ConfigMap metadata: name: #@ defaultResourceNameWithSuffix("config") @@ -47,7 +54,7 @@ data: impersonationTLSCertificateSecret: (@= defaultResourceNameWithSuffix("impersonation-proxy-tls-serving-certificate") @) impersonationCACertificateSecret: (@= defaultResourceNameWithSuffix("impersonation-proxy-ca-certificate") @) impersonationSignerSecret: (@= defaultResourceNameWithSuffix("impersonation-proxy-signer-ca-certificate") @) - serviceAccount: (@= defaultResourceName() @) + agentServiceAccount: (@= defaultResourceNameWithSuffix("kube-cert-agent") @) labels: (@= json.encode(labels()).rstrip() @) kubeCertAgent: namePrefix: (@= defaultResourceNameWithSuffix("kube-cert-agent-") @) diff --git a/deploy/concierge/rbac.yaml b/deploy/concierge/rbac.yaml index 74e5653e..12350714 100644 --- a/deploy/concierge/rbac.yaml +++ b/deploy/concierge/rbac.yaml @@ -24,9 +24,6 @@ rules: - apiGroups: [ flowcontrol.apiserver.k8s.io ] resources: [ flowschemas, prioritylevelconfigurations ] verbs: [ get, list, watch ] - - apiGroups: [ policy ] - resources: [ podsecuritypolicies ] - verbs: [ use ] - apiGroups: [ security.openshift.io ] resources: [ securitycontextconstraints ] verbs: [ use ] @@ -67,6 +64,34 @@ roleRef: name: #@ defaultResourceNameWithSuffix("aggregated-api-server") apiGroup: rbac.authorization.k8s.io +#! Give permission to the kube-cert-agent Pod to run privileged. +--- +apiVersion: rbac.authorization.k8s.io/v1 +kind: Role +metadata: + name: #@ defaultResourceNameWithSuffix("kube-cert-agent") + namespace: #@ namespace() + labels: #@ labels() +rules: + - apiGroups: [ policy ] + resources: [ podsecuritypolicies ] + verbs: [ use ] +--- +kind: RoleBinding +apiVersion: rbac.authorization.k8s.io/v1 +metadata: + name: #@ defaultResourceNameWithSuffix("kube-cert-agent") + namespace: #@ namespace() + labels: #@ labels() +subjects: + - kind: ServiceAccount + name: #@ defaultResourceNameWithSuffix("kube-cert-agent") + namespace: #@ namespace() +roleRef: + kind: Role + name: #@ defaultResourceNameWithSuffix("kube-cert-agent") + apiGroup: rbac.authorization.k8s.io + #! Give permission to various objects within the app's own namespace --- apiVersion: rbac.authorization.k8s.io/v1 diff --git a/internal/config/concierge/config.go b/internal/config/concierge/config.go index 4c080a48..610caa7e 100644 --- a/internal/config/concierge/config.go +++ b/internal/config/concierge/config.go @@ -122,8 +122,8 @@ func validateNames(names *NamesConfigSpec) error { if names.ImpersonationSignerSecret == "" { missingNames = append(missingNames, "impersonationSignerSecret") } - if names.ServiceAccount == "" { - missingNames = append(missingNames, "serviceAccount") + if names.AgentServiceAccount == "" { + missingNames = append(missingNames, "agentServiceAccount") } if len(missingNames) > 0 { return constable.Error("missing required names: " + strings.Join(missingNames, ", ")) diff --git a/internal/config/concierge/config_test.go b/internal/config/concierge/config_test.go index da1172e7..1101d2d5 100644 --- a/internal/config/concierge/config_test.go +++ b/internal/config/concierge/config_test.go @@ -43,7 +43,7 @@ func TestFromPath(t *testing.T) { impersonationCACertificateSecret: impersonationCACertificateSecret-value impersonationSignerSecret: impersonationSignerSecret-value impersonationSignerSecret: impersonationSignerSecret-value - serviceAccount: serviceAccount-value + agentServiceAccount: agentServiceAccount-value labels: myLabelKey1: myLabelValue1 myLabelKey2: myLabelValue2 @@ -73,7 +73,7 @@ func TestFromPath(t *testing.T) { ImpersonationTLSCertificateSecret: "impersonationTLSCertificateSecret-value", ImpersonationCACertificateSecret: "impersonationCACertificateSecret-value", ImpersonationSignerSecret: "impersonationSignerSecret-value", - ServiceAccount: "serviceAccount-value", + AgentServiceAccount: "agentServiceAccount-value", }, Labels: map[string]string{ "myLabelKey1": "myLabelValue1", @@ -100,7 +100,7 @@ func TestFromPath(t *testing.T) { impersonationTLSCertificateSecret: impersonationTLSCertificateSecret-value impersonationCACertificateSecret: impersonationCACertificateSecret-value impersonationSignerSecret: impersonationSignerSecret-value - serviceAccount: serviceAccount-value + agentServiceAccount: agentServiceAccount-value `), wantConfig: &Config{ DiscoveryInfo: DiscoveryInfoSpec{ @@ -122,7 +122,7 @@ func TestFromPath(t *testing.T) { ImpersonationTLSCertificateSecret: "impersonationTLSCertificateSecret-value", ImpersonationCACertificateSecret: "impersonationCACertificateSecret-value", ImpersonationSignerSecret: "impersonationSignerSecret-value", - ServiceAccount: "serviceAccount-value", + AgentServiceAccount: "agentServiceAccount-value", }, Labels: map[string]string{}, KubeCertAgentConfig: KubeCertAgentSpec{ @@ -137,7 +137,7 @@ func TestFromPath(t *testing.T) { wantError: "validate names: missing required names: servingCertificateSecret, credentialIssuer, " + "apiService, impersonationConfigMap, impersonationLoadBalancerService, " + "impersonationTLSCertificateSecret, impersonationCACertificateSecret, " + - "impersonationSignerSecret, serviceAccount", + "impersonationSignerSecret, agentServiceAccount", }, { name: "Missing apiService name", @@ -151,7 +151,7 @@ func TestFromPath(t *testing.T) { impersonationTLSCertificateSecret: impersonationTLSCertificateSecret-value impersonationCACertificateSecret: impersonationCACertificateSecret-value impersonationSignerSecret: impersonationSignerSecret-value - serviceAccount: serviceAccount-value + agentServiceAccount: agentServiceAccount-value `), wantError: "validate names: missing required names: apiService", }, @@ -167,7 +167,7 @@ func TestFromPath(t *testing.T) { impersonationTLSCertificateSecret: impersonationTLSCertificateSecret-value impersonationCACertificateSecret: impersonationCACertificateSecret-value impersonationSignerSecret: impersonationSignerSecret-value - serviceAccount: serviceAccount-value + agentServiceAccount: agentServiceAccount-value `), wantError: "validate names: missing required names: credentialIssuer", }, @@ -183,7 +183,7 @@ func TestFromPath(t *testing.T) { impersonationTLSCertificateSecret: impersonationTLSCertificateSecret-value impersonationCACertificateSecret: impersonationCACertificateSecret-value impersonationSignerSecret: impersonationSignerSecret-value - serviceAccount: serviceAccount-value + agentServiceAccount: agentServiceAccount-value `), wantError: "validate names: missing required names: servingCertificateSecret", }, @@ -199,7 +199,7 @@ func TestFromPath(t *testing.T) { impersonationTLSCertificateSecret: impersonationTLSCertificateSecret-value impersonationCACertificateSecret: impersonationCACertificateSecret-value impersonationSignerSecret: impersonationSignerSecret-value - serviceAccount: serviceAccount-value + agentServiceAccount: agentServiceAccount-value `), wantError: "validate names: missing required names: impersonationConfigMap", }, @@ -215,7 +215,7 @@ func TestFromPath(t *testing.T) { impersonationTLSCertificateSecret: impersonationTLSCertificateSecret-value impersonationCACertificateSecret: impersonationCACertificateSecret-value impersonationSignerSecret: impersonationSignerSecret-value - serviceAccount: serviceAccount-value + agentServiceAccount: agentServiceAccount-value `), wantError: "validate names: missing required names: impersonationLoadBalancerService", }, @@ -231,7 +231,7 @@ func TestFromPath(t *testing.T) { impersonationLoadBalancerService: impersonationLoadBalancerService-value impersonationCACertificateSecret: impersonationCACertificateSecret-value impersonationSignerSecret: impersonationSignerSecret-value - serviceAccount: serviceAccount-value + agentServiceAccount: agentServiceAccount-value `), wantError: "validate names: missing required names: impersonationTLSCertificateSecret", }, @@ -247,7 +247,7 @@ func TestFromPath(t *testing.T) { impersonationLoadBalancerService: impersonationLoadBalancerService-value impersonationTLSCertificateSecret: impersonationTLSCertificateSecret-value impersonationSignerSecret: impersonationSignerSecret-value - serviceAccount: serviceAccount-value + agentServiceAccount: agentServiceAccount-value `), wantError: "validate names: missing required names: impersonationCACertificateSecret", }, @@ -263,7 +263,7 @@ func TestFromPath(t *testing.T) { impersonationLoadBalancerService: impersonationLoadBalancerService-value impersonationTLSCertificateSecret: impersonationTLSCertificateSecret-value impersonationCACertificateSecret: impersonationCACertificateSecret-value - serviceAccount: serviceAccount-value + agentServiceAccount: agentServiceAccount-value `), wantError: "validate names: missing required names: impersonationSignerSecret", }, @@ -277,7 +277,7 @@ func TestFromPath(t *testing.T) { apiService: pinniped-api impersonationLoadBalancerService: impersonationLoadBalancerService-value impersonationSignerSecret: impersonationSignerSecret-value - serviceAccount: serviceAccount-value + agentServiceAccount: agentServiceAccount-value `), wantError: "validate names: missing required names: impersonationConfigMap, " + "impersonationTLSCertificateSecret, impersonationCACertificateSecret", diff --git a/internal/config/concierge/types.go b/internal/config/concierge/types.go index b2fed37b..ecd36d0a 100644 --- a/internal/config/concierge/types.go +++ b/internal/config/concierge/types.go @@ -41,7 +41,7 @@ type NamesConfigSpec struct { ImpersonationTLSCertificateSecret string `json:"impersonationTLSCertificateSecret"` ImpersonationCACertificateSecret string `json:"impersonationCACertificateSecret"` ImpersonationSignerSecret string `json:"impersonationSignerSecret"` - ServiceAccount string `json:"serviceAccount"` + AgentServiceAccount string `json:"agentServiceAccount"` } // ServingCertificateConfigSpec contains the configuration knobs for the API's diff --git a/internal/controllermanager/prepare_controllers.go b/internal/controllermanager/prepare_controllers.go index ed74c4ac..acd5bee7 100644 --- a/internal/controllermanager/prepare_controllers.go +++ b/internal/controllermanager/prepare_controllers.go @@ -121,7 +121,7 @@ func PrepareControllers(c *Config) (func(ctx context.Context), error) { agentConfig := kubecertagent.AgentConfig{ Namespace: c.ServerInstallationInfo.Namespace, - ServiceAccountName: c.NamesConfig.ServiceAccount, + ServiceAccountName: c.NamesConfig.AgentServiceAccount, ContainerImage: *c.KubeCertAgentConfig.Image, NamePrefix: *c.KubeCertAgentConfig.NamePrefix, ContainerImagePullSecrets: c.KubeCertAgentConfig.ImagePullSecrets,