From aa92dc860bd95ef941c6fe2f77143a79996514d3 Mon Sep 17 00:00:00 2001 From: Alexander Zobnin Date: Thu, 5 Jun 2025 22:11:26 +0200 Subject: [PATCH] Zanzana: Improve server side error handling (#106378) * Zanzana: Split client and server logs * Zanzana: Improve error handling and logging * log internal error at the server side * refactor * improve errors for list request * update go modules * handle errors for read and write * refactor * reset go.mod changes --- pkg/services/authz/zanzana.go | 8 +- pkg/services/authz/zanzana/client/client.go | 2 +- pkg/services/authz/zanzana/server.go | 4 +- .../authz/zanzana/server/openfga_server.go | 4 +- .../authz/zanzana/server/server_check.go | 103 ++++++++---------- .../authz/zanzana/server/server_list.go | 25 ++++- .../authz/zanzana/server/server_read.go | 15 ++- .../authz/zanzana/server/server_test.go | 2 +- .../authz/zanzana/server/server_write.go | 15 ++- 9 files changed, 106 insertions(+), 72 deletions(-) diff --git a/pkg/services/authz/zanzana.go b/pkg/services/authz/zanzana.go index 521db54c13e..b735ba3b2c7 100644 --- a/pkg/services/authz/zanzana.go +++ b/pkg/services/authz/zanzana.go @@ -37,7 +37,7 @@ func ProvideZanzana(cfg *setting.Cfg, db db.DB, tracer tracing.Tracer, features return zanzana.NewNoopClient(), nil } - logger := log.New("zanzana") + logger := log.New("zanzana.server") var client zanzana.Client switch cfg.ZanzanaClient.Mode { @@ -83,7 +83,7 @@ func ProvideZanzana(cfg *setting.Cfg, db db.DB, tracer tracing.Tracer, features return nil, fmt.Errorf("failed to start zanzana: %w", err) } - openfga, err := zanzana.NewOpenFGAServer(cfg.ZanzanaServer, store, logger) + openfga, err := zanzana.NewOpenFGAServer(cfg.ZanzanaServer, store) if err != nil { return nil, fmt.Errorf("failed to start zanzana: %w", err) } @@ -131,7 +131,7 @@ func ProvideZanzanaService(cfg *setting.Cfg, features featuremgmt.FeatureToggles s := &Zanzana{ cfg: cfg, features: features, - logger: log.New("zanzana"), + logger: log.New("zanzana.server"), } s.BasicService = services.NewBasicService(s.start, s.running, s.stopping).WithName("zanzana") @@ -167,7 +167,7 @@ func (z *Zanzana) start(ctx context.Context) error { return fmt.Errorf("failed to initilize zanana store: %w", err) } - openfgaServer, err := zanzana.NewOpenFGAServer(z.cfg.ZanzanaServer, store, z.logger) + openfgaServer, err := zanzana.NewOpenFGAServer(z.cfg.ZanzanaServer, store) if err != nil { return fmt.Errorf("failed to start zanzana: %w", err) } diff --git a/pkg/services/authz/zanzana/client/client.go b/pkg/services/authz/zanzana/client/client.go index 964b00d7724..d5705273ca5 100644 --- a/pkg/services/authz/zanzana/client/client.go +++ b/pkg/services/authz/zanzana/client/client.go @@ -30,7 +30,7 @@ func New(cc grpc.ClientConnInterface) (*Client, error) { authzlibclient: authzlibclient, authz: authzv1.NewAuthzServiceClient(cc), authzext: authzextv1.NewAuthzExtentionServiceClient(cc), - logger: log.New("zanzana-client"), + logger: log.New("zanzana.client"), } return c, nil diff --git a/pkg/services/authz/zanzana/server.go b/pkg/services/authz/zanzana/server.go index 859e5d9a9bc..1e46aa52324 100644 --- a/pkg/services/authz/zanzana/server.go +++ b/pkg/services/authz/zanzana/server.go @@ -21,8 +21,8 @@ func NewHealthServer(target server.DiagnosticServer) *server.HealthServer { return server.NewHealthServer(target) } -func NewOpenFGAServer(cfg setting.ZanzanaServerSettings, store openfgastorage.OpenFGADatastore, logger log.Logger) (*openfgaserver.Server, error) { - return server.NewOpenFGAServer(cfg, store, logger) +func NewOpenFGAServer(cfg setting.ZanzanaServerSettings, store openfgastorage.OpenFGADatastore) (*openfgaserver.Server, error) { + return server.NewOpenFGAServer(cfg, store) } func NewOpenFGAHttpServer(cfg setting.ZanzanaServerSettings, srv grpcserver.Provider) (*http.Server, error) { diff --git a/pkg/services/authz/zanzana/server/openfga_server.go b/pkg/services/authz/zanzana/server/openfga_server.go index 2eb854a81c1..3df2d944604 100644 --- a/pkg/services/authz/zanzana/server/openfga_server.go +++ b/pkg/services/authz/zanzana/server/openfga_server.go @@ -26,7 +26,9 @@ import ( zlogger "github.com/grafana/grafana/pkg/services/authz/zanzana/logger" ) -func NewOpenFGAServer(cfg setting.ZanzanaServerSettings, store storage.OpenFGADatastore, logger log.Logger) (*server.Server, error) { +func NewOpenFGAServer(cfg setting.ZanzanaServerSettings, store storage.OpenFGADatastore) (*server.Server, error) { + logger := log.New("openfga.server") + opts := []server.OpenFGAServiceV1Option{ server.WithDatastore(store), server.WithLogger(zlogger.New(logger)), diff --git a/pkg/services/authz/zanzana/server/server_check.go b/pkg/services/authz/zanzana/server/server_check.go index 1c5094158d2..d43ee1ab36a 100644 --- a/pkg/services/authz/zanzana/server/server_check.go +++ b/pkg/services/authz/zanzana/server/server_check.go @@ -2,9 +2,12 @@ package server import ( "context" + "errors" + "fmt" authzv1 "github.com/grafana/authlib/authz/proto/v1" openfgav1 "github.com/openfga/api/proto/openfga/v1" + "google.golang.org/protobuf/types/known/structpb" "github.com/grafana/grafana/pkg/services/authz/zanzana/common" ) @@ -13,26 +16,36 @@ func (s *Server) Check(ctx context.Context, r *authzv1.CheckRequest) (*authzv1.C ctx, span := s.tracer.Start(ctx, "server.Check") defer span.End() + res, err := s.check(ctx, r) + if err != nil { + s.logger.Error("failed to perform check request", "error", err, "namespace", r.GetNamespace()) + return nil, errors.New("failed to perform check request") + } + + return res, nil +} + +func (s *Server) check(ctx context.Context, r *authzv1.CheckRequest) (*authzv1.CheckResponse, error) { if err := authorize(ctx, r.GetNamespace()); err != nil { return nil, err } store, err := s.getStoreInfo(ctx, r.GetNamespace()) if err != nil { - return nil, err + return nil, fmt.Errorf("failed to get openfga store: %w", err) } relation := common.VerbMapping[r.GetVerb()] contextuals, err := s.getContextuals(r.GetSubject()) if err != nil { - return nil, err + return nil, fmt.Errorf("failed to get contextual tuples: %w", err) } resource := common.NewResourceInfoFromCheck(r) res, err := s.checkGroupResource(ctx, r.GetSubject(), relation, resource, contextuals, store) if err != nil { - return nil, err + return nil, fmt.Errorf("failed to check group resource: %w", err) } if res.GetAllowed() { @@ -40,10 +53,18 @@ func (s *Server) Check(ctx context.Context, r *authzv1.CheckRequest) (*authzv1.C } if resource.IsGeneric() { - return s.checkGeneric(ctx, r.GetSubject(), relation, resource, contextuals, store) + res, err = s.checkGeneric(ctx, r.GetSubject(), relation, resource, contextuals, store) + if err != nil { + return nil, fmt.Errorf("failed to check generic resource: %w", err) + } + return res, nil } - return s.checkTyped(ctx, r.GetSubject(), relation, resource, contextuals, store) + res, err = s.checkTyped(ctx, r.GetSubject(), relation, resource, contextuals, store) + if err != nil { + return nil, fmt.Errorf("failed to check typed resource: %w", err) + } + return res, nil } // checkGroupResource check if subject has access to the full "GroupResource", if they do they can access every object @@ -53,16 +74,7 @@ func (s *Server) checkGroupResource(ctx context.Context, subject, relation strin return &authzv1.CheckResponse{Allowed: false}, nil } - res, err := s.openfga.Check(ctx, &openfgav1.CheckRequest{ - StoreId: store.ID, - AuthorizationModelId: store.ModelID, - TupleKey: &openfgav1.CheckRequestTupleKey{ - User: subject, - Relation: relation, - Object: resource.GroupResourceIdent(), - }, - ContextualTuples: contextuals, - }) + res, err := s.openfgaCheck(ctx, store, subject, relation, resource.GroupResourceIdent(), contextuals, nil) if err != nil { return nil, err } @@ -84,18 +96,7 @@ func (s *Server) checkTyped(ctx context.Context, subject, relation string, resou if resource.HasSubresource() { // Check if subject has access as a subresource - res, err := s.openfga.Check(ctx, &openfgav1.CheckRequest{ - StoreId: store.ID, - AuthorizationModelId: store.ModelID, - TupleKey: &openfgav1.CheckRequestTupleKey{ - User: subject, - Relation: subresourceRelation, - Object: resourceIdent, - }, - Context: resourceCtx, - ContextualTuples: contextuals, - }) - + res, err := s.openfgaCheck(ctx, store, subject, subresourceRelation, resourceIdent, contextuals, resourceCtx) if err != nil { return nil, err } @@ -110,25 +111,12 @@ func (s *Server) checkTyped(ctx context.Context, subject, relation string, resou } // Check if subject has direct access to resource - res, err := s.openfga.Check(ctx, &openfgav1.CheckRequest{ - StoreId: store.ID, - AuthorizationModelId: store.ModelID, - TupleKey: &openfgav1.CheckRequestTupleKey{ - User: subject, - Relation: relation, - Object: resourceIdent, - }, - ContextualTuples: contextuals, - }) + res, err := s.openfgaCheck(ctx, store, subject, relation, resourceIdent, contextuals, nil) if err != nil { return nil, err } - if res.GetAllowed() { - return &authzv1.CheckResponse{Allowed: true}, nil - } - - return &authzv1.CheckResponse{Allowed: false}, nil + return &authzv1.CheckResponse{Allowed: res.GetAllowed()}, nil } // checkGeneric check our generic "resource" type. It checks: @@ -143,18 +131,7 @@ func (s *Server) checkGeneric(ctx context.Context, subject, relation string, res if folderIdent != "" && common.IsSubresourceRelation(folderRelation) { // Check if subject has access as a sub resource for the folder - res, err := s.openfga.Check(ctx, &openfgav1.CheckRequest{ - StoreId: store.ID, - AuthorizationModelId: store.ModelID, - TupleKey: &openfgav1.CheckRequestTupleKey{ - User: subject, - Relation: folderRelation, - Object: folderIdent, - }, - Context: resourceCtx, - ContextualTuples: contextuals, - }) - + res, err := s.openfgaCheck(ctx, store, subject, folderRelation, folderIdent, contextuals, resourceCtx) if err != nil { return nil, err } @@ -170,21 +147,33 @@ func (s *Server) checkGeneric(ctx context.Context, subject, relation string, res } // Check if subject has direct access to resource + res, err := s.openfgaCheck(ctx, store, subject, relation, resourceIdent, contextuals, resourceCtx) + if err != nil { + return nil, err + } + + return &authzv1.CheckResponse{Allowed: res.GetAllowed()}, nil +} + +func (s *Server) openfgaCheck(ctx context.Context, store *storeInfo, subject, relation, object string, contextuals *openfgav1.ContextualTupleKeys, resourceCtx *structpb.Struct) (*openfgav1.CheckResponse, error) { res, err := s.openfga.Check(ctx, &openfgav1.CheckRequest{ StoreId: store.ID, AuthorizationModelId: store.ModelID, TupleKey: &openfgav1.CheckRequestTupleKey{ User: subject, Relation: relation, - Object: resourceIdent, + Object: object, }, Context: resourceCtx, ContextualTuples: contextuals, }) if err != nil { - return nil, err + // error is decorated by openfga with a public-facing error message, so we need to unwrap it to get the actual error and log it server-side, + // but we want to return wrapped error to the client to prevent leaking internal error details + s.logger.Error("failed to perform check", "error", errors.Unwrap(err), "subject", subject, "relation", relation, "object", object) + return nil, fmt.Errorf("failed to perform openfga Check request: %w", err) } - return &authzv1.CheckResponse{Allowed: res.GetAllowed()}, nil + return res, nil } diff --git a/pkg/services/authz/zanzana/server/server_list.go b/pkg/services/authz/zanzana/server/server_list.go index ba864645d82..c3543ca253e 100644 --- a/pkg/services/authz/zanzana/server/server_list.go +++ b/pkg/services/authz/zanzana/server/server_list.go @@ -4,6 +4,7 @@ import ( "context" "encoding/base64" "errors" + "fmt" "hash/fnv" "io" "strings" @@ -18,18 +19,28 @@ func (s *Server) List(ctx context.Context, r *authzv1.ListRequest) (*authzv1.Lis ctx, span := s.tracer.Start(ctx, "server.List") defer span.End() + res, err := s.list(ctx, r) + if err != nil { + s.logger.Error("failed to perform list request", "error", err, "namespace", r.GetNamespace()) + return nil, errors.New("failed to perform list request") + } + + return res, nil +} + +func (s *Server) list(ctx context.Context, r *authzv1.ListRequest) (*authzv1.ListResponse, error) { if err := authorize(ctx, r.GetNamespace()); err != nil { return nil, err } store, err := s.getStoreInfo(ctx, r.GetNamespace()) if err != nil { - return nil, err + return nil, fmt.Errorf("failed to get openfga store: %w", err) } contextuals, err := s.getContextuals(r.GetSubject()) if err != nil { - return nil, err + return nil, fmt.Errorf("failed to get contextual tuples: %w", err) } relation := common.VerbMapping[r.GetVerb()] @@ -37,7 +48,7 @@ func (s *Server) List(ctx context.Context, r *authzv1.ListRequest) (*authzv1.Lis res, err := s.checkGroupResource(ctx, r.GetSubject(), relation, resource, contextuals, store) if err != nil { - return nil, err + return nil, fmt.Errorf("failed to check group resource: %w", err) } if res.GetAllowed() { @@ -161,7 +172,13 @@ func (s *Server) listObjects(ctx context.Context, req *openfgav1.ListObjectsRequ return s.listObjectCached(ctx, req, fn) } - return fn(ctx, req) + res, err := fn(ctx, req) + if err != nil { + s.logger.Error("failed to perform openfga ListObjects request", "error", errors.Unwrap(err), "user", req.GetUser(), "type", req.GetType(), "relation", req.GetRelation()) + return nil, err + } + + return res, nil } type listFn func(ctx context.Context, req *openfgav1.ListObjectsRequest) (*openfgav1.ListObjectsResponse, error) diff --git a/pkg/services/authz/zanzana/server/server_read.go b/pkg/services/authz/zanzana/server/server_read.go index 755f769c11b..88dbf5c9fd9 100644 --- a/pkg/services/authz/zanzana/server/server_read.go +++ b/pkg/services/authz/zanzana/server/server_read.go @@ -2,6 +2,8 @@ package server import ( "context" + "errors" + "fmt" openfgav1 "github.com/openfga/api/proto/openfga/v1" @@ -13,13 +15,23 @@ func (s *Server) Read(ctx context.Context, req *authzextv1.ReadRequest) (*authze ctx, span := s.tracer.Start(ctx, "server.Read") defer span.End() + res, err := s.read(ctx, req) + if err != nil { + s.logger.Error("failed to perform read request", "error", err, "namespace", req.GetNamespace()) + return nil, errors.New("failed to perform read request") + } + + return res, nil +} + +func (s *Server) read(ctx context.Context, req *authzextv1.ReadRequest) (*authzextv1.ReadResponse, error) { if err := authorize(ctx, req.GetNamespace()); err != nil { return nil, err } storeInf, err := s.getStoreInfo(ctx, req.Namespace) if err != nil { - return nil, err + return nil, fmt.Errorf("failed to get openfga store: %w", err) } readReq := &openfgav1.ReadRequest{ @@ -38,6 +50,7 @@ func (s *Server) Read(ctx context.Context, req *authzextv1.ReadRequest) (*authze res, err := s.openfga.Read(ctx, readReq) if err != nil { + s.logger.Error("failed to perform openfga Read request", "error", errors.Unwrap(err)) return nil, err } diff --git a/pkg/services/authz/zanzana/server/server_test.go b/pkg/services/authz/zanzana/server/server_test.go index da441d60914..7b3c57155de 100644 --- a/pkg/services/authz/zanzana/server/server_test.go +++ b/pkg/services/authz/zanzana/server/server_test.go @@ -89,7 +89,7 @@ func setup(t *testing.T, testDB db.DB, cfg *setting.Cfg) *Server { store, err := store.NewEmbeddedStore(cfg, testDB, log.NewNopLogger()) require.NoError(t, err) - openfga, err := NewOpenFGAServer(cfg.ZanzanaServer, store, log.NewNopLogger()) + openfga, err := NewOpenFGAServer(cfg.ZanzanaServer, store) require.NoError(t, err) srv, err := NewServer(cfg.ZanzanaServer, openfga, log.NewNopLogger(), tracing.NewNoopTracerService()) diff --git a/pkg/services/authz/zanzana/server/server_write.go b/pkg/services/authz/zanzana/server/server_write.go index 0aa932bc3f8..c0c249d36a5 100644 --- a/pkg/services/authz/zanzana/server/server_write.go +++ b/pkg/services/authz/zanzana/server/server_write.go @@ -2,6 +2,8 @@ package server import ( "context" + "errors" + "fmt" openfgav1 "github.com/openfga/api/proto/openfga/v1" @@ -13,13 +15,23 @@ func (s *Server) Write(ctx context.Context, req *authzextv1.WriteRequest) (*auth ctx, span := s.tracer.Start(ctx, "server.Write") defer span.End() + res, err := s.write(ctx, req) + if err != nil { + s.logger.Error("failed to perform write request", "error", err, "namespace", req.GetNamespace()) + return nil, errors.New("failed to perform write request") + } + + return res, nil +} + +func (s *Server) write(ctx context.Context, req *authzextv1.WriteRequest) (*authzextv1.WriteResponse, error) { if err := authorize(ctx, req.GetNamespace()); err != nil { return nil, err } storeInf, err := s.getStoreInfo(ctx, req.Namespace) if err != nil { - return nil, err + return nil, fmt.Errorf("failed to get openfga store: %w", err) } writeTuples := make([]*openfgav1.TupleKey, 0) @@ -49,6 +61,7 @@ func (s *Server) Write(ctx context.Context, req *authzextv1.WriteRequest) (*auth _, err = s.openfga.Write(ctx, writeReq) if err != nil { + s.logger.Error("failed to perform openfga Write request", "error", errors.Unwrap(err)) return nil, err }