[#1249] object: Remove all APE pre-checks in handlers #1252

Merged
fyrchik merged 1 commit from aarifullin/frostfs-node:fix/ape_attr into master 2024-07-18 13:52:49 +00:00
Member
  • Methods Head, Get, GetRangeHash should no longer use APE pre-checks
    as that leads only to incorrect rule chain processing for requests:
    1. Immediate return with NoRuleFound may be unexpected as some Allow
      rule is actually defined but can't be matched yet as it gets no object
      attributes;
    2. Immdediate return with Allow may be incorrect as some Deny rule
      is actually defined but can't bet matched yet as it gets no object
      attirbutes;
    3. Pre-check breaks compatibility for converted EACL-tables.

Close #1249

* Methods `Head`, `Get`, `GetRangeHash` should no longer use APE pre-checks as that leads only to incorrect rule chain processing for requests: 1. Immediate return with `NoRuleFound` may be unexpected as some `Allow` rule is actually defined but can't be matched yet as it gets no object attributes; 2. Immdediate return with `Allow` may be incorrect as some `Deny` rule is actually defined but can't bet matched yet as it gets no object attirbutes; 3. Pre-check breaks compatibility for converted EACL-tables. Close #1249
aarifullin added the
bug
discussion
labels 2024-07-16 17:48:26 +00:00
aarifullin requested review from storage-core-committers 2024-07-16 17:56:08 +00:00
aarifullin requested review from storage-core-developers 2024-07-16 18:08:19 +00:00
dstepanov-yadro approved these changes 2024-07-17 06:38:08 +00:00
fyrchik reviewed 2024-07-17 08:14:54 +00:00
@ -251,2 +261,3 @@
return nil, toStatusErr(err)
}
}
Owner

unrelated to the commit

unrelated to the commit
Author
Member

Fixed

Fixed
aarifullin marked this conversation as resolved
fyrchik reviewed 2024-07-17 08:17:02 +00:00
@ -248,3 +254,3 @@
})
if err != nil {
return nil, toStatusErr(err)
// For `Head`` case `NoRuleFound` status doesn't mean yet that the request is not allowed.
Owner

Where in code will the DENY rule be found afterwise?

Where in code will the DENY rule be found afterwise?
Author
Member

288 line: err = c.apeChecker.CheckAPE(ctx, Prm{...}) on backward check

288 line: `err = c.apeChecker.CheckAPE(ctx, Prm{...})` on backward check
fyrchik reviewed 2024-07-17 08:17:03 +00:00
@ -248,3 +254,3 @@
})
if err != nil {
return nil, toStatusErr(err)
// For `Head`` case `NoRuleFound` status doesn't mean yet that the request is not allowed.
Owner

Where in code will the DENY rule be found afterwise?

Where in code will the DENY rule be found afterwise?
aarifullin marked this conversation as resolved
aarifullin force-pushed fix/ape_attr from db20381f63 to cc03658e69 2024-07-17 08:29:22 +00:00 Compare
fyrchik approved these changes 2024-07-17 08:30:27 +00:00
Member

Just FYI, not very important.

In PR's description:

even CheckAPE returns NoRuleFound

In the commit header:

even APE returns NoRuleFound

In the commit message:

even CheckAPE returns NoRuleFound

It should be even IF, not just even.

Just FYI, not very important. In PR's description: > even CheckAPE returns `NoRuleFound` In the commit header: > even APE returns `NoRuleFound` In the commit message: > even CheckAPE returns `NoRuleFound` It should be `even IF`, not just `even`.
elebedeva reviewed 2024-07-17 08:38:41 +00:00
@ -204,0 +206,4 @@
}
}
// nonAllowStatusError is returned by CheckAPE in the case if non-allow status returned by the chain router.
Member

I suggest rephrasing the comment:
nonAllowStatusError is returned by CheckAPE if the chain router returns non-allow status.

I suggest rephrasing the comment: `nonAllowStatusError is returned by CheckAPE if the chain router returns non-allow status.`
Author
Member

Fixed

