Add crio compatibility to WorkloadAttestor k8s - fixes #3183 #3242

goergch · 2022-07-13T11:59:12Z

Commit conforms to CONTRIBUTING.md?
Proper tests/regressions included?
Documentation updated?

Affected functionality

WorkloadAttestation Plugin k8s: compatibility with cri-o

Description of change
WorkloadAttestation Plugin k8s:

Adding cgroups regex to also match cgroup entries, that don't include a pod UI. That is explicitly currently crio, as no other container runtimes are known at the moment, that have the same behavior. The new regex therefore also explicitly checks for the "crio" string. It wasn't possible for me to combine the regex with the existing.
search through all pods and check, that the containerId found in cgroups, is is only present in one pod

Which issue this PR fixes
fixes #3183

MarcosDY

Thanks for this!!!

MarcosDY · 2022-07-14T15:47:49Z

pkg/agent/plugin/workloadattestor/k8s/k8s_posix.go

- if item.UID != podUID {
+
+ // podUID can be empty, when cgroup contains only containerID
+ if item.UID != podUID && podUID != "" {


On windows it is not possible to get podUID as it is happining in your use case, in order to solve that I created a function
what do you think about this:

if isNotPod(item.UID, podUID) { continue }

and function:

func isNotPod(itemPodUID, podUID types.UID) bool { // podUID can be empty if podUID == "" { return false } return item.UID != podUID } ``

The logic of the expression in line 198 is different: if podUI is set, only matching list Items are checked for equality.
I reformatted the condition to make it clearer

MarcosDY · 2022-07-14T15:49:39Z

pkg/agent/plugin/workloadattestor/k8s/k8s_posix.go

- }, nil
+ if attestResponse != nil {
+ log.Warn("Two pods found with same container Id")
+ return nil, status.Error(codes.Aborted, "Two pods found with same container Id")


errors must never start with uppercase on golang,
I'm not sure about using aborted or internal... as code, generally we use Internal in this cases

I'm not sure if we really need to make this verification, as long I know containerIDs are unique per node, even when a runtime is called to remove a container it only requires containerID.
In any case I'm ok keeping this validation

errors must never start with uppercase on golang,
I'm not sure about using aborted or internal... as code, generally we use Internal in this cases

done

I'm not sure if we really need to make this verification, as long I know containerIDs are unique per node, even when a runtime is called to remove a container it only requires containerID.
In any case I'm ok keeping this validation

I don't have a opinion on this. I just took the idea from @azdagron in #3183

MarcosDY · 2022-07-14T16:55:28Z

pkg/agent/plugin/workloadattestor/k8s/k8s_posix.go

@@ -598,6 +618,11 @@ func getPodUIDAndContainerIDFromCGroupPath(cgroupPath string) (types.UID, string
 matches := cgroupRE.FindStringSubmatch(cgroupPath)
 if matches != nil {


I found than there are cases where CRIO-O is providing pods on cgroups.
With minikube using cri-o as runtime,

Preparing Kubernetes v1.23.3 on CRI-O 1.22.1 ...

I got pod on cgroup

9:devices:/kubepods.slice/kubepods-besteffort.slice/kubepods-besteffort-pod561fd272_d131_47ef_a01b_46a997a778f3.slice/crio-030ded69d4c98fcf69c988f75a5eb3a1b4357e1432bd5510c936a40d7e9a1198.scope

And attestation works with that,

So instead of an if/else, what do you think about getting a list of regex, and validate cgroups against both regex?
and in case we get results from both make it fails?

in case of cgroupNoPodUidRE, maybe you can it always return 2 values, where the pod(first result) is always empty

I implemented the list of regex. In addition I moved to named groups in the regex to be more flexible with the sequence of podUID and containerid

It's interesting to see, that the behavior in our system is caused by kubeedge and not by cri-o. Sadly I could not find an easy reason for this in kubeedge`s settings or source code.

MarcosDY

Great Job!! adding some additional comments

MarcosDY · 2022-07-20T13:29:12Z

pkg/agent/plugin/workloadattestor/k8s/k8s_posix.go

+ // Currently only cri-o in combination with kubeedge is known for this abnormaly.
+ regexp.MustCompile(`` +
+ // /crio-
+ `(?P<poduid>)[[:punct:]]crio[[:punct:]]` +


mmm, I think it will cause that actual attestation against a regular CRI-O will stop working, because both regex will be true

good point.
Regex in gelang is quite limited. It would need a negative lookahead to not match, when a poduid is present.
I introduced a "mustnotmatch" group for this.

pkg/agent/plugin/workloadattestor/k8s/k8s_posix.go

pkg/agent/plugin/workloadattestor/k8s/k8s_test.go

pkg/agent/plugin/workloadattestor/k8s/k8s_posix.go

MarcosDY · 2022-07-20T13:35:12Z

pkg/agent/plugin/workloadattestor/k8s/k8s_posix.go

- }, nil
+ 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")


can you add an unit test for this?

TestAttestFailDuplicateContainerId already does this

MarcosDY · 2022-07-20T13:47:03Z

pkg/agent/plugin/workloadattestor/k8s/k8s_posix.go

+func reSubMatchMap(r *regexp.Regexp, str string) map[string]string {
+ match := r.FindStringSubmatch(str)
+ var subMatchMap map[string]string = nil
+ if match != nil {


an early return maybe can simplify this code:

match := r.FindStringSubmatch(str) if match == nil { return nil } subMatchMap = make(map[string]string) for i, name := range r.SubexpNames() { if i != 0 { subMatchMap[name] = match[i] } } return subMatchMap

MarcosDY · 2022-07-20T13:49:34Z

pkg/agent/plugin/workloadattestor/k8s/k8s_posix.go

+ if matches != nil {
+ if matchResults != nil {
+ log.Printf("More than one regex matches for cgroup %s", cgroupPath)
+ return "", "", false


can you add an unit test to verify that regular CRI-O (for example the one I put from minikube) still works?

MarcosDY

Changes looks good, but you will need to solve conflicts and solve DCO issues, you will need to signoff all commits

Signed-off-by: Christian Görg <christian.goerg@trumpf.com>

* Add integration test for CRD mode Signed-off-by: Marcos Yacob <marcos.yacob@hpe.com> Signed-off-by: Christian Görg <christian.goerg@trumpf.com>

) Bumps [github.com/open-policy-agent/opa](https://github.com/open-policy-agent/opa) from 0.42.0 to 0.42.1. - [Release notes](https://github.com/open-policy-agent/opa/releases) - [Changelog](https://github.com/open-policy-agent/opa/blob/main/CHANGELOG.md) - [Commits](open-policy-agent/opa@v0.42.0...v0.42.1) --- updated-dependencies: - dependency-name: github.com/open-policy-agent/opa dependency-type: direct:production update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Signed-off-by: Christian Görg <christian.goerg@trumpf.com>

Also pulled in CHANGELOG for 1.3.2 and updated upgrade IT Signed-off-by: Andrew Harding <aharding@vmware.com> Signed-off-by: Christian Görg <christian.goerg@trumpf.com>

* Remove webhook mode from k8s-workload-registrar This was deprecated in 1.3.0. Removing for 1.4.0. Signed-off-by: Andrew Harding <aharding@vmware.com> * Address PR comments Signed-off-by: Andrew Harding <aharding@vmware.com> Signed-off-by: Christian Görg <christian.goerg@trumpf.com>

checks cgroups against regex array Signed-off-by: Christian Görg <christian.goerg@trumpf.com>

Co-authored-by: Marcos Yacob <marcos.yacob@hpe.com> Signed-off-by: Christian Görg <christian.goerg@trumpf.com>

Signed-off-by: Christian Görg <christian.goerg@trumpf.com>

goergch · 2022-07-26T06:03:43Z

Changes looks good, but you will need to solve conflicts and solve DCO issues, you will need to signoff all commits

main is now freshly merged into the pull request and every commit signed off. Thanks for the productive review @MarcosDY

MarcosDY

Looks great!! just a pretty simple comment, and there are 2 lint failing

MarcosDY · 2022-07-26T14:43:26Z

pkg/agent/plugin/workloadattestor/k8s/k8s.go

 for _, item := range list.Items {
 item := item
- if isNotPod(item.UID, podUID) {
+ if podUID != "" && isNotPod(item.UID, podUID) {


this podUID != "" may go into isNotPod() if not it can affect windows code, in case in a future we are able to get podUID on windows instances

Fixes lint Signed-off-by: Christian Görg <christian.goerg@trumpf.com>

MarcosDY

lgtm!

…piffe#3242) - Adding cgroups regex to also match cgroup entries, that don't include a pod UI. That is explicitly currently crio, as no other container runtimes are known at the moment, that have the same behavior. The new regex therefore also explicitly checks for the "crio" string. It wasn't possible for me to combine the regex with the existing. - search through all pods and check, that the containerId found in cgroups, is is only present in one pod Signed-off-by: Christian Görg <christian.goerg@trumpf.com> Signed-off-by: Christian Görg <christian.goerg@trumpf.com>

goergch requested review from evan2645, amartinezfayo, azdagron, MarcosDY and rturner3 as code owners July 13, 2022 11:59

MarcosDY reviewed Jul 14, 2022

View reviewed changes

MarcosDY reviewed Jul 20, 2022

View reviewed changes

MarcosDY reviewed Jul 25, 2022

View reviewed changes

goergch and others added 11 commits July 26, 2022 07:20

closes spiffe#3183

65aba2c

Signed-off-by: Christian Görg <christian.goerg@trumpf.com>

Add integration test to validate CRD mode (spiffe#3219)

68007b2

* Add integration test for CRD mode Signed-off-by: Marcos Yacob <marcos.yacob@hpe.com> Signed-off-by: Christian Görg <christian.goerg@trumpf.com>

Bump version to 1.4.0 (spiffe#3234)

8b197e1

Also pulled in CHANGELOG for 1.3.2 and updated upgrade IT Signed-off-by: Andrew Harding <aharding@vmware.com> Signed-off-by: Christian Görg <christian.goerg@trumpf.com>

fixes review finding

e415459

checks cgroups against regex array Signed-off-by: Christian Görg <christian.goerg@trumpf.com>

Update pkg/agent/plugin/workloadattestor/k8s/k8s_posix.go

14eb3e8

Co-authored-by: Marcos Yacob <marcos.yacob@hpe.com> Signed-off-by: Christian Görg <christian.goerg@trumpf.com>

Update pkg/agent/plugin/workloadattestor/k8s/k8s_posix.go

37dd3bf

Co-authored-by: Marcos Yacob <marcos.yacob@hpe.com> Signed-off-by: Christian Görg <christian.goerg@trumpf.com>

Update pkg/agent/plugin/workloadattestor/k8s/k8s_posix.go

82fde37

Co-authored-by: Marcos Yacob <marcos.yacob@hpe.com> Signed-off-by: Christian Görg <christian.goerg@trumpf.com>

Update pkg/agent/plugin/workloadattestor/k8s/k8s_posix.go

4e46a86

Co-authored-by: Marcos Yacob <marcos.yacob@hpe.com> Signed-off-by: Christian Görg <christian.goerg@trumpf.com>

fixes compatibility with cri-o without kubeedge

10bb062

Signed-off-by: Christian Görg <christian.goerg@trumpf.com>

goergch force-pushed the main branch from ca9defc to 10bb062 Compare July 26, 2022 05:20

goergch added 2 commits July 26, 2022 07:43

Merge remote-tracking branch 'spiffe/spire/main' into kubeedge

d65deea

fixes after merge

d4eccac

Signed-off-by: Christian Görg <christian.goerg@trumpf.com>

MarcosDY reviewed Jul 26, 2022

View reviewed changes

puts podUID!="" into IsNotPod

2579119

Fixes lint Signed-off-by: Christian Görg <christian.goerg@trumpf.com>

goergch force-pushed the main branch from 6f6d392 to 2579119 Compare July 27, 2022 07:28

Merge branch 'main' into main

bd35cd8

MarcosDY approved these changes Jul 29, 2022

View reviewed changes

Merge branch 'main' into main

12b15a9

goergch mentioned this pull request Aug 1, 2022

Incompatibility WorkloadAttestation k8s and Kubeedge k8s #3183

Closed

Merge branch 'main' into main

74113d5

MarcosDY merged commit 1d447fb into spiffe:main Aug 1, 2022

azdagron added this to the 1.4.0 milestone Aug 1, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add crio compatibility to WorkloadAttestor k8s - fixes #3183 #3242

Add crio compatibility to WorkloadAttestor k8s - fixes #3183 #3242

goergch commented Jul 13, 2022

MarcosDY left a comment

MarcosDY Jul 14, 2022

goergch Jul 19, 2022

MarcosDY Jul 14, 2022

MarcosDY Jul 14, 2022

goergch Jul 19, 2022

goergch Jul 19, 2022

MarcosDY Jul 14, 2022

goergch Jul 19, 2022

MarcosDY left a comment

MarcosDY Jul 20, 2022

goergch Jul 25, 2022

MarcosDY Jul 20, 2022

goergch Jul 25, 2022

MarcosDY Jul 20, 2022

MarcosDY Jul 20, 2022

MarcosDY left a comment

goergch commented Jul 26, 2022

MarcosDY left a comment

MarcosDY Jul 26, 2022

MarcosDY left a comment

		@@ -598,6 +618,11 @@ func getPodUIDAndContainerIDFromCGroupPath(cgroupPath string) (types.UID, string
		matches := cgroupRE.FindStringSubmatch(cgroupPath)
		if matches != nil {

Add crio compatibility to WorkloadAttestor k8s - fixes #3183 #3242

Add crio compatibility to WorkloadAttestor k8s - fixes #3183 #3242

Conversation

goergch commented Jul 13, 2022

MarcosDY left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MarcosDY left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MarcosDY left a comment

Choose a reason for hiding this comment

goergch commented Jul 26, 2022

MarcosDY left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MarcosDY left a comment

Choose a reason for hiding this comment