Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Return InvalidArgument for invalid input entries #5506

Merged
merged 2 commits into from
Oct 17, 2024
Merged

Return InvalidArgument for invalid input entries #5506

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
0cf97bb
Return InvalidArgument for invalid input entries
sorindumitru Oct 15, 2024
9b2cf88
Merge branch 'main' into entry-errors
amartinezfayo Oct 17, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 10 additions & 2 deletions pkg/server/api/entry/v1/service.go
View file
Open in desktop
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
Open in desktop
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
Open in desktop
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]
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we'll ever do this, so better to remove the TODO.

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
}
Comment on lines +478 to +480
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've moved this here because errors withing a withTx block get wrapped into a grpc.Status (I don't know exactly why, maybe it's due to the plugin's history as an actual plugin). Having it in here makes it the handling for create and update be the same. Otherwise one would be wrapped in a grpc Status and one wouldn't so we'd need to handle them differently. I hope this doesn't change observed behaviour by clients too much.


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
Open in desktop
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
Loading