-
Notifications
You must be signed in to change notification settings - Fork 484
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
Conversation
Enables the x509pop node attestor server plugin to be configured to use the SPIRE Servers own trust bundle. Signed-off-by: Kevin Fox <[email protected]>
Signed-off-by: Kevin Fox <[email protected]>
There was a problem hiding this 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?
} | ||
go func() { | ||
ctx := context.Background() | ||
sleep := 1 * time.Second |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
Yeah, that I think can be handled like:
if it agent_path renders out to "", then it should be disallowed. I did hit:
Trying to get it to work though. |
Signed-off-by: Kevin Fox <[email protected]>
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. |
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. 🐢 |
Related to: #5206 |
Signed-off-by: Kevin Fox <[email protected]>
… into x509pop-trustbundle
Signed-off-by: Kevin Fox <[email protected]>
Signed-off-by: Kevin Fox <[email protected]>
Used with the sprig pr, this works:
|
Updated diagram |
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 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. |
@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) |
I agree it's going to be very similar to |
Signed-off-by: Kevin Fox <[email protected]>
Signed-off-by: Kevin Fox <[email protected]>
Signed-off-by: Kevin Fox <[email protected]>
if hclConfig.CABundlePath != "" && len(hclConfig.CABundlePaths) > 0 { | ||
status.ReportError("only one of ca_bundle_path or ca_bundle_paths can be configured, not both") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is not the same error happens in line 80?
may we return here instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm.... The check here checks if both are configured. The check at 80 ensures at least one is configured. Both are needed I think.
I was thinking status.ReportError was fatal. Is it not? Maybe all the ReportError's here then need retruns?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
mmm, it is like if both are configured, you must never get the second one right?
status.Report is not a fatal, but just adding a list of errors, than then are used in configure or verify,
but this approach has a good win than you can get the complete list of errors to resolve all together,
but it makes very hard to follow and see dependency errors...
In any case we are fine for now
if len(trustBundles) > 0 { | ||
return util.NewCertPool(trustBundles...), nil | ||
} | ||
return nil, nil |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if this happens we'll reject attestations, but it is going to be hard to understand why it is happening (basically we are not going to have a trusted bundle)
May we at least return wartning or error?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we log a warning about not currently finding a trust bundle? Its nor fatal as it might show up at some point later?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeap, I agree but this attestation will fail and mention a verification error
so a warning may help users to understand than that the that attestation failure is cuased by the attestor being unabled to get the trust bundle.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added a warning.
@@ -158,7 +160,9 @@ func (s *Suite) TestAttestFailure() { | |||
|
|||
s.T().Run("not configured", func(t *testing.T) { | |||
attestor := new(nodeattestor.V1) | |||
plugintest.Load(t, BuiltIn(), attestor) | |||
plugintest.Load(t, BuiltIn(), attestor, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you add test case where no trustedbundle is returned?
and how about defaults?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Struggling a bit with how to write the tests....
Seems like I need to create a new cert in ./test/fixture/nodeattestor/x509pop/generate.go with the url based spiffe id to test with? There are some magic numbers in that file. Are they just arbitrary, or are there some reason for the specific numbers chosen?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reading through it some more, it looks like they just have patterns in it. so rather arbitrary.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok. I have tests for no trust bundle, along with a valid svid, an invalid svid, and a no svid.
Co-authored-by: Marcos Yacob <[email protected]> Signed-off-by: kfox1111 <[email protected]>
Signed-off-by: Kevin Fox <[email protected]>
Signed-off-by: Kevin Fox <[email protected]>
… into x509pop-trustbundle
Signed-off-by: Kevin Fox <[email protected]>
Signed-off-by: Kevin Fox <[email protected]>
… into x509pop-trustbundle
Signed-off-by: Kevin Fox <[email protected]>
Signed-off-by: Kevin Fox <[email protected]>
Signed-off-by: Kevin Fox <[email protected]>
Signed-off-by: Kevin Fox <[email protected]>
Enables the x509pop node attestor server plugin to be configured to use the SPIRE Servers own trust bundle.