Fixed
elebedeva marked this conversation as resolved
elebedeva reviewed 2024-07-17 08:45:07 +00:00
@ -157,3 +157,3 @@
})
if err != nil {
return toStatusErr(err)
// For Get case `NoRuleFound` status doesn't mean yet that the request is not allowed.
Member

For Get request `NoRuleFound` status doesn't mean that the request is not allowed.

```For Get request `NoRuleFound` status doesn't mean that the request is not allowed.```
elebedeva marked this conversation as resolved
elebedeva reviewed 2024-07-17 08:50:24 +00:00
@ -158,2 +158,3 @@
if err != nil {
return toStatusErr(err)
// For Get case `NoRuleFound` status doesn't mean yet that the request is not allowed.
// Probably, the request may be allowed by specific attributes that coudln't be read on forward
Member

This comment is hard to understand. Can you please rephrase it using simpler wording?

This comment is hard to understand. Can you please rephrase it using simpler wording?
Author
Member

Sure!

Rephrased:

`Get` is checked by APE in both direction. The forward check is performed without the object header (see `WithoutHeaderRequest` flag). Thus, the resource properties cannot be fully filled with. This may lead to `NoRuleFound` error.
However, allowing rule might be defined on specific object attributes that may NOT be retrieved on the forward check (if the request is handled by non-container node). But they can be retrieved on the backward check for sure as it handles object init part that contains more context about the object.

What do you think?

Sure! Rephrased: ``` `Get` is checked by APE in both direction. The forward check is performed without the object header (see `WithoutHeaderRequest` flag). Thus, the resource properties cannot be fully filled with. This may lead to `NoRuleFound` error. However, allowing rule might be defined on specific object attributes that may NOT be retrieved on the forward check (if the request is handled by non-container node). But they can be retrieved on the backward check for sure as it handles object init part that contains more context about the object. ``` What do you think?
Member

Much better!
Only one question:

Thus, the resource properties cannot be fully filled with.

Filled with what?

Much better! Only one question: > Thus, the resource properties cannot be fully filled with. Filled with what?
Author
Member

Slightly fixed and changed the text. Check it out! :)

Slightly fixed and changed the text. Check it out! :)
Author
Member

Slightly fixed and changed the text. Check it out! :)

