generated from TrueCloudLab/basic
WIP: chain: Introduce new condition operations #93
No reviewers
TrueCloudLab/storage-core-committers
TrueCloudLab/storage-core-developers
TrueCloudLab/storage-services-committers
TrueCloudLab/storage-services-developers
Labels
No labels
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
3 participants
Notifications
Due date
No due date set.
Dependencies
No dependencies set.
Reference: TrueCloudLab/policy-engine#93
Loading…
Reference in a new issue
No description provided.
Delete branch "aarifullin/policy-engine:feat/cond_if_exists"
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?
Property()
: since it returns two values - the second indicates whether the property exists;Context
TrueCloudLab/frostfs-node#1406
fc750ccedd
to687e18c50d
@ -175,1 +212,3 @@
return val != c.Value
return exists && val != c.Value
case CondStringNotEqualsIfExists:
return !exists || val != c.Value
@alexvanin @dkirillov could you recheck this bit?
I am baffled by this sentence from https://docs.aws.amazon.com/IAM/latest/UserGuide/reference_policies_elements_condition_operators.html#Conditions_IfExists:
It seems we behave differently. What am I missing?
In current implementation:
s3:x-amz-server-side-encryption
is not set, thenval_found && StringNotEquals(val, "s3:x-amz-server-side-encryption") -> false
that seems fair;s3:x-amz-server-side-encryption
is set, thenval_found && StringNotEquals(val, "s3:x-amz-server-side-encryption") -> true/false
that seems fair;s3:x-amz-server-side-encryption
is not set, thenStringNotEqualsIfExists(...) -> !val_found || StringNotEquals(val, "s3:x-amz-server-side-encryption") -> true
s3:x-amz-server-side-encryption
is set, thenStringNotEqualsIfExists(...) -> !val_found || StringNotEquals(val, "s3:x-amz-server-side-encryption") -> true/false
. If"s3:x-amz-server-side-encryption"
is set to""
, then it returnstrue
Anyway, the current implementation fits to this requirement even for
StringNotEquals
:BTW, this is a consequence of this statement
IfExists" suffix specifies the following: if the condition key is present in the context of the request, process the key as specified in the policy. If the key is not present, evaluate the condition element as true
I'm not sure. The behavior being changed in this PR. Currently such condition evaluates to
true
because missing key has "" (empty) value that doesn't equal to "AES256" (in example)We shouldn't change this behavior I suppose. Moreover AWS behaves the same way
Using this policy
I can put object only if provide tag
environment=production
in requestIt it difficult to add a test here and check with other implementations to be sure?
aa82bd16ae/s3tests_boto3/functional/test_s3.py
Probably, I incorrectly pointed to "current implementation". I meant - "current" like in this PR, not
master
I suppose it's possible, but first we need to know what policies we are about to test
Currently I have results only for deny policies. In AWS there are some problem make bucket public.
These results for AWS.
Yandex badly support policies and we always get AccessDenied
I couldn't validate policies with Allow statement and
PutObject
but I could validate it withPutObjectTagging
.So policy was
Results:
branch with tests https://git.frostfs.info/dkirillov/s3-tests/src/branch/feature/ape_if_exists_conditions
This clarifies the quoted sentence from the IAM doc:
TEST 3: matched -> Allow
, this means that if the attribute is missing thenNotEquals
treats it as if it was empty. Which is logical, given that tests 4-6 have the opposite results (and the opposite operation).For tests 7-12 (ALLOW) we have the behaviour similar to (DENY) in terms of matching.
But I don't understand what is the different between
StringNotEquals
andStringNotEqualsIfExists
?Probably
IfExists
will NOT match in the test 3?Here are the same tests, but for
IfExists
, could you check them too?The same as previous (TEST 1-6 used PutObjectTagging instead of PutObject, TEST 7-12 used PubObject)
P.S. For the history tests 1-6 for the PutObject (It looks weird, probably I missed something)
So we have different results in test 6
Tests 7-12 seem to differ a lot from your previous comment, is there some copy-paste error here?
No, there shouldn't be any mistakes. As for me we got the same match result for pair tests
and it's expected in general (I means symmetric results for Allow/Deny Equals/NotEquals).
So
NotEquals
doesn't differ fromNotEqualsIfExists
, right?Equals
differs fromEqualsIfExists
, though.Hm, I think I have found the discriminator:
Basically NotEquals/NotEqualsIfExists when 0 or 1 key is missing and the second match is false.
If my understanding is correct, the results should be the following:
or no, my brain has melted
It seems no.
I have the following policy (
StringNotEqualsIfExists
can be added only once):And I get the following results:
Currently, in this pull-request - it doesn't, but I believe that's why the doc clarifies:
So, status should be propagated to
Match
function to check...NotEqualsIfExists
operatorsHm, so you have understood this sentence (I have not). Could you give an example of policy, where notequal and notequalifexists behave differently?
Anyway, let's not use negative ifexists in this PR.
OK. I have already removed these operators and updated PR but let me check the tests provided by @dkirillov first
I mean, I confirmed that you was right - these both operators, actually, had had no difference if key is present.
StringNotEquals
returnsfalse
if key is not present (seems OK)StringEqualsIfExists
returnstrue
if key is not present ("If the key is not present, evaluate the condition element as true.")StringNotEqualsIfExists
: returnstrue
if key is not present. Okay, I was wrong about "propagating status" - it doesn't make sense. This mind-blowing sentence that confuses us declares the same behavior for...Not...IfExists
operators ("If the key is not present, evaluate the condition element as true."). Possibly, that's because negated could be mistaken forSo, it must be interpreted only as (corresponding to the specification):
while
Anyway, let's keep these negated operators far away for the goodness sake
@ -16,2 +16,2 @@
func (r *Resource) Property(name string) string {
return r.properties[name]
func (r *Resource) Property(name string) (val string, exists bool) {
val, exists = r.properties[name]
Why don't we write just
return r.properties[name]
?Because compiler doesn't allow this:
687e18c50d
toab67b9028e
Re-review, please. Negated operators have been factored out
cc @fyrchik @dkirillov
@ -174,2 +202,3 @@
return !exists || val == c.Value
case CondStringNotEquals:
return val != c.Value
return exists && val != c.Value
Why have you changed this?
The reason why this PR has been created:
CondStringNotEquals
works out even key is not present (`"" != value -> true) - and we don't want this complicating behavior. If we don't need these changes, then the PR is not needed at all and policy must explicitly specify that a value (got by a key) can't be emptyWe need this change for positive conditions, not the negative ones.
As you can see below, negative conditions are handled differently.
Oh, sorry! I misunderstood this - have been keeping the idea about
...IfExists
operators in my mind@ -171,3 +198,3 @@
panic(fmt.Sprintf("unimplemented: %d", c.Op))
case CondStringEquals:
return val == c.Value
return exists && val == c.Value
Why do we change this?
#93 (comment)
chain: Introduce new condition operationsto WIP: chain: Introduce new condition operationsThe issue #1406 was resolved within
frostfs-node
.The discussions in this PR made me think that the behavior for new and old operators may lead to over complication and unexpected errors. So, it's going to be closed
Pull request closed