-
Notifications
You must be signed in to change notification settings - Fork 623
Add an agent_id_parent selector to x509pop in spiffe mode #7002
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
Open
kfox1111
wants to merge
6
commits into
spiffe:main
Choose a base branch
from
kfox1111:x509pop-parent
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+40
−17
Open
Changes from all commits
Commits
Show all changes
6 commits
Select commit
Hold shift + click to select a range
8fb6a21
Add an agent_id_parent selector to x509pop in spiffe mode
kfox1111 d2267cf
Update docs
kfox1111 6da3970
Fix review issues
kfox1111 ac412dd
Fix review issues
kfox1111 f235994
Fix docs
kfox1111 3d5166d
Merge branch 'main' into x509pop-parent
kfox1111 File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back 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.
This is a very single-use selector, for the case that you mentioned. We can publish the SPIFFE ID as the selector. For your particular case you could make the agent SPIFFE ID template be derived from the SHA1 fingerprint, to get unique SPIFFE IDs, and have the full SPIFFE ID be
spiffe://trust domain/x509pop/identity-exchange, which you can then use as a node alias. You might have to make sure that no 2 agents get deployed to the same node so that they do not use the same exact SVID to attest themselves.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 please give an example of what the selector for the node alias would actually be in this configuration? I dont understand how it is possible with the approach your suggesting.
the docs mention selectors:
the san selector wont work as a spiffe svid will never have a url with x509pop.
the serial number/sha1 fingerprint selectors are not useful as you will never know what it is to alias with. common name wont be defined with spiffe certs.
the experimental spiffe_id selector will always be the full spiffeid that is genreated from the template. if it has the fingerprint in it, its not useable as a generic point to alias over either.
So, there is a lack of a selector that fits the use case I think.
It is ok if it is narrowly targeted a selector. it only is needed for x509 pop with spiffe mode. There are lots of selectors that only are useful for certain use cases.
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.
The SPIFFE ID of the SVID the agent presents to spire-server to attest would be:
spiffe://trust domain/spire-identity-exchange(or something like that this). The same one for all agents.spiffe://trust domain/spire/agent/x509pop/<cert fingerprint>, instead of the usual format for spiffe mode. You'd need to make sure that the agents get unique "spire-identity-exchange" SVIDs so that they get unique fingerprints.We'd still need to add the id as a selector so you can use in the node aliase, but that feels like a reasonable thing to do.
It's not that it's only needed for spiffe mode, it's only needed for this particular deployment. Most deployments wouldn't care about the "parent id".
Uh oh!
There was an error while loading. Please reload this page.
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.
Ah. I see.
I think that has two drawbacks.
I agree though, either way, we need a new selector. We're just debating the name of it I guess. So we could pick a name potentially independent of the rest.
This does raise a discussion I wanted to have at some point about dynamic entries. This idea does kind of match an idea I've been rolling around in my head. It would be useful to simplify my own entry management, as well as potentially helping dynamic entries for spire-identity-exchange too.
In most of my usage, I've run across needing to tack on the node name over and over to identifiers to guarantee uniqueness of identity.
Times the number of nodes. It gets a bit painful after a while.
But, what if we could set some labels on the node:
example.com/spire/agent/node123 (label nodeName=node123)
we could put generic workload entries on it:
{example.com/node123, example.com/node234}-> example.com/k8s-node-group -> {example.com/sshd, example.com/fluent-bit, example.com/node-agent, example.com/kubelet}
Each one would use those spiffeid's for lookups and resolving workloads. but, during issuence of the svid, it would have an extra template that would resolve before issuing the final spiffeid. so it could say, append "/{{ nodeLabels.nodeName }}"
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.
It's not clear to me if the solution above is ok for you, which would mean that we'd have just an
agent_spiffe_idselector, based on the full SPIFFE ID of the presented SVID.We can continue the other discussion independently of this.
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 your asking if the proposed solution of just making the underlying spiffe id be the common group part, and then making them unique by adding the SHA1 fingerprint automatically to it for this special case, then no, I dont think that is a good way to solve it. It makes it too hard to:
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 I understand correctly, your proposed solution here also has a random, changing, identifier in the agent identity, the pod UUID. So that's something you have to deal with anyway. I also don't think it's that big of a deal since you're planning on using the node alias.
I'm not sure it's worth adding this selector which is very specific to your deployment and is likely hard to reason about in a large deployment. The full spiffe id would be ok to have.
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.
You keep falling back to very specific cases, designing a feature that is so specific, when the next case comes out, we're back to square one. IMO we should design features that are useful for more then just one use case so we're not constantly adding features.
The way I'm using it in this particular case, yes a pod uid is somewhat unknowable up front. But, it will never change during the life of the pod. So adding it later is stable. Reboot the node unexpectedly, the pod uid stays the same. The spire-agent gets reattested, it still gets the same spiffeid while the fingerprint changes. If you really want to put a stable identity on it so you can hang off per agent entries, you use the {.Metadata.Name}} from the pod and a statefulset to give it a persistent name. Persistent identity. done. I'm not 100% sure I will never need to do that. So, why preoptimize the ability to do that completely away? Besides, we still havent addressed how your proposal will work when the fingerprint changes. That is deal breaking on its own IMO. So, I'm still at the point of, that isnt a good solution.
I"m not asking for a ton of code here. We're talking for like 12 lines of code and a unit test. This really shoudlnt be burdensome on the spire team. That saves 57 lines of code https://github.com/spiffe/spire-identity-exchange/blob/main/cmd/spire-credentialcomposer-identity-exchange/main.go
and a whole separate plugin and infrastructure, and is much easier for the end user to deal with. This is a fair optimization I think. Over all the spire projects, spire is maintains less code.