From 4d238a1086f9099c41715df620df0921175a6fcc Mon Sep 17 00:00:00 2001 From: Andrew Harding Date: Fri, 23 Sep 2022 15:51:47 -0600 Subject: [PATCH] Allow k8s workload attestation when container is not ready This change introduces a new configurable, `disable_container_selectors` which configures the K8s Workload Attestor to only produce pod-related selectors. This allows for workload attesation to succeed when the attestor can positively locate the workload pod but cannot yet locate the workload container at the time of attestation (e.g. postStart hook is still executing). See issue #3092 for more details. Fixes: #3092 Signed-off-by: Andrew Harding --- doc/plugin_agent_workloadattestor_k8s.md | 3 +- pkg/agent/plugin/workloadattestor/k8s/k8s.go | 88 +++++++++++-------- .../plugin/workloadattestor/k8s/k8s_posix.go | 4 - .../workloadattestor/k8s/k8s_posix_test.go | 2 +- .../plugin/workloadattestor/k8s/k8s_test.go | 43 +++++++-- .../workloadattestor/k8s/k8s_windows.go | 4 - 6 files changed, 93 insertions(+), 51 deletions(-) diff --git a/doc/plugin_agent_workloadattestor_k8s.md b/doc/plugin_agent_workloadattestor_k8s.md index 0297fd43b8..a6a6fbc606 100644 --- a/doc/plugin_agent_workloadattestor_k8s.md +++ b/doc/plugin_agent_workloadattestor_k8s.md @@ -42,6 +42,7 @@ since [hostprocess](https://kubernetes.io/docs/tasks/configure-pod-container/cre | Configuration | Description | | ------------- | ----------- | +| `disable_container_selectors` | If true, container selectors are not produced. This can be used to produce pod selectors when the workload pod is known but the workload container is not ready at the time of attestation. | | `kubelet_read_only_port` | The kubelet read-only port. This is mutually exlusive with `kubelet_secure_port`. | | `kubelet_secure_port` | The kubelet secure port. It defaults to `10250` unless `kubelet_read_only_port` is set. | | `kubelet_ca_path` | The path on disk to a file containing CA certificates used to verify the kubelet certificate. Required unless `skip_kubelet_verification` is set. Defaults to the cluster CA bundle `/run/secrets/kubernetes.io/serviceaccount/ca.crt`. | @@ -134,4 +135,4 @@ This plugin is only supported on Unix systems. ### Known issues -* This plugin may fail to correctly attest workloads in pods that use lifecycle hooks to alter pod start behavior. This includes Istio workloads when the `holdApplicationUntilProxyStarts` configurable is set to true. Please see [#3092](https://github.com/spiffe/spire/issues/3092) for more information. +* This plugin may fail to correctly attest workloads in pods that use lifecycle hooks to alter pod start behavior. This includes Istio workloads when the `holdApplicationUntilProxyStarts` configurable is set to true. Please see [#3092](https://github.com/spiffe/spire/issues/3092) for more information. The `disable_container_selectors` configurable can be used successfully attest workloads in this situation, albeit with reduced selector granularity (i.e. pod selectors only). diff --git a/pkg/agent/plugin/workloadattestor/k8s/k8s.go b/pkg/agent/plugin/workloadattestor/k8s/k8s.go index d9b59017d8..27952a7bfa 100644 --- a/pkg/agent/plugin/workloadattestor/k8s/k8s.go +++ b/pkg/agent/plugin/workloadattestor/k8s/k8s.go @@ -46,13 +46,6 @@ func BuiltIn() catalog.BuiltIn { return builtin(New()) } -type containerLookup int - -const ( - containerInPod = iota - containerNotInPod -) - func builtin(p *Plugin) catalog.BuiltIn { return catalog.MakeBuiltIn(pluginName, workloadattestorv1.WorkloadAttestorPluginServer(p), @@ -119,6 +112,13 @@ type HCLConfig struct { // ReloadInterval controls how often TLS and token configuration is loaded // from the disk. ReloadInterval string `hcl:"reload_interval"` + + // DisableContainerSelectors disables the gathering of selectors for the + // specific container running the workload. This allows attestation to + // succeed with just pod related selectors when the workload pod is known + // but the container may not be in a ready state at the time of attestation + // (e.g. when a postStart hook has yet to complete). + DisableContainerSelectors bool `hcl:"disable_container_selectors"` } // k8sConfig holds the configuration distilled from HCL @@ -135,6 +135,7 @@ type k8sConfig struct { KubeletCAPath string NodeName string ReloadInterval time.Duration + DisableContainerSelectors bool Client *kubeletClient LastReload time.Time @@ -180,6 +181,7 @@ func (p *Plugin) Attest(ctx context.Context, req *workloadattestorv1.AttestReque if err != nil { return nil, err } + podKnown := podUID != "" // Not a Kubernetes pod if containerID == "" { @@ -204,21 +206,36 @@ func (p *Plugin) Attest(ctx context.Context, req *workloadattestorv1.AttestReque var attestResponse *workloadattestorv1.AttestResponse for _, item := range list.Items { item := item - if isNotPod(item.UID, podUID) { + if podKnown && item.UID != podUID { + // The pod holding the container is known. Skip unrelated pods. continue } - lookupStatus, lookup := lookUpContainerInPod(containerID, item.Status, log) - switch lookup { - case containerInPod: + var selectorValues []string + + containerStatus, containerFound := lookUpContainerInPod(containerID, item.Status, log) + switch { + case containerFound: + // The workload container was found in this pod. Add pod + // selectors. Only add workload container selectors if + // container selectors have not been disabled. + selectorValues = append(selectorValues, getSelectorValuesFromPodInfo(&item)...) + if !config.DisableContainerSelectors { + selectorValues = append(selectorValues, getSelectorValuesFromWorkloadContainerStatus(containerStatus)...) + } + case podKnown && config.DisableContainerSelectors: + // The workload container was not found (i.e. not ready yet?) + // but the pod is known. If container selectors have been + // disabled, then allow the pod selectors to be used. + selectorValues = append(selectorValues, getSelectorValuesFromPodInfo(&item)...) + } + + if len(selectorValues) > 0 { if attestResponse != nil { log.Warn("Two pods found with same container Id") return nil, status.Error(codes.Internal, "two pods found with same container Id") } - attestResponse = &workloadattestorv1.AttestResponse{ - SelectorValues: getSelectorValuesFromPodInfo(&item, lookupStatus), - } - case containerNotInPod: + attestResponse = &workloadattestorv1.AttestResponse{SelectorValues: selectorValues} } } @@ -321,6 +338,7 @@ func (p *Plugin) Configure(ctx context.Context, req *configv1.ConfigureRequest) KubeletCAPath: config.KubeletCAPath, NodeName: nodeName, ReloadInterval: reloadInterval, + DisableContainerSelectors: config.DisableContainerSelectors, } if err := p.reloadKubeletClient(c); err != nil { return nil, err @@ -565,7 +583,7 @@ func (c *kubeletClient) GetPodList() (*corev1.PodList, error) { return out, nil } -func lookUpContainerInPod(containerID string, status corev1.PodStatus, log hclog.Logger) (*corev1.ContainerStatus, containerLookup) { +func lookUpContainerInPod(containerID string, status corev1.PodStatus, log hclog.Logger) (*corev1.ContainerStatus, bool) { for _, status := range status.ContainerStatuses { // TODO: should we be keying off of the status or is the lack of a // container id sufficient to know the container is not ready? @@ -582,7 +600,7 @@ func lookUpContainerInPod(containerID string, status corev1.PodStatus, log hclog } if containerID == containerURL.Host { - return &status, containerInPod + return &status, true } } @@ -602,16 +620,16 @@ func lookUpContainerInPod(containerID string, status corev1.PodStatus, log hclog } if containerID == containerURL.Host { - return &status, containerInPod + return &status, true } } - return nil, containerNotInPod + return nil, false } -func getPodImageIdentifiers(containerStatusArray []corev1.ContainerStatus) map[string]bool { +func getPodImageIdentifiers(containerStatuses ...corev1.ContainerStatus) map[string]struct{} { // Map is used purely to exclude duplicate selectors, value is unused. - podImages := make(map[string]bool) + podImages := make(map[string]struct{}) // Note that for each pod image we generate *2* matching selectors. // This is to support matching against ImageID, which has a SHA // docker.io/envoyproxy/envoy-alpine@sha256:bf862e5f5eca0a73e7e538224578c5cf867ce2be91b5eaed22afc153c00363eb @@ -620,36 +638,28 @@ func getPodImageIdentifiers(containerStatusArray []corev1.ContainerStatus) map[s // while also maintaining backwards compatibility and allowing for dynamic workload registration (k8s operator) // when the SHA is not yet known (e.g. before the image pull is initiated at workload creation time) // More info here: https://github.com/spiffe/spire/issues/2026 - for _, status := range containerStatusArray { - podImages[status.ImageID] = true - podImages[status.Image] = true + for _, containerStatus := range containerStatuses { + podImages[containerStatus.ImageID] = struct{}{} + podImages[containerStatus.Image] = struct{}{} } return podImages } -func getSelectorValuesFromPodInfo(pod *corev1.Pod, status *corev1.ContainerStatus) []string { - podImageIdentifiers := getPodImageIdentifiers(pod.Status.ContainerStatuses) - podInitImageIdentifiers := getPodImageIdentifiers(pod.Status.InitContainerStatuses) - containerImageIdentifiers := getPodImageIdentifiers([]corev1.ContainerStatus{*status}) - +func getSelectorValuesFromPodInfo(pod *corev1.Pod) []string { selectorValues := []string{ fmt.Sprintf("sa:%s", pod.Spec.ServiceAccountName), fmt.Sprintf("ns:%s", pod.Namespace), fmt.Sprintf("node-name:%s", pod.Spec.NodeName), fmt.Sprintf("pod-uid:%s", pod.UID), fmt.Sprintf("pod-name:%s", pod.Name), - fmt.Sprintf("container-name:%s", status.Name), fmt.Sprintf("pod-image-count:%s", strconv.Itoa(len(pod.Status.ContainerStatuses))), fmt.Sprintf("pod-init-image-count:%s", strconv.Itoa(len(pod.Status.InitContainerStatuses))), } - for containerImage := range containerImageIdentifiers { - selectorValues = append(selectorValues, fmt.Sprintf("container-image:%s", containerImage)) - } - for podImage := range podImageIdentifiers { + for podImage := range getPodImageIdentifiers(pod.Status.ContainerStatuses...) { selectorValues = append(selectorValues, fmt.Sprintf("pod-image:%s", podImage)) } - for podInitImage := range podInitImageIdentifiers { + for podInitImage := range getPodImageIdentifiers(pod.Status.InitContainerStatuses...) { selectorValues = append(selectorValues, fmt.Sprintf("pod-init-image:%s", podInitImage)) } @@ -664,6 +674,14 @@ func getSelectorValuesFromPodInfo(pod *corev1.Pod, status *corev1.ContainerStatu return selectorValues } +func getSelectorValuesFromWorkloadContainerStatus(status *corev1.ContainerStatus) []string { + selectorValues := []string{fmt.Sprintf("container-name:%s", status.Name)} + for containerImage := range getPodImageIdentifiers(*status) { + selectorValues = append(selectorValues, fmt.Sprintf("container-image:%s", containerImage)) + } + return selectorValues +} + func tryRead(r io.Reader) string { buf := make([]byte, 1024) n, _ := r.Read(buf) diff --git a/pkg/agent/plugin/workloadattestor/k8s/k8s_posix.go b/pkg/agent/plugin/workloadattestor/k8s/k8s_posix.go index 15e2fade92..a81555e8ba 100644 --- a/pkg/agent/plugin/workloadattestor/k8s/k8s_posix.go +++ b/pkg/agent/plugin/workloadattestor/k8s/k8s_posix.go @@ -175,7 +175,3 @@ func canonicalizePodUID(uid string) types.UID { return r }, uid)) } - -func isNotPod(itemPodUID, podUID types.UID) bool { - return podUID != "" && itemPodUID != podUID -} diff --git a/pkg/agent/plugin/workloadattestor/k8s/k8s_posix_test.go b/pkg/agent/plugin/workloadattestor/k8s/k8s_posix_test.go index 975b45fb79..9d8f3b233e 100644 --- a/pkg/agent/plugin/workloadattestor/k8s/k8s_posix_test.go +++ b/pkg/agent/plugin/workloadattestor/k8s/k8s_posix_test.go @@ -192,7 +192,7 @@ func (s *Suite) requireAttestFailWithDuplicateContainerID(p workloadattestor.Wor func (s *Suite) requireAttestSuccessWithPodSystemdCgroups(p workloadattestor.WorkloadAttestor) { s.addPodListResponse(podListFilePath) s.addCgroupsResponse(cgSystemdPidInPodFilePath) - s.requireAttestSuccess(p, testPodSelectors) + s.requireAttestSuccess(p, testPodAndContainerSelectors) } func TestGetContainerIDFromCGroups(t *testing.T) { diff --git a/pkg/agent/plugin/workloadattestor/k8s/k8s_test.go b/pkg/agent/plugin/workloadattestor/k8s/k8s_test.go index df688410f4..3b4ffa2568 100644 --- a/pkg/agent/plugin/workloadattestor/k8s/k8s_test.go +++ b/pkg/agent/plugin/workloadattestor/k8s/k8s_test.go @@ -56,9 +56,6 @@ FwOGLt+I3+9beT0vo+pn9Rq0squewFYe3aJbwpkyfP2xOovQCdm4PC8y `)) testPodSelectors = []*common.Selector{ - {Type: "k8s", Value: "container-image:docker-pullable://localhost/spiffe/blog@sha256:0cfdaced91cb46dd7af48309799a3c351e4ca2d5e1ee9737ca0cbd932cb79898"}, - {Type: "k8s", Value: "container-image:localhost/spiffe/blog:latest"}, - {Type: "k8s", Value: "container-name:blog"}, {Type: "k8s", Value: "node-name:k8s-node-1"}, {Type: "k8s", Value: "ns:default"}, {Type: "k8s", Value: "pod-image-count:2"}, @@ -75,6 +72,12 @@ FwOGLt+I3+9beT0vo+pn9Rq0squewFYe3aJbwpkyfP2xOovQCdm4PC8y {Type: "k8s", Value: "pod-uid:2c48913c-b29f-11e7-9350-020968147796"}, {Type: "k8s", Value: "sa:default"}, } + testContainerSelectors = []*common.Selector{ + {Type: "k8s", Value: "container-image:docker-pullable://localhost/spiffe/blog@sha256:0cfdaced91cb46dd7af48309799a3c351e4ca2d5e1ee9737ca0cbd932cb79898"}, + {Type: "k8s", Value: "container-image:localhost/spiffe/blog:latest"}, + {Type: "k8s", Value: "container-name:blog"}, + } + testPodAndContainerSelectors = append(testPodSelectors, testContainerSelectors...) ) type attestResult struct { @@ -146,7 +149,7 @@ func (s *Suite) TestAttestWithPidInPodAfterRetry() { select { case result := <-resultCh: s.Require().Nil(result.err) - s.requireSelectorsEqual(testPodSelectors, result.selectors) + s.requireSelectorsEqual(testPodAndContainerSelectors, result.selectors) case <-time.After(time.Minute): s.FailNow("timed out waiting for attest response") } @@ -262,6 +265,22 @@ func (s *Suite) TestAttestReachingKubeletViaNodeName() { `)) } +func (s *Suite) TestAttestWhenContainerReadyButContainerSelectorsDisabled() { + s.startInsecureKubelet() + p := s.loadInsecurePluginWithExtra("disable_container_selectors = true") + s.addPodListResponse(podListFilePath) + s.addGetContainerResponsePidInPod() + s.requireAttestSuccess(p, testPodSelectors) +} + +func (s *Suite) TestAttestWhenContainerNotReadyButContainerSelectorsDisabled() { + s.startInsecureKubelet() + p := s.loadInsecurePluginWithExtra("disable_container_selectors = true") + s.addPodListResponse(podListNotRunningFilePath) + s.addGetContainerResponsePidInPod() + s.requireAttestSuccess(p, testPodSelectors) +} + func (s *Suite) TestConfigure() { s.generateCerts("") @@ -607,6 +626,15 @@ func (s *Suite) loadInsecurePlugin() workloadattestor.WorkloadAttestor { `, s.kubeletPort())) } +func (s *Suite) loadInsecurePluginWithExtra(extraConfig string) workloadattestor.WorkloadAttestor { + return s.loadPlugin(fmt.Sprintf(` + kubelet_read_only_port = %d + max_poll_attempts = 5 + poll_retry_interval = "1s" + %s +`, s.kubeletPort(), extraConfig)) +} + func (s *Suite) startInsecureKubelet() { s.setServer(httptest.NewServer(http.HandlerFunc(s.serveHTTP))) } @@ -746,7 +774,7 @@ func (s *Suite) writeKey(path string, key *ecdsa.PrivateKey) { func (s *Suite) requireAttestSuccessWithPod(p workloadattestor.WorkloadAttestor) { s.addPodListResponse(podListFilePath) s.addGetContainerResponsePidInPod() - s.requireAttestSuccess(p, testPodSelectors) + s.requireAttestSuccess(p, testPodAndContainerSelectors) } func (s *Suite) requireAttestSuccess(p workloadattestor.WorkloadAttestor, expectedSelectors []*common.Selector) { @@ -762,8 +790,11 @@ func (s *Suite) requireAttestFailure(p workloadattestor.WorkloadAttestor, code c } func (s *Suite) requireSelectorsEqual(expected, actual []*common.Selector) { - // assert the selectors (sorting for consistency) + // assert the selectors (non-destructively sorting for consistency) + actual = append([]*common.Selector(nil), actual...) + expected = append([]*common.Selector(nil), expected...) util.SortSelectors(actual) + util.SortSelectors(expected) s.RequireProtoListEqual(expected, actual) } diff --git a/pkg/agent/plugin/workloadattestor/k8s/k8s_windows.go b/pkg/agent/plugin/workloadattestor/k8s/k8s_windows.go index 609e7dc29a..e18be5f5f6 100644 --- a/pkg/agent/plugin/workloadattestor/k8s/k8s_windows.go +++ b/pkg/agent/plugin/workloadattestor/k8s/k8s_windows.go @@ -45,7 +45,3 @@ func (p *Plugin) defaultTokenPath() string { mountPoint := p.getenv(containerMountPointEnvVar) return filepath.Join(mountPoint, defaultTokenPath) } - -func isNotPod(itemPodUID, podUID types.UID) bool { - return false -}