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

x509pop server plugin support for servers trust bundle #5572

Open
wants to merge 40 commits into
base: main
Choose a base branch
from

Conversation

kfox1111
Copy link
Contributor

Enables the x509pop node attestor server plugin to be configured to use the SPIRE Servers own trust bundle.

Enables the x509pop node attestor server plugin to be
configured to use the SPIRE Servers own trust bundle.

Signed-off-by: Kevin Fox <Kevin.Fox@pnnl.gov>
Signed-off-by: Kevin Fox <Kevin.Fox@pnnl.gov>
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.

Using the bundle, without any sort of restriction, seems scary. Doesn't this imply that any workload can just turn around and attest as a node?

pkg/server/plugin/nodeattestor/x509pop/x509pop.go Outdated Show resolved Hide resolved
}
go func() {
ctx := context.Background()
sleep := 1 * time.Second
Copy link
Member

Choose a reason for hiding this comment

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

Rather than kick off a goroutine that may or may not be run after the IdentityProvider host service is ready, it would be better to lazily initialize this in the first call to Attest.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@kfox1111
Copy link
Contributor Author

Using the bundle, without any sort of restriction, seems scary. Doesn't this imply that any workload can just turn around and attest as a node?

Yeah, that I think can be handled like:

    NodeAttestor "x509pop" {
      plugin_data {
        spire_trust_bundle = true
        # Only allow spiffe identifiers of the form spiffe://<trustDomain>/k8s-spire-agent-helper/<hostname>
        # Generate identifiers that look like spiffe://<trustDomain>/x509pop/k8s-spire-agent/<hostname>
        agent_path_template = "{{ ${d}p := printf \"spiffe://%s/k8s-spire-agent-helper/\" .TrustBundle }}{{ ${d}s := printf \"%s\" (index .Certificate.URIs 0) }}{{ if gt (len ${d}s) (len ${d}p) }}{{ ${d}ps := slice ${d}s 0 (len ${d}p)
 }}{{ if eq ${d}ps ${d}p }}{{ printf \"/x509pop/k8s-spire-agent/%s\" (slice ${d}s (len ${d}p)) }}{{ end }}{{ end }}"
      }
    }

if it agent_path renders out to "", then it should be disallowed.

I did hit:

Trying to get it to work though.

@azdagron azdagron self-assigned this Oct 15, 2024
@azdagron
Copy link
Member

Hey @kfox1111, do you have an arch diagram or something that spells out the use case clearly that you can share (either here or privately in slack with the maintainer group)? Before we consider taking this we'd like to see if there are alternatives.

@kfox1111
Copy link
Contributor Author

kfox1111 commented Oct 18, 2024

Here's the general idea. Got the host -> k8s part working. Still working on the k8s -> vm part.

With this configuration, the spire-agent on the host managed by systemd is the first to start, and establishes the trust chain used by all other parts of the node. The trust never leaves the node. The SPIRE server then really is the bottom turtle in this setup. 🐢

x509pop

@kfox1111
Copy link
Contributor Author

Related to: #5206

Signed-off-by: Kevin Fox <Kevin.Fox@pnnl.gov>
Signed-off-by: Kevin Fox <Kevin.Fox@pnnl.gov>
Signed-off-by: Kevin Fox <Kevin.Fox@pnnl.gov>
@kfox1111
Copy link
Contributor Author

Used with the sprig pr, this works:

    agent_path_template = "{{ $$p := printf \"spiffe://%s/k8s-spire-agent-helper/\" .TrustDomain }}{{ $$s := printf \"%s\" (index .Certificate.URIs 0) }}{{ if hasPrefix $$p $$s }}{{ printf \"/x509pop/k8s-spire-agent/%s\" (trimPrefix $$p $$s) }}{{ else }}{{ fail \"Invalid prefix\" }}{{ end }}"

@kfox1111
Copy link
Contributor Author

kfox1111 commented Oct 19, 2024

Updated diagram

@sorindumitru
Copy link
Contributor

Here's the general idea. Got the host -> k8s part working. Still working on the k8s -> vm part.

With this configuration, the spire-agent on the host managed by systemd is the first to start, and establishes the trust chain used by all other parts of the node. The trust never leaves the node. The SPIRE server then really is the bottom turtle in this setup. 🐢

x509pop

I've had some thoughts about a similar (or maybe the same, I have yet to think it through fully) architecture, chaining trust all the way to the hardware (which would likely be tpms too). Just a +1 that this kind of thing is something that other users might want to do.

My thinking here is that for something like this we might want a separate node attestor (maybe x509svidpop?) because the differences between x509 and x509-SVID matter here. For example you might want to limit which SPIFFE-IDs you allow in some way (e.g. a list of SPIFFE-IDs or at least some kind of validation of the SPIFFE-ID format) as well as extracting some information for the SPIFFE ID to make available to the template.

Another thing that would be useful to me would be to allow having different trust domains, e.g. the physical nodes using TPMs could be a different trust domain than the ones on k8s. In large corporations it's possible that different groups manage those different infrastructures so they may be different trust domains.

@kfox1111
Copy link
Contributor Author

@sorindumitru I think we're pretty much on the same page.

We could make a new x509svidpop plugin but it would end up being almost the same code as the x509pop one. (The patch here is pretty small). So a lot more stuff to maintain.

the svid use case really isn't that different from the normal x509pop one. Either way you probably want some way to filter out which certs are allowed to a subset of all possible ones. That can be done via agent_path_template as done in this pr without any special code in the plugin.

100% agree on the future ability to use some other spire trustDomain too. Was going to propose that in a different patch. The primary use case for me being:

k8s multitenant cluster. Secured with spire. Call this, the resource provider cluster.

Tenant uses resource provider cluster with kubevirt to launch a bunch of vm's for their tenant. Inside, they launch their own spire server, and workloads (maybe even their own k8s cluster in the vms). It would greatly simplify management if they could use x509 certs from the resource provider cluster to node attest to their own spire-server.

That again, would basically be this same x509pop plugin as described here, but getting its trustBundle from disk and keep it refreshed (existing functionality extended to refresh from disk), or get it over vsock (not sure which is best yet)

@sorindumitru
Copy link
Contributor

I agree it's going to be very similar to x509pop, but dealing with that is an implementation detail, I'm sure some parts can be shared. I just don't think that the template language is a good way of dealing with those differences.

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 for this, @kfox1111. The new design eliminates most of my concerns.

I'd like to explore names other than spire_trust_bundle and the default /spire-exchange prefix value.... I think we can find something a little clearer....

Also, there are no tests for the new functionality. We'll need those before we can merge.

doc/plugin_server_nodeattestor_x509pop.md Outdated Show resolved Hide resolved
doc/plugin_server_nodeattestor_x509pop.md Outdated Show resolved Hide resolved
doc/plugin_server_nodeattestor_x509pop.md Outdated Show resolved Hide resolved
@@ -57,3 +77,4 @@ Some useful values are:
| .TrustDomain | The configured trust domain |
| .Subject.CommonName | The common name field of the agent's x509 certificate |
| .SerialNumberHex | The serial number field of the agent's x509 certificate represented as lowercase hexadecimal |
| .SVIDPath | The SVID Path after removing the SVID Prefix |
Copy link
Member

Choose a reason for hiding this comment

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

Maybe SVIDPathSuffix better implies that a prefix has been stripped?

pkg/server/plugin/nodeattestor/x509pop/x509pop.go Outdated Show resolved Hide resolved
pkg/server/plugin/nodeattestor/x509pop/x509pop.go Outdated Show resolved Hide resolved
pkg/server/plugin/nodeattestor/x509pop/x509pop.go Outdated Show resolved Hide resolved
pkg/server/plugin/nodeattestor/x509pop/x509pop.go Outdated Show resolved Hide resolved
pkg/server/plugin/nodeattestor/x509pop/x509pop.go Outdated Show resolved Hide resolved
pkg/server/plugin/nodeattestor/x509pop/x509pop.go Outdated Show resolved Hide resolved
Co-authored-by: Andrew Harding <azdagron@gmail.com>
Signed-off-by: kfox1111 <Kevin.Fox@pnnl.gov>
Signed-off-by: kfox1111 <Kevin.Fox@pnnl.gov>
Signed-off-by: Kevin Fox <Kevin.Fox@pnnl.gov>
Signed-off-by: Kevin Fox <Kevin.Fox@pnnl.gov>
Signed-off-by: Kevin Fox <Kevin.Fox@pnnl.gov>
Signed-off-by: Kevin Fox <Kevin.Fox@pnnl.gov>
Signed-off-by: Kevin Fox <Kevin.Fox@pnnl.gov>
Signed-off-by: Kevin Fox <Kevin.Fox@pnnl.gov>
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.

3 participants