Skip to content

Commit

Permalink
Return InvalidArgument for invalid input entries (#5506)
Browse files Browse the repository at this point in the history 
fixes #5444

Signed-off-by: Sorin Dumitru <[email protected]>
  • Loading branch information
@sorindumitru
sorindumitru authored Oct 17, 2024
1 parent c8d35fe commit d7d1ccd
Show file tree
Hide file tree
Showing 4 changed files with 59 additions and 47 deletions.
12 changes: 10 additions & 2 deletions pkg/server/api/entry/v1/service.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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")
Expand Down Expand Up @@ -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),
}
}

Expand Down
34 changes: 17 additions & 17 deletions pkg/server/api/entry/v1/service_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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",
},
},
Expand All @@ -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",
},
},
Expand All @@ -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",
},
},
},
Expand Down Expand Up @@ -4130,26 +4130,26 @@ 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",
},
},
{
Level: logrus.InfoLevel,
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",
Expand Down
42 changes: 23 additions & 19 deletions pkg/server/datastore/sqlstore/sqlstore.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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}, // - | .
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -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,
Expand All @@ -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
Expand All @@ -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
Expand Down
18 changes: 9 additions & 9 deletions pkg/server/datastore/sqlstore/sqlstore_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1870,39 +1870,39 @@ 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",
modifyEntry: func(e *common.RegistrationEntry) *common.RegistrationEntry {
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",
modifyEntry: func(e *common.RegistrationEntry) *common.RegistrationEntry {
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",
modifyEntry: func(e *common.RegistrationEntry) *common.RegistrationEntry {
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",
modifyEntry: func(e *common.RegistrationEntry) *common.RegistrationEntry {
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",
Expand Down Expand Up @@ -1968,15 +1968,15 @@ 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",
modifyEntry: func(e *common.RegistrationEntry) *common.RegistrationEntry {
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) {
Expand Down Expand Up @@ -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() {
Expand Down Expand Up @@ -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
Expand Down

0 comments on commit d7d1ccd

Please sign in to comment.