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

MarcosDY · 2023-03-22T19:24:45Z

Force Rotation: local authority scaffolding and implements X.509 authority endpoints

Which issue this PR fixes
fixes #3895 and fixes #3897

MarcosDY · 2023-03-22T19:25:51Z

go.mod

@@ -2,6 +2,8 @@ module github.com/spiffe/spire

 go 1.19

+replace github.com/spiffe/spire-api-sdk => github.com/MarcosDY/spire-api-sdk v1.0.0-pre.0.20230321150416-5642a0450459


This PR depends of spire-api-sdk PR

guilhermocc · 2023-03-23T14:26:13Z

pkg/server/api/localauthority/v1/service.go

+}
+
+func (s *Service) TaintX509Authority(ctx context.Context, req *localauthorityv1.TaintX509AuthorityRequest) (*localauthorityv1.TaintX509AuthorityResponse, error) {
+	parseRequest := func() logrus.Fields {


@MarcosDY What do you think about deanonymizing this function? It is duplicated in the method below

func getRequestLogFields(publicKey []byte) logrus.Fields { fields := logrus.Fields{} if len(publicKey) > 0 { fields[telemetry.X509AuthorityPublicKeySHA256] = api.HashByte(publicKey) } return fields }

guilhermocc · 2023-03-23T14:33:43Z

pkg/server/api/localauthority/v1/service_test.go

+			},
+		},
+		{
+			name:        "slot has no public key",


Suggested change

name: "slot has no public key",

name: "current slot has no public key",

guilhermocc · 2023-03-23T14:37:32Z

pkg/server/api/localauthority/v1/service.go

+	if next.GetPublicKey() != nil {
+		status := localauthorityv1.AuthorityState_PREPARED
+		// CA is removed from slot on rotation
+		if next.IsEmpty() {


@MarcosDY, why do we need this check here? If the "next.GetPublicKey()" method returns something different than nil, wouldn't this method always return false?

no, when we reset a slot, the x509 field is set to be nil but we keep the public key, so with that we know than that slot contains an OLD authority (you can take a look to RotateX509

guilhermocc · 2023-03-23T14:41:37Z

pkg/server/api/localauthority/v1/service_test.go

+	localauthorityv1 "github.com/spiffe/spire-api-sdk/proto/spire/api/server/localauthority/v1"
+	"github.com/spiffe/spire/pkg/common/telemetry"
+	"github.com/spiffe/spire/pkg/server/api"
+	localauthority "github.com/spiffe/spire/pkg/server/api/localauthority/v1"


Redundant alias

it is not, if we does not set alias it will try to use v1

I've tried to run the tests without this alias, and they are all passing; the package name being imported from github.com/spiffe/spire/pkg/server/api/localauthority/v1 is already "localauthority".

guilhermocc · 2023-03-23T14:44:16Z

pkg/server/api/localauthority/v1/service_test.go

+		expectResp       *localauthorityv1.RevokeX509AuthorityResponse
+	}{
+		{
+			name:        "taint old authority",


Suggested change

name: "taint old authority",

name: "revoke old authority",

evan2645

Very nice work @MarcosDY thank you so much! Code quality is high and it was easy to read through ❤️ 🙏

evan2645 · 2023-06-13T17:44:50Z

pkg/server/api/localauthority/v1/service.go

+
+	var states []*localauthorityv1.AuthorityState
+	current := s.ca.GetCurrentX509CASlot()
+	if !current.IsEmpty() {


Under what conditions could current be empty?

technically this must never happens... we can just not validate it.

Ok, then perhaps we can have this conditional just immediately return an error if it's true? This logic was a little harder to follow than the rest and removing some layers will help

evan2645 · 2023-06-13T17:51:04Z

pkg/server/api/localauthority/v1/service.go

+	next := s.ca.GetNextX509CASlot()
+	// when next has a key indicates that it was initialized
+	if next.AuthorityID() != "" {
+		status := localauthorityv1.AuthorityState_PREPARED
+		// CA is removed from slot on rotation
+		if next.IsEmpty() {
+			status = localauthorityv1.AuthorityState_OLD
+		}
+
+		states = append(states, &localauthorityv1.AuthorityState{
+			AuthorityId: next.AuthorityID(),
+			Status:      status,
+		})
+	}


I think all of this stuff is good enough for now, but in the long term, we will probably want to move state tracking into the CA package itself somewhere ... these kind of detection that depends on internal behavior of CA package might break someday. Also could be a good time to introduce a third "previous" key slot

Actually we have an State field on journal now, and we can just parse that status into a localauthorityv1.AuthorityState I was thinking on do that on this PR but I choose to keep it out for now,
About the third slot approach, are you ok with creating another issue for that?
I can start working on that and we can adapt this code if required, I think since it is an ID we'll not affect key managers but I will need to review that,

are you ok with creating another issue for that?

Yes for sure! Didn't intend for all that to be in scope for this PR, just that it seems like a good future direction

I created #4271

evan2645 · 2023-06-13T17:53:51Z

pkg/server/api/localauthority/v1/service.go

+	rpccontext.AuditRPC(ctx)
+
+	return &localauthorityv1.GetX509AuthorityStateResponse{
+		States: states,


Now that I see this here, I wonder if it would be more ergonomic to have a field for each of the valid states, and a state value for each one ... which would prevent weird things like multiple active keys ... and may also make it easier to consume (e.g. don't have to tear through a slice looking for the active key)?

so, basically refactor local authority interface to have something like:

message GetX509AuthorityStateResponse { // In use Authority AuthorityState Active = 1; // Ready to use AuthorityState Prepared = 2; // Previously used AuthorityState Old = 3; // WE MUST NEVER GET THIS AuthorityState Unknown = 4; }

For sure easier to consume and prevents multiple uses of the same type, I think we can avoid Unknown we may never get into that one

This feels much better 😁 but I also know that we already reviewed the API etc and I missed this, so I don't think we should block on it here

It is always expected to have changes on implementation, based on how things looks,
I like to have improvements as soon as possible,
I created a PR on spire-api-sdk,
as soon it is merged I'll apply changes here

evan2645 · 2023-06-13T17:56:15Z

pkg/server/api/localauthority/v1/service.go

+	// Prepare is going to use current slot when it is empty
+	if slot.IsEmpty() {
+		slot = s.ca.GetCurrentX509CASlot()
+	}


Similar question here, in what cases is current slot empty? Mostly wondering if it's limited to startup or not, and if it is worth handling such edge case in this API (as opposed to returning some kind of error like "initializing")

agree it must never happen, I'm happy to return an error instead, but we must never get into this case, even when initializing, we must load authorities before open APIs, so I dont expect it to happen

If it's unexpected, feels like a good idea to return an error

evan2645 · 2023-06-13T18:11:49Z

pkg/server/api/localauthority/v1/service.go

+	// TODO: implement a way to activate an OLD authority
+	if req.AuthorityId != "" {
+		log = log.WithField(telemetry.LocalAuthorityID, req.AuthorityId)
+		return nil, api.MakeErr(log, codes.InvalidArgument, "activating an old authority is not supported yet", nil)


This could also be a bad request (probably more likely bad request than attempt to activate old key?)

my intention here is that we can activate the Previous activated one, may as part of adding that new slot on journal?
however, what do you suggest to do here? validate if the ID really exists before failing?

IMO, activating a previous key is a big use case, but we should not support it until CA has changes to know about a previous slot, we can directly address it, etc. I worry about overloading the meaning of empty string because it could signal an error condition that we would then take a sensitive action on. So maybe for now, we don't support activating previous key .. and if/when we get CA changes to support a previous slot, we can also add the functionality here?

evan2645 · 2023-06-13T18:16:17Z

pkg/server/api/localauthority/v1/service.go

+	}
+	log = log.WithField(telemetry.LocalAuthorityID, authorityID)
+
+	if err := s.ds.TaintX509CA(ctx, s.td.IDString(), publicKey); err != nil {


Kind of neat that all we need to do is write this bit into the datastore

What happens if I try to taint the active authority? Same question for Revoke

in getX509PublicKey I validate that the provided authority is not the active one, so this must not happens, another option is to update datastore to store states too

evan2645 · 2023-06-13T18:19:40Z

pkg/server/api/localauthority/v1/service.go

+	}, nil
+}
+
+// getX509PublicKey validates provided authority ID, and return OLD associated publick key, in case of authority ID is not provided, use the current OLD authority


Suggested change

// getX509PublicKey validates provided authority ID, and return OLD associated publick key, in case of authority ID is not provided, use the current OLD authority

// getX509PublicKey validates provided authority ID, and return OLD associated public key, in case of authority ID is not provided, use the current OLD authority

Relying on empty authority id to mean the old authority feels kind of dangerous (related to above comment about missing authority id in req)

so do you suggest here to make authorityID required, but it must be the old one?

evan2645 · 2023-06-13T18:21:50Z

pkg/server/api/localauthority/v1/service.go

+	bundle, err := s.ds.FetchBundle(ctx, s.td.IDString())
+	if err != nil {
+		return "", nil, err
+	}
+
+	for _, ca := range bundle.RootCas {
+		cert, err := x509.ParseCertificate(ca.DerBytes)
+		if err != nil {
+			return "", nil, err
+		}
+
+		subjectKeyID := x509util.SubjectKeyIDToString(cert.SubjectKeyId)
+		if authorityID == subjectKeyID {
+			return subjectKeyID, cert.PublicKey, nil
+		}
+	}


Should we be searching the bundle for these keys? I think one side effect of this is I can taint a key in the bundle that isn't managed locally ... Since it's the Local Authority API, kind of feels like you should be interacting with the server instance which owns the key? Or are there use cases that require this kind of thing?

searching be publickey was my initial idea too, as you said we may be able to taint a no local authority, I dont have a use case about that "yet",
So are you ok with getting authorityID and validate if it is used in the Old authority and reject if that is not the case? (we can have the Old one once we add the third Slot)

Yea that sounds good .. probably better to start with local only, and extend it if/when we have a good use case for that

evan2645 · 2023-06-13T18:24:08Z

pkg/server/datastore/sqlstore/sqlstore.go

+	}
+
+	if !taintedKeyFound {
+		return status.Error(codes.InvalidArgument, "it is not possible to revoke an untainted root CA")


Good check! I think it may be better to handle in local authority api though?

I was not sure about that, I'm ok in moving it to service layer, but revoking on datastore without this validation worry me

Ok .. it felt weird to me for datastore to be knowing about this kind of thing, but I think it's fine to leave as a defensive measure 👍

evan2645 · 2023-06-13T18:26:05Z

proto/spire/common/common.proto

-
-    /** Whether or not the key used to sign the certificate is tainted */
-    bool tainted_key = 2;


Why move this to the top level? Feels natural to have on a per-cert and per-key basis ... it remains set on PublicKey message

the issue here is that we does not have intermediate certificates on bundle when UpstreamAuthority is used,
So we keep "only" bundle here in that case, but intermediates lives on Journal,
So in order to taint a key in that case we must store the public key in some place

evan2645 · 2023-06-13T18:42:58Z

pkg/common/x509util/keyid.go

@@ -27,3 +28,21 @@ func GetSubjectKeyID(pubKey interface{}) ([]byte, error) {
 	keyID := sha1.Sum(subjectKeyInfo.SubjectPublicKey.Bytes) //nolint: gosec // usage of SHA1 is according to specification
 	return keyID[:], nil
 }
+
+// SubjectKeyIDToString parse Subject Key ID into string
+func SubjectKeyIDToString(ski []byte) string {


In #4216 @rturner3 landed on lowercase hex, no colon

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

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

…alized, and update authorityID format for x.509 local authorities Signed-off-by: Marcos Yacob <[email protected]>

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

evan2645

Left a couple more comments, I think this is good to go otherwise. Thanks again @MarcosDY 🙏

evan2645 · 2023-07-20T17:46:17Z

pkg/server/api/localauthority/v1/service.go

+	case req.AuthorityId == "":
+		return nil, api.MakeErr(log, codes.InvalidArgument, "no authority ID provided", nil)
+
+		// It is not possible to taint Active authority


non-blocking nit: indentation error

evan2645 · 2023-07-20T17:48:56Z

pkg/server/api/localauthority/v1/service.go

+	// Only next local authority can be tainted
+	case req.AuthorityId != nextSlot.AuthorityID():
+		return nil, api.MakeErr(log, codes.InvalidArgument, "unexpected authority ID", nil)


We only want to taint "old" authorities, right?

yeap, the next case validates that the next is not in prepared status (so only old ones can be tainted)

evan2645 · 2023-07-20T17:56:54Z

pkg/server/api/localauthority/v1/service.go

+	bundle, err := s.ds.FetchBundle(ctx, s.td.IDString())
+	if err != nil {
+		return "", nil, err
+	}
+
+	for _, ca := range bundle.RootCas {
+		cert, err := x509.ParseCertificate(ca.DerBytes)
+		if err != nil {
+			return "", nil, err
+		}
+
+		subjectKeyID := x509util.SubjectKeyIDToString(cert.SubjectKeyId)
+		if authorityID == subjectKeyID {
+			return subjectKeyID, cert.PublicKey, nil
+		}
+	}


Yea that sounds good .. probably better to start with local only, and extend it if/when we have a good use case for that

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

evan2645

Amazing work @MarcosDY thank you!! 🎸

…ority endpoints (spiffe#4020) * Create local authority scaffolding, and implements X.509 local authority service Signed-off-by: Marcos Yacob <[email protected]> Signed-off-by: Neniel <[email protected]>

MarcosDY commented Mar 22, 2023

View reviewed changes

guilhermocc reviewed Mar 23, 2023

View reviewed changes

rturner3 assigned evan2645 Mar 23, 2023

MarcosDY force-pushed the implement-localauthority-api branch from bab498c to 358989e Compare May 2, 2023 13:40

MarcosDY marked this pull request as ready for review May 2, 2023 18:02

MarcosDY requested review from evan2645, amartinezfayo, azdagron and rturner3 as code owners May 2, 2023 18:02

MarcosDY force-pushed the implement-localauthority-api branch from 93300c7 to 4d9e494 Compare June 13, 2023 17:41

evan2645 reviewed Jun 13, 2023

View reviewed changes

MarcosDY mentioned this pull request Jun 20, 2023

Introduce "previous" key slot #4271

Open

MarcosDY added 13 commits July 3, 2023 11:43

Create local authority scaffolding, and implements X.509 local author…

ee4e9a3

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

PR changes

54d9a73

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

Set public key on journal loading

3ae5520

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

Add log when tainting or revoking

06259d6

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

Upgrade spire api

c7c7463

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

Resolve unit tests

66dca75

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

Resolve rebase issues

9afa055

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

Push missing test

42e2ee2

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

Remove replace from go.mod

90ce3cb

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

Resolve issue on prepare

e44fb46

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

Resolve some naming

c4a388d

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

resolve bad valuer returned on slot.AuthorityID

b569590

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

solve prepare x509 authority

61c4a9a

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

MarcosDY added 5 commits July 3, 2023 11:51

Add missing log field

e255eb0

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

Refactor to allow to taint keys that are not in bundle

b1ee65a

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

Adapt changes to new main

a2e96bf

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

Prepare and get x.509 authorities fails when no current slot is initi…

6dafbff

…alized, and update authorityID format for x.509 local authorities Signed-off-by: Marcos Yacob <[email protected]>

PR changes

430cc5a

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

MarcosDY force-pushed the implement-localauthority-api branch from d59dc81 to 430cc5a Compare July 3, 2023 15:50

Solve lint errors

ea2bffc

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

MarcosDY requested a review from evan2645 July 17, 2023 14:54

evan2645 reviewed Jul 20, 2023

View reviewed changes

evan2645 added this to the 1.7.2 milestone Jul 20, 2023

PR cahnges

a7c4b29

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

evan2645 approved these changes Jul 21, 2023

View reviewed changes

Merge branch 'main' into implement-localauthority-api

727da9a

MarcosDY merged commit fcf921e into spiffe:main Jul 21, 2023

MarcosDY deleted the implement-localauthority-api branch July 21, 2023 20:50

		@@ -2,6 +2,8 @@ module github.com/spiffe/spire

		go 1.19

		replace github.com/spiffe/spire-api-sdk => github.com/MarcosDY/spire-api-sdk v1.0.0-pre.0.20230321150416-5642a0450459

	name: "slot has no public key",
	name: "current slot has no public key",

	// getX509PublicKey validates provided authority ID, and return OLD associated publick key, in case of authority ID is not provided, use the current OLD authority
	// getX509PublicKey validates provided authority ID, and return OLD associated public key, in case of authority ID is not provided, use the current OLD authority


		/** Whether or not the key used to sign the certificate is tainted */
		bool tainted_key = 2;

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

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

Conversation

MarcosDY commented Mar 22, 2023

Choose a reason for hiding this comment

guilhermocc Mar 23, 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

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

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

MarcosDY Jun 14, 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

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

evan2645 left a comment

Choose a reason for hiding this comment

guilhermocc Mar 23, 2023 •

edited

Loading

MarcosDY Jun 14, 2023 •

edited

Loading