TrueCloudLab/frostfs-node
21
1
Fork 27
Code Issues 108 Pull requests 3 Projects Releases 1 Activity Actions

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

Closed
aarifullin wants to merge 1 commit from aarifullin/frostfs-node:fix/cnt_svc_ape_target into master
pull from: aarifullin/frostfs-node:fix/cnt_svc_ape_target
merge into: TrueCloudLab:master
Conversation 1 Commits 1 Files changed 2 +67 -60
2 changed files with 67 additions and 60 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

12
pkg/services/container/ape.go
View file

@ -327,9 +327,15 @@ func (ac *apeChecker) validateContainerBoundedOperation(ctx context.Context, con
reqProps,
)
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
Outdated
Review
Copy link

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
Outdated
Review
Copy link

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
Outdated
Review
Copy link

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
Outdated
Review
Copy link

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
Outdated
Review
Copy link

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
userTarget := policyengine.UserTarget(fmt.Sprintf("%s:%s", namespace, pk.Address()))
rt.User = &userTarget
rt.Groups = make([]policyengine.Target, len(groups))
for i := range groups {
rt.Groups[i] = policyengine.GroupTarget(groups[i])
}
s, found, err := ac.router.IsAllowed(apechain.Ingress, rt, request)
if err != nil {
return err
}

115
pkg/services/container/ape_test.go
View file

