proto/agent: add Delegated Identity API #8

rscampos · 2021-07-22T21:23:01Z

Add proto definitions for new Delegated Identity API. This API includes two
rpcs: SubscribeToX509SVIDs and SubscribeToX509Bundles.

The SubscribeToX509SVIDs rpc allows a privileged client to get X509-SVIDs
for a given workload. This method uses a directional gRPC stream.

The request is used to subscribe a workload, and the response (stream
from server to client) is used to send SVID updates (for the subscribed
workload). The client subscription is based on the workload's selectors.
X509-SVIDs for registration entries matching all the passed selectors
are returned.

The SubscribeToX509Bundles rpc streams get local and all federated
bundles.

Fixes #4
Related #7

Co-authored-by: Raphael Campos [email protected]
Co-authored-by: Thiago Navarro [email protected]
Signed-off-by: Mauricio Vásquez [email protected]
Signed-off-by: Mauricio Vásquez [email protected]
Signed-off-by: Raphael Campos [email protected]
Signed-off-by: Thiago Navarro [email protected]

evan2645

Thanks for cleaning this up @rscampos

I think it is looking awesome. No big structural stuff, I left mostly comments about comments 😆

Behavior of bundle subscription, as well as where we put the federated bundle references, are the two "biggest" things. Curious to hear everyone's thoughts there ... hope to see this get merged soon

evan2645 · 2021-07-27T22:46:00Z

proto/spire/api/agent/delegatedidentity/v1/delegatedidentity.proto

