patch alt cgroups regex #1522

vbotez · 2020-04-28T13:25:58Z

Pull Request check list

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

Affected functionality
spire-agent worload attestor docker plugin does not match id from cgroups matching pattern docker-<id>.scope

Description of change
added two new constants: altContainerIDToken and regexpAltContainerID plus a new case statement:

		case altContainerIDToken:
			idTokenCount++
			elems[i] = regexpAltContainerID

This will work on matching patterns of type /system.slice/docker-<id>.scope.

As sugested in the issue comment - it will need the following configuration in spire-agent:

WorkloadAttestor "docker" {
    plugin_data = {
        container_id_cgroup_matchers = ["/system.slice/docker-<id>.scope"]
    }
}

Which issue this PR fixes
Issue #1518

Tested on RHEL 7.7

uname -a 
3.10.0-1062.12.1.el7.x86_64

amartinezfayo

Thank you for this @vbotez!
I think that it would be good to update dockerfinder_test.go to capture this case.
It should be easy to add a matcher for this and an expected match in the test cases.

amartinezfayo · 2020-04-28T14:19:36Z

pkg/agent/plugin/workloadattestor/docker/cgroup/dockerfinder.go

@@ -101,7 +108,7 @@ func (f *containerIDFinder) FindContainerID(cgroup string) (string, bool) {
 	if len(matches) == 0 {
 		return "", false
 	}
-	return matches[submatchIndex], true
+	return string(matches[submatchIndex]), true


I don't think that converting to string is needed.

Indeed the conversion is not needed. I was working on code from 0.9.0 tag that has this conversion. I just noticed that this was removed in master branch (I guess also removed from 0.10.0 release). I will fix this - maybe a rebase on master branch should do the trick

pkg/agent/plugin/workloadattestor/docker/cgroup/dockerfinder.go

azdagron

This solution is very specifically allowing an exception to the matcher pattern rules to allow a pattern specific for FHEL. When the matcher functionality was introduced, we wanted to be strict about what patterns we allowed to make this simple for the operator. However, perhaps it is time to make the pattern slightly more flexible.

For id matching we could allow an optional prefix or suffix to the <id> token. The code could search for <id> in the segment, and if found, replace it with regexp.QuoteMeta(prefix) + regexpContainerId + regexp.QuoteMeta(suffix). There are some corner cases, like when more than one <id> token is found in a segment (or across multiple segments), but assuming we handle those, i think this becomes slightly more flexible than the current pattern syntax but enough to cover RHEL and maybe others.

We could keep the whole segment match for * as is.

Thoughts?

azdagron · 2020-05-06T17:37:15Z

Hi @vbotez! Just following up to see if you had time/interest in making the suggested changes or if we should pick up the work and get this over the finish line? Either way is fine, of course!

vbotez · 2020-05-08T05:18:29Z

Hello @azdagron! I do intend to make the suggested changes and pass this PR. It’s just that I’m a bit caught up in some other topics as well now and that’s why you did not see to much activity on this.

azdagron · 2020-05-08T15:49:53Z

Thanks @vbotez! I was going over my proposed solution again in my head and realized that it would complicate the "ambiguous pattern" configuration validation step we do. I haven't thought through all of the implications yet.

I'm curious what others think about simplifying our approach to a heuristics based one that just looks for cgroups with "docker" in the name and have a single 64-hex character ID (docker container IDs are 256-bit). The pro is that we can get rid of this fragile pattern approach and immediately accommodate differing cgroup name variants without requiring operator intervention. The downside is the risk that we identify something as a container ID that isn't one. I'm not sure what an attack vector would look like for that though since it would imply injecting the malicious container ID into the cgroups of the valid container. We could also fail the heuristic if we obtain more than one unique ID across the set of cgroup names.

So the algorithm I'm suggesting is:

Loop over cgroup names
Filter out those that don't container the whole word "docker"
Scan those that remain for unique 64-hex character identifiers
Reject the whole set if there is more than one.

Thoughts anybody?

vbotez · 2020-05-11T14:52:53Z

I’m just wondering if there is so much cgroup naming diversity out there to account for this.

APTy · 2020-05-12T21:49:06Z

So the algorithm I'm suggesting is:

Loop over cgroup names

Filter out those that don't container the whole word "docker"

Scan those that remain for unique 64-hex character identifiers

Reject the whole set if there is more than one.

Thoughts anybody?

I like this 👍

amartinezfayo · 2020-05-12T22:34:50Z

So the algorithm I'm suggesting is:

Loop over cgroup names

Filter out those that don't container the whole word "docker"

Scan those that remain for unique 64-hex character identifiers

Reject the whole set if there is more than one.

I think that should work well to solve this. I've been unable to come up with a realistic attack injecting a malicious container ID into the cgroups of the valid container.

vbotez · 2020-05-17T06:03:13Z

Marknig this PR as 'draft' for now until I test the necessary changes proposed by @azdagron

azdagron · 2020-05-22T16:54:13Z

I pushed a commit to vbotez:issue-1518-patch-alt-cgroups-for-docker-agent-plugin but it isn't showing up on this PR for some reason...

azdagron · 2020-05-22T16:56:12Z

Tried toggling "draft" status to see if that would help but it didn't.

Signed-off-by: Vlad <vlad.botez@orange.com>

Co-Authored-By: Agustín Martínez Fayó <amartinezfayo@users.noreply.github.com>

- Uses a simple regex to extract the container ID - Replaces the default finder with the above regex - Still supports the legacy and pattern-based finders Signed-off-by: Andrew Harding <andrew.harding@hpe.com>

azdagron · 2020-06-01T20:21:52Z

Closing this in favor of #1605. Thank you @vbotez for opening this, preparing the initial targetted fix, and starting this discussion.

vbotez requested review from amartinezfayo, APTy, azdagron, evan2645 and marcosy as code owners April 28, 2020 13:25

vbotez force-pushed the issue-1518-patch-alt-cgroups-for-docker-agent-plugin branch 2 times, most recently from 703e462 to 7a31f3b Compare April 28, 2020 14:02

amartinezfayo reviewed Apr 28, 2020

View reviewed changes

azdagron reviewed Apr 28, 2020

View reviewed changes

vbotez marked this pull request as draft May 17, 2020 06:02

azdagron marked this pull request as ready for review May 22, 2020 16:54

azdagron requested a review from mcpherrinm as a code owner May 22, 2020 16:54

azdagron marked this pull request as draft May 22, 2020 16:55

Vlad and others added 3 commits May 22, 2020 12:17

patch alt cgroups regex

f4bedc4

Signed-off-by: Vlad <vlad.botez@orange.com>

Update pkg/agent/plugin/workloadattestor/docker/cgroup/dockerfinder.go

d56b1ce

Co-Authored-By: Agustín Martínez Fayó <amartinezfayo@users.noreply.github.com>

Robust docker cgroups container ID parsing

71eb755

- Uses a simple regex to extract the container ID - Replaces the default finder with the above regex - Still supports the legacy and pattern-based finders Signed-off-by: Andrew Harding <andrew.harding@hpe.com>

azdagron force-pushed the issue-1518-patch-alt-cgroups-for-docker-agent-plugin branch from 91d9bae to 71eb755 Compare May 22, 2020 18:18

azdagron mentioned this pull request Jun 1, 2020

Robust cgroup path parsing of docker container ID #1605

Merged

azdagron closed this Jun 1, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

patch alt cgroups regex #1522

patch alt cgroups regex #1522

vbotez commented Apr 28, 2020 •

edited

Loading

amartinezfayo left a comment

amartinezfayo Apr 28, 2020

vbotez Apr 28, 2020

azdagron left a comment

azdagron commented May 6, 2020

vbotez commented May 8, 2020

azdagron commented May 8, 2020

vbotez commented May 11, 2020

APTy commented May 12, 2020

amartinezfayo commented May 12, 2020

vbotez commented May 17, 2020

azdagron commented May 22, 2020

azdagron commented May 22, 2020

azdagron commented Jun 1, 2020

patch alt cgroups regex #1522

patch alt cgroups regex #1522

Conversation

vbotez commented Apr 28, 2020 • edited Loading

amartinezfayo left a comment

Choose a reason for hiding this comment

amartinezfayo Apr 28, 2020

Choose a reason for hiding this comment

vbotez Apr 28, 2020

Choose a reason for hiding this comment

azdagron left a comment

Choose a reason for hiding this comment

azdagron commented May 6, 2020

vbotez commented May 8, 2020

azdagron commented May 8, 2020

vbotez commented May 11, 2020

APTy commented May 12, 2020

amartinezfayo commented May 12, 2020

vbotez commented May 17, 2020

azdagron commented May 22, 2020

azdagron commented May 22, 2020

azdagron commented Jun 1, 2020

vbotez commented Apr 28, 2020 •

edited

Loading