[#872] object: Use APE instead EACL for object service handlers #888

Closed
aarifullin wants to merge 1 commit from feature/872-object_svc_ape into master
Member
  • Make APE Request implementation to get properties for a resource
    from object SDK
  • Make object service handlers check requests and responses with
    APE instead EACL

Close #872

Close #844

* Make APE Request implementation to get properties for a resource from object SDK * Make object service handlers check requests and responses with APE instead EACL Close #872 Close #844
aarifullin force-pushed feature/872-object_svc_ape from b275f23369 to 41a3fd0fce 2023-12-25 11:07:46 +00:00 Compare
aarifullin requested review from storage-core-committers 2023-12-25 13:00:24 +00:00
aarifullin requested review from storage-core-developers 2023-12-25 13:00:24 +00:00
aarifullin reviewed 2023-12-25 13:01:15 +00:00
@ -28,0 +75,4 @@
errMissingOID = fmt.Errorf("object ID is not set")
)
func (c *apeCheckerImpl) objectSDKFromRequest(ctx context.Context, request any, reqInfo v2.RequestInfo) (*objectSDK.Object, error) {
Author
Member
See [readObjectHeadersFromRequestXHeaderSource](https://git.frostfs.info/TrueCloudLab/frostfs-node/src/branch/master/pkg/services/object/acl/eacl/v2/headers.go#L110-L155)
aarifullin reviewed 2023-12-25 13:02:12 +00:00
@ -28,0 +111,4 @@
}
}
func (c *apeCheckerImpl) objectSDKFromResponse(ctx context.Context, response any, reqInfo v2.RequestInfo) (*objectSDK.Object, error) {
Author
Member
See [readObjectHeadersResponseXHeaderSource](https://git.frostfs.info/TrueCloudLab/frostfs-node/src/branch/master/pkg/services/object/acl/eacl/v2/headers.go#L157-L199)
aarifullin reviewed 2023-12-25 14:18:56 +00:00
@ -53,0 +90,4 @@
return
}
func nativeSchemaRole(reqInfo v2.RequestInfo) string {
Author
Member

I intended to re-use RequestInfo structure. Its definition is

type RequestInfo struct {
	basicACL    acl.Basic
	requestRole acl.Role
	operation   acl.Op  // put, get, head, etc.
	cnrOwner    user.ID // container owner

	idCnr cid.ID

	// optional for some request
	// e.g. Put, Search
	obj *oid.ID

	senderKey []byte

	bearer *bearer.Token // bearer token of request

	srcRequest any
}

This is quite usable for this purpose. However, these SDK-fields have been remained: acl.Role, acl.Op.

How do you think should we to refactor these fields to string-s to make them set as APE-constants?

I intended to re-use `RequestInfo` structure. Its definition is ```go type RequestInfo struct { basicACL acl.Basic requestRole acl.Role operation acl.Op // put, get, head, etc. cnrOwner user.ID // container owner idCnr cid.ID // optional for some request // e.g. Put, Search obj *oid.ID senderKey []byte bearer *bearer.Token // bearer token of request srcRequest any } ``` This is quite usable for this purpose. However, these SDK-fields have been remained: `acl.Role`, `acl.Op`. How do you think should we to refactor these fields to `string`-s to make them set as APE-constants?
Owner

Each op is defined in a native schema, so old acl.Op for object service can be translated.
The role is less useful now, because request initiator for ingress chain is by definition not an innerring or storage node.

Each op is defined in a native schema, so old `acl.Op` for object service can be translated. The role is less useful now, because request initiator for `ingress` chain is _by definition_ not an innerring or storage node.
Author
Member

Not actual

Not actual
aarifullin marked this conversation as resolved
dstepanov-yadro reviewed 2023-12-25 15:21:01 +00:00
@ -28,0 +131,4 @@
hdr.SetCreationEpoch(headerPart.GetCreationEpoch())
hdr.SetOwnerID(headerPart.GetOwnerID())
hdr.SetObjectType(headerPart.GetObjectType())
hdr.SetPayloadLength(headerPart.GetPayloadLength())

Why SetPayloadHash and SetHomomorphicHash skipped?

Why `SetPayloadHash` and `SetHomomorphicHash` skipped?
Owner

Also, don't we have any helper in SDK/api?

Also, don't we have any helper in SDK/api?
Author
Member

I've added these setters.

Also, don't we have any helper in SDK/api?

I didn't find anything. But I can move this to some util

I've added these setters. > Also, don't we have any helper in SDK/api? I didn't find anything. But I can move this to some util
dstepanov-yadro marked this conversation as resolved
dstepanov-yadro reviewed 2023-12-25 15:24:43 +00:00
@ -53,0 +87,4 @@
res[nativeschema.PropertyKeyObjectHomomorphicHash] = hcs.String()
}
return

Object can have other user-defined attributes, i think they should be checked too.

Object can have other user-defined attributes, i think they should be checked too.
Author
Member

Added

Added
dstepanov-yadro marked this conversation as resolved
dstepanov-yadro reviewed 2023-12-25 15:26:00 +00:00
@ -155,3 +153,2 @@
return basicACLErr(reqInfo)
} else if err := b.checker.CheckEACL(request, reqInfo); err != nil {
return eACLErr(reqInfo, err)
} else if err := b.apeChecker.CheckIfAPEPermitsRequest(context.TODO(), request, reqInfo); err != nil {

stream has context.

`stream` has context.
aarifullin marked this conversation as resolved
fyrchik reviewed 2023-12-26 08:32:39 +00:00
@ -41,2 +181,4 @@
}
func (c *apeCheckerImpl) CheckIfAPEPermitsRequest(ctx context.Context, request any, reqInfo v2.RequestInfo) error {
if !util.IsObjectServiceRequest(request) {
Owner

Please, take a look at #881/
Can we do it in a similar way? In particular, this check seems superfluous.
Also, what about forming headers inside each service? So instead of objectSDKFromRequest with a switch we have a function inside each of object/..., possibly with some reusable helpers here.

Please, take a look at https://git.frostfs.info/TrueCloudLab/frostfs-node/pulls/881/ Can we do it in a similar way? In particular, this check seems superfluous. Also, what about forming headers inside each service? So instead of `objectSDKFromRequest` with a switch we have a function inside each of `object/...`, possibly with some reusable helpers here.
Author
Member

I've made apeSvc. Please, check it out

I've made `apeSvc`. Please, check it out
@ -0,0 +4,4 @@
objectV2 "git.frostfs.info/TrueCloudLab/frostfs-api-go/v2/object"
)
func IsObjectServiceRequest(msg any) bool {
Owner

IMO we are doing sth wrong if we need this: APE application doesn't seem do depend on particular structs in any way, so why not provide exactly the needed interface?

IMO we are doing sth wrong if we need this: APE application doesn't seem do depend on particular structs in any way, so why not provide exactly the needed interface?
Author
Member

APE application doesn't seem do depend on particular structs

It is true. And the access policy engine does not depend on these request/response structs itself. But APE-checker does, because it some kind of adapter for object service to perform checks in the engine.
My point is that this is correct because APEChainChecker is part of object service and not standalone checker.

Another option is to make the interface like that:

type APEChainChecker interface {
	CheckIfAPEPermitsRequest(ctx context.Context, object objectSDK.Object, reqInfo RequestInfo) error

	CheckIfAPEPermitsResponse(ctx context.Context, object objectSDK.Object, reqInfo RequestInfo) error
}
> APE application doesn't seem do depend on particular structs It is true. And the access policy engine does not depend on these request/response structs itself. But APE-checker does, because it some kind of adapter for object service to perform checks in the engine. My point is that this is correct because `APEChainChecker` is part of object service and not standalone checker. Another option is to make the interface like that: ```go type APEChainChecker interface { CheckIfAPEPermitsRequest(ctx context.Context, object objectSDK.Object, reqInfo RequestInfo) error CheckIfAPEPermitsResponse(ctx context.Context, object objectSDK.Object, reqInfo RequestInfo) error } ```
Author
Member

Not actual

Not actual
aarifullin marked this conversation as resolved
aarifullin force-pushed feature/872-object_svc_ape from 41a3fd0fce to 8019b7cee4 2023-12-27 14:19:09 +00:00 Compare
aarifullin force-pushed feature/872-object_svc_ape from 8019b7cee4 to 4762d81c14 2023-12-27 15:27:53 +00:00 Compare
aarifullin force-pushed feature/872-object_svc_ape from 4762d81c14 to f5b860d722 2023-12-27 20:17:57 +00:00 Compare
dstepanov-yadro reviewed 2023-12-28 08:19:10 +00:00
@ -0,0 +357,4 @@
return nil, err
}
// nothing to check with ape

Don't understand: line 356 is APE check.

Don't understand: line 356 is APE check.
Author
Member

Sorry, that was left after previous workaround :). It'll be removed!

