Make container service set correct APE-request targets before the check #1504

aarifullin · 2024-11-19T07:42:54Z

aarifullin commented

2024-11-19 07:42:54 +00:00

Request target shouldn't contain container target as container operations can't be checked with container-targeted rules.
Fix unit-tests: container operation must be allowed/denied for namespace container

The scenario in #1503 in short:
We set APE-chain for a container target. While it's being set up in Policy contract (1), GetContainer is performed. GetContainer performs APE-check by multi-target.
If the container target is also set to this multi-target, then it triggers an APE-chain cache refresh, initially with an empty chain list, because (1) is not finished yet. So, we should wait for the cache value invalidation when it's unnecessary in fact.

Close #1503

* Request target shouldn't contain container target as container operations can't be checked with container-targeted rules. * Fix unit-tests: container operation must be allowed/denied for namespace container The scenario in #1503 in short: We set APE-chain for a container target. While it's being set up in `Policy` contract (1), `GetContainer` is performed. `GetContainer` performs APE-check by multi-target. If the container target is also set to this multi-target, then it triggers an APE-chain cache refresh, initially with an empty chain list, because (1) is not finished yet. So, we should wait for the cache value invalidation when it's unnecessary in fact. Close #1503

aarifullin added the

bug

label 2024-11-19 07:42:54 +00:00

aarifullin added 1 commit 2024-11-19 07:42:55 +00:00

