From d7d1ccde86989f10606793531fa7caea804d94e4 Mon Sep 17 00:00:00 2001 From: Sorin Dumitru Date: Thu, 17 Oct 2024 21:21:52 +0100 Subject: [PATCH] Return InvalidArgument for invalid input entries (#5506) fixes #5444 Signed-off-by: Sorin Dumitru --- pkg/server/api/entry/v1/service.go | 12 +++++- pkg/server/api/entry/v1/service_test.go | 34 +++++++-------- pkg/server/datastore/sqlstore/sqlstore.go | 42 ++++++++++--------- .../datastore/sqlstore/sqlstore_test.go | 18 ++++---- 4 files changed, 59 insertions(+), 47 deletions(-) diff --git a/pkg/server/api/entry/v1/service.go b/pkg/server/api/entry/v1/service.go index 180d66c241..7cfeadcbfa 100644 --- a/pkg/server/api/entry/v1/service.go +++ b/pkg/server/api/entry/v1/service.go @@ -291,8 +291,12 @@ func (s *Service) createEntry(ctx context.Context, e *types.Entry, outputMask *t regEntry, existing, err := s.ds.CreateOrReturnRegistrationEntry(ctx, cEntry) switch { case err != nil: + statusCode := status.Code(err) + if statusCode == codes.Unknown { + statusCode = codes.Internal + } return &entryv1.BatchCreateEntryResponse_Result{ - Status: api.MakeStatus(log, codes.Internal, "failed to create entry", err), + Status: api.MakeStatus(log, statusCode, "failed to create entry", err), } case existing: resultStatus = api.CreateStatus(codes.AlreadyExists, "similar entry already exists") @@ -660,8 +664,12 @@ func (s *Service) updateEntry(ctx context.Context, e *types.Entry, inputMask *ty } dsEntry, err := s.ds.UpdateRegistrationEntry(ctx, convEntry, mask) if err != nil { + statusCode := status.Code(err) + if statusCode == codes.Unknown { + statusCode = codes.Internal + } return &entryv1.BatchUpdateEntryResponse_Result{ - Status: api.MakeStatus(log, codes.Internal, "failed to update entry", err), + Status: api.MakeStatus(log, statusCode, "failed to update entry", err), } } diff --git a/pkg/server/api/entry/v1/service_test.go b/pkg/server/api/entry/v1/service_test.go index 429c01ec60..3f98469bf1 100644 --- a/pkg/server/api/entry/v1/service_test.go +++ b/pkg/server/api/entry/v1/service_test.go @@ -2185,23 +2185,23 @@ func TestBatchCreateEntry(t *testing.T) { expectResults: []*entryv1.BatchCreateEntryResponse_Result{ { Status: &types.Status{ - Code: int32(codes.Internal), - Message: "failed to create entry: datastore-sql: invalid registration entry: entry ID contains invalid characters", + Code: int32(codes.InvalidArgument), + Message: "failed to create entry: datastore-validation: invalid registration entry: entry ID contains invalid characters", }, }, { Status: &types.Status{ - Code: int32(codes.Internal), - Message: "failed to create entry: datastore-sql: invalid registration entry: entry ID too long", + Code: int32(codes.InvalidArgument), + Message: "failed to create entry: datastore-validation: invalid registration entry: entry ID too long", }, }, }, expectLogs: []spiretest.LogEntry{ { Level: logrus.ErrorLevel, - Message: "Failed to create entry", + Message: "Invalid argument: failed to create entry", Data: logrus.Fields{ - logrus.ErrorKey: "datastore-sql: invalid registration entry: entry ID contains invalid characters", + logrus.ErrorKey: "rpc error: code = InvalidArgument desc = datastore-validation: invalid registration entry: entry ID contains invalid characters", telemetry.SPIFFEID: "spiffe://example.org/bar", }, }, @@ -2224,15 +2224,15 @@ func TestBatchCreateEntry(t *testing.T) { telemetry.Hint: "", telemetry.CreatedAt: "0", telemetry.StoreSvid: "false", - telemetry.StatusCode: "Internal", - telemetry.StatusMessage: "failed to create entry: datastore-sql: invalid registration entry: entry ID contains invalid characters", + telemetry.StatusCode: "InvalidArgument", + telemetry.StatusMessage: "failed to create entry: datastore-validation: invalid registration entry: entry ID contains invalid characters", }, }, { Level: logrus.ErrorLevel, - Message: "Failed to create entry", + Message: "Invalid argument: failed to create entry", Data: logrus.Fields{ - logrus.ErrorKey: "datastore-sql: invalid registration entry: entry ID too long", + logrus.ErrorKey: "rpc error: code = InvalidArgument desc = datastore-validation: invalid registration entry: entry ID too long", telemetry.SPIFFEID: "spiffe://example.org/bar", }, }, @@ -2255,8 +2255,8 @@ func TestBatchCreateEntry(t *testing.T) { telemetry.Hint: "", telemetry.CreatedAt: "0", telemetry.StoreSvid: "false", - telemetry.StatusCode: "Internal", - telemetry.StatusMessage: "failed to create entry: datastore-sql: invalid registration entry: entry ID too long", + telemetry.StatusCode: "InvalidArgument", + telemetry.StatusMessage: "failed to create entry: datastore-validation: invalid registration entry: entry ID too long", }, }, }, @@ -4130,17 +4130,17 @@ func TestBatchUpdateEntry(t *testing.T) { }, expectResults: []*entryv1.BatchUpdateEntryResponse_Result{ { - Status: &types.Status{Code: int32(codes.Internal), Message: "failed to update entry: datastore-sql: invalid registration entry: selector types must be the same when store SVID is enabled"}, + Status: &types.Status{Code: int32(codes.InvalidArgument), Message: "failed to update entry: datastore-validation: invalid registration entry: selector types must be the same when store SVID is enabled"}, }, }, expectLogs: func(m map[string]string) []spiretest.LogEntry { return []spiretest.LogEntry{ { Level: logrus.ErrorLevel, - Message: "Failed to update entry", + Message: "Invalid argument: failed to update entry", Data: logrus.Fields{ telemetry.RegistrationID: m[entry1SpiffeID.Path], - logrus.ErrorKey: "rpc error: code = Unknown desc = datastore-sql: invalid registration entry: selector types must be the same when store SVID is enabled", + logrus.ErrorKey: "rpc error: code = InvalidArgument desc = datastore-validation: invalid registration entry: selector types must be the same when store SVID is enabled", }, }, { @@ -4148,8 +4148,8 @@ func TestBatchUpdateEntry(t *testing.T) { Message: "API accessed", Data: logrus.Fields{ telemetry.Status: "error", - telemetry.StatusCode: "Internal", - telemetry.StatusMessage: "failed to update entry: datastore-sql: invalid registration entry: selector types must be the same when store SVID is enabled", + telemetry.StatusCode: "InvalidArgument", + telemetry.StatusMessage: "failed to update entry: datastore-validation: invalid registration entry: selector types must be the same when store SVID is enabled", telemetry.Type: "audit", telemetry.RegistrationID: m[entry1SpiffeID.Path], telemetry.Selectors: "type1:key1:value,type2:key2:value", diff --git a/pkg/server/datastore/sqlstore/sqlstore.go b/pkg/server/datastore/sqlstore/sqlstore.go index 21ebeda1cb..cda5262b33 100644 --- a/pkg/server/datastore/sqlstore/sqlstore.go +++ b/pkg/server/datastore/sqlstore/sqlstore.go @@ -38,6 +38,7 @@ import ( var ( sqlError = errs.Class("datastore-sql") + validationError = errs.Class("datastore-validation") validEntryIDChars = &unicode.RangeTable{ R16: []unicode.Range16{ {0x002d, 0x002e, 1}, // - | . @@ -473,12 +474,11 @@ func (ds *Plugin) CreateOrReturnRegistrationEntry(ctx context.Context, func (ds *Plugin) createOrReturnRegistrationEntry(ctx context.Context, entry *common.RegistrationEntry, ) (registrationEntry *common.RegistrationEntry, existing bool, err error) { - // TODO: Validations should be done in the ProtoBuf level [https://github.com/spiffe/spire/issues/44] - if err = validateRegistrationEntry(entry); err != nil { - return nil, false, err - } - if err = ds.withWriteTx(ctx, func(tx *gorm.DB) (err error) { + if err = validateRegistrationEntry(entry); err != nil { + return err + } + registrationEntry, err = lookupSimilarEntry(ctx, ds.db, tx, entry) if err != nil { return err @@ -1015,6 +1015,10 @@ func (ds *Plugin) gormToGRPCStatus(err error) error { } code := codes.Unknown + if validationError.Has(err) { + code = codes.InvalidArgument + } + switch { case gorm.IsRecordNotFoundError(unwrapped): code = codes.NotFound @@ -3911,7 +3915,7 @@ func updateRegistrationEntry(tx *gorm.DB, e *common.RegistrationEntry, mask *com // Verify that final selectors contains the same 'type' when entry is used for store SVIDs if entry.StoreSvid && !equalSelectorTypes(entry.Selectors) { - return nil, sqlError.New("invalid registration entry: selector types must be the same when store SVID is enabled") + return nil, validationError.New("invalid registration entry: selector types must be the same when store SVID is enabled") } if mask == nil || mask.DnsNames { @@ -4398,11 +4402,11 @@ func modelToBundle(model *Bundle) (*common.Bundle, error) { func validateRegistrationEntry(entry *common.RegistrationEntry) error { if entry == nil { - return sqlError.New("invalid request: missing registered entry") + return validationError.New("invalid request: missing registered entry") } if len(entry.Selectors) == 0 { - return sqlError.New("invalid registration entry: missing selector list") + return validationError.New("invalid registration entry: missing selector list") } // In case of StoreSvid is set, all entries 'must' be the same type, @@ -4413,31 +4417,31 @@ func validateRegistrationEntry(entry *common.RegistrationEntry) error { tpe := entry.Selectors[0].Type for _, t := range entry.Selectors { if tpe != t.Type { - return sqlError.New("invalid registration entry: selector types must be the same when store SVID is enabled") + return validationError.New("invalid registration entry: selector types must be the same when store SVID is enabled") } } } if len(entry.EntryId) > 255 { - return sqlError.New("invalid registration entry: entry ID too long") + return validationError.New("invalid registration entry: entry ID too long") } for _, e := range entry.EntryId { if !unicode.In(e, validEntryIDChars) { - return sqlError.New("invalid registration entry: entry ID contains invalid characters") + return validationError.New("invalid registration entry: entry ID contains invalid characters") } } if len(entry.SpiffeId) == 0 { - return sqlError.New("invalid registration entry: missing SPIFFE ID") + return validationError.New("invalid registration entry: missing SPIFFE ID") } if entry.X509SvidTtl < 0 { - return sqlError.New("invalid registration entry: X509SvidTtl is not set") + return validationError.New("invalid registration entry: X509SvidTtl is not set") } if entry.JwtSvidTtl < 0 { - return sqlError.New("invalid registration entry: JwtSvidTtl is not set") + return validationError.New("invalid registration entry: JwtSvidTtl is not set") } return nil @@ -4459,26 +4463,26 @@ func equalSelectorTypes(selectors []Selector) bool { func validateRegistrationEntryForUpdate(entry *common.RegistrationEntry, mask *common.RegistrationEntryMask) error { if entry == nil { - return sqlError.New("invalid request: missing registered entry") + return validationError.New("invalid request: missing registered entry") } if (mask == nil || mask.Selectors) && len(entry.Selectors) == 0 { - return sqlError.New("invalid registration entry: missing selector list") + return validationError.New("invalid registration entry: missing selector list") } if (mask == nil || mask.SpiffeId) && entry.SpiffeId == "" { - return sqlError.New("invalid registration entry: missing SPIFFE ID") + return validationError.New("invalid registration entry: missing SPIFFE ID") } if (mask == nil || mask.X509SvidTtl) && (entry.X509SvidTtl < 0) { - return sqlError.New("invalid registration entry: X509SvidTtl is not set") + return validationError.New("invalid registration entry: X509SvidTtl is not set") } if (mask == nil || mask.JwtSvidTtl) && (entry.JwtSvidTtl < 0) { - return sqlError.New("invalid registration entry: JwtSvidTtl is not set") + return validationError.New("invalid registration entry: JwtSvidTtl is not set") } return nil diff --git a/pkg/server/datastore/sqlstore/sqlstore_test.go b/pkg/server/datastore/sqlstore/sqlstore_test.go index 4f18f0c32c..e0d11ab020 100644 --- a/pkg/server/datastore/sqlstore/sqlstore_test.go +++ b/pkg/server/datastore/sqlstore/sqlstore_test.go @@ -1870,7 +1870,7 @@ func (s *PluginSuite) TestCreateOrReturnRegistrationEntry() { modifyEntry: func(e *common.RegistrationEntry) *common.RegistrationEntry { return nil }, - expectError: "datastore-sql: invalid request: missing registered entry", + expectError: "rpc error: code = InvalidArgument desc = datastore-validation: invalid request: missing registered entry", }, { name: "no selectors", @@ -1878,7 +1878,7 @@ func (s *PluginSuite) TestCreateOrReturnRegistrationEntry() { e.Selectors = nil return e }, - expectError: "datastore-sql: invalid registration entry: missing selector list", + expectError: "rpc error: code = InvalidArgument desc = datastore-validation: invalid registration entry: missing selector list", }, { name: "no SPIFFE ID", @@ -1886,7 +1886,7 @@ func (s *PluginSuite) TestCreateOrReturnRegistrationEntry() { e.SpiffeId = "" return e }, - expectError: "datastore-sql: invalid registration entry: missing SPIFFE ID", + expectError: "rpc error: code = InvalidArgument desc = datastore-validation: invalid registration entry: missing SPIFFE ID", }, { name: "negative X509 ttl", @@ -1894,7 +1894,7 @@ func (s *PluginSuite) TestCreateOrReturnRegistrationEntry() { e.X509SvidTtl = -1 return e }, - expectError: "datastore-sql: invalid registration entry: X509SvidTtl is not set", + expectError: "rpc error: code = InvalidArgument desc = datastore-validation: invalid registration entry: X509SvidTtl is not set", }, { name: "negative JWT ttl", @@ -1902,7 +1902,7 @@ func (s *PluginSuite) TestCreateOrReturnRegistrationEntry() { e.JwtSvidTtl = -1 return e }, - expectError: "datastore-sql: invalid registration entry: JwtSvidTtl is not set", + expectError: "rpc error: code = InvalidArgument desc = datastore-validation: invalid registration entry: JwtSvidTtl is not set", }, { name: "create entry successfully", @@ -1968,7 +1968,7 @@ func (s *PluginSuite) TestCreateOrReturnRegistrationEntry() { e.EntryId = strings.Repeat("e", 256) return e }, - expectError: "datastore-sql: invalid registration entry: entry ID too long", + expectError: "rpc error: code = InvalidArgument desc = datastore-validation: invalid registration entry: entry ID too long", }, { name: "entry ID contains invalid characters", @@ -1976,7 +1976,7 @@ func (s *PluginSuite) TestCreateOrReturnRegistrationEntry() { e.EntryId = "éntry😊" return e }, - expectError: "datastore-sql: invalid registration entry: entry ID contains invalid characters", + expectError: "rpc error: code = InvalidArgument desc = datastore-validation: invalid registration entry: entry ID contains invalid characters", }, } { s.T().Run(tt.name, func(t *testing.T) { @@ -3033,7 +3033,7 @@ func (s *PluginSuite) TestUpdateRegistrationEntryWithStoreSvid() { } resp, err := s.ds.UpdateRegistrationEntry(ctx, entry, nil) s.Require().Nil(resp) - s.Require().EqualError(err, "rpc error: code = Unknown desc = datastore-sql: invalid registration entry: selector types must be the same when store SVID is enabled") + s.Require().EqualError(err, "rpc error: code = InvalidArgument desc = datastore-validation: invalid registration entry: selector types must be the same when store SVID is enabled") } func (s *PluginSuite) TestUpdateRegistrationEntryWithMask() { @@ -3208,7 +3208,7 @@ func (s *PluginSuite) TestUpdateRegistrationEntryWithMask() { {Type: "Type2", Value: "Value2"}, } }, - err: sqlError.New("invalid registration entry: selector types must be the same when store SVID is enabled"), + err: validationError.New("invalid registration entry: selector types must be the same when store SVID is enabled"), }, // ENTRYEXPIRY FIELD -- This field isn't validated so we just check with good data