Sorry, that was left after previous workaround :). It'll be removed!
dstepanov-yadro marked this conversation as resolved

I think some unit-tests are required.

I think some unit-tests are required.
aarifullin force-pushed feature/872-object_svc_ape from f5b860d722 to 5a75f6dd52 2023-12-28 11:07:14 +00:00 Compare
fyrchik reviewed 2024-01-09 07:27:23 +00:00
@ -0,0 +2,4 @@
import "git.frostfs.info/TrueCloudLab/frostfs-sdk-go/container/acl"
type RequestContextKeyT string
Owner

It is usually better to use RequestContextKey struct{} and empty value as a key: this is the approach used in golang net stdlib.

It is usually better to use `RequestContextKey struct{}` and empty value as a key: this is the approach used in golang `net` stdlib.
Author
Member

Thank you, fixed

Thank you, fixed
aarifullin force-pushed feature/872-object_svc_ape from 5a75f6dd52 to 29debad95f 2024-01-09 12:06:50 +00:00 Compare
aarifullin force-pushed feature/872-object_svc_ape from 29debad95f to 4c81991888 2024-01-09 12:15:30 +00:00 Compare
aarifullin force-pushed feature/872-object_svc_ape from 4c81991888 to e528c9f966 2024-01-09 12:23:32 +00:00 Compare
aarifullin force-pushed feature/872-object_svc_ape from e528c9f966 to d7e7610756 2024-01-09 12:42:37 +00:00 Compare
Author
Member

