ape: Make services use bearer chains fed router #1216

aarifullin · 2024-07-01T15:50:12Z

aarifullin commented

2024-07-01 15:50:12 +00:00

Unlike default chain router, BearerChainFedRouter performs checks for overrides defined in the bearer token;
Add unit-test for the introduced router;
Refactor object and tree service - they should instantiate chain router cheking the bearer token. If there are no bearer token rules, then defaul chain router is used.
Fix unit-tests.

* Unlike default chain router, `BearerChainFedRouter` performs checks for overrides defined in the bearer token; * Add unit-test for the introduced router; * Refactor object and tree service - they should instantiate chain router cheking the bearer token. If there are no bearer token rules, then defaul chain router is used. * Fix unit-tests.

aarifullin force-pushed feat/bt_router from 2459a508ee to 51e35f200f

2024-07-01 15:51:42 +00:00

Compare

aarifullin requested review from storage-core-committers 2024-07-01 15:52:45 +00:00

aarifullin requested review from dkirillov 2024-07-01 15:52:53 +00:00

aarifullin requested review from storage-core-developers 2024-07-01 15:53:19 +00:00

dkirillov approved these changes 2024-07-02 06:22:54 +00:00

dkirillov left a comment

LGTM

acid-ant reviewed 2024-07-02 06:56:11 +00:00

