Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

update crio regex for intra-pod workload attestation #4946

Closed
wants to merge 3 commits into from

Conversation

critterjohnson
Copy link

Pull Request check list

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

Affected functionality

This PR updates the cgroup regex for workloads in the same pod as the agent. It affects the k8s WorkloadAttestor.

Description of change

This PR updates the cgroup regex for workloads in the same pod as the agent. Without this, any containers in the the agent's pod cannot verify with the agent.

Which issue this PR fixes

fixes #4917

Signed-off-by: Christopher Johnson <critterjohnson45@gmail.com>
Signed-off-by: Christopher Johnson <critterjohnson45@gmail.com>
@evan2645
Copy link
Member

Thank you very much for sending this @critterjohnson - we assigned @azdagron who is doing some related work right now. He should touch back here in the next week or two with an update. Thanks for your patience

@critterjohnson
Copy link
Author

@azdagron any updates here? I saw y'all moved the milestone for the cgroups issue to 1.9.3. Of course this is part of that larger issue, but this is a good band-aid for this use case.

Copy link
Member

@azdagron azdagron left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, @critterjohnson. This looks good and I'm glad it solves your edge case.

The work to more fully support cgroups v2 will almost completely replace our algorithm used to determine the container ID. When I have a PR ready, I'd love for you to give it a go in your environment.

@azdagron
Copy link
Member

azdagron commented Apr 3, 2024

Hmm, looks like this breaks existing unit tests.

2024/04/03 15:20:10 More than one regex matches for cgroup /kubepods-besteffort-pod72f7f152_440c_66ac_9084_e0fc1d8a910c.slice:cri-containerd:b2a102854b4969b2ce98dc329c86b4fb2b06e4ad2cc8da9d8a7578c9cd2004a2

There is a check that a given cgroup only matches one regex. Matching more than one regex isn't actually problematic unless the match would yield a different set of results, but the code isn't that smart (yet).

Since both this PR and the cgroups v2 support work that is under way are both slated for 1.9.3, one option is to just wait to see if that cgroups v2 work covers this case sufficiently. Otherwise, we'll need to change the code to relax a little like i described above.

@critterjohnson
Copy link
Author

Or we could tighten the regex a little. I'll take a look this weekend; I had this passing unit tests locally originally but they're failing now.

@azdagron
Copy link
Member

Thanks again for putting this fix together @critterjohnson. I was finally able to get #5076 opened to more robustly handle cgroups v2 support. This fix should no longer be necessary. So I'll go ahead and close out the PR.

Do you think you'd have time to test #5076 against your crio setup? You'd need to check it out and build an agent image. Then, to enable it, you have to turn on the new support in the agent configuration for the "k8s" WorkloadAttestor plugin.

WorkloadAttestor "k8s" {
     .... other config values ....
     use_new_container_locator = true
}

If enabled, you should see a line like the following on startup:

INFO[0000] Plugin loaded                                 external=false plugin_name=unix plugin_type=WorkloadAttestor subsystem_name=catalog
INFO[0000] Using the new container locator               external=false plugin_name=k8s plugin_type=WorkloadAttestor subsystem_name=catalog

@azdagron azdagron closed this Apr 24, 2024
@critterjohnson
Copy link
Author

@azdagron Would love to try this on our env - I'll do it next week. Thanks!

@azdagron
Copy link
Member

@critterjohnson awesome! For what it's worth, the nightly image has been built with the change, so it should be a little easier to test with.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

SPIRE agent cannot verify identity within own pod
4 participants