I think some unit-tests are required.

I have implemented unit-tests and tried to cover cases as more as possible

> I think some unit-tests are required. I have implemented unit-tests and tried to cover cases as more as possible
acid-ant reviewed 2024-01-10 08:29:21 +00:00
@ -0,0 +91,4 @@
func requestContext(ctx context.Context) (*objectSvc.RequestContext, error) {
untyped := ctx.Value(objectSvc.RequestContextKey)
if untyped == nil {
return nil, fmt.Errorf("context does not contain this key %s", objectSvc.RequestContextKey)
Member

Maybe just no key %s in context?

Maybe just `no key %s in context`?
Author
Member

Fixed

Fixed
acid-ant marked this conversation as resolved
@ -0,0 +289,4 @@
return nil, err
}
err = c.apeChecker.CheckAPE(ctx, Prm{
Member

Looks like redundant check.

Looks like redundant check.
Author
Member

I think you are right. I will remove this check

I think you are right. I will remove this check
Author
Member

Nah, let's keep it like that. Please, check this comment out

Nah, let's keep it like that. Please, check this [comment](https://git.frostfs.info/TrueCloudLab/frostfs-node/pulls/888/files#issuecomment-30310) out
Member

I agree for get like requests, now it is clear, but for delete it looks strange. Even if we reject the whole delete request, we perform physical deletion, isn't it?

I agree for `get` like requests, now it is clear, but for delete it looks strange. Even if we reject the whole `delete` request, we perform physical deletion, isn't it?
Author
Member

Even if we reject the whole delete request, we perform physical deletion, isn't it?

This sounds really reasonable, okay. I'll remove it

> Even if we reject the whole delete request, we perform physical deletion, isn't it? This sounds really reasonable, okay. I'll remove it
Author
Member

Fixed

Fixed
Member

Just for my information, in this case when we will get attributes of the object to do some checks via ape?

Just for my information, in this case when we will get attributes of the object to do some checks via `ape`?
Author
Member

we will get attributes

#888/files

objectProperties method

> we will get attributes https://git.frostfs.info/TrueCloudLab/frostfs-node/pulls/888/files#diff-1e1d20897061c8991e8e64e87a89cd65aef8d86f ` objectProperties` method
acid-ant marked this conversation as resolved
@ -0,0 +355,4 @@
return nil, err
}
if err = c.apeChecker.CheckAPE(ctx, prm); err != nil {
Member

Why we need to perform this check twice?

Why we need to perform this check twice?
Author
Member

This idea is not mine, it is already used in object service.

That doesn't look obvious but first check c.apeChecker.CheckAPE is kind of trial invocation: if an object is placed locally, then an object's header can be recieved immediately and thus we are able to collect all object properties like system and user attributes . If it cannot be found locally, then only CID, OID attributes passed to APE: at least this can be used for checks like "does APE allow to put more object to the container?" - we can reduce response time because we don't need to get entire object to check if the request is denined by <CID, OID> in APE.

If it is passed after first check, we get the requested object, we get it with filled header (-> properties) and we need to perform the check second time to these cases when APE check can be performed only on object attributes like payloadHash < 1000? or objectType != regular. That is fair for all get-like methods but you was right delete barely needs this second check (however, we still want to check if an object with some attributes is allowed to get deleted)

This idea is not mine, it is already used in object service. That doesn't look obvious but first check `c.apeChecker.CheckAPE` is kind of _trial_ invocation: if an object is placed _locally_, then an object's header can be recieved immediately and thus we are able to collect all object properties like [system](https://git.frostfs.info/TrueCloudLab/policy-engine/src/branch/master/schema/native/consts.go#L40-L48) and [user](https://git.frostfs.info/TrueCloudLab/frostfs-sdk-go/src/branch/master/object/wellknown_attributes.go) attributes . If it cannot be found locally, then only CID, OID attributes passed to APE: at least this can be used for checks like "does APE _allow_ to put more object to the container?" - we can reduce response time because we don't need to get entire object to check if the request is denined by <CID, OID> in APE. If it is passed after first check, we get the requested object, we get it with filled header (-> properties) and we need to perform the check second time to these cases when APE check can be performed only on object attributes like `payloadHash < 1000?` or `objectType != regular`. That is fair for all get-like methods but you was right `delete` barely needs this second check (_however, we still want to check if an object with some attributes is allowed to get deleted_)
Member

Now it is pretty much clear, thanks.

Now it is pretty much clear, thanks.
acid-ant marked this conversation as resolved
aarifullin force-pushed feature/872-object_svc_ape from d7e7610756 to e19109a3bb 2024-01-10 11:02:25 +00:00 Compare
dstepanov-yadro reviewed 2024-01-10 11:36:16 +00:00
@ -0,0 +134,4 @@
header = prm.Header
} else if prm.Object != nil {
headerObjSDK, err := c.headerProvider.GetHeader(ctx, prm.Container, *prm.Object)
if err == nil {

Is it ok to skip getHeader error?

Is it ok to skip getHeader error?
Author
Member

Yes. If err != nil, then it means the object is not found locally (check this out).
Do you suggest to check it for NotFoundError?

Yes. If `err != nil`, then it means the object is not found locally (check [this](https://git.frostfs.info/TrueCloudLab/frostfs-node/pulls/888#issuecomment-30310) out). Do you suggest to check it for `NotFoundError`?
dstepanov-yadro marked this conversation as resolved
aarifullin force-pushed feature/872-object_svc_ape from e19109a3bb to 3d62af38e9 2024-01-10 11:55:08 +00:00 Compare
aarifullin force-pushed feature/872-object_svc_ape from 3d62af38e9 to accad44c40 2024-01-10 12:29:42 +00:00 Compare
aarifullin force-pushed feature/872-object_svc_ape from accad44c40 to d2b9a262d1 2024-01-11 12:58:54 +00:00 Compare
Author
Member

Sorry, the source branch has been accidentally created in origin stream. The PR is moved to here

Sorry, the source branch has been accidentally created in origin stream. The PR is moved to [here](https://git.frostfs.info/TrueCloudLab/frostfs-node/pulls/903)
aarifullin closed this pull request 2024-01-11 13:21:45 +00:00
aarifullin deleted branch feature/872-object_svc_ape 2024-01-11 13:21:54 +00:00
All checks were successful
DCO action / DCO (pull_request) Successful in 1m35s
Required
Details
Vulncheck / Vulncheck (pull_request) Successful in 3m13s
Required
Details
Build / Build Components (1.21) (pull_request) Successful in 4m2s
Required
Details
Build / Build Components (1.20) (pull_request) Successful in 4m11s
Required
Details
Tests and linters / Staticcheck (pull_request) Successful in 4m2s
Required
Details
Tests and linters / Lint (pull_request) Successful in 5m42s
Required
Details
Tests and linters / Tests (1.20) (pull_request) Successful in 10m36s
Required
Details
Tests and linters / Tests (1.21) (pull_request) Successful in 10m59s
Required
Details
Tests and linters / Tests with -race (pull_request) Successful in 11m34s
Required
Details

Pull request closed

Sign in to join this conversation.
No reviewers
TrueCloudLab/storage-core-committers
TrueCloudLab/storage-core-developers
No milestone
No project
No assignees
4 participants
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference: TrueCloudLab/frostfs-node#888
No description provided.