From a847f28b01d12a4cf1e0a7c70e5b9befaaf695ac Mon Sep 17 00:00:00 2001 From: Airat Arifullin Date: Fri, 28 Jun 2024 14:38:05 +0300 Subject: [PATCH] [#82] router: Make IsAllow receive client defined overrides * Change `ChainRouter` interface: `IsAllow` should also receive client defined overrides that are checked after local overrides but before morph chains. Client defined overrides are checked for per request. * Fix code that uses `ChainRouter`. * Fix unit-tests. Signed-off-by: Airat Arifullin --- iam/converter_test.go | 14 ++--- iam/policy_test.go | 2 +- pkg/engine/chain_router.go | 52 +++++++++++++++++-- pkg/engine/inmemory/inmemory.go | 4 +- pkg/engine/inmemory/inmemory_test.go | 78 +++++++++++++++++++++++----- pkg/engine/interface.go | 19 ++++++- 6 files changed, 140 insertions(+), 29 deletions(-) diff --git a/iam/converter_test.go b/iam/converter_test.go index 5a57fd7..d3a24e5 100644 --- a/iam/converter_test.go +++ b/iam/converter_test.go @@ -717,13 +717,13 @@ func TestIPConditions(t *testing.T) { req := testutil.NewRequest("s3:CreateBucket", testutil.NewResource(fmt.Sprintf(s3.ResourceFormatS3Bucket, "bkt"), nil), map[string]string{common.PropertyKeyFrostFSSourceIP: "203.0.113.128"}) - status, _, err := s.IsAllowed(chain.S3, engine.NewRequestTargetWithNamespace(""), req) + status, _, err := s.IsAllowed(chain.S3, engine.NewRequestTargetWithNamespace(""), req, engine.NoClientDefined) require.NoError(t, err) require.Equal(t, chain.Allow.String(), status.String()) req = testutil.NewRequest("s3:CreateBucket", testutil.NewResource(fmt.Sprintf(s3.ResourceFormatS3Bucket, "bkt"), nil), map[string]string{common.PropertyKeyFrostFSSourceIP: "203.0.114.0"}) - status, _, err = s.IsAllowed(chain.S3, engine.NewRequestTargetWithNamespace(""), req) + status, _, err = s.IsAllowed(chain.S3, engine.NewRequestTargetWithNamespace(""), req, engine.NoClientDefined) require.NoError(t, err) require.Equal(t, chain.NoRuleFound.String(), status.String()) }) @@ -1092,7 +1092,7 @@ func TestComplexNativeConditions(t *testing.T) { } { t.Run(tc.name, func(t *testing.T) { req := testutil.NewRequest(tc.action, testutil.NewResource(tc.resource, tc.resourceMap), tc.requestMap) - status, _, err := s.IsAllowed("name", engine.NewRequestTargetWithNamespace("ns"), req) + status, _, err := s.IsAllowed("name", engine.NewRequestTargetWithNamespace("ns"), req, engine.NoClientDefined) require.NoError(t, err) require.Equal(t, tc.status.String(), status.String()) }) @@ -1324,7 +1324,7 @@ func TestComplexS3Conditions(t *testing.T) { } { t.Run(tc.name, func(t *testing.T) { req := testutil.NewRequest(tc.action, testutil.NewResource(tc.resource, tc.resourceMap), tc.requestMap) - status, _, err := s.IsAllowed("name", engine.NewRequestTargetWithNamespace("ns"), req) + status, _, err := s.IsAllowed("name", engine.NewRequestTargetWithNamespace("ns"), req, engine.NoClientDefined) require.NoError(t, err) require.Equal(t, tc.status.String(), status.String()) }) @@ -1365,19 +1365,19 @@ func TestS3BucketResource(t *testing.T) { // check we match just "bucket1" resource req := testutil.NewRequest("s3:HeadBucket", testutil.NewResource(fmt.Sprintf(s3.ResourceFormatS3Bucket, bktName1), nil), nil) - status, _, err := s.IsAllowed(chainName, engine.NewRequestTargetWithNamespace(namespace), req) + status, _, err := s.IsAllowed(chainName, engine.NewRequestTargetWithNamespace(namespace), req, engine.NoClientDefined) require.NoError(t, err) require.Equal(t, chain.AccessDenied.String(), status.String()) // check we match just "bucket2" resource req = testutil.NewRequest("s3:HeadBucket", testutil.NewResource(fmt.Sprintf(s3.ResourceFormatS3Bucket, bktName2), nil), nil) - status, _, err = s.IsAllowed(chainName, engine.NewRequestTargetWithNamespace(namespace), req) + status, _, err = s.IsAllowed(chainName, engine.NewRequestTargetWithNamespace(namespace), req, engine.NoClientDefined) require.NoError(t, err) require.Equal(t, chain.Allow.String(), status.String()) // check we also match "bucket2/object" resource req = testutil.NewRequest("s3:PutObject", testutil.NewResource(fmt.Sprintf(s3.ResourceFormatS3BucketObject, bktName2, "object"), nil), nil) - status, _, err = s.IsAllowed(chainName, engine.NewRequestTargetWithNamespace(namespace), req) + status, _, err = s.IsAllowed(chainName, engine.NewRequestTargetWithNamespace(namespace), req, engine.NoClientDefined) require.NoError(t, err) require.Equal(t, chain.Allow.String(), status.String()) } diff --git a/iam/policy_test.go b/iam/policy_test.go index 79d93c6..8063f69 100644 --- a/iam/policy_test.go +++ b/iam/policy_test.go @@ -544,7 +544,7 @@ func TestProcessDenyFirst(t *testing.T) { resource := testutil.NewResource("arn:aws:s3:::test-bucket/object", nil) request := testutil.NewRequest("s3:PutObject", resource, map[string]string{s3.PropertyKeyOwner: mockResolver.users["root/user-name"]}) - status, found, err := s.IsAllowed(chain.S3, engine.NewRequestTarget("ns", ""), request) + status, found, err := s.IsAllowed(chain.S3, engine.NewRequestTarget("ns", ""), request, engine.NoClientDefined) require.NoError(t, err) require.True(t, found) require.Equal(t, chain.AccessDenied, status) diff --git a/pkg/engine/chain_router.go b/pkg/engine/chain_router.go index 830919f..1e607e4 100644 --- a/pkg/engine/chain_router.go +++ b/pkg/engine/chain_router.go @@ -1,6 +1,8 @@ package engine import ( + "fmt" + "git.frostfs.info/TrueCloudLab/policy-engine/pkg/chain" "git.frostfs.info/TrueCloudLab/policy-engine/pkg/resource" ) @@ -24,13 +26,21 @@ func NewDefaultChainRouterWithLocalOverrides(morph MorphRuleChainStorageReader, } } -func (dr *defaultChainRouter) IsAllowed(name chain.Name, rt RequestTarget, r resource.Request) (status chain.Status, ruleFound bool, err error) { +func (dr *defaultChainRouter) IsAllowed(name chain.Name, rt RequestTarget, r resource.Request, clientDefined ClientDefinedOverrides) (status chain.Status, ruleFound bool, err error) { status, ruleFound, err = dr.checkLocal(name, rt, r) if err != nil { return chain.NoRuleFound, false, err } else if ruleFound { // The local overrides have the highest priority and thus - // morph rules are not considered if a local one is found. + // morph rules and client overrides are not considered if a local one is found. + return + } + + status, ruleFound, err = checkClientDefined(name, rt, r, clientDefined) + if err != nil { + return chain.NoRuleFound, false, fmt.Errorf("client defined: %w", err) + } else if ruleFound { + // The client defined overrides have the higher priority then morph chains. return } @@ -61,6 +71,29 @@ func (dr *defaultChainRouter) checkLocal(name chain.Name, rt RequestTarget, r re return } +func checkClientDefined(name chain.Name, rt RequestTarget, r resource.Request, clientDefined ClientDefinedOverrides) (status chain.Status, ruleFound bool, err error) { + if clientDefined == NoClientDefined { + return + } + var ruleFounds []bool + for _, target := range rt.Targets() { + status, ruleFound, err = matchClientDefined(name, target, r, clientDefined) + if err != nil || ruleFound && status != chain.Allow { + return + } + ruleFounds = append(ruleFounds, ruleFound) + } + + status = chain.NoRuleFound + for _, ruleFound = range ruleFounds { + if ruleFound { + status = chain.Allow + break + } + } + return +} + func (dr *defaultChainRouter) checkMorph(name chain.Name, rt RequestTarget, r resource.Request) (status chain.Status, ruleFound bool, err error) { var ruleFounds []bool for _, target := range rt.Targets() { @@ -86,7 +119,16 @@ func (dr *defaultChainRouter) matchLocalOverrides(name chain.Name, target Target if err != nil { return } - status, ruleFound = dr.getStatusFromChains(localOverrides, r) + status, ruleFound = getStatusFromChains(localOverrides, r) + return +} + +func matchClientDefined(name chain.Name, target Target, r resource.Request, clientDefined ClientDefinedOverrides) (status chain.Status, ruleFound bool, err error) { + overrides, err := clientDefined.ListOverrides(name, target) + if err != nil { + return + } + status, ruleFound = getStatusFromChains(overrides, r) return } @@ -95,11 +137,11 @@ func (dr *defaultChainRouter) matchMorphRuleChains(name chain.Name, target Targe if err != nil { return chain.NoRuleFound, false, err } - status, ruleFound = dr.getStatusFromChains(namespaceChains, r) + status, ruleFound = getStatusFromChains(namespaceChains, r) return } -func (dr *defaultChainRouter) getStatusFromChains(chains []*chain.Chain, r resource.Request) (chain.Status, bool) { +func getStatusFromChains(chains []*chain.Chain, r resource.Request) (chain.Status, bool) { var allow bool for _, c := range chains { if status, found := c.Match(r); found { diff --git a/pkg/engine/inmemory/inmemory.go b/pkg/engine/inmemory/inmemory.go index 90287a2..c02a445 100644 --- a/pkg/engine/inmemory/inmemory.go +++ b/pkg/engine/inmemory/inmemory.go @@ -43,6 +43,6 @@ func (im *inmemory) MorphRuleChainStorage() engine.MorphRuleChainStorage { return im.morph } -func (im *inmemory) IsAllowed(name chain.Name, rt engine.RequestTarget, r resource.Request) (status chain.Status, ruleFound bool, err error) { - return im.router.IsAllowed(name, rt, r) +func (im *inmemory) IsAllowed(name chain.Name, rt engine.RequestTarget, r resource.Request, clientDefined engine.ClientDefinedOverrides) (status chain.Status, ruleFound bool, err error) { + return im.router.IsAllowed(name, rt, r, clientDefined) } diff --git a/pkg/engine/inmemory/inmemory_test.go b/pkg/engine/inmemory/inmemory_test.go index fca6935..d048448 100644 --- a/pkg/engine/inmemory/inmemory_test.go +++ b/pkg/engine/inmemory/inmemory_test.go @@ -51,7 +51,7 @@ func TestInmemory(t *testing.T) { "Actor": actor1, }) - status, ok, _ := s.IsAllowed(chain.Ingress, engine.NewRequestTargetWithNamespace(namespace), reqGood) + status, ok, _ := s.IsAllowed(chain.Ingress, engine.NewRequestTargetWithNamespace(namespace), reqGood, engine.NoClientDefined) require.Equal(t, chain.NoRuleFound, status) require.False(t, ok) @@ -141,7 +141,59 @@ func TestInmemory(t *testing.T) { "SourceIP": "10.122.1.20", "Actor": actor1, }) - status, ok, _ := s.IsAllowed(chain.Ingress, engine.NewRequestTarget(namespace, container), reqBadIP) + status, ok, _ := s.IsAllowed(chain.Ingress, engine.NewRequestTarget(namespace, container), reqBadIP, engine.NoClientDefined) + require.Equal(t, chain.AccessDenied, status) + require.True(t, ok) + }) + t.Run("allow by client defined then deny by local overrides", func(t *testing.T) { + reqBadIP := resourcetest.NewRequest("native::object::put", res, map[string]string{ + "SourceIP": "10.122.1.20", + "Actor": actor1, + }) + + clientDef := NewInMemoryLocalOverrides() + clientDef.LocalStorage().AddOverride(chain.Ingress, engine.ContainerTarget(container), &chain.Chain{ + Rules: []chain.Rule{ + { + Status: chain.Allow, + Actions: chain.Actions{Names: []string{"native::object::put"}}, + Resources: chain.Resources{Names: []string{"native::object::abc/*"}}, + Condition: []chain.Condition{ + { + Op: chain.CondStringEquals, + Kind: chain.KindRequest, + Key: "SourceIP", + Value: "10.122.1.20", + }, + }, + }, + }, + }) + + status, ok, _ := s.IsAllowed(chain.Ingress, engine.NewRequestTarget(namespace, container), reqBadIP, clientDef.LocalStorage()) + require.Equal(t, chain.Allow, status) + require.True(t, ok) + + s.LocalStorage().AddOverride(chain.Ingress, engine.ContainerTarget(container), &chain.Chain{ + Rules: []chain.Rule{ + { + Status: chain.AccessDenied, + Actions: chain.Actions{Names: []string{"native::object::put"}}, + Resources: chain.Resources{Names: []string{"native::object::abc/*"}}, + Condition: []chain.Condition{ + { + Op: chain.CondStringEquals, + Kind: chain.KindRequest, + Key: "SourceIP", + Value: "10.122.1.20", + }, + }, + }, + }, + }) + + status, ok, _ = s.IsAllowed(chain.Ingress, engine.NewRequestTarget(namespace, container), reqBadIP, clientDef.LocalStorage()) + // Local overrides have higher priority then client defined overrides. require.Equal(t, chain.AccessDenied, status) require.True(t, ok) }) @@ -151,7 +203,7 @@ func TestInmemory(t *testing.T) { "SourceIP": "10.1.1.13", "Actor": actor2, }) - status, ok, _ := s.IsAllowed(chain.Ingress, engine.NewRequestTarget(namespace, container), reqBadActor) + status, ok, _ := s.IsAllowed(chain.Ingress, engine.NewRequestTarget(namespace, container), reqBadActor, engine.NoClientDefined) require.Equal(t, chain.AccessDenied, status) require.True(t, ok) }) @@ -162,14 +214,14 @@ func TestInmemory(t *testing.T) { status, ok, _ := s.IsAllowed(chain.Ingress, engine.NewRequestTarget(namespace, container), resourcetest.NewRequest("native::object::get", objGood, map[string]string{ "SourceIP": "10.1.1.14", "Actor": actor2, - })) + }), engine.NoClientDefined) require.Equal(t, chain.Allow, status) require.True(t, ok) status, ok, _ = s.IsAllowed(chain.Ingress, engine.NewRequestTarget(namespace, container), resourcetest.NewRequest("native::object::get", objBadAttr, map[string]string{ "SourceIP": "10.1.1.14", "Actor": actor2, - })) + }), engine.NoClientDefined) require.Equal(t, chain.NoRuleFound, status) require.False(t, ok) }) @@ -179,28 +231,28 @@ func TestInmemory(t *testing.T) { "SourceIP": "10.1.1.12", "Actor": actor1, }) - status, ok, _ := s.IsAllowed(chain.Ingress, engine.NewRequestTarget(namespace, container), reqBadOperation) + status, ok, _ := s.IsAllowed(chain.Ingress, engine.NewRequestTarget(namespace, container), reqBadOperation, engine.NoClientDefined) require.Equal(t, chain.AccessDenied, status) require.True(t, ok) }) t.Run("inverted rules", func(t *testing.T) { req := resourcetest.NewRequest("native::object::put", resourcetest.NewResource(object, nil), nil) - status, ok, _ = s.IsAllowed(chain.Ingress, engine.NewRequestTarget(namespace2, container), req) + status, ok, _ = s.IsAllowed(chain.Ingress, engine.NewRequestTarget(namespace2, container), req, engine.NoClientDefined) require.Equal(t, chain.NoRuleFound, status) require.False(t, ok) req = resourcetest.NewRequest("native::object::put", resourcetest.NewResource("native::object::cba/def", nil), nil) - status, ok, _ = s.IsAllowed(chain.Ingress, engine.NewRequestTarget(namespace2, container), req) + status, ok, _ = s.IsAllowed(chain.Ingress, engine.NewRequestTarget(namespace2, container), req, engine.NoClientDefined) require.Equal(t, chain.AccessDenied, status) require.True(t, ok) req = resourcetest.NewRequest("native::object::get", resourcetest.NewResource("native::object::cba/def", nil), nil) - status, ok, _ = s.IsAllowed(chain.Ingress, engine.NewRequestTarget(namespace2, container), req) + status, ok, _ = s.IsAllowed(chain.Ingress, engine.NewRequestTarget(namespace2, container), req, engine.NoClientDefined) require.Equal(t, chain.NoRuleFound, status) require.False(t, ok) }) t.Run("good", func(t *testing.T) { - status, ok, _ = s.IsAllowed(chain.Ingress, engine.NewRequestTarget(namespace, container), reqGood) + status, ok, _ = s.IsAllowed(chain.Ingress, engine.NewRequestTarget(namespace, container), reqGood, engine.NoClientDefined) require.Equal(t, chain.NoRuleFound, status) require.False(t, ok) @@ -221,7 +273,7 @@ func TestInmemory(t *testing.T) { require.NoError(t, err) itemStacksEqual(t, it.Values, toStackItems(container)) - status, ok, _ = s.IsAllowed(chain.Ingress, engine.NewRequestTarget(namespace, container), reqGood) + status, ok, _ = s.IsAllowed(chain.Ingress, engine.NewRequestTarget(namespace, container), reqGood, engine.NoClientDefined) require.Equal(t, chain.NoRuleFound, status) require.False(t, ok) }) @@ -244,7 +296,7 @@ func TestInmemory(t *testing.T) { require.NoError(t, err) itemStacksEqual(t, it.Values, toStackItems(container)) - status, ok, _ = s.IsAllowed(chain.Ingress, engine.NewRequestTarget(namespace, container), reqGood) + status, ok, _ = s.IsAllowed(chain.Ingress, engine.NewRequestTarget(namespace, container), reqGood, engine.NoClientDefined) require.Equal(t, chain.QuotaLimitReached, status) require.True(t, ok) }) @@ -260,7 +312,7 @@ func TestInmemory(t *testing.T) { require.NoError(t, err) itemStacksEqual(t, it.Values, toStackItems(container)) - status, ok, _ = s.IsAllowed(chain.Ingress, engine.NewRequestTarget(namespace, container), reqGood) + status, ok, _ = s.IsAllowed(chain.Ingress, engine.NewRequestTarget(namespace, container), reqGood, engine.NoClientDefined) require.Equal(t, chain.NoRuleFound, status) require.False(t, ok) }) diff --git a/pkg/engine/interface.go b/pkg/engine/interface.go index ab026b3..ae2c3a5 100644 --- a/pkg/engine/interface.go +++ b/pkg/engine/interface.go @@ -8,10 +8,27 @@ import ( "github.com/nspcc-dev/neo-go/pkg/util" ) +type ClientDefinedOverrides interface { + ListOverrides(name chain.Name, target Target) ([]*chain.Chain, error) +} + +type noClientDefinedImpl struct{} + +func (n *noClientDefinedImpl) ListOverrides(_ chain.Name, _ Target) ([]*chain.Chain, error) { + return []*chain.Chain{}, nil +} + +var NoClientDefined ClientDefinedOverrides = &noClientDefinedImpl{} + type ChainRouter interface { // IsAllowed returns status for the operation after all checks. // The second return value signifies whether a matching rule was found. - IsAllowed(name chain.Name, reqTarget RequestTarget, r resource.Request) (status chain.Status, found bool, err error) + // + // IsAllowed can be invoked with overrides defined by a client to check the request. + // This check is performed after local overrides but before checking of chains defined + // in morph storage. + // If a client defines no extra chains, NoClientDefined should be passed. + IsAllowed(name chain.Name, reqTarget RequestTarget, r resource.Request, clientDefined ClientDefinedOverrides) (status chain.Status, found bool, err error) } // LocalOverrideStorage is the interface to manage local overrides defined