pkg/ape/router/bearer_overrides.go Outdated

					
				@ -0,0 +47,4 @@

					}

					return &bearerChainFedRouter{

						// morphReaderStub is the trick to check only local overrides skipping policy contract chains.

acid-ant commented

2024-07-02 06:56:11 +00:00

How about to pass nil instead of stub and fix policy-engine a bit?

How about to pass `nil` instead of `stub` and fix `policy-engine` a bit?

aarifullin commented

2024-07-02 08:25:14 +00:00

From the point of defaultChainRouter morph rules are mandatory. I could disable the check by passing nil, but I prefer thorough refactoring of defaultChainRouter in the future.

From the point of [defaultChainRouter](https://git.frostfs.info/TrueCloudLab/policy-engine/src/branch/master/pkg/engine/chain_router.go) morph rules are mandatory. I could disable the check by passing `nil`, but I prefer *thorough refactoring* of `defaultChainRouter` in the future.

👍 1

acid-ant marked this conversation as resolved

acid-ant approved these changes 2024-07-02 08:28:26 +00:00

aarifullin force-pushed feat/bt_router from 51e35f200f to 11173991c1

2024-07-02 08:39:39 +00:00

Compare

fyrchik reviewed 2024-07-03 08:49:21 +00:00

cmd/frostfs-node/policy_engine.go Outdated

					
				@ -15,11 +13,9 @@ import (

				)

				type accessPolicyEngine struct {

					mtx sync.RWMutex

fyrchik commented

2024-07-02 12:21:22 +00:00

Why was it protected with mutex before?

aarifullin commented

2024-07-03 10:42:07 +00:00

TBH, I don't remember. As far as I can recall I put this mutex guard had been added on someone's demand in pull request's review and probably the design was changed but mutex has been left.
When I glanced at the code again I found this guard useless.
After I removed this guard I tested the storage node with k6 (10 writers, 10 readers) - I found no concurrency panics

TBH, I don't remember. As far as I can recall I put this mutex guard had been added on someone's demand in pull request's review and probably the design was changed but mutex has been left. When I glanced at the code again I found this guard useless. After I removed this guard I tested the storage node with k6 (10 writers, 10 readers) - I found no concurrency panics

pkg/ape/router/bearer_overrides.go Outdated

					
				@ -0,0 +71,4 @@

						return

					}

					status, ruleFound, err = br.morphRuleChainStorageRouter.IsAllowed(name, rt, r)

fyrchik commented

2024-07-03 08:49:17 +00:00

There are container chains in the bearer token which are overridden, so no chains from the contract should be applied. It it true for this implementation?

There are container chains in the bearer token which are _overridden_, so no chains from the contract should be applied. It it true for this implementation?

aarifullin commented

2024-07-03 10:53:38 +00:00

Let's analyze the implementation

No bearer token: The standard scheme with default router is used as it was earlier (seems OK)
Bearer token bears the chains for the target container:

Local overrides are checked always first -> local overrides override both bearer token and policy contract policies (seems OK)
If local overrides do not match, then we check if the bearer token's chain matches the request properties. If it does not, it goes on checking in policy contract. That means the bearer token overrides the contract's chains but does not override local overrides.

So, if the issuer of the bearer token is the container's owner and bearer token contains allowing chains (overrides) for an operation that means an operation would be applied despite policy contract may deny it

Let's analyze the implementation 1. *No bearer token*: The standard scheme with default router is used as it was earlier (seems OK) 2. *Bearer token bears the chains for the target container*: - Local overrides are checked always first -> local overrides *override* both bearer token and policy contract policies (seems OK) - If local overrides do not match, then we check if the bearer token's chain matches the request properties. If it does *not*, it goes on checking in policy contract. That means the bearer token *overrides* the contract's chains but does not override local overrides. So, if the issuer of the bearer token is the container's owner and bearer token contains allowing chains (overrides) for an operation that means an operation would be applied despite policy contract may *deny* it

fyrchik commented

2024-07-03 11:05:30 +00:00

If it does not, it goes on checking in policy contract.

And if we have some restrictions on the namespace level?
It seems I can circumvent them by emitting a bearer token for myself.

>If it does not, it goes on checking in policy contract. And if we have some restrictions on the namespace level? It seems I can circumvent them by emitting a bearer token for myself.

aarifullin commented

2024-07-03 11:23:01 +00:00

Okay. You mean that a bearer token can be used as kind of exploit to bypass restrictions on namespace level

Earlier a bearer token lived in a different paradigm - everything was considered on container level. So, a bearer token allowed to bypass restrictions within eACL table, but that wasn't the exploit
A bearer token should deal with object operations (I can add this checks) only as it can be validated only for container target (see isValidBearer)
Namespace policies, in a good way, set container operation restrictions. Yes, we can set restriction on object operation within a namespace but there is no agreement yet. If we agree a namespace policy with object operations has a higher priority, then... we must redesign policy-engine again

Okay. You mean that a bearer token can be used as kind of exploit to bypass restrictions on namespace level 1. Earlier a bearer token lived in a different paradigm - everything was considered on container level. So, a bearer token allowed to bypass restrictions within eACL table, but that wasn't the exploit 2. A bearer token should deal with object operations (I can add this checks) only as it can be validated only for container target (see `isValidBearer`) 3. Namespace policies, in a good way, set container operation restrictions. Yes, we can set restriction on object operation within a namespace but there is no agreement yet. If we agree a namespace policy with object operations has a higher priority, then... we must redesign policy-engine again

aarifullin force-pushed feat/bt_router from 11173991c1 to 48022de5ef

2024-07-04 10:45:01 +00:00

Compare

aarifullin requested review from dkirillov 2024-07-04 10:48:56 +00:00

aarifullin requested review from acid-ant 2024-07-04 10:48:57 +00:00

aarifullin commented

2024-07-04 10:54:17 +00:00

We've had a discussion with @fyrchik (see also this comm).
So, we agreed that namespace-defined chains from Policy contract have got higher priority. Then the router should check bearer token overrides if they're defined for the container target.

An important point:

NoRuleFound (no matches with request) over bearer token chain rules is returned immediately - even morph storage may contain allowing rule for the target. Thus bearer token totally overrides container target rules in morph storage

We've had a discussion with @fyrchik (see also [this comm](https://git.frostfs.info/TrueCloudLab/frostfs-node/pulls/1216#issuecomment-43080)). So, we agreed that namespace-defined chains from Policy contract have got higher priority. Then the router should check bearer token overrides if they're defined for the container target. ### An important point: `NoRuleFound` (no matches with request) over bearer token chain rules is returned immediately - even morph storage *may* contain allowing rule for the target. Thus bearer token totally *overrides* container target rules in morph storage

acid-ant approved these changes 2024-07-04 11:04:03 +00:00

dstepanov-yadro approved these changes 2024-07-05 13:50:06 +00:00

dstepanov-yadro commented

2024-07-05 14:14:50 +00:00

It looks like now a local override can allow any operations, regardless of what rules are in the chain. For example, if local override has allow everything for all, but blockchain contract contains deny all operation for all except owner, then local override's allow everything for all wins.
I think that local override must only reduce access rights, but not extend.

It looks like now a local override can _allow_ any operations, regardless of what rules are in the chain. For example, if local override has `allow everything for all`, but blockchain contract contains `deny all operation for all except owner`, then local override's `allow everything for all` wins. I think that local override must only reduce access rights, but not extend.

fyrchik merged commit 0c2b6f3dac into master

2024-07-05 18:26:56 +00:00

fyrchik referenced this pull request from a commit

2024-07-05 18:26:59 +00:00

[#1216] ape: Introduce `BearerChainFeedRouter`

fyrchik referenced this pull request from a commit

2024-07-05 18:26:59 +00:00

[#1216] ape: Make services use bearer chains fed router

Sign in to join this conversation.

No reviewers