Slightly fixed and changed the text. Check it out! :)
elebedeva marked this conversation as resolved
aarifullin force-pushed fix/ape_attr from cc03658e69 to 47d8eb92c6 2024-07-17 11:42:05 +00:00 Compare
aarifullin force-pushed fix/ape_attr from 47d8eb92c6 to 4ad28b8831 2024-07-17 11:42:51 +00:00 Compare
aarifullin requested review from dstepanov-yadro 2024-07-17 11:47:14 +00:00
aarifullin changed title from [#1249] object: Go on handling even APE returns `NoRuleFound` to [#1249] object: Go on handling even if APE returns `NoRuleFound` 2024-07-17 11:47:15 +00:00
aarifullin requested review from fyrchik 2024-07-17 11:47:32 +00:00
elebedeva requested changes 2024-07-17 12:18:08 +00:00
@ -157,3 +157,3 @@
})
if err != nil {
return toStatusErr(err)
// `Get` is checked by APE in both direction. The forward check is performed without the object header
Member

in both direction

directionS

> in both direction `directionS`
elebedeva marked this conversation as resolved
@ -248,3 +256,3 @@
})
if err != nil {
return nil, toStatusErr(err)
// `Head` is checked by APE in both direction. The forward check is performed without the object header
Member

in both direction

directionS

> in both direction `directionS`
Author
Member

fixed!

fixed!
elebedeva marked this conversation as resolved
dstepanov-yadro approved these changes 2024-07-17 12:37:00 +00:00
aarifullin force-pushed fix/ape_attr from 4ad28b8831 to 639c96a4aa 2024-07-17 13:24:24 +00:00 Compare
aarifullin requested review from dstepanov-yadro 2024-07-17 13:26:53 +00:00
dstepanov-yadro approved these changes 2024-07-17 13:29:06 +00:00
elebedeva approved these changes 2024-07-17 14:12:56 +00:00
fyrchik requested changes 2024-07-17 14:30:35 +00:00
@ -160,0 +162,4 @@
// specific object attributes that aren't retrieved yet. But still they can be retrieved
// on the backward check and the rule may match on the fully filled APE-request.
var statError nonAllowStatusError
if errors.As(err, &statError) && !statError.IsNoRuleFoundStatus() {
Owner

What about EACL-compatible tables? They are processed in order and we can encounter DENY because we skipped some valid ALLOW.
Do we need this check at all if we also check everything afterwards?

What about EACL-compatible tables? They are processed in order and we can encounter DENY because we skipped some valid ALLOW. Do we need this check at all if we also check everything afterwards?
Owner

What about EACL-compatible tables? They are processed in order and we can encounter DENY because we skipped some valid ALLOW.
Do we need this check at all if we also check everything afterwards?

What about EACL-compatible tables? They are processed in order and we can encounter DENY because we skipped some valid ALLOW. Do we need this check at all if we also check everything afterwards?
Author
Member

Do we need this check

It works out when either local overrides or Policy contract defines general denying rule like deny all object operation on this container. So, we've got the deny status immediatly.
I can say - yes, we need as it makes this handling faster

> Do we need this check It works out when either local overrides or Policy contract defines *general* denying rule like `deny all object operation on this container`. So, we've got the deny status immediatly. I can say - yes, we need as it makes this handling faster
Author
Member

Probably, I didn't get your point. Could you clarify, please.

For now I am wondering if going on checking if we get NoRuleFound is correct way. It seems immediate return makes sense only for denying statutes like AccessDenied and QuotaLimitReached

Probably, I didn't get your point. Could you clarify, please. For now I am wondering if going on checking if we get `NoRuleFound` is correct way. It seems immediate return makes sense only for denying statutes like `AccessDenied` and `QuotaLimitReached`
Owner

EACL-compatible tables go over rules in order, so we may skip applying ALLOW rule (because of the missing attributes) and hit DENY, even though it should be ALLOW, had we have all attributes.

Returning DENY faster is certainly beneficial in some cases. However, if we know that the data is incomplete and we will check it again, does it worth the effort?

The proper solution would probably include lazy attribute fetching and sth like NotEnoughData error from the IsAllowed method.

EACL-compatible tables go over rules in order, so we may skip applying ALLOW rule (because of the missing attributes) and hit DENY, even though it should be ALLOW, had we have all attributes. Returning DENY faster is certainly beneficial in some cases. However, if we _know_ that the data is incomplete and we _will_ check it again, does it worth the effort? The proper solution would probably include lazy attribute fetching and sth like NotEnoughData error from the `IsAllowed` method.
Author
Member

Returning DENY faster is certainly beneficial in some cases. However, if we know that the data is incomplete and we will check it again, does it worth the effort?

We had an agreement that any DENY has the highest priority over any ALLOW-ing rule from the point of access policy engine. It's okay if the data is incomplete because it'll be declined by DENY anyway afterwards. That's not the same way like with EACL tables

> Returning DENY faster is certainly beneficial in some cases. However, if we know that the data is incomplete and we will check it again, does it worth the effort? We had an agreement that any `DENY` has the highest priority over any `ALLOW`-ing rule from the point of access policy engine. It's okay if the data is incomplete because it'll be declined by `DENY` anyway afterwards. That's not the same way like with `EACL` tables
Author
Member

Okay, @fyrchik meant if a chain sets MatchTypeFirstMatch flag that means it works like with EACL-table.

So, the conclusion is to remove pre-check as it leads to errors only

Okay, @fyrchik meant if a chain sets `MatchTypeFirstMatch` flag that means it works like with EACL-table. So, the conclusion is to remove pre-check as it leads to errors only
aarifullin force-pushed fix/ape_attr from 639c96a4aa to 33857a6372 2024-07-18 10:30:50 +00:00 Compare
aarifullin changed title from [#1249] object: Go on handling even if APE returns `NoRuleFound` to [#1249] object: Remove all APE pre-checks in handlers 2024-07-18 10:31:27 +00:00
aarifullin force-pushed fix/ape_attr from 33857a6372 to 732851dd8a 2024-07-18 10:34:54 +00:00 Compare
aarifullin requested review from dstepanov-yadro 2024-07-18 10:36:43 +00:00
aarifullin requested review from fyrchik 2024-07-18 10:36:44 +00:00
aarifullin requested review from elebedeva 2024-07-18 10:36:44 +00:00
elebedeva approved these changes 2024-07-18 12:10:59 +00:00
fyrchik merged commit eadcea8df0 into master 2024-07-18 13:52:49 +00:00
Sign in to join this conversation.
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#1252
No description provided.