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

resource: mutate and validate before acls on write #18868

Merged
merged 1 commit into from
Sep 18, 2023
Merged

resource: mutate and validate before acls on write #18868

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
34 changes: 17 additions & 17 deletions agent/grpc-external/services/resource/write.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,15 +49,6 @@ func (s *Server) Write(ctx context.Context, req *pbresource.WriteRequest) (*pbre
}
v1EntMetaToV2Tenancy(reg, v1EntMeta, req.Resource.Id.Tenancy)

// ACL check comes before tenancy existence checks to not leak tenancy "existence".
err = reg.ACLs.Write(authz, authzContext, req.Resource)
switch {
case acl.IsErrPermissionDenied(err):
return nil, status.Error(codes.PermissionDenied, err.Error())
case err != nil:
return nil, status.Errorf(codes.Internal, "failed write acl: %v", err)
}

// Check the user sent the correct type of data.
if req.Resource.Data != nil && !req.Resource.Data.MessageIs(reg.Proto) {
got := strings.TrimPrefix(req.Resource.Data.TypeUrl, "type.googleapis.com/")
Expand All @@ -70,6 +61,23 @@ func (s *Server) Write(ctx context.Context, req *pbresource.WriteRequest) (*pbre
)
}

if err = reg.Mutate(req.Resource); err != nil {
return nil, status.Errorf(codes.Internal, "failed mutate hook: %v", err.Error())
}

if err = reg.Validate(req.Resource); err != nil {
return nil, status.Error(codes.InvalidArgument, err.Error())
}

// ACL check comes before tenancy existence checks to not leak tenancy "existence".
err = reg.ACLs.Write(authz, authzContext, req.Resource)
switch {
case acl.IsErrPermissionDenied(err):
return nil, status.Error(codes.PermissionDenied, err.Error())
case err != nil:
return nil, status.Errorf(codes.Internal, "failed write acl: %v", err)
}

// Check V1 tenancy exists for the V2 resource
if err = v1TenancyExists(reg, s.TenancyBridge, req.Resource.Id.Tenancy, codes.InvalidArgument); err != nil {
return nil, err
Expand All @@ -80,14 +88,6 @@ func (s *Server) Write(ctx context.Context, req *pbresource.WriteRequest) (*pbre
return nil, err
}

if err = reg.Mutate(req.Resource); err != nil {
return nil, status.Errorf(codes.Internal, "failed mutate hook: %v", err.Error())
}

if err = reg.Validate(req.Resource); err != nil {
return nil, status.Error(codes.InvalidArgument, err.Error())
}

// At the storage backend layer, all writes are CAS operations.
//
// This makes it possible to *safely* do things like keeping the Uid stable
Expand Down
8 changes: 6 additions & 2 deletions internal/resource/registry.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,14 +49,18 @@ type Registration struct {
Proto proto.Message

// ACLs are hooks called to perform authorization on RPCs.
// The hooks can assume that Validate has been called.
ACLs *ACLHooks

// Validate is called to structurally validate the resource (e.g.
// check for required fields).
// check for required fields). Validate can assume that Mutate
// has been called.
Validate func(*pbresource.Resource) error

// Mutate is called to fill out any autogenerated fields (e.g. UUIDs) or
// apply defaults before validation.
// apply defaults before validation. Mutate can assume that
// Resource.ID is populated and has non-empty tenancy fields. This does
// not mean those tenancy fields actually exist.
Mutate func(*pbresource.Resource) error

// Scope describes the tenancy scope of a resource.
Expand Down