agent/workloadattestor: Add docker plugin #687

APTy · 2019-01-23T00:17:07Z

Pull Request check list

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

Affected functionality
Adds a docker-based workload attestor that enables selectors based on docker labels and the container's image id.

TODO

add docs for image id selector
add any other selectors wanted, e.g. image sha

evan2645

This is awesome!! Thank you greatly for the contrib @APTy!

Left a few small comments and nits. Curious to see if @azdagron has anything else to say... looks good to me otherwise!

evan2645 · 2019-01-23T01:21:50Z

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

+		parts := strings.Split(cgroup.GroupPath, "/")
+
+		if len(parts) <= p.cgroupContainerIndex+1 {
+			log.Printf("docker entry found, but without container id: %v", cgroup.GroupPath)


nit: Log messages should be capitalized. Also, might be a bit clearer s/without container id/is missing the container id/

evan2645 · 2019-01-23T01:26:17Z

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

+	}
+
+	if !hasDockerEntries {
+		return nil, fmt.Errorf("workloadattestor/docker: no cgroup entries found with prefix %q", p.cgroupPrefix)


We used to have logging like this in the k8s attestor, but ended up removing it because it would cause the agent to log errors when trying to attest non-k8s workloads. Generally speaking, I think we should support the mix-and-match case well and expect that maybe some dockerized workloads will call us, and some others will call us too... in that vein, perhaps we should not log or return an error here? Curious to hear thoughts on this point

Ah okay, I noticed that but wasn't sure of the reason. Agree with this reasoning and updated to reflect

evan2645 · 2019-01-23T01:28:29Z

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

+		return &spi.ConfigureResponse{ErrorList: []string{err.Error()}}, err
+	}
+	p.cgroupPrefix = config.CgroupPrefix
+	p.cgroupContainerIndex = config.CgroupContainerIndex


Do we want to set the defaults on these here?

evan2645 · 2019-01-23T01:33:06Z

doc/plugin_agent_workloadattestor_docker.md

+| docker_socket_path | The location of the docker daemon socket (default: "unix:///var/run/docker.sock" on unix). |
+| docker_version | The API version of the docker daemon (default: "1.25").
+| cgroup_prefix | The cgroup prefix to look for in the cgroup entries (e.g. "/docker"). |
+| cgroup_container_index | The index within the cgroup path where the container ID should be found (default: 0). |


IMO it is desirable to limit the number of knobs available to users wherever possible... otherwise, they tend to tweak things inappropriately and end up with either misconfigured or broken software, sometimes without even knowing. In that vein, I'd vote to leave cgroup_prefix and cgroup_container_index undocumented... I don't think they are very likely to change... If someone comes to us with a broken attestor we can ask them to tweak these undocumented values, or if an operator finds them in the code, then the assumption is that they know what they are doing

If we remove these configurables from the documentation, then we can also remove the Cgroup entry section below

evan2645 · 2019-01-23T01:33:57Z

doc/plugin_agent_workloadattestor_docker.md

+| cgroup_container_index | The index within the cgroup path where the container ID should be found (default: 0). |
+
+Since selectors are created dynamically based on the container's docker labels, there isn't a list of known selectors.
+Instead, all of the container's labels are used in creating the list of selectors.


I don't think this applies any longer?

evan2645 · 2019-01-23T01:37:28Z

doc/plugin_agent_workloadattestor_docker.md

+| Selector | Value |
+| -------- | ----- |
+| docker:label:abc | The value of the docker label `abc` |
+| docker:label:xyz | The value of the docker label `xyz` |


nit: I know that this is taken from the k8s workload attestor docs, but I think separating by selector and value is confusing to users. Even though the difference between type and value is distinguished internally, we don't really surface that very strongly in the CLI tools. I'd suggest a table like this one instead: https://github.com/spiffe/spire/blob/master/doc/plugin_server_nodeattestor_k8s_sat.md

evan2645 · 2019-01-23T01:38:10Z

doc/plugin_agent_workloadattestor_docker.md

