Introduce local authority api #37

MarcosDY · 2023-02-20T15:24:14Z

Update Evan's proposal with a the introduction of some required changes found on POC implementation,
and add some simplifications.

Fixes: spiffe/spire#3885

MarcosDY · 2023-02-20T15:26:35Z

proto/spire/api/server/agent/v1/agent.proto

+
+message PushStatusRequest {
+    // Required. Serial number of the bundle used to sign Agent identity.
+    string authority_serial = 1;


The idea of this serial is to keep on server database the current serial number used for registered agent.
In POC I didn't implement this, so we'll require some changes to access this data (maybe on agent endpoint)
We can avoid this on initial implementation too...

I think this is a pretty important part of the forced rotation story. Whether it should be introduced now or later, I'm not sure.

I think the comment and name of the field should be updated to reflect the fact that this is the serial number of the bundle copy that the agent currently has. E.g. Serial number of the bundle currently being served by the agent and current_bundle_serial. The agent can't know against what bundle serial its SVID was issued if the CA that issued it is present across multiple serial numbers. The serial number also pertains to the bundle, which IMO is a collection of authorities.

Finally, taking this will require changes in other APIs and probably schema, we know that right away agent version will be very useful to have here, so it probably makes sense to throw a version number in here while we're at it 😁

agree, maybe we can create an issue on spire to track all the work about persisting and recovering this information with some APIs

MarcosDY · 2023-02-20T15:29:22Z

proto/spire/api/server/agent/v1/agent.proto

+
+message PushStatusResponse {
+    // Keys that are no longer secure and must be rotated.
+    repeated bytes taintedKeys = 1;


The intention here is to return the list of tainted keys (JWT or X.509) so agent can react renewing all affected SVIDs (agent SVID, cached workload X.509 SVIDs, cached JWT SVIDs)

These are the actual pubkeys themselves? Or pubkey for JWT and CA for X.509? How are they encoded etc? We should capture this information in the comment

Also I think that snake case is standard for these field names?

Suggested change

repeated bytes taintedKeys = 1;

repeated bytes tainted_keys = 1;

for now it is a simple list, and we verify if that key was used for jwt or x509, and taint any of them,
Maybe I overthink this, and we can just create 2 lists and didn't verify if the key was used by only one of our bundles

MarcosDY · 2023-02-20T15:33:09Z

proto/spire/api/server/localauthority/v1/localauthority.proto

+
+message TaintJWTAuthorityRequest {
+        // The key ID of the local authority to taint
+        string key_id = 1;


the idea is to use "old" authority if no key_id is provided, and X.509 implementation requires a public key...
I'm worried about the difference about both, we may use public key here too..
but our endpoints returns a key_id so I think this can be more natural to use that key id (and our JWT SVIDs has the key ID too)

I'm worried about the difference about both

I worry about this too. We should be able to address/name an authority in a single way regardless of what type it is (x.509 or jwt).

ok, so move to taint using public keys on both looks like the correct call, and more natural based in our "Get" responses

MarcosDY · 2023-02-20T15:36:41Z

proto/spire/api/server/localauthority/v1/localauthority.proto

+
+message TaintX509AuthorityRequest {
+        // The public key of the local authority to taint
+        bytes public_key  = 1;


an alternative to public key is to use Serial number,
and this proposal has the PushStatus that contains the serial number of the bundle used to sign Agent SVID too,
I was worried about 2 bundles using the same Serial number, but I have the feeling I overkill this...

We have certificate serial number, and bundle serial number, which are different. For this RPC, I think we don't want bundle serial number, we want the one associated with the CA cert we're trying to taint.

As you noted, JWT has a nice solution because there we already have a key ID. For X.509, I feel it is natural to have this identifier be a function of the key itself rather than the CA certificate, because if we are doing this it's a result of key compromise. For SPIRE I don't think it makes too much of a difference because we always cut a new key when creating a new CA cert, but there's nothing stopping us from changing that behavior in the future.

X.509 does have an "SKID" field (Subject Key Identifier) .. I believe it is required for CA certificates, and that SPIRE sets it. Maybe we can use this SKID or some derivative of it as our key ID?

Making sure that these key IDs are easily typed and copy/pasted will also be important .. they will pop up in CLI commands etc.

yeah that was what worry me...
a simple key ID is easier to use, than a public key...
but our GET are getting that public keys, and at the same time, it is easy to get the public key from the affected bundle if you have it..
So maybe it is safer to keep public key for now... and as you said nothing prevents to get the same key in multiple bundles (that is why I added a single TaintedKey list to agent, so we can be sure we taint any affected bundle, no matter if it was a JWT or X.509)

Using SKID looks like something possible, of course it will be only for endpoints but internally I think we must keep tainting Public keys, and another endpoints may propagates pubic keys

proto/spire/api/server/svid/v1/svid.proto

This commit introduces an API that allows callers to manage key preparation/activation/rotation in SPIRE. It is currently a work in progress. Signed-off-by: Evan Gilman <[email protected]>

Signed-off-by: Marcos Yacob <[email protected]>

evan2645 · 2023-03-29T05:56:02Z

proto/spire/api/server/agent/v1/agent.proto

+    //
+    // The caller must present an active agent X509-SVID, i.e. the X509-SVID
+    // returned by the AttestAgent or the most recent RenewAgent call.
+    rpc PushStatus(PushStatusRequest) returns (PushStatusResponse);


When I see Push here, it makes me think that there should be a stream, or that this is being sent from Server to somewhere else synchronously. Maybe something like PostStatus or ReportStatus would be clearer?

agree, that sounds like an stream, I'm rename to PostStatus

evan2645 · 2023-03-29T05:59:18Z

proto/spire/api/server/agent/v1/agent.proto

+
+message PushStatusRequest {
+    // Required. Serial number of the bundle used to sign Agent identity.
+    string authority_serial = 1;


I think this is a pretty important part of the forced rotation story. Whether it should be introduced now or later, I'm not sure.

I think the comment and name of the field should be updated to reflect the fact that this is the serial number of the bundle copy that the agent currently has. E.g. Serial number of the bundle currently being served by the agent and current_bundle_serial. The agent can't know against what bundle serial its SVID was issued if the CA that issued it is present across multiple serial numbers. The serial number also pertains to the bundle, which IMO is a collection of authorities.

Finally, taking this will require changes in other APIs and probably schema, we know that right away agent version will be very useful to have here, so it probably makes sense to throw a version number in here while we're at it 😁

evan2645 · 2023-03-29T06:02:41Z

proto/spire/api/server/agent/v1/agent.proto

+
+message PushStatusResponse {
+    // Keys that are no longer secure and must be rotated.
+    repeated bytes taintedKeys = 1;


These are the actual pubkeys themselves? Or pubkey for JWT and CA for X.509? How are they encoded etc? We should capture this information in the comment

Also I think that snake case is standard for these field names?

Suggested change

repeated bytes taintedKeys = 1;

repeated bytes tainted_keys = 1;

proto/spire/api/server/svid/v1/svid.proto

evan2645 · 2023-03-29T09:34:41Z

proto/spire/api/server/localauthority/v1/localauthority.proto

+
+message TaintJWTAuthorityRequest {
+        // The key ID of the local authority to taint
+        string key_id = 1;


I'm worried about the difference about both

I worry about this too. We should be able to address/name an authority in a single way regardless of what type it is (x.509 or jwt).

evan2645 · 2023-03-29T09:40:02Z

proto/spire/api/server/localauthority/v1/localauthority.proto

+
+message TaintX509AuthorityRequest {
+        // The public key of the local authority to taint
+        bytes public_key  = 1;


We have certificate serial number, and bundle serial number, which are different. For this RPC, I think we don't want bundle serial number, we want the one associated with the CA cert we're trying to taint.

As you noted, JWT has a nice solution because there we already have a key ID. For X.509, I feel it is natural to have this identifier be a function of the key itself rather than the CA certificate, because if we are doing this it's a result of key compromise. For SPIRE I don't think it makes too much of a difference because we always cut a new key when creating a new CA cert, but there's nothing stopping us from changing that behavior in the future.

X.509 does have an "SKID" field (Subject Key Identifier) .. I believe it is required for CA certificates, and that SPIRE sets it. Maybe we can use this SKID or some derivative of it as our key ID?

evan2645 · 2023-03-29T09:41:51Z

proto/spire/api/server/localauthority/v1/localauthority.proto

+
+message TaintX509AuthorityRequest {
+        // The public key of the local authority to taint
+        bytes public_key  = 1;


Making sure that these key IDs are easily typed and copy/pasted will also be important .. they will pop up in CLI commands etc.

evan2645 · 2023-03-29T09:43:41Z

proto/spire/api/server/localauthority/v1/localauthority.proto

+        AuthorityState revoked_authority = 1;
+}
+
+message AuthorityState {


I think we need key ID as part of this message. An operator will first want to list the authorities, and then will want to taint one, and to taint you need the key ID.

yeah, I was thinking on adding that based is what we decide about taint and revoke, if unify using public key on both or use key_id for one and public key for another one,
based in what we choose here is what we must return

Signed-off-by: Marcos Yacob <[email protected]>

…tifier instead of public key Refactor Taint and Revoke JWT authority to use authoirty_id Remove tained keys from Agent.PostStatus, the idea is to use Bundle updates to get the tainted keys. Signed-off-by: Marcos Yacob <[email protected]>

MarcosDY · 2023-04-13T15:34:13Z

proto/spire/api/server/agent/v1/agent.proto

+
+message PostStatusRequest {
+    // Required. Serial number of the bundle currently being served by the agent
+    int64 current_bundle_serial = 1;


may we use uint64 here?

Signed-off-by: Marcos Yacob <[email protected]>

evan2645

thanks for your patience @MarcosDY ❤️

evan2645 · 2023-04-25T17:50:45Z

proto/spire/api/server/agent/v1/agent.proto

+    // Publishes Agent status, relevant information is returned to the agent to keep it 
+    // updated on server changes.


nit: stale comment

evan2645 · 2023-04-25T17:55:34Z

proto/spire/api/server/localauthority/v1/localauthority.proto

+        AuthorityState prepared_authority = 1;
+}
+
+message ActivateJWTAuthorityRequest {}


I don't think it's strictly required right now (maybe it is something we can implement later), but I wonder if we might have an authority ID parameter here in the future, or something that would otherwise allow an operator to re-activate an "old" authority

I was thinking the same thing, but I was not sure about implementing that on the first version,
I agree it can be useful, and we can implement this, with some changes on code (nothing too bad)
So I'll add it now, and we can discuss on implementation.

evan2645 · 2023-04-25T17:57:03Z

proto/spire/api/server/localauthority/v1/localauthority.proto

+        // will perform proactive rotations of any key material related to
+        // the tainted authority. The result of this action will be observed
+        // cluster-wide.
+        // It can receive the KeyID of an old JWT authority.


Nit: a few of these lines referencing KeyID that I think should be updated to authority ID or similar?

evan2645 · 2023-04-25T17:59:54Z

proto/spire/api/server/localauthority/v1/localauthority.proto

+        // The authority ID of the local X.509 authority to taint.
+        // This is the subject key identifier, that's calculated by
+        // doing a SHA-1 hash over the ASN.1 encoding of the public key.


nit: it would be good to be specific about how/where this SKID can be found

Suggested change

// The authority ID of the local X.509 authority to taint.

// This is the subject key identifier, that's calculated by

// doing a SHA-1 hash over the ASN.1 encoding of the public key.

// The authority ID of the local X.509 authority to taint.

// This is the X.509 Subject Key Identifier (or SKID) of the

// authority's CA certificate, which is calculated by doing a

// SHA-1 hash over the ASN.1 encoding of the public key.

proto/spire/api/server/svid/v1/svid.proto

MarcosDY · 2023-04-26T15:11:50Z

Agree using bundles as a workaround can be useful, in the end we'll be using that after initial connection is done.
What do you think about removing it for now, and we can reintroduce when implementing upstream authority? (if we found it was really required)

Signed-off-by: Marcos Yacob <[email protected]>

…bundle Signed-off-by: Marcos Yacob <[email protected]>

evan2645

🪨 🥧

thank you Marcos ❤️ looks great

* Introduce an API for key management Introduces an API that allows callers to manage key preparation/activation/rotation/revocation in SPIRE. Update bundle API to propagate tainted keys. Add an endpoint to Agent API to post current agent status. --------- Signed-off-by: Evan Gilman <[email protected]> Signed-off-by: Marcos Yacob <[email protected]> Co-authored-by: Evan Gilman <[email protected]>

MarcosDY requested review from evan2645, amartinezfayo, azdagron and rturner3 as code owners February 20, 2023 15:24

MarcosDY commented Feb 20, 2023

View reviewed changes

proto/spire/api/server/svid/v1/svid.proto Outdated Show resolved Hide resolved

MarcosDY mentioned this pull request Feb 21, 2023

Add Local Authority API to API SDK spiffe/spire#3885

Closed

evan2645 mentioned this pull request Mar 2, 2023

Introduce an API for key management #21

Closed

meyskens mentioned this pull request Mar 3, 2023

Add CFP-22215 mTLS Authentication document with images cilium/design-cfps#5

Merged

evan2645 and others added 2 commits March 9, 2023 11:27

Introduce an API for key management

79422e6

This commit introduces an API that allows callers to manage key preparation/activation/rotation in SPIRE. It is currently a work in progress. Signed-off-by: Evan Gilman <[email protected]>

Update proposal

cf1e886

Signed-off-by: Marcos Yacob <[email protected]>

MarcosDY force-pushed the introduce-local-authority-api branch from aef980d to cf1e886 Compare March 9, 2023 14:28

MarcosDY added 2 commits March 17, 2023 10:54

Merge branch 'next' into introduce-local-authority-api

7b11880

Update AuthorityState with some missing changes

5642a04

Signed-off-by: Marcos Yacob <[email protected]>

MarcosDY mentioned this pull request Mar 22, 2023

Force Rotation: local authority scaffolding and implements X.509 authority endpoints spiffe/spire#4020

Merged

rturner3 assigned evan2645 Mar 23, 2023

evan2645 reviewed Mar 29, 2023

View reviewed changes

MarcosDY added 3 commits April 4, 2023 10:03

Rename PushStatus to PostStatus and resolve some naming issues

7c7b31c

Signed-off-by: Marcos Yacob <[email protected]>

Update missing comment

26b6518

Signed-off-by: Marcos Yacob <[email protected]>

MarcosDY commented Apr 13, 2023

View reviewed changes

update sequence number to uint64

839f63c

Signed-off-by: Marcos Yacob <[email protected]>

evan2645 previously approved these changes Apr 25, 2023

View reviewed changes

PR changes

7470e79

Signed-off-by: Marcos Yacob <[email protected]>

MarcosDY dismissed evan2645’s stale review via 7470e79 April 26, 2023 15:27

Merge branch 'next' into introduce-local-authority-api

1220029

Remove x509_authorities_with_metadata from now, in favor of usin get …

58ec6ec

…bundle Signed-off-by: Marcos Yacob <[email protected]>

evan2645 approved these changes May 2, 2023

View reviewed changes

MarcosDY merged commit ada4bc8 into spiffe:next May 2, 2023

MarcosDY deleted the introduce-local-authority-api branch May 2, 2023 17:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce local authority api #37

Introduce local authority api #37

MarcosDY commented Feb 20, 2023 •

edited

Loading

MarcosDY Feb 20, 2023

evan2645 Mar 29, 2023

MarcosDY Mar 29, 2023

MarcosDY Feb 20, 2023

evan2645 Mar 29, 2023

MarcosDY Mar 29, 2023

MarcosDY Feb 20, 2023

evan2645 Mar 29, 2023

MarcosDY Mar 29, 2023 •

edited

Loading

MarcosDY Feb 20, 2023

evan2645 Mar 29, 2023

evan2645 Mar 29, 2023

MarcosDY Mar 29, 2023

MarcosDY Apr 3, 2023

evan2645 Mar 29, 2023

MarcosDY Mar 29, 2023

evan2645 Mar 29, 2023

evan2645 Mar 29, 2023

evan2645 Mar 29, 2023

evan2645 Mar 29, 2023

evan2645 Mar 29, 2023

evan2645 Mar 29, 2023

MarcosDY Mar 29, 2023

MarcosDY Apr 13, 2023

evan2645 left a comment

evan2645 Apr 25, 2023

evan2645 Apr 25, 2023

MarcosDY Apr 26, 2023

evan2645 Apr 25, 2023

evan2645 Apr 25, 2023

MarcosDY commented Apr 26, 2023

evan2645 left a comment

	repeated bytes taintedKeys = 1;
	repeated bytes tainted_keys = 1;

		// Publishes Agent status, relevant information is returned to the agent to keep it
		// updated on server changes.

Introduce local authority api #37

Introduce local authority api #37

Conversation

MarcosDY commented Feb 20, 2023 • 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

MarcosDY Mar 29, 2023 • 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

Choose a reason for hiding this comment

MarcosDY commented Apr 26, 2023

evan2645 left a comment

Choose a reason for hiding this comment

MarcosDY commented Feb 20, 2023 •

edited

Loading

MarcosDY Mar 29, 2023 •

edited

Loading