ape, adm, container: Form APE requests properly #940
No reviewers
Labels
No labels
P0
P1
P2
P3
badger
frostfs-adm
frostfs-cli
frostfs-ir
frostfs-lens
frostfs-node
good first issue
triage
Infrastructure
blocked
bug
config
discussion
documentation
duplicate
enhancement
go
help wanted
internal
invalid
kludge
observability
perfomance
question
refactoring
wontfix
No milestone
No project
No assignees
5 participants
Notifications
Due date
No due date set.
Dependencies
No dependencies set.
Reference: TrueCloudLab/frostfs-node#940
Loading…
Reference in a new issue
No description provided.
Delete branch "aarifullin/frostfs-node:fix/934-invalid_ape_request"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
-- Those methods that can access already existing containers and thus can get container properties should read namespace from Zone property. If Zone is not set, take a namespace for root.
-- Otherwise, define namespaces by owner ID via frostfs-id contract.
-- Improve unit-tests, consider more cases.
@ -34,4 +34,2 @@
}
// parseSubject from https://git.frostfs.info/TrueCloudLab/frostfs-contract/src/commit/dd5919348da9731f24504e7bc485516c2ba5f11c/frostfsid/client/client.go#L592
func parseSubject(structArr []stackitem.Item) (*frostfsidclient.Subject, error) {
The current implementation does not work - I was obliged to slightly fix it
@ -38,6 +38,10 @@ func Container(c *config.Config) util.Uint160 {
return contractAddress(c, "container")
}
func FrostfsID(c *config.Config) util.Uint160 {
Please, write a comment similar to other functions.
I wonder why do we need this section -- it has been here since before-NNS times, now everything can be fetched.
ape, adm, container: Form APE requests properlyto WIP: ape, adm, container: Form APE requests properly@ -492,0 +980,4 @@
})
require.NoError(t, err)
req := &container.GetRequest{}
These 5 lines create dummy request and repeat in every test, can we move them to a function to make tests a bit more readable?
It seems private key is used only to sign the request, probably a good idea to return already signed request from this function.
@ -492,0 +980,4 @@
})
require.NoError(t, err)
req := &container.GetRequest{}
These 5 lines create dummy request and repeat in every test, can we move them to a function to make tests a bit more readable?
@ -492,0 +980,4 @@
})
require.NoError(t, err)
req := &container.GetRequest{}
These 5 lines create dummy request and repeat in every test, can we move them to a function to make tests a bit more readable?
Have fixed for all cases :)
Hm, either I don't see it or you haven't pushed the update?
Sorry, I forgot to push yesterday. Now it is fine
2d2ab42e83
to6b342c34ec
WIP: ape, adm, container: Form APE requests properlyto ape, adm, container: Form APE requests properly@ -162,9 +179,14 @@ func (ac *apeChecker) Put(ctx context.Context, req *container.PutRequest) (*cont
nativeschema.PropertyKeyActorRole: role,
}
namespace, err := ac.namespaceByOwner(req.GetBody().GetContainer().GetOwnerID())
Should we check that the user is trying to create a container in the namespace it belongs to?
Otherwise it seems he can bypass rule that restrict any container creation in
namespace1
if he belongs tonamespace2
I was thinking about this scenario.
If we are putting a conatiner where a namespace cut from
Zone()
differs from a namespace got from FFsID contract (by OwnerID), then the check is performed on "from FFsID"-namespace anyway. When the creation is pushed to the sidechain to be processed by an inner-ring node, it performs this check and denies to create a conatiner because of different containers.But now I think this check should be used here because using "from FFsID"-namespace is implicit and an error may seem not obvious
Actually, now I can create container in
namespace1.ns
zone if I use key fromnamespace2
and have rule that forbids any creation innamespace1
. But you said that I couldn't do that. Am I missing something?Have you added a subject into frostfs-id contract?
BTW, I have added validation
Yes
Let's take this step by step.
First, we need to register
ns
,namespace2.ns
in NNS contrace (frostfs-adm
util does not support registring yet, I do it locally in mishmashed manner :)).Then, when
namespace2
exists, we need to bind an owner with this namespace.Note
"022bb4041c50d607ff871dec7e4cd7778388e0ea6849d84ccbd9aa8f32e16a8131"
is got from owner in unit-test created above.So, let's go on:
Therefore, you cannot create container in
"namespace1.ns"
6b342c34ec
todd86e88a57
@ -125,9 +137,14 @@ func (ac *apeChecker) List(ctx context.Context, req *container.ListRequest) (*co
nativeschema.PropertyKeyActorRole: role,
}
namespace, err := ac.namespaceByOwner(req.GetBody().GetOwnerID())
Shouldn't we get namespace from actor instead?
Although we discussed this privately with @dkirillov, I am going to describe the idea:
This may lead to a situation when resource name can be calculated incorrectly, because an actor can be out of owner's namespace
@ -116,2 +116,4 @@
}
if target.Type == policyengine.Namespace && target.Name == "" {
target.Name = "root"
Such renaming is actual only for local overrides or also for rules in contract?
Only for local overrides (this is because boltdb does not allow to create buckets with empty names)
I also would like to highlight that listing with empty namespace does not behave the same way as in Policy contract
dd86e88a57
to7eb90e7c00
@ -517,0 +596,4 @@
return err
}
cntNamespace, hasNamespace := strings.CutSuffix(cnrSDK.ReadDomain(cnr).Zone(), ".ns")
if hasNamespace && cntNamespace != ownerIDNamespace {
This doesn't work if we are trying to create container in
container
zone (aka root namespace)Let me check
Sorry, I didn't get your point.
Let's consider all cases
cnr.zone
is empty, i.e.Zone()
returns"container"
;ownerIDNamespace
is emptycnr.zone
is"container"
, i.e.Zone()
returns"container"
;ownerIDNamespace
is emptycnr.zone
isnamespace1.ns
;ownerIDNamespace
is emptycnr.zone
isnamespace1.ns
;ownerIDNamespace
isnamespace1
cnr.zone
isnamespace1.ns
;ownerIDNamespace
isnamespace2
(1) -
hasNamespace = false
-> returnsnil
. Correct(2) -
hasNamespace = false
-> returnsnil
. Correct(3) -
hasNamespace = true
-> returnserror
. Correct, because frostfs-id has not bound the owner with the namespace(4) -
hasNamespace = true
-> returnsnil
. Correct, because frostfs-id bound the owner with the namespace(5) -
hasNamespace = true
-> returnserror
. Correct, because frostfs-id has not bound the owner with thenamespace1
, onlynamespace2
Why are the cases 1 and 2 correct? We will get no error here and fetch rules in
ownerIDnamespace
but if we can have denial rules in root namespace we still create container there (but shouldn't I suppose)I still do not see any contradiction here. If container is being put without domain and an owner ID is not found in frostfs-id contract, then we suppose a container is created within root namespace
And because it is empty - we fetch rules for root namespace
If denial rules are defined for root namespace, they will work out.
This validation is rather needed to make sure a client sets correct zone (if he sets it for some reason - although, it is not required)
Well, I want the following test to pass:
I have updated PR with more strict validation for namespaces for Put and List. Your test-case above is no longer actual. Please, check this out
If owner ID can be found within frostfs-id and no domain params are set in cnr before Put - container won't be created, because a client must set these params by
SetDomain
. Therefore, a resource name for such a container will define namespace (cannot be root so)7eb90e7c00
to8fd5d3b8cc
8fd5d3b8cc
to0587fce97a