-
Notifications
You must be signed in to change notification settings - Fork 33
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
Allow DelegatedIdentity API clients to subscribe by PID #58
Allow DelegatedIdentity API clients to subscribe by PID #58
Conversation
Signed-off-by: Benjamin Leggett <[email protected]>
Signed-off-by: Benjamin Leggett <[email protected]>
Signed-off-by: Benjamin Leggett <[email protected]>
Signed-off-by: Benjamin Leggett <[email protected]>
dec110c
to
191064c
Compare
Thanks for this @bleggett. We discussed this a bit and I think at this time the preference is to:
|
Okay, I'm fine with that. Will redo it along those lines. Thanks for getting back! |
@azdagron There's one gotcha with this - you cannot use This could be fixed by creating a new message for the The problem with this is that it will break back compat for people using this API that already use the selectors as-is. I don't mind that much myself, but I was originally trying to avoid altering the existing API in any way. Thoughts? |
Oh that's right. We need to preserve backcompat. I think the best option is to not leverage oneof at this point and just check for mutual exclusivity in the handler implementation. |
Signed-off-by: Benjamin Leggett <[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.
Looks good overall. Just a few small nitpicks.
proto/spire/api/agent/delegatedidentity/v1/delegatedidentity.proto
Outdated
Show resolved
Hide resolved
proto/spire/api/agent/delegatedidentity/v1/delegatedidentity.proto
Outdated
Show resolved
Hide resolved
proto/spire/api/agent/delegatedidentity/v1/delegatedidentity.proto
Outdated
Show resolved
Hide resolved
proto/spire/api/agent/delegatedidentity/v1/delegatedidentity.proto
Outdated
Show resolved
Hide resolved
Signed-off-by: Benjamin Leggett <[email protected]> Update proto/spire/api/agent/delegatedidentity/v1/delegatedidentity.proto Co-authored-by: Andrew Harding <[email protected]> Update proto/spire/api/agent/delegatedidentity/v1/delegatedidentity.proto Co-authored-by: Andrew Harding <[email protected]> Update proto/spire/api/agent/delegatedidentity/v1/delegatedidentity.proto Co-authored-by: Andrew Harding <[email protected]> Update proto/spire/api/agent/delegatedidentity/v1/delegatedidentity.proto Co-authored-by: Andrew Harding <[email protected]>
a2141c0
to
b99e12c
Compare
@azdagron all right, suggestions applied. I also updated spiffe/spire#5272 Note that one consequence of sharing the same message and not using 0 is technically a valid PID, and with this approach we basically cannot attest a PID of 0, but I cannot imagine how or why SPIRE might need to do that given that PID 0 is special, so I don't think it's a big deal. |
Yeah, AFAIK, no userspace process will have pid 0, so I think that's a practical tradeoff. |
Signed-off-by: Benjamin Leggett <[email protected]>
Ty @azdagron and others! |
Signed-off-by: Benjamin Leggett <[email protected]>
This reverts commit 9e70b1d. This PR shouldn't land in main until we're ready for it to be in a release. Signed-off-by: Andrew Harding <[email protected]>
#62) This reverts commit 9e70b1d. This PR shouldn't land in main until we're ready for it to be in a release. Signed-off-by: Andrew Harding <[email protected]>
Signed-off-by: Benjamin Leggett <[email protected]>
This is designed to provoke discussion around the DelegateIdentity API changes required for spiffe/spire#5019
This is following the "pass PIDs" approach settled on in the RFC doc: https://docs.google.com/document/d/1A1oQHuR6z3bvQtXN17r2EwBr5lazGGPbUPkxoURAAh4/edit
This assumes that the caller/client is ensuring that the PID is not recycled for the duration of the stream/call, using
pidfd
or similar.Looking for the following feedbacks:
Is this additional functionality a good fit for the DelegateIdentity API or should we add a new API? I think we should just extend DI.
Do we object to supporting
stream
/subscriber based APIs? I think we should support them, even though the PID recycling protection requirement puts more onus on the client.Do we want to do this for both JWT and x509 SVIDs? I assume so, and added new calls for both of the ones that previously required selectors.
Rather than make currently-required fields optional (selectors), I added parallel calls with new, parallel messages/fields. Do we want to do it like this, or do we want to use a single message/call and deprecate the old fields?
The impl side of this is here: spiffe/spire#5272