@ -102,7 +102,7 @@ func testAllowThenDenyGetContainerRuleDefined(t *testing.T) {
nm.netmaps[nm.currentEpoch] = &testNetmap
nm.netmaps[nm.currentEpoch-1] = &testNetmap
addDefaultAllowGetPolicy(t, router, contID)
addDefaultAllowGetPolicy(t, router)
req := &container.GetRequest{}
req.SetBody(&container.GetRequestBody{})
@ -117,7 +117,7 @@ func testAllowThenDenyGetContainerRuleDefined(t *testing.T) {
_, err = apeSrv.Get(context.Background(), req)
require.NoError(t, err)
_, _, err = router.MorphRuleChainStorage().AddMorphRuleChain(chain.Ingress, engine.ContainerTarget(contID.EncodeToString()), &chain.Chain{
_, _, err = router.MorphRuleChainStorage().AddMorphRuleChain(chain.Ingress, engine.NamespaceTarget(""), &chain.Chain{
Rules: []chain.Rule{
{
Status: chain.AccessDenied,
@ -324,7 +324,7 @@ func testDenyGetContainerForOthers(t *testing.T) {
nm.netmaps[nm.currentEpoch] = &testNetmap
nm.netmaps[nm.currentEpoch-1] = &testNetmap
_, _, err := router.MorphRuleChainStorage().AddMorphRuleChain(chain.Ingress, engine.ContainerTarget(contID.EncodeToString()), &chain.Chain{
_, _, err := router.MorphRuleChainStorage().AddMorphRuleChain(chain.Ingress, engine.NamespaceTarget(""), &chain.Chain{
Rules: []chain.Rule{
{
Status: chain.AccessDenied,
@ -424,7 +424,7 @@ func testDenyGetContainerByUserClaimTag(t *testing.T) {
nm.netmaps[nm.currentEpoch] = &testNetmap
nm.netmaps[nm.currentEpoch-1] = &testNetmap
_, _, err = router.MorphRuleChainStorage().AddMorphRuleChain(chain.Ingress, engine.ContainerTarget(contID.EncodeToString()), &chain.Chain{
_, _, err = router.MorphRuleChainStorage().AddMorphRuleChain(chain.Ingress, engine.NamespaceTarget(""), &chain.Chain{
Rules: []chain.Rule{
{
Status: chain.AccessDenied,
@ -522,7 +522,7 @@ func testDenyGetContainerByIP(t *testing.T) {
nm.netmaps[nm.currentEpoch] = &testNetmap
nm.netmaps[nm.currentEpoch-1] = &testNetmap
_, _, err = router.MorphRuleChainStorage().AddMorphRuleChain(chain.Ingress, engine.ContainerTarget(contID.EncodeToString()), &chain.Chain{
_, _, err = router.MorphRuleChainStorage().AddMorphRuleChain(chain.Ingress, engine.NamespaceTarget(""), &chain.Chain{
Rules: []chain.Rule{
{
Status: chain.AccessDenied,
@ -621,7 +621,7 @@ func testDenyGetContainerByGroupID(t *testing.T) {
nm.netmaps[nm.currentEpoch] = &testNetmap
nm.netmaps[nm.currentEpoch-1] = &testNetmap
_, _, err = router.MorphRuleChainStorage().AddMorphRuleChain(chain.Ingress, engine.ContainerTarget(contID.EncodeToString()), &chain.Chain{
_, _, err = router.MorphRuleChainStorage().AddMorphRuleChain(chain.Ingress, engine.NamespaceTarget(""), &chain.Chain{
Rules: []chain.Rule{
{
Status: chain.AccessDenied,
@ -1213,7 +1213,7 @@ func TestValidateContainerBoundedOperation(t *testing.T) {
components.containerReader.c[contID] = &containercore.Container{Value: testContainer}
initTestNetmap(components.netmap)
_, _, err := components.engine.MorphRuleChainStorage().AddMorphRuleChain(chain.Ingress, engine.ContainerTarget(contID.EncodeToString()), &chain.Chain{
_, _, err := components.engine.MorphRuleChainStorage().AddMorphRuleChain(chain.Ingress, engine.NamespaceTarget(""), &chain.Chain{
Rules: []chain.Rule{
{
Status: chain.AccessDenied,
@ -1255,7 +1255,7 @@ func TestValidateContainerBoundedOperation(t *testing.T) {
components.containerReader.c[contID] = &containercore.Container{Value: testContainer}
initTestNetmap(components.netmap)
_, _, err := components.engine.MorphRuleChainStorage().AddMorphRuleChain(chain.Ingress, engine.ContainerTarget(contID.EncodeToString()), &chain.Chain{
_, _, err := components.engine.MorphRuleChainStorage().AddMorphRuleChain(chain.Ingress, engine.NamespaceTarget(""), &chain.Chain{
Rules: []chain.Rule{
{
Status: chain.AccessDenied,
@ -1282,7 +1282,7 @@ func TestValidateContainerBoundedOperation(t *testing.T) {
})
require.NoError(t, err)
addDefaultAllowGetPolicy(t, components.engine, contID)
addDefaultAllowGetPolicy(t, components.engine)
req := initTestGetContainerRequest(t, contID)
@ -1325,7 +1325,7 @@ func TestValidateContainerBoundedOperation(t *testing.T) {
})
require.NoError(t, err)
addDefaultAllowGetPolicy(t, components.engine, contID)
addDefaultAllowGetPolicy(t, components.engine)
req := initTestGetContainerRequest(t, contID)
@ -1341,50 +1341,7 @@ func TestValidateContainerBoundedOperation(t *testing.T) {
components.containerReader.c[contID] = &containercore.Container{Value: testContainer}
initTestNetmap(components.netmap)
_, _, err := components.engine.MorphRuleChainStorage().AddMorphRuleChain(chain.Ingress, engine.ContainerTarget(contID.EncodeToString()), &chain.Chain{
Rules: []chain.Rule{
{
Status: chain.AccessDenied,
Actions: chain.Actions{
Names: []string{
nativeschema.MethodGetContainer,
},
},
Resources: chain.Resources{
Names: []string{
fmt.Sprintf(nativeschema.ResourceFormatRootContainer, contID.EncodeToString()),
},
},
Condition: []chain.Condition{
{
Kind: chain.KindRequest,
Key: nativeschema.PropertyKeyActorRole,
Value: nativeschema.PropertyValueContainerRoleOthers,
Op: chain.CondStringEquals,
},
},
},
},
})
require.NoError(t, err)
addDefaultAllowGetPolicy(t, components.engine, contID)
req := initTestGetContainerRequest(t, contID)
err = components.apeChecker.validateContainerBoundedOperation(ctxWithPeerInfo(), req.GetBody().GetContainerID(), req.GetMetaHeader(), req.GetVerificationHeader(), nativeschema.MethodGetContainer)
require.NoError(t, err)
})
t.Run("check testdomain-defined container in testdomain-defined container target rule", func(t *testing.T) {
t.Parallel()
components := newTestAPEServer()
contID, testContainer := initTestContainer(t, true)
components.containerReader.c[contID] = &containercore.Container{Value: testContainer}
initTestNetmap(components.netmap)
_, _, err := components.engine.MorphRuleChainStorage().AddMorphRuleChain(chain.Ingress, engine.ContainerTarget(contID.EncodeToString()), &chain.Chain{
_, _, err := components.engine.MorphRuleChainStorage().AddMorphRuleChain(chain.Ingress, engine.NamespaceTarget(testDomainName), &chain.Chain{
Rules: []chain.Rule{
{
Status: chain.AccessDenied,
@ -1411,7 +1368,51 @@ func TestValidateContainerBoundedOperation(t *testing.T) {
})
require.NoError(t, err)
addDefaultAllowGetPolicy(t, components.engine, contID)
addDefaultAllowGetPolicy(t, components.engine)
req := initTestGetContainerRequest(t, contID)
err = components.apeChecker.validateContainerBoundedOperation(ctxWithPeerInfo(), req.GetBody().GetContainerID(), req.GetMetaHeader(), req.GetVerificationHeader(), nativeschema.MethodGetContainer)
aErr := apeErr(nativeschema.MethodGetContainer, chain.AccessDenied)
require.ErrorContains(t, err, aErr.Error())
})
t.Run("check testdomain-defined container in testdomain-defined container target rule", func(t *testing.T) {
t.Parallel()
components := newTestAPEServer()
contID, testContainer := initTestContainer(t, true)
components.containerReader.c[contID] = &containercore.Container{Value: testContainer}
initTestNetmap(components.netmap)
_, _, err := components.engine.MorphRuleChainStorage().AddMorphRuleChain(chain.Ingress, engine.NamespaceTarget(""), &chain.Chain{
Rules: []chain.Rule{
{
Status: chain.AccessDenied,
Actions: chain.Actions{
Names: []string{
nativeschema.MethodGetContainer,
},
},
Resources: chain.Resources{
Names: []string{
fmt.Sprintf(nativeschema.ResourceFormatNamespaceContainer, testDomainName, contID.EncodeToString()),
},
},
Condition: []chain.Condition{
{
Kind: chain.KindRequest,
Key: nativeschema.PropertyKeyActorRole,
Value: nativeschema.PropertyValueContainerRoleOthers,
Op: chain.CondStringEquals,
},
},
},
},
})
require.NoError(t, err)
addDefaultAllowGetPolicy(t, components.engine)
req := initTestGetContainerRequest(t, contID)
@ -1565,8 +1566,8 @@ func initListRequest(t *testing.T, actorPK *keys.PrivateKey, ownerPK *keys.Priva
return req
}
func addDefaultAllowGetPolicy(t *testing.T, e engine.Engine, contID cid.ID) {
_, _, err := e.MorphRuleChainStorage().AddMorphRuleChain(chain.Ingress, engine.ContainerTarget(contID.EncodeToString()), &chain.Chain{
func addDefaultAllowGetPolicy(t *testing.T, e engine.Engine) {
_, _, err := e.MorphRuleChainStorage().AddMorphRuleChain(chain.Ingress, engine.NamespaceTarget(""), &chain.Chain{
Rules: []chain.Rule{
{
Status: chain.Allow,