+
+// X.509 SPIFFE Verifiable Identity Document with the private key.
+message X509SVIDWithKey {
+    // The agent X509-SVID.


Suggested change

// The agent X509-SVID.

// The workload X509-SVID.

evan2645 · 2021-07-27T22:46:52Z

proto/spire/api/agent/delegatedidentity/v1/delegatedidentity.proto

+    // The agent X509-SVID.
+    spire.api.types.X509SVID x509_svid = 1;
+
+    // Key encoding (DER PKCS#8).


Suggested change

// Key encoding (DER PKCS#8).

// Private key (encoding DER PKCS#8).

evan2645 · 2021-07-27T22:50:30Z

proto/spire/api/agent/delegatedidentity/v1/delegatedidentity.proto

+// SubscribeToX509SVIDsRequest is used by clients to subscribe the set of workloads that
+// it receives SVIDs for.


I think that "subscribe the set of workloads" made sense with the old API shape, but less so with the new one. Since we're trying to encourage one subscription per workload, it would be good for the comment to reflect that kind of use

Suggested change

// SubscribeToX509SVIDsRequest is used by clients to subscribe the set of workloads that

// it receives SVIDs for.

// SubscribeToX509SVIDsRequest is used by clients to subscribe the set of SVIDs that

// any given workload is entitled to. Clients subscribe to a workload's SVIDs by providing

// a set of selectors describing the workload.

evan2645 · 2021-07-27T22:51:57Z

proto/spire/api/agent/delegatedidentity/v1/delegatedidentity.proto

+// SubscribeToX509SVIDsRequest is used by clients to subscribe the set of workloads that
+// it receives SVIDs for.
+message SubscribeToX509SVIDsRequest {
+    // Required. Selectors of the workload to add to the watched set.


I think this "watched set" language made sense with the old API shape, but less so with the new one.

Suggested change

// Required. Selectors of the workload to add to the watched set.

// Required. Selectors describing the workload to subscribe to.

evan2645 · 2021-07-27T23:10:49Z

proto/spire/api/agent/delegatedidentity/v1/delegatedidentity.proto

+    bytes x509_svid_key = 2;
+
+    // Names of the trust domains that this workload should federates with.
+    repeated string federates_with = 3;


In the workload api, we return this at the top level (i.e. as part of SubscribeToX509SVIDsResponse). The thinking was that we didn't want to slide into authorization land by telling workloads which trust domains a particular SVID should be used in conjunction with.

It's hard for me to tell whether that's the right thing to do here or not. There is also the question of consistency with the workload api, but we've already departed from that a bit.

Hey @evan2645, I took a step back to follow the discuss in old #7. If I misunderstanding something, pls let me know.

Are you saying that, today a workload can receive all trust domains (in federates_with) which is able to federate with, and this is not a good way to slide into authorization land?
Let's say that an alternative way would be a workload asking if its could federate with a specific trust domain? And, in spire, a check, answering true/false for the trust domain informed?

I don't know if I catch the ideia. Do you think that a critical point to an initial version of the API?

It is not a critical point. Since this API is not part of SPIFFE spec we can deprecate and change as needed.

Workload-to-identity is 1:N. The question here is whether federated_bundle should be in the workload or the identity.

I'm fine with this as-is, I just wanted to point it out in case someone had an opinion on it :)

I think I'd slightly prefer it top-level in the response instead of duplicated for each identity, for both consistency sake with the Workload API and for reasons even has mentioned (sliding into authorization land).

@azdagron In the case we put federates_with at top-level in the response (as the message X509SVIDResponse), we need to aggregate all the foreign trust domains (based on each registration entry) before sending them to workload, right?

@azdagron @evan2645 maybe put the federates_with at top-level in the response make more sense once the Workload API do it in this way. At first I didn't get the ideia, but now I understand looking at X509SVIDResponse. By now I can't see a problem doing in this way :)
Tks again for all the discuss guys :D

evan2645 · 2021-07-27T23:15:47Z

proto/spire/api/agent/delegatedidentity/v1/delegatedidentity.proto

+// SubscribeToX509BundleResponse represents the bundle for a given trust domain. An
+// empty bundle indicates that it was removed. Use DER-encoded in X.509 certs.


An empty bundle indicates that it was removed.

An alternative could be to have one message that sends all the bundles. When any of them change (or are removed), the entire set is retransmitted. This is closer to the behavior of the workload api (and the behavior of SubscribeToX509SVIDs I assume?), but also means the message size will be much larger. I'm not sure how easy it would be to start running into limitations there

@evan2645 , we discuss it and we think would be better to send all the bundles in one message too. About sending an empty message is it ok?
About the message size, would be it critical for now? what do you think?

Re: message size ... the experience here will be, the integration will break entirely for any node that is running one or more federated workloads where the sum of the federated bundles across all workloads on the node is >~4MB. It can be that the addition of a new federated bundle causes us to cross this threshold (and fail suddenly). How likely this is, I'm not sure ...

We already have this problem on workload api, but it is on a per-workload basis. Here, we are talking about per-node. We should probably figure out message overhead and how many bytes per cert to better understand how realistic this worry is.

I think I'd be fine with starting with a "send the world" approach to this at first. We can always add a different RPC later if we determine this is too problematic. I think the "send the world" approach is going to be simpler to implement on both sides, and is probably an ok place to start. Certainly we can imagine a world where the number of federation relationships is high enough to trip the message limit, but in most instances it feels contrived.

Certainly we can imagine a world where the number of federation relationships is high enough to trip the message limit, but in most instances it feels contrived.

How many certs do we think here to bust the limit? 100? 1,000? 10,000? The first doesn't seem impossible, the second is unlikely IMO. I think the important thing here is the failure mode and how we handle it. We probably don't want an outage being caused across the fleet because an operator added a new federation relationship.

So, a cert over an EC256 public key with a SHA256 hash with minimal extensions is ~300 bytes. A cert over an RSA 2048-bit key of similar complexity is ~700 bytes (somewhat rough ballpark figures). We're probably looking on the order of thousands of certificates before tripping up.

👍 thanks @azdagron, sounds like we'll be ok for now.

if we're alright on message size, my vote is for one message with all the bundles.

evan2645 · 2021-07-27T23:18:52Z

proto/spire/api/agent/delegatedidentity/v1/delegatedidentity.proto

+// The caller must be local and its identity must be listed in the allowed
+// clients on the spire-agent configuration.
+service DelegatedIdentity {
+    // Subscribe to get X.509-SVIDs for identities that match the given selectors.


Since we are delegating workload SVIDs here, I think it is natural to frame the comment as obtaining a workload's SVIDs rather than obtaining an identity's SVIDs

Suggested change

// Subscribe to get X.509-SVIDs for identities that match the given selectors.

// Subscribe to get X.509-SVIDs for workloads that match the given selectors.

evan2645 · 2021-07-27T23:20:50Z

proto/spire/api/agent/delegatedidentity/v1/delegatedidentity.proto

+    // The lifetime of the subscription aligns to the lifetime of the stream.
+    rpc SubscribeToX509SVIDs(SubscribeToX509SVIDsRequest) returns (stream SubscribeToX509SVIDsResponse);
+
+    // Subscribe to get local and federated bundle.


Suggested change

// Subscribe to get local and federated bundle.

// Subscribe to get local and all federated bundles.

evan2645

I've left some suggestions here to bring the PR up to the single-message-for-all-bundles shape that we've recently discussed. Please let me know what you think. If you agree with this change, I think it's the last outstanding thing! Thanks so much for hanging in there, I trust that we have spent our time here well.

evan2645 · 2021-08-10T17:29:08Z

proto/spire/api/agent/delegatedidentity/v1/delegatedidentity.proto

+
+    // Subscribe to get local and federated bundle.
+    // The lifetime of the subscription aligns to the lifetime of the stream.
+    rpc SubscribeToX509Bundle(SubscribeToX509BundleRequest) returns (stream SubscribeToX509BundleResponse);


Suggested change

rpc SubscribeToX509Bundle(SubscribeToX509BundleRequest) returns (stream SubscribeToX509BundleResponse);

rpc SubscribeToX509Bundles(SubscribeToX509BundlesRequest) returns (stream SubscribeToX509BundlesResponse);

evan2645 · 2021-08-10T17:29:23Z

proto/spire/api/agent/delegatedidentity/v1/delegatedidentity.proto

+    repeated X509SVIDWithKey x509_svids = 1;
+}
+
+message SubscribeToX509BundleRequest {}


Suggested change

message SubscribeToX509BundleRequest {}

message SubscribeToX509BundlesRequest {}

evan2645 · 2021-08-10T17:31:31Z

proto/spire/api/agent/delegatedidentity/v1/delegatedidentity.proto

+// SubscribeToX509BundleResponse represents the bundle for a given trust domain. An
+// empty bundle indicates that it was removed. Use DER-encoded in X.509 certs.


Suggested change

// SubscribeToX509BundleResponse represents the bundle for a given trust domain. An

// empty bundle indicates that it was removed. Use DER-encoded in X.509 certs.

// SubscribeToX509BundlesResponse contains all bundles that the agent is tracking,

// including the local bundle. When an update occurs, or bundles are added or removed,

// a new response with the full set of bundles is sent.

evan2645 · 2021-08-10T20:16:42Z

proto/spire/api/agent/delegatedidentity/v1/delegatedidentity.proto

+message SubscribeToX509BundleResponse {
+    string trust_domain_name = 1;
+
+    // DER-encoded X.509 certs
+    repeated bytes ca_certificates = 2;
+}


Suggested change

message SubscribeToX509BundleResponse {

string trust_domain_name = 1;

// DER-encoded X.509 certs

repeated bytes ca_certificates = 2;

}

message SubscribeToX509BundlesResponse {

// A map keyed by trust domain name, with ASN.1 DER-encoded

// X.509 CA certificates as the values

map<string, bytes> ca_certificates = 1;

}

azdagron · 2021-08-10T20:29:12Z

Those suggestions look good to me.

Add proto definitions for new Delegated Identity API. This API includes two rpcs: SubscribeToX509SVIDs and SubscribeToX509Bundles. The SubscribeToX509SVIDs rpc allows a privileged client to get X509-SVIDs for a given workload. This method uses a directional gRPC stream. The request is used to subscribe a workload, and the response (stream from server to client) is used to send SVID updates (for the subscribed workload). The client subscription is based on the workload's selectors. X509-SVIDs for registration entries matching *all* the passed selectors are returned. The SubscribeToX509Bundles rpc streams get local and all federated bundles. Co-authored-by: Raphael Campos <[email protected]> Co-authored-by: Thiago Navarro <[email protected]> Signed-off-by: Mauricio Vásquez <[email protected]> Signed-off-by: Mauricio Vásquez <[email protected]> Signed-off-by: Raphael Campos <[email protected]> Signed-off-by: Thiago Navarro <[email protected]>

rscampos · 2021-08-13T01:30:07Z

@azdagron @evan2645
I sent a new commit with all new modifications. I also put the federates_with in top-level response. Please let me know if everything is correct.

Thank you a lot folks :D

evan2645

Thanks so much everyone! I think we're ready to go here

This commit adds the following: 1. Last version of spire-api-sdk. The shape of the API as defined here: spiffe/spire-api-sdk#8 2. Add vendor github.com/go-spiffe/v2/spiffeid/ 3. Add/update others deps Signed-off-by: Raphael Campos <[email protected]> Signed-off-by: Mauricio Vásquez <[email protected]> Signed-off-by: Mauricio Vásquez <[email protected]>

Signed-off-by: Andrew Harding <[email protected]>

rscampos requested review from amartinezfayo, azdagron, evan2645 and rturner3 as code owners July 22, 2021 21:23

rscampos mentioned this pull request Jul 22, 2021

proto/agent: add privileged API #7

Closed

evan2645 reviewed Jul 27, 2021

View reviewed changes

evan2645 reviewed Aug 10, 2021

View reviewed changes

rscampos force-pushed the accuknox-add-privileged-api-proto branch from b02e4fe to 71605ae Compare August 13, 2021 01:19

rscampos force-pushed the accuknox-add-privileged-api-proto branch from 71605ae to f8def1c Compare August 13, 2021 01:24

evan2645 approved these changes Aug 16, 2021

View reviewed changes

evan2645 merged commit 68c0316 into spiffe:main Aug 17, 2021

rscampos mentioned this pull request Sep 7, 2021

Spiffe Integration cilium/cilium#17335

Closed

9 tasks

evan2645 mentioned this pull request Sep 15, 2021

RFC: Delegated Identity API implementation spiffe/spire#2481

Merged

loveyana mentioned this pull request Feb 21, 2022

proto/agent: add JWT SVID privileged API #20

Merged

youngnick mentioned this pull request Nov 1, 2022

Datapath support for Cilium mTLS cilium/cilium#21822

Merged

7 tasks

youngnick mentioned this pull request Feb 16, 2023

Cilium Agent support for DelegatedIdentity SPIFFE API cilium/cilium#23804

Closed

guilhermocc pushed a commit to guilhermocc/spire-api-sdk that referenced this pull request Apr 10, 2023

V1 Server UpstreamAuthority definition (spiffe#8)

0b929b6

Signed-off-by: Andrew Harding <[email protected]>

evan2645 mentioned this pull request May 10, 2023

Delegated Identity API reaches gRPC size limit on large deployment spiffe/spire#3965

Closed

bleggett mentioned this pull request May 26, 2023

WIP - SPIRE integration with ambient mesh istio/istio#43273

Closed

12 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

proto/agent: add Delegated Identity API #8

proto/agent: add Delegated Identity API #8

rscampos commented Jul 22, 2021 •

edited

Loading

evan2645 left a comment

evan2645 Jul 27, 2021

evan2645 Jul 27, 2021

evan2645 Jul 27, 2021

evan2645 Jul 27, 2021

evan2645 Jul 27, 2021

rscampos Jul 29, 2021 •

edited

Loading

evan2645 Aug 9, 2021

azdagron Aug 9, 2021

rscampos Aug 12, 2021

azdagron Aug 12, 2021

rscampos Aug 13, 2021

evan2645 Jul 27, 2021

rscampos Jul 30, 2021

evan2645 Aug 9, 2021

azdagron Aug 9, 2021

evan2645 Aug 9, 2021

azdagron Aug 9, 2021

evan2645 Aug 10, 2021

azdagron Aug 10, 2021

evan2645 Jul 27, 2021

evan2645 Jul 27, 2021

evan2645 left a comment

evan2645 Aug 10, 2021

evan2645 Aug 10, 2021

evan2645 Aug 10, 2021

evan2645 Aug 10, 2021

azdagron commented Aug 10, 2021

rscampos commented Aug 13, 2021

evan2645 left a comment

	// Key encoding (DER PKCS#8).
	// Private key (encoding DER PKCS#8).

		// SubscribeToX509SVIDsRequest is used by clients to subscribe the set of workloads that
		// it receives SVIDs for.

-// SubscribeToX509SVIDsRequest is used by clients to subscribe the set of workloads that
-// it receives SVIDs for.
+// SubscribeToX509SVIDsRequest is used by clients to subscribe the set of SVIDs that
+// any given workload is entitled to. Clients subscribe to a workload's SVIDs by providing
+// a set of selectors describing the workload.

	// Required. Selectors of the workload to add to the watched set.
	// Required. Selectors describing the workload to subscribe to.

		// SubscribeToX509BundleResponse represents the bundle for a given trust domain. An
		// empty bundle indicates that it was removed. Use DER-encoded in X.509 certs.

	// Subscribe to get X.509-SVIDs for identities that match the given selectors.
	// Subscribe to get X.509-SVIDs for workloads that match the given selectors.

	// Subscribe to get local and federated bundle.
	// Subscribe to get local and all federated bundles.

	rpc SubscribeToX509Bundle(SubscribeToX509BundleRequest) returns (stream SubscribeToX509BundleResponse);
	rpc SubscribeToX509Bundles(SubscribeToX509BundlesRequest) returns (stream SubscribeToX509BundlesResponse);

	message SubscribeToX509BundleRequest {}
	message SubscribeToX509BundlesRequest {}

-// SubscribeToX509BundleResponse represents the bundle for a given trust domain. An
-// empty bundle indicates that it was removed. Use DER-encoded in X.509 certs.
+// SubscribeToX509BundlesResponse contains all bundles that the agent is tracking,
+// including the local bundle. When an update occurs, or bundles are added or removed,
+// a new response with the full set of bundles is sent.

proto/agent: add Delegated Identity API #8

proto/agent: add Delegated Identity API #8

Conversation

rscampos commented Jul 22, 2021 • edited Loading

evan2645 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rscampos Jul 29, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

evan2645 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

azdagron commented Aug 10, 2021

rscampos commented Aug 13, 2021

evan2645 left a comment

Choose a reason for hiding this comment

rscampos commented Jul 22, 2021 •

edited

Loading

rscampos Jul 29, 2021 •

edited

Loading