Add taint upstream authority #5340

MarcosDY · 2024-07-30T15:02:18Z

Pull Request check list

Commit conforms to CONTRIBUTING.md?
Proper tests/regressions included?
Documentation updated?

Affected functionality
no actual functionalities are affected

Description of change
To taint a local authority that is using an upstream authority, we need a way to taint the upstream authority itself and rotate intermediates + SVIDs using that tainted upstream authority.
This PR aims to add a new RPC that allows tainting an upstream authority using SKID as the identifier, update the datastore with that information, and propagate it to downstream servers.
This PR is not forcing rotation but only propagating tainted authorities. Forcing rotations will happen in other PRs.

Changes to the SDK can be found here.

Which issue this PR fixes
fixes #3899, #3893, #3900

amartinezfayo · 2024-08-08T20:38:14Z

pkg/common/coretypes/jwtkey/jwtkey.go

 }

-func toProtoFields(jwtKey JWTKey) (string, []byte, int64, error) {
+func toProtoFields(jwtKey JWTKey) (string, []byte, int64, bool, error) {


It's becoming more difficult to guess what each return value means in this function. I think that it would be good to have named return values as an easy way to document the meaning of each of them.

amartinezfayo · 2024-08-08T20:43:30Z

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

 	if req.AuthorityId != "" {
 		log = log.WithField(telemetry.LocalAuthorityID, req.AuthorityId)
 	}

+	if s.ca.IsUpstreamAuthority() {
+		return nil, api.MakeErr(log, codes.FailedPrecondition, "local authority can't be tainted if there is an upstream authorit", nil)


Suggested change

return nil, api.MakeErr(log, codes.FailedPrecondition, "local authority can't be tainted if there is an upstream authorit", nil)

return nil, api.MakeErr(log, codes.FailedPrecondition, "local authority can't be tainted if there is an upstream authority", nil)

amartinezfayo · 2024-08-12T23:53:11Z

pkg/common/telemetry/names.go

@@ -541,6 +541,9 @@ const (
 	// with other tags to add clarity
 	Subject = "subject"

+	// Subject tags a certificate subject key ID


Suggested change

// Subject tags a certificate subject key ID

// SubjectKeyID tags a certificate subject key ID

amartinezfayo · 2024-08-12T23:58:51Z

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

+	}
+
+	rpccontext.AuditRPC(ctx)
+	log.Info("X.509 upstream authority revoked successfully")


Suggested change

log.Info("X.509 upstream authority revoked successfully")

log.Info("X.509 upstream authority successfully revoked")

amartinezfayo · 2024-08-13T00:03:42Z

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

-		return "", nil, err
+	nextSlot := s.ca.GetNextX509CASlot()
+	if subjectKeyIDRequest != nextSlot.UpstreamAuthorityID() {
+		return errors.New("upstream authority is not signing Old local authority")


Suggested change

return errors.New("upstream authority is not signing Old local authority")

return errors.New("upstream authority didn't sign the old local authority")

amartinezfayo · 2024-08-13T01:10:02Z

pkg/server/datastore/sqlstore/sqlstore.go

-	bundle.X509TaintedKeys = append(bundle.X509TaintedKeys, &common.X509TaintedKey{PublicKey: pKey})
+	if !found {
+		return status.Errorf(codes.NotFound, "no ca found with provided subject key ID")
+	}

 	_, err = updateBundle(tx, bundle, nil)


I think that we should increment bundle.SequenceNumber before calling updateBundle, otherwise SequenceNumber will remain the same.

amartinezfayo · 2024-08-13T01:10:30Z

pkg/server/datastore/sqlstore/sqlstore.go

+	if !keyFound {
+		return status.Error(codes.NotFound, "no root CA found with provided subject key ID")
+	}
+
 	bundle.RootCas = rootCAs

 	if _, err := updateBundle(tx, bundle, nil); err != nil {


I think that we should increment bundle.SequenceNumber before calling updateBundle, otherwise SequenceNumber will remain the same.

amartinezfayo · 2024-08-13T01:12:24Z

pkg/server/datastore/sqlstore/sqlstore_test.go

-	// No able to revoke untainted bundles
-	err = s.ds.RevokeX509CA(ctx, "spiffe://foo", s.cert.PublicKey)
-	spiretest.RequireGRPCStatus(t, err, codes.InvalidArgument, "it is not possible to revoke an untainted root CA")
+	t.Run("Unable to revoke untainted bundles", func(t *testing.T) {


It would be good to check that the sequence number of the bundle remains the same for those cases that fail.

amartinezfayo · 2024-08-13T01:13:26Z

pkg/server/datastore/sqlstore/sqlstore_test.go

-		{PublicKey: certPublicKeyRaw},
-	}
-	require.Equal(t, expectedTaintedKeys, fetchedBundle.X509TaintedKeys)
+	t.Run("no bundle with provided skID", func(t *testing.T) {


It would be good to check that the sequence number of the bundle remains the same for those cases that fail.

amartinezfayo · 2024-08-13T01:14:43Z

pkg/server/datastore/sqlstore/sqlstore_test.go

-
-	fetchedBundle, err := s.ds.FetchBundle(ctx, "spiffe://foo")
-	require.NoError(t, err)
+	t.Run("Revoke successfully", func(t *testing.T) {


It would be good to check that the sequence number of the bundle is incremented on successful operations.

amartinezfayo · 2024-08-16T12:12:12Z

It's looking great! I think that we are missing propagating the tainted bit in some places:

amartinezfayo · 2024-08-16T12:29:02Z

TestCommonBundleFromProto should capture the proper propagation of the tainted bit.
It doesn't seem like we have explicit tests for ParseX509Authorities and ParseJWTAuthorities?
We should make sure that propagation of the tainted bit is covered by tests in all those functions. We can handle that in a separate PR if that's more convenient.

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

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

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

MarcosDY · 2024-08-16T14:06:37Z

we discuss to add that propagation as part of #5394 to reduce changes to apply in this PR and it is not required for current PR functionality

amartinezfayo

🎉 🚀

MarcosDY force-pushed the poc-taint-upstream-authority branch 2 times, most recently from 7b19089 to a429675 Compare August 6, 2024 13:30

azdagron assigned amartinezfayo Aug 8, 2024

MarcosDY changed the title ~~POC to add taint upstream authority~~ Add taint upstream authority Aug 12, 2024

MarcosDY marked this pull request as ready for review August 12, 2024 15:07

MarcosDY requested review from evan2645, amartinezfayo, azdagron and rturner3 as code owners August 12, 2024 15:07

amartinezfayo reviewed Aug 13, 2024

View reviewed changes

MarcosDY added 16 commits August 16, 2024 10:57

POC to add taint upstream authority

ce58a48

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

Propagate taining and revoke into downstream server, updating upstream

1430e95

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

start working in unit tests for 'common/coretypes/jwtkey'

499c8e8

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

refactor x509certificate package

3579707

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

Add update test in coretypes bundle

1684d07

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

Add more tests for api bundle

f03306e

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

Add tests for local authority service

a2d6a0f

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

more test

cb7091b

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

more tests

23ccc55

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

more

2e62a3c

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

more test

9897cf4

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

resolve some lints

78b6454

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

more

2cc9007

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

upgrade spire-api-sdk, and resolve lint

1661148

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

Resolve lint...

837048f

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

PR changes

ed3ef5a

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

MarcosDY force-pushed the poc-taint-upstream-authority branch from ae61b82 to ed3ef5a Compare August 16, 2024 14:05

amartinezfayo approved these changes Aug 16, 2024

View reviewed changes

amartinezfayo merged commit e3dac17 into spiffe:main Aug 16, 2024
34 checks passed

amartinezfayo added this to the 1.10.2 milestone Aug 16, 2024

MarcosDY mentioned this pull request Aug 16, 2024

Propagate Taint keys on SPIRE upstream authority #3900

Closed

MarcosDY linked an issue Aug 16, 2024 that may be closed by this pull request

Propagate Taint keys on SPIRE upstream authority #3900

Closed

MarcosDY mentioned this pull request Aug 16, 2024

Add taint to common Bundle to utils #3893

Closed

amartinezfayo mentioned this pull request Sep 2, 2024

Introduce EJBCA UpstreamAuthority plugin for SPIRE Server #5378

Merged

3 tasks

hiyosi mentioned this pull request Sep 9, 2024

Taint fields of JWTKey and X509Certificate are not exposed in the spire-plugin-sdk. #5484

Closed

MarcosDY deleted the poc-taint-upstream-authority branch October 10, 2024 13:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add taint upstream authority #5340

Add taint upstream authority #5340

MarcosDY commented Jul 30, 2024 •

edited

Loading

amartinezfayo Aug 8, 2024

amartinezfayo Aug 8, 2024

amartinezfayo Aug 12, 2024

amartinezfayo Aug 12, 2024

amartinezfayo Aug 13, 2024

amartinezfayo Aug 13, 2024

amartinezfayo Aug 13, 2024

amartinezfayo Aug 13, 2024

amartinezfayo Aug 13, 2024

amartinezfayo Aug 13, 2024

amartinezfayo commented Aug 16, 2024

amartinezfayo commented Aug 16, 2024

MarcosDY commented Aug 16, 2024

amartinezfayo left a comment

	return nil, api.MakeErr(log, codes.FailedPrecondition, "local authority can't be tainted if there is an upstream authorit", nil)
	return nil, api.MakeErr(log, codes.FailedPrecondition, "local authority can't be tainted if there is an upstream authority", nil)

	// Subject tags a certificate subject key ID
	// SubjectKeyID tags a certificate subject key ID

	log.Info("X.509 upstream authority revoked successfully")
	log.Info("X.509 upstream authority successfully revoked")

	return errors.New("upstream authority is not signing Old local authority")
	return errors.New("upstream authority didn't sign the old local authority")

Add taint upstream authority #5340

Add taint upstream authority #5340

Conversation

MarcosDY commented Jul 30, 2024 • 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

amartinezfayo commented Aug 16, 2024

amartinezfayo commented Aug 16, 2024

MarcosDY commented Aug 16, 2024

amartinezfayo left a comment

Choose a reason for hiding this comment

MarcosDY commented Jul 30, 2024 •

edited

Loading