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

Allow DelegatedIdentity API clients to subscribe by PID #58

Merged
merged 7 commits into from
Jul 20, 2024

Conversation

bleggett
Copy link
Contributor

@bleggett bleggett commented May 3, 2024

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

bleggett added a commit to bleggett/spire that referenced this pull request Jul 2, 2024
Signed-off-by: Benjamin Leggett <[email protected]>
@bleggett bleggett changed the title WIP: Allow DelegatedIdentity API clients to subscribe by PID Allow DelegatedIdentity API clients to subscribe by PID Jul 3, 2024
Signed-off-by: Benjamin Leggett <[email protected]>
@bleggett bleggett force-pushed the bleggett/delegated-identity-pid branch from dec110c to 191064c Compare July 9, 2024 15:23
@azdagron
Copy link
Member

Thanks for this @bleggett. We discussed this a bit and I think at this time the preference is to:

  1. Extend the existing RPCs instead of creating new ones
  2. Use a oneof to either provide selectors or the pid

@bleggett
Copy link
Contributor Author

Thanks for this @bleggett. We discussed this a bit and I think at this time the preference is to:

  1. Extend the existing RPCs instead of creating new ones
  2. Use a oneof to either provide selectors or the pid

Okay, I'm fine with that. Will redo it along those lines.

Thanks for getting back!

@bleggett
Copy link
Contributor Author

bleggett commented Jul 19, 2024

Use a oneof to either provide selectors or the pid

@azdagron There's one gotcha with this - you cannot use repeated fields in a oneof. And the existing selectors are repeated.

This could be fixed by creating a new message for the selectors, nesting the repeated selectors in that message, and using oneof {message, pid}

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?

@azdagron
Copy link
Member

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]>
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.

Looks good overall. Just a few small nitpicks.

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]>
@bleggett bleggett force-pushed the bleggett/delegated-identity-pid branch from a2141c0 to b99e12c Compare July 19, 2024 22:29
@bleggett
Copy link
Contributor Author

@azdagron all right, suggestions applied.

I also updated spiffe/spire#5272

Note that one consequence of sharing the same message and not using oneof is that an "unset" PID is naturally protobuf'd to 0: https://github.com/spiffe/spire/pull/5272/files#diff-1007e43f9ac81915b4fd0f98b5a937ae07f2d12c5d20fa69a2919788663c868eR121

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.

@bleggett bleggett requested a review from azdagron July 19, 2024 22:35
@azdagron
Copy link
Member

Yeah, AFAIK, no userspace process will have pid 0, so I think that's a practical tradeoff.

Signed-off-by: Benjamin Leggett <[email protected]>
@azdagron azdagron merged commit 9e70b1d into spiffe:main Jul 20, 2024
2 checks passed
@bleggett
Copy link
Contributor Author

Ty @azdagron and others!

azdagron pushed a commit that referenced this pull request Jul 22, 2024
azdagron added a commit that referenced this pull request Jul 22, 2024
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]>
azdagron added a commit that referenced this pull request Jul 22, 2024
#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]>
MarcosDY pushed a commit that referenced this pull request Sep 3, 2024
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.

2 participants