TrueCloudLab/frostfs-node
20
1
Fork 27
Code Issues 105 Pull requests 9 Projects Releases 2 Activity Actions

Introduce ChainRouterError error type and wrap only these errors with ObjectAccessDenied status #1563

Merged
fyrchik merged 3 commits from aarifullin/frostfs-node:fix/ape_logicalerr into master 2024-12-17 08:24:32 +00:00
Conversation 1 Commits 3 Files changed 4 +50 -1
4 changed files with 50 additions and 1 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

2
pkg/services/common/ape/checker.go
View file

@ -103,7 +103,7 @@ func (c *checkerCoreImpl) CheckAPE(prm CheckPrm) error {
if found && status == apechain.Allow {
return nil
}
return fmt.Errorf("access to operation %s is denied by access policy engine: %s", prm.Request.Operation(), status.String())
return newChainRouterError(prm.Request.Operation(), status)
}
// isValidBearer checks whether bearer token was correctly signed by authorized

33
pkg/services/common/ape/error.go Normal file
View file

@ -0,0 +1,33 @@
package ape
import (
"fmt"
apechain "git.frostfs.info/TrueCloudLab/policy-engine/pkg/chain"
)
// ChainRouterError is returned when chain router validation prevents
// the APE request from being processed (no rule found, access denied, etc.).
type ChainRouterError struct {
operation string
status apechain.Status
}
func (e *ChainRouterError) Error() string {
return fmt.Sprintf("access to operation %s is denied by access policy engine: %s", e.Operation(), e.Status())
fyrchik marked this conversation as resolved Outdated
fyrchik commented 2024-12-16 11:31:32 +00:00
Outdated
Review
Copy link

.String() can be omitted for %s.

`.String()` can be omitted for `%s`.
👍 1
aarifullin commented 2024-12-16 12:18:16 +00:00
Outdated
Review
Copy link

Fixed

Fixed
}
func (e *ChainRouterError) Operation() string {
return e.operation
}
func (e *ChainRouterError) Status() apechain.Status {
return e.status
}
func newChainRouterError(operation string, status apechain.Status) *ChainRouterError {
fyrchik marked this conversation as resolved Outdated
fyrchik commented 2024-12-16 11:31:22 +00:00
Outdated
Review
Copy link

Do we need to export this?

Do we need to export this?
👍 1
aarifullin commented 2024-12-16 12:18:19 +00:00
Outdated
Review
Copy link

Fixed

Fixed
return &ChainRouterError{
operation: operation,
status: status,
}
}

9
pkg/services/object/ape/errors.go
View file

@ -1,10 +1,19 @@
package ape
import (
"errors"
checkercore "git.frostfs.info/TrueCloudLab/frostfs-node/pkg/services/common/ape"
apistatus "git.frostfs.info/TrueCloudLab/frostfs-sdk-go/client/status"
)
func toStatusErr(err error) error {
var chRouterErr *checkercore.ChainRouterError
if !errors.As(err, &chRouterErr) {
errServerInternal := &apistatus.ServerInternal{}
fyrchik marked this conversation as resolved Outdated
fyrchik commented 2024-12-16 11:33:59 +00:00
Outdated
Review
Copy link

Where do we do wrapping to internal status (if we have any)?
I am wondering whether these changes may break client code (see PrmInit.ResolveFrostFSFailures() in SDK), because we will get an error instead of status.

Where do we do wrapping to internal status (if we have any)? I am wondering whether these changes may break client code (see `PrmInit.ResolveFrostFSFailures()` in SDK), because we will get an error instead of status.
aarifullin commented 2024-12-16 12:02:27 +00:00
Outdated
Review
Copy link

We don't do that for all errors, but they are implicitly (from the point of this middleware) wrapped to internalServerError anyway. Aren't they?

func setStatusV2(resp ResponseMessage, err error) {
	// unwrap error
	for e := errors.Unwrap(err); e != nil; e = errors.Unwrap(err) {
		err = e
	}

	session.SetStatus(resp, apistatus.ToStatusV2(apistatus.ErrToStatus(err)))
}

func ToStatusV2(st Status) *status.Status {
	if v, ok := st.(StatusV2); ok {
		return v.ToStatusV2()
	}

	if IsSuccessful(st) {
		return newStatusV2WithLocalCode(status.OK, status.GlobalizeSuccess)
	}

	internalErrorStatus := newStatusV2WithLocalCode(status.Internal, status.GlobalizeCommonFail)
	internalErrorStatus.SetMessage(st.(error).Error()) // type cast never panics because IsSuccessful() checks cast

	return internalErrorStatus
}

Although that's not true for tree service

We don't do that for all errors, but they are implicitly (from the point of this middleware) wrapped to `internalServerError` anyway. Aren't they? ```go func setStatusV2(resp ResponseMessage, err error) { // unwrap error for e := errors.Unwrap(err); e != nil; e = errors.Unwrap(err) { err = e } session.SetStatus(resp, apistatus.ToStatusV2(apistatus.ErrToStatus(err))) } ``` ```go func ToStatusV2(st Status) *status.Status { if v, ok := st.(StatusV2); ok { return v.ToStatusV2() } if IsSuccessful(st) { return newStatusV2WithLocalCode(status.OK, status.GlobalizeSuccess) } internalErrorStatus := newStatusV2WithLocalCode(status.Internal, status.GlobalizeCommonFail) internalErrorStatus.SetMessage(st.(error).Error()) // type cast never panics because IsSuccessful() checks cast return internalErrorStatus } ``` Although that's not true for `tree` service
aarifullin commented 2024-12-16 12:18:10 +00:00
Outdated
Review
Copy link

For now non-chainRouterError errors are explicitly wrapped with ServerInternal status for the all goodness sake

For now non-`chainRouterError` errors are explicitly wrapped with ServerInternal status for the all goodness sake
apistatus.WriteInternalServerErr(errServerInternal, err)
return errServerInternal
}
errAccessDenied := &apistatus.ObjectAccessDenied{}
errAccessDenied.WriteReason("ape denied request: " + err.Error())
return errAccessDenied

7
pkg/services/tree/signature.go
View file

@ -9,6 +9,7 @@ import (
"fmt"
core "git.frostfs.info/TrueCloudLab/frostfs-node/pkg/core/container"
checkercore "git.frostfs.info/TrueCloudLab/frostfs-node/pkg/services/common/ape"
"git.frostfs.info/TrueCloudLab/frostfs-sdk-go/api/refs"
"git.frostfs.info/TrueCloudLab/frostfs-sdk-go/bearer"
apistatus "git.frostfs.info/TrueCloudLab/frostfs-sdk-go/client/status"
@ -70,6 +71,12 @@ func (s *Service) verifyClient(ctx context.Context, req message, cid cidSDK.ID,
}
func apeErr(err error) error {
var chRouterErr *checkercore.ChainRouterError
if !errors.As(err, &chRouterErr) {
errServerInternal := &apistatus.ServerInternal{}
apistatus.WriteInternalServerErr(errServerInternal, err)
return errServerInternal
}
errAccessDenied := &apistatus.ObjectAccessDenied{}
errAccessDenied.WriteReason(err.Error())
return errAccessDenied