[#1503 ] container: Fix APE-request target

Tests and linters / Run gofumpt (pull_request) Successful in 1m21s

Details

DCO action / DCO (pull_request) Successful in 1m58s

Details

Vulncheck / Vulncheck (pull_request) Successful in 2m21s

Details

Pre-commit hooks / Pre-commit (pull_request) Successful in 2m33s

Details

Build / Build Components (pull_request) Successful in 2m43s

Details

Tests and linters / gopls check (pull_request) Successful in 2m58s

Details

Tests and linters / Staticcheck (pull_request) Successful in 3m3s

Details

Tests and linters / Tests (pull_request) Failing after 3m50s

Details

Tests and linters / Lint (pull_request) Successful in 4m1s

Details

Tests and linters / Tests with -race (pull_request) Failing after 5m0s

Details

8408635c94

* Request target shouldn't contain container target as container
  operations can't be checked with container-targeted rules.

Signed-off-by: Airat Arifullin <a.arifullin@yadro.com>

aarifullin requested review from storage-core-committers 2024-11-19 07:44:01 +00:00

aarifullin requested review from storage-core-developers 2024-11-19 07:44:10 +00:00

aarifullin requested review from dkirillov 2024-11-19 07:44:17 +00:00

aarifullin changed title from ~~Make container service set correct APE-request before the check~~ to Make container service set correct APE-request targets before the check

2024-11-19 07:44:39 +00:00

dkirillov approved these changes 2024-11-19 07:49:44 +00:00

Dismissed

fyrchik requested changes 2024-11-19 08:14:38 +00:00

pkg/services/container/ape.go Outdated

					
				@ -330,3 +330,1 @@

					s, found, err := ac.router.IsAllowed(apechain.Ingress,

						policyengine.NewRequestTargetExtended(namespace, id.EncodeToString(), fmt.Sprintf("%s:%s", namespace, pk.Address()), groups),

						request)

					rt := policyengine.NewRequestTargetWithNamespace(namespace)

fyrchik commented

2024-11-19 08:14:36 +00:00

Changing resource here is a functional change which is completely unrelated to cache.
Please, explain why nothing breaks with this change for already existing change.

Changing resource here is a functional change which is _completely_ unrelated to cache. Please, explain why nothing breaks with this change for already existing change.

aarifullin commented

2024-11-19 09:07:54 +00:00

completely

IsAllowed -> checkMorph -> matchMorphRuleChains -> ListMorphRuleChains

Please, explain why nothing breaks with this change for already existing change.

Check Put method or List. It's correct that container id is not set to the multi-target

Initially for Get, this looked like that:

s, found, err := ac.router.IsAllowed(apechain.Ingress, policyengine.NewRequestTargetWithContainer(id.EncodeToString()), request)
if err != nil {
	return err
}

then we left id.EncodeToString() but added namespace. This is nonsense for the container operation GetContainer as container operations must be defined only within namespace, user and groups. Container target defines object operations over container.

This fix indirectly resolves #1503 but from my POV we need this change anyway. I don't like the idea that GetContainer can be allowed/restricted by setting chain to container target . We have namespace for this

> completely [IsAllowed](https://git.frostfs.info/TrueCloudLab/policy-engine/src/branch/master/pkg/engine/chain_router.go#L27) -> [checkMorph](https://git.frostfs.info/TrueCloudLab/policy-engine/src/branch/master/pkg/engine/chain_router.go#L61) -> [matchMorphRuleChains](https://git.frostfs.info/TrueCloudLab/policy-engine/src/branch/master/pkg/engine/chain_router.go#L86) -> [ListMorphRuleChains](https://git.frostfs.info/TrueCloudLab/frostfs-node/src/branch/master/cmd/frostfs-node/policy_engine.go#L46) > Please, explain why nothing breaks with this change for already existing change. Check [Put](https://git.frostfs.info/TrueCloudLab/frostfs-node/src/branch/master/pkg/services/container/ape.go#L229-L234) method or `List`. It's correct that container id is not set to the multi-target Initially for Get, this looked like that: ```go s, found, err := ac.router.IsAllowed(apechain.Ingress, policyengine.NewRequestTargetWithContainer(id.EncodeToString()), request) if err != nil { return err } ``` then we left `id.EncodeToString()` but added namespace. This is nonsense for the container operation `GetContainer` as container operations must be defined only within namespace, user and groups. Container target defines object operations over container. This fix indirectly resolves #1503 but from my POV we need this change anyway. I don't like the idea that `GetContainer` can be allowed/restricted by setting chain to container target . We have namespace for this

aarifullin commented

2024-11-19 09:49:18 +00:00

Moreover, I found a mistake of mine in a unit-test with name "check testdomain-defined container in testdomain namespace target rule".
I believe I did it in haste because the test didn't check what it was supposed to. It passed so far because the default policy (addDefaultAllowGetPolicy) has reset container policy - that was absolutely incorrect

This is a proof why Container target shouldn't be set in the multi-target

Moreover, I found a mistake of mine in a unit-test with name `"check testdomain-defined container in testdomain namespace target rule"`. I believe I did it in haste because the test didn't check what it was supposed to. It passed so far because the default policy (`addDefaultAllowGetPolicy`) has reset container policy - that was absolutely incorrect This is a proof why Container target shouldn't be set in the multi-target

fyrchik commented

2024-11-19 13:35:42 +00:00

IsAllowed -> checkMorph -> matchMorphRuleChains -> ListMorphRuleChains

It doesn't matter, the code in pkg/services/container should know nothing about the cache, it creates request context and applies APE rules. You seem to change what we do here.

from my POV we need this change anyway
I don't like the idea that GetContainer can be allowed/restricted by setting chain to container target.

We also need to stay compatible with existing deployments. My question about "nothing breaks" was because of this, and it has remained unanswered.

Following your logic, what should the resources for object.* operations be?

>IsAllowed -> checkMorph -> matchMorphRuleChains -> ListMorphRuleChains It doesn't matter, the code in `pkg/services/container` should know nothing about the cache, it creates request context and applies APE rules. You seem to change _what_ we do here. >from my POV we need this change anyway >I don't like the idea that GetContainer can be allowed/restricted by setting chain to container target. We also need to stay compatible with existing deployments. My question about "nothing breaks" was because of this, and it has remained unanswered. Following your logic, what should the resources for `object.*` operations be?

aarifullin commented

2024-11-19 19:03:24 +00:00

It doesn't matter, the code in pkg/services/container should know nothing about the cache

The problem with cache pushed me to check the listing from GetContainer operation. I was confused when I saw containerTarget in the multi-target (see explanation below). I agree it looks unrelated and absolutely not obvious but it seems it accidently helped a bit with #1503 (see comment). The proper solution for #1503 is #1506.

Following your logic, what should the resources for object.* operations be?

Object resources and operations are associated with container (and can be with namespace target although it's not correct). If you meant container.* then I associated them with namespace, user and group.

We also need to stay compatible with existing deployments.

I believe that could be checked with auto-tests. We could make sure no client creates GetContainer, DeleteContainer rules within the container target.

Anyway, this problem requires thorough research and testing. I am going to close this PR

> It doesn't matter, the code in pkg/services/container should know nothing about the cache The problem with cache pushed me to check the listing from `GetContainer` operation. I was confused when I saw `containerTarget` in the multi-target (see explanation below). I agree it looks unrelated and absolutely not obvious but it seems it _accidently_ helped a bit with #1503 (see [comment](https://git.frostfs.info/TrueCloudLab/frostfs-node/issues/1503#issuecomment-58429)). The proper solution for #1503 is #1506. > Following your logic, what should the resources for object.* operations be? Object resources and operations are associated with container (and _can be_ with namespace target although it's not correct). If you meant `container.*` then I associated them with namespace, user and group. > We also need to stay compatible with existing deployments. I believe that could be checked with auto-tests. We could make sure no client creates `GetContainer, DeleteContainer` rules within the container target. Anyway, this problem requires **thorough research and testing**. I am going to close this PR

aarifullin force-pushed fix/cnt_svc_ape_target from 8408635c94 to cd97b1a1ba

2024-11-19 09:17:13 +00:00

Compare

aarifullin dismissed dkirillov's review 2024-11-19 09:17:13 +00:00

Reason:

New commits pushed, approval review dismissed automatically according to repository settings

aarifullin force-pushed fix/cnt_svc_ape_target from cd97b1a1ba to bb2488acfe

2024-11-19 09:28:50 +00:00

Compare

aarifullin changed title from ~~Make container service set correct APE-request targets before the check~~ to WIP: Make container service set correct APE-request targets before the check

2024-11-19 09:32:47 +00:00

aarifullin force-pushed fix/cnt_svc_ape_target from bb2488acfe to e883fa35f1

2024-11-19 09:40:02 +00:00

Compare

aarifullin changed title from ~~WIP: Make container service set correct APE-request targets before the check~~ to Make container service set correct APE-request targets before the check

2024-11-19 09:40:13 +00:00

aarifullin referenced this pull request

2024-11-19 17:43:42 +00:00

Should AddChain update/invalidate APE cache? #1503

aarifullin commented

2024-11-19 19:05:36 +00:00

This PR affects sensitive middleware in container service. So, this requires detailed research and, actually, this is not proper fix for #1503.
See this comment

This PR affects sensitive middleware in container service. So, this requires detailed research and, actually, this is not proper fix for #1503. See this [comment](https://git.frostfs.info/TrueCloudLab/frostfs-node/pulls/1504#issuecomment-58432)

aarifullin closed this pull request

2024-11-19 19:05:37 +00:00