diff --git a/pkg/common/coretypes/jwtkey/jwtkey.go b/pkg/common/coretypes/jwtkey/jwtkey.go index 1c4e943d083..fb960d0573f 100644 --- a/pkg/common/coretypes/jwtkey/jwtkey.go +++ b/pkg/common/coretypes/jwtkey/jwtkey.go @@ -15,7 +15,7 @@ type JWTKey struct { Tainted bool } -func toProtoFields(jwtKey JWTKey) (string, []byte, int64, bool, error) { +func toProtoFields(jwtKey JWTKey) (id string, publicKey []byte, expiresAt int64, tainted bool, err error) { if jwtKey.ID == "" { return "", nil, 0, false, errors.New("missing key ID for JWT key") } @@ -23,12 +23,11 @@ func toProtoFields(jwtKey JWTKey) (string, []byte, int64, bool, error) { if jwtKey.PublicKey == nil { return "", nil, 0, false, fmt.Errorf("missing public key for JWT key %q", jwtKey.ID) } - publicKey, err := x509.MarshalPKIXPublicKey(jwtKey.PublicKey) + publicKey, err = x509.MarshalPKIXPublicKey(jwtKey.PublicKey) if err != nil { return "", nil, 0, false, fmt.Errorf("failed to marshal public key for JWT key %q: %w", jwtKey.ID, err) } - var expiresAt int64 if !jwtKey.ExpiresAt.IsZero() { expiresAt = jwtKey.ExpiresAt.Unix() } diff --git a/pkg/common/telemetry/names.go b/pkg/common/telemetry/names.go index 866e9362a5a..4e40260ee3a 100644 --- a/pkg/common/telemetry/names.go +++ b/pkg/common/telemetry/names.go @@ -541,7 +541,7 @@ const ( // with other tags to add clarity Subject = "subject" - // Subject tags a certificate subject key ID + // SubjectKeyID tags a certificate subject key ID SubjectKeyID = "subject_key_id" // SVIDMapSize is the gauge key for the size of the LRU cache SVID map diff --git a/pkg/server/api/localauthority/v1/service.go b/pkg/server/api/localauthority/v1/service.go index a60e5faf406..97f8af987dc 100644 --- a/pkg/server/api/localauthority/v1/service.go +++ b/pkg/server/api/localauthority/v1/service.go @@ -461,7 +461,7 @@ func (s *Service) RevokeX509UpstreamAuthority(ctx context.Context, req *localaut } rpccontext.AuditRPC(ctx) - log.Info("X.509 upstream authority revoked successfully") + log.Info("X.509 upstream authority successfully revoked") return &localauthorityv1.RevokeX509UpstreamAuthorityResponse{}, nil } @@ -495,7 +495,7 @@ func (s *Service) validateUpstreamAuthoritySubjectKey(subjectKeyIDRequest string nextSlot := s.ca.GetNextX509CASlot() if subjectKeyIDRequest != nextSlot.UpstreamAuthorityID() { - return errors.New("upstream authority is not signing Old local authority") + return errors.New("upstream authority didn't sign the old local authority") } if nextSlot.Status() == journal.Status_PREPARED { diff --git a/pkg/server/api/localauthority/v1/service_test.go b/pkg/server/api/localauthority/v1/service_test.go index 91fb6029829..badce0565f9 100644 --- a/pkg/server/api/localauthority/v1/service_test.go +++ b/pkg/server/api/localauthority/v1/service_test.go @@ -1298,8 +1298,6 @@ func TestTaintX509Authority(t *testing.T) { nextCA, nextKey, err := testutil.SelfSign(template) require.NoError(t, err) - // nextPublicKeyRaw, err := x509.MarshalPKIXPublicKey(nextKey.Public()) - // require.NoError(t, err) nextKeySKI, err := x509util.GetSubjectKeyID(nextKey.Public()) require.NoError(t, err) nextAuthorityID := x509util.SubjectKeyIDToString(nextKeySKI) @@ -1572,7 +1570,7 @@ func TestTaintX509Authority(t *testing.T) { func TestTaintX509UpstreamAuthority(t *testing.T) { getUpstreamCertAndSubjectID := func(ca *testca.CA) (*x509.Certificate, string) { - // Selfsigned CA will return itself + // Self signed CA will return itself cert := ca.X509Authorities()[0] return cert, x509util.SubjectKeyIDToString(cert.SubjectKeyId) } @@ -1729,13 +1727,13 @@ func TestTaintX509UpstreamAuthority(t *testing.T) { nextSlot: createSlotWithUpstream(journal.Status_OLD, nextIntermediateCA, notAfterNext), subjectKeyIDToTaint: "invalidID", expectCode: codes.InvalidArgument, - expectMsg: "provided subject key id is not valid: upstream authority is not signing Old local authority", + expectMsg: "provided subject key id is not valid: upstream authority didn't sign the old local authority", expectLogs: []spiretest.LogEntry{ { Level: logrus.ErrorLevel, Message: "Invalid argument: provided subject key id is not valid", Data: logrus.Fields{ - logrus.ErrorKey: "upstream authority is not signing Old local authority", + logrus.ErrorKey: "upstream authority didn't sign the old local authority", telemetry.SubjectKeyID: "invalidID", }, }, @@ -1745,7 +1743,7 @@ func TestTaintX509UpstreamAuthority(t *testing.T) { Data: logrus.Fields{ telemetry.Status: "error", telemetry.StatusCode: "InvalidArgument", - telemetry.StatusMessage: "provided subject key id is not valid: upstream authority is not signing Old local authority", + telemetry.StatusMessage: "provided subject key id is not valid: upstream authority didn't sign the old local authority", telemetry.Type: "audit", telemetry.SubjectKeyID: "invalidID", }, @@ -2120,7 +2118,7 @@ func TestRevokeX509Authority(t *testing.T) { func TestRevokeX509UpstreamAuthority(t *testing.T) { getUpstreamCertAndSubjectID := func(ca *testca.CA) (*x509.Certificate, string) { - // Selfsigned CA will return itself + // Self signed CA will return itself cert := ca.X509Authorities()[0] return cert, x509util.SubjectKeyIDToString(cert.SubjectKeyId) } @@ -2171,7 +2169,7 @@ func TestRevokeX509UpstreamAuthority(t *testing.T) { }, { Level: logrus.InfoLevel, - Message: "X.509 upstream authority revoked successfully", + Message: "X.509 upstream authority successfully revoked", Data: logrus.Fields{ telemetry.SubjectKeyID: deactivatedUpstreamAuthorityID, }, @@ -2268,13 +2266,13 @@ func TestRevokeX509UpstreamAuthority(t *testing.T) { nextSlot: createSlotWithUpstream(journal.Status_OLD, nextIntermediateCA, notAfterNext), subjectKeyIDToRevoke: "invalidID", expectCode: codes.InvalidArgument, - expectMsg: "invalid subject key ID: upstream authority is not signing Old local authority", + expectMsg: "invalid subject key ID: upstream authority didn't sign the old local authority", expectLogs: []spiretest.LogEntry{ { Level: logrus.ErrorLevel, Message: "Invalid argument: invalid subject key ID", Data: logrus.Fields{ - logrus.ErrorKey: "upstream authority is not signing Old local authority", + logrus.ErrorKey: "upstream authority didn't sign the old local authority", telemetry.SubjectKeyID: "invalidID", }, }, @@ -2284,7 +2282,7 @@ func TestRevokeX509UpstreamAuthority(t *testing.T) { Data: logrus.Fields{ telemetry.Status: "error", telemetry.StatusCode: "InvalidArgument", - telemetry.StatusMessage: "invalid subject key ID: upstream authority is not signing Old local authority", + telemetry.StatusMessage: "invalid subject key ID: upstream authority didn't sign the old local authority", telemetry.Type: "audit", telemetry.SubjectKeyID: "invalidID", }, diff --git a/pkg/server/ca/manager/manager_test.go b/pkg/server/ca/manager/manager_test.go index 9d627ddf3db..cc249d147d3 100644 --- a/pkg/server/ca/manager/manager_test.go +++ b/pkg/server/ca/manager/manager_test.go @@ -303,12 +303,12 @@ func TestUpstreamSigned(t *testing.T) { err := fakeUA.TaintAuthority(0) require.NoError(t, err) - // Get root again an verify firt item is tainted + // Get the roots again and verify that the first X.509 authority is tainted x509Roots := fakeUA.X509Roots() require.True(t, x509Roots[0].Tainted) commonCertificates := x509certificate.RequireToCommonProtos(x509Roots) - // Retry until tainted is propagated to database + // Retry until the Tainted attribute is propagated to the database require.Eventually(t, func() bool { bundle := test.fetchBundle(ctx) return spiretest.AssertProtoListEqual(t, commonCertificates, bundle.RootCas) diff --git a/pkg/server/ca/manager/slot.go b/pkg/server/ca/manager/slot.go index 69139871d24..d47a0c9b393 100644 --- a/pkg/server/ca/manager/slot.go +++ b/pkg/server/ca/manager/slot.go @@ -42,7 +42,7 @@ type Slot interface { Status() journal.Status UpstreamAuthorityID() string AuthorityID() string - // TODO: Should we remove public key? + // TODO: This will be removed as part of #5390 PublicKey() crypto.PublicKey NotAfter() time.Time } diff --git a/pkg/server/ca/upstream_client.go b/pkg/server/ca/upstream_client.go index fe2414b3893..c4bd64f6319 100644 --- a/pkg/server/ca/upstream_client.go +++ b/pkg/server/ca/upstream_client.go @@ -140,7 +140,7 @@ func (u *UpstreamClient) runMintX509CAStream(ctx context.Context, csr []byte, tt } defer x509RootsStream.Close() - // Extract all roots certificates + // Extract all root certificates var x509RootCerts []*x509.Certificate for _, eachRoot := range x509Roots { x509RootCerts = append(x509RootCerts, eachRoot.Certificate) diff --git a/pkg/server/datastore/sqlstore/sqlstore.go b/pkg/server/datastore/sqlstore/sqlstore.go index f11ba02033f..5cc1fa4ef17 100644 --- a/pkg/server/datastore/sqlstore/sqlstore.go +++ b/pkg/server/datastore/sqlstore/sqlstore.go @@ -1398,9 +1398,11 @@ func taintX509CA(tx *gorm.DB, trustDomainID string, subjectKeyIDToTaint string) } if !found { - return status.Errorf(codes.NotFound, "no ca found with provided subject key ID") + return status.Error(codes.NotFound, "no ca found with provided subject key ID") } + bundle.SequenceNumber++ + _, err = updateBundle(tx, bundle, nil) if err != nil { return err @@ -1440,6 +1442,7 @@ func revokeX509CA(tx *gorm.DB, trustDomainID string, subjectKeyIDToRevoke string } bundle.RootCas = rootCAs + bundle.SequenceNumber++ if _, err := updateBundle(tx, bundle, nil); err != nil { return status.Errorf(codes.Internal, "failed to update bundle: %v", err) @@ -1478,6 +1481,7 @@ func taintJWTKey(tx *gorm.DB, trustDomainID string, authorityID string) (*common return nil, status.Error(codes.NotFound, "no JWT Key found with provided key ID") } + bundle.SequenceNumber++ if _, err := updateBundle(tx, bundle, nil); err != nil { return nil, err } @@ -1517,6 +1521,7 @@ func revokeJWTKey(tx *gorm.DB, trustDomainID string, authorityID string) (*commo return nil, status.Error(codes.NotFound, "no JWT Key found with provided key ID") } + bundle.SequenceNumber++ if _, err := updateBundle(tx, bundle, nil); err != nil { return nil, err } diff --git a/pkg/server/datastore/sqlstore/sqlstore_test.go b/pkg/server/datastore/sqlstore/sqlstore_test.go index 138bf2492d3..1a831e317d5 100644 --- a/pkg/server/datastore/sqlstore/sqlstore_test.go +++ b/pkg/server/datastore/sqlstore/sqlstore_test.go @@ -660,6 +660,18 @@ func (s *PluginSuite) TestTaintX509CA() { spiretest.RequireGRPCStatus(t, err, codes.Internal, "failed to parse rootCA: x509: malformed certificate") }) + validateBundle := func(expectSequenceNumber uint64) { + expectedRootCAs := []*common.Certificate{ + {DerBytes: s.cert.Raw, TaintedKey: true}, + {DerBytes: s.cacert.Raw}, + } + + fetchedBundle, err := s.ds.FetchBundle(ctx, "spiffe://foo") + require.NoError(t, err) + require.Equal(t, expectedRootCAs, fetchedBundle.RootCas) + require.Equal(t, expectSequenceNumber, fetchedBundle.SequenceNumber) + } + // Update bundle bundle = bundleutil.BundleProtoFromRootCAs("spiffe://foo", []*x509.Certificate{s.cert, s.cacert}) _, err = s.ds.UpdateBundle(ctx, bundle, nil) @@ -669,27 +681,25 @@ func (s *PluginSuite) TestTaintX509CA() { err := s.ds.TaintX509CA(ctx, "spiffe://foo", skID) require.NoError(t, err) - fetchedBundle, err := s.ds.FetchBundle(ctx, "spiffe://foo") - require.NoError(t, err) - - expectedRootCAs := []*common.Certificate{ - {DerBytes: s.cert.Raw, TaintedKey: true}, - {DerBytes: s.cacert.Raw}, - } - - require.Equal(t, expectedRootCAs, fetchedBundle.RootCas) + validateBundle(1) }) t.Run("no bundle with provided skID", func(t *testing.T) { // Not able to taint a tainted CA err := s.ds.TaintX509CA(ctx, "spiffe://foo", "foo") spiretest.RequireGRPCStatus(t, err, codes.NotFound, "no ca found with provided subject key ID") + + // Validate than sequence number is not incremented + validateBundle(1) }) t.Run("failed to taint already tainted ca", func(t *testing.T) { // Not able to taint a tainted CA err := s.ds.TaintX509CA(ctx, "spiffe://foo", skID) spiretest.RequireGRPCStatus(t, err, codes.InvalidArgument, "root CA is already tainted") + + // Validate than sequence number is not incremented + validateBundle(1) }) } @@ -715,7 +725,6 @@ func (s *PluginSuite) TestRevokeX509CA() { _, err := s.ds.CreateBundle(ctx, bundle) require.NoError(t, err) - // t.Run("Bundle contains a malformed certificate", func(t *testing.T) { err := s.ds.RevokeX509CA(ctx, "spiffe://foo", "foo") spiretest.RequireGRPCStatusHasPrefix(t, err, codes.Internal, "failed to parse root CA: x509: malformed certificate") @@ -726,14 +735,30 @@ func (s *PluginSuite) TestRevokeX509CA() { _, err = s.ds.UpdateBundle(ctx, bundle, nil) require.NoError(t, err) + originalBundles := []*common.Certificate{ + {DerBytes: s.cert.Raw}, + {DerBytes: s.cacert.Raw}, + } + + validateBundle := func(expectedRootCAs []*common.Certificate, expectSequenceNumber uint64) { + fetchedBundle, err := s.ds.FetchBundle(ctx, "spiffe://foo") + require.NoError(t, err) + require.Equal(t, expectedRootCAs, fetchedBundle.RootCas) + require.Equal(t, expectSequenceNumber, fetchedBundle.SequenceNumber) + } + t.Run("No root CA is using provided skID", func(t *testing.T) { err := s.ds.RevokeX509CA(ctx, "spiffe://foo", "foo") spiretest.RequireGRPCStatus(t, err, codes.NotFound, "no root CA found with provided subject key ID") + + validateBundle(originalBundles, 0) }) t.Run("Unable to revoke untainted bundles", func(t *testing.T) { err := s.ds.RevokeX509CA(ctx, "spiffe://foo", certID) spiretest.RequireGRPCStatus(t, err, codes.InvalidArgument, "it is not possible to revoke an untainted root CA") + + validateBundle(originalBundles, 0) }) // Mark cert as tainted @@ -741,16 +766,22 @@ func (s *PluginSuite) TestRevokeX509CA() { require.NoError(t, err) t.Run("Revoke successfully", func(t *testing.T) { - err = s.ds.RevokeX509CA(ctx, "spiffe://foo", certID) - require.NoError(t, err) + taintedBundles := []*common.Certificate{ + {DerBytes: s.cert.Raw, TaintedKey: true}, + {DerBytes: s.cacert.Raw}, + } + // Validating precondition, with 2 bundles and sequence + validateBundle(taintedBundles, 1) - fetchedBundle, err := s.ds.FetchBundle(ctx, "spiffe://foo") + // Revoke + err = s.ds.RevokeX509CA(ctx, "spiffe://foo", certID) require.NoError(t, err) + // CA is removed and sequence incremented expectedRootCAs := []*common.Certificate{ {DerBytes: s.cacert.Raw}, } - require.Equal(t, expectedRootCAs, fetchedBundle.RootCas) + validateBundle(expectedRootCAs, 2) }) } @@ -759,11 +790,12 @@ func (s *PluginSuite) TestTaintJWTKey() { // Setup // Create new bundle with two JWT Keys bundle := bundleutil.BundleProtoFromRootCAs("spiffe://foo", nil) - bundle.JwtSigningKeys = []*common.PublicKey{ + originalKeys := []*common.PublicKey{ {Kid: "key1"}, {Kid: "key2"}, {Kid: "key2"}, } + bundle.JwtSigningKeys = originalKeys // Bundle not found publicKey, err := s.ds.TaintJWTKey(ctx, "spiffe://foo", "key1") @@ -783,25 +815,37 @@ func (s *PluginSuite) TestTaintJWTKey() { spiretest.RequireGRPCStatus(t, err, codes.NotFound, "no JWT Key found with provided key ID") require.Nil(t, publicKey) + validateBundle := func(expectedKeys []*common.PublicKey, expectSequenceNumber uint64) { + fetchedBundle, err := s.ds.FetchBundle(ctx, "spiffe://foo") + require.NoError(t, err) + + spiretest.RequireProtoListEqual(t, expectedKeys, fetchedBundle.JwtSigningKeys) + require.Equal(t, expectSequenceNumber, fetchedBundle.SequenceNumber) + } + + // Validate no changes + validateBundle(originalKeys, 0) + // Taint successfully publicKey, err = s.ds.TaintJWTKey(ctx, "spiffe://foo", "key1") require.NoError(t, err) require.NotNil(t, publicKey) - fetchedBundle, err := s.ds.FetchBundle(ctx, "spiffe://foo") - require.NoError(t, err) - - expectedKeys := []*common.PublicKey{ + taintedKey := []*common.PublicKey{ {Kid: "key1", TaintedKey: true}, {Kid: "key2"}, {Kid: "key2"}, } - require.Equal(t, expectedKeys, fetchedBundle.JwtSigningKeys) + // Validate expected response + validateBundle(taintedKey, 1) // No able to taint Key again publicKey, err = s.ds.TaintJWTKey(ctx, "spiffe://foo", "key1") spiretest.RequireGRPCStatus(t, err, codes.InvalidArgument, "key is already tainted") require.Nil(t, publicKey) + + // No changes + validateBundle(taintedKey, 1) } func (s *PluginSuite) TestRevokeJWTKey() { @@ -847,23 +891,31 @@ func (s *PluginSuite) TestRevokeJWTKey() { require.Nil(t, publicKey) // Remove duplicated key - bundle.JwtSigningKeys = []*common.PublicKey{ + originalKeys := []*common.PublicKey{ {Kid: "key1"}, {Kid: "key2", TaintedKey: true}, } + bundle.JwtSigningKeys = originalKeys _, err = s.ds.UpdateBundle(ctx, bundle, nil) require.NoError(t, err) + validateBundle := func(expectedKeys []*common.PublicKey, expectSequenceNumber uint64) { + fetchedBundle, err := s.ds.FetchBundle(ctx, "spiffe://foo") + require.NoError(t, err) + + spiretest.RequireProtoListEqual(t, expectedKeys, fetchedBundle.JwtSigningKeys) + require.Equal(t, expectSequenceNumber, fetchedBundle.SequenceNumber) + } + + validateBundle(originalKeys, 0) + // Revoke successfully publicKey, err = s.ds.RevokeJWTKey(ctx, "spiffe://foo", "key2") require.NoError(t, err) require.Equal(t, &common.PublicKey{Kid: "key2", TaintedKey: true}, publicKey) - fetchedBundle, err := s.ds.FetchBundle(ctx, "spiffe://foo") - require.NoError(t, err) - expectedJWTKeys := []*common.PublicKey{{Kid: "key1"}} - require.Equal(t, expectedJWTKeys, fetchedBundle.JwtSigningKeys) + validateBundle(expectedJWTKeys, 1) } func (s *PluginSuite) TestCreateAttestedNode() {