+If a workload container is started with `docker run --label com.example.name=foo [...]`, then its workload selector will be:
+```
+type: "docker:label"
+value: "com.example.name=foo"


= or :?

Similar nit here on selector type/value

evan2645 · 2019-01-23T01:39:35Z

conf/agent/agent.conf

+        plugin_data {
+            cgroup_container_index = 3
+            cgroup_prefix = ""
+            labels = [""]


I think we can remove labels and possibly the other two as well if we decide to leave them undocumented

azdagron

Looking great :) Just a couple of small nits...

azdagron · 2019-01-23T01:57:10Z

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

+)
+
+const (
+	_selectorType       = "docker"


nit: package consts should be camelCase w/o the leading underscore.

azdagron · 2019-01-23T02:00:35Z

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

+
+	// Parse HCL config payload into config struct
+	config := &dockerPluginConfig{}
+	hclTree, err := hcl.Parse(req.Configuration)


nit: hcl can be parsed and decoded in a single step via hcl.Decode(config, req.Configuration)

azdagron · 2019-01-23T02:02:27Z

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

+	config := &dockerPluginConfig{}
+	hclTree, err := hcl.Parse(req.Configuration)
+	if err != nil {
+		return &spi.ConfigureResponse{ErrorList: []string{err.Error()}}, err


i know this was likely a pattern followed from other plugins, but we should just return nil, err here. The ErrorList stuff isn't used and gRPC ignores the returned response anyway if an error is returned. I've been meaning to clean up the plugins.... :)

Signed-off-by: R. Tyler Julian <tjulian@uber.com>

APTy · 2019-01-23T15:51:15Z

Okay I've made the recommended changes. Sorry I had to force push to rebase onto master and signoff correctly, so some of the history is lost. Anyway, one more thing I wanted to discuss:

Regarding the docker dependency, the library has a weird versioning/support story. I tried including the latest version, v17.05.0-ce, which results in:

go test -race -timeout 8m github.com/spiffe/spire/...
go: finding github.com/docker/docker v17.05.0-ce
go: finding github.com/docker/libtrust latest
go: finding github.com/docker/distribution/reference latest
go: finding github.com/docker/go-connections/nat latest
go: finding github.com/docker/go-connections/tlsconfig latest
go: finding github.com/docker/go-connections/sockets latest
go: finding github.com/docker/distribution v2.7.1+incompatible
go: downloading github.com/docker/distribution v2.7.1+incompatible
go: github.com/Sirupsen/logrus@v1.3.0: parsing go.mod: unexpected module path "github.com/sirupsen/logrus"
go: error loading module requirements
make: *** [test] Error 1

I've run into logrus import errors too many times to care to debug this lol. The current state of this PR pins to master, which is less than ideal IMO. My preference here is to just write our own docker client, but we could also vendor the library. Thoughts?

Signed-off-by: R. Tyler Julian <tjulian@uber.com>

azdagron · 2019-01-23T16:38:38Z

Hmm. I pulled down your branch and ran go mod tidy and it complained about the checksum in go.sum for docker (travis had the same problem). I removed the docker entries from go.sum and ran go mod tidy and then was able to build and run tests....

APTy · 2019-01-23T17:30:55Z

Ah okay yeah I'm new to go mod stuff, should be able to fix that. But I think we should first answer the question of "do we (1) pin to master, (2) vendor master, (3) pin to the last stable release 1.5 years ago and try to debug the logrus nightmare, or (4) build our own docker client with limited functionality"

APTy · 2019-01-23T18:06:56Z

Here's an example of what (4) would look like: APTy/spire@tj/docker-attestor...APTy:tj/docker-attestor-2. It's probably missing some things like client version negotiation, but it's not too complex to create the core functionality ourselves.

Signed-off-by: R. Tyler Julian <tjulian@uber.com>

azdagron · 2019-01-23T21:31:00Z

My opinion is that since github.com/docker/docker/client is from the official docker repo, we should probably just pin to latest master (assuming that it works). I think enough has changed in 1.5 years that using the old release will be problematic from a dependency standpoint (as you have discovered). I'd be totally for vendoring or writing our own if this was some problematic one-off library, but since it is the official repo, I'm pretty comfortable just using it. Dunno if @evan2645 has other thoughts...

Signed-off-by: R. Tyler Julian <tjulian@uber.com>

azdagron

Awesome. Thanks @APTy!

APTy requested review from ajessup, amartinezfayo, azdagron, drrt, evan2645, MarcosDY, martincapello and walmav as code owners January 23, 2019 00:17

APTy force-pushed the tj/docker-attestor branch 2 times, most recently from 3c9bc8f to ea48a32 Compare January 23, 2019 00:18

evan2645 reviewed Jan 23, 2019

View reviewed changes

azdagron reviewed Jan 23, 2019

View reviewed changes

agent/workloadattestor: Add docker attestor plugin

3fca261

Signed-off-by: R. Tyler Julian <tjulian@uber.com>

APTy force-pushed the tj/docker-attestor branch from ea48a32 to 3fca261 Compare January 23, 2019 15:39

Remove unnecessary config and fix go.sum

e2393ac

Signed-off-by: R. Tyler Julian <tjulian@uber.com>

go mod tidy

bda474f

Signed-off-by: R. Tyler Julian <tjulian@uber.com>

go mod tidy

bd603e7

Signed-off-by: R. Tyler Julian <tjulian@uber.com>

azdagron approved these changes Jan 24, 2019

View reviewed changes

Merge branch 'master' into tj/docker-attestor

718fed1

azdagron merged commit fcce613 into spiffe:master Jan 24, 2019

evan2645 mentioned this pull request Feb 4, 2019

Prepare for 0.7.3 #703

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

agent/workloadattestor: Add docker plugin #687

agent/workloadattestor: Add docker plugin #687

APTy commented Jan 23, 2019 •

edited

Loading

evan2645 left a comment

evan2645 Jan 23, 2019

evan2645 Jan 23, 2019

APTy Jan 23, 2019

evan2645 Jan 23, 2019

evan2645 Jan 23, 2019

evan2645 Jan 23, 2019

evan2645 Jan 23, 2019

evan2645 Jan 23, 2019

evan2645 Jan 23, 2019

azdagron left a comment

azdagron Jan 23, 2019

azdagron Jan 23, 2019

azdagron Jan 23, 2019

APTy commented Jan 23, 2019

azdagron commented Jan 23, 2019

APTy commented Jan 23, 2019

APTy commented Jan 23, 2019

azdagron commented Jan 23, 2019

azdagron left a comment

agent/workloadattestor: Add docker plugin #687

agent/workloadattestor: Add docker plugin #687

Conversation

APTy commented Jan 23, 2019 • edited Loading

evan2645 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

Choose a reason for hiding this comment

azdagron 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

APTy commented Jan 23, 2019

azdagron commented Jan 23, 2019

APTy commented Jan 23, 2019

APTy commented Jan 23, 2019

azdagron commented Jan 23, 2019

azdagron left a comment

Choose a reason for hiding this comment

APTy commented Jan 23, 2019 •

edited

Loading