WIP: chain: Introduce new condition operations #93

Closed
aarifullin wants to merge 1 commit from aarifullin/policy-engine:feat/cond_if_exists into master
Member
  • Introduce new operation with suffix "IfExists" for all string and numeric condition operations;
  • An operation with "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 (see AWS doc)
  • Change the signature for interface method Property(): since it returns two values - the second indicates whether the property exists;
  • This also means that original condition operations for string and number comparison is slightly changed: if property doesn't exist, then the condition is not evaluated.

Context

TrueCloudLab/frostfs-node#1406

* Introduce new operation with suffix "IfExists" for all string and numeric condition operations; * An operation with "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 (see [AWS doc](https://docs.aws.amazon.com/IAM/latest/UserGuide/reference_policies_elements_condition_operators.html#Conditions_IfExists)) * Change the signature for interface method `Property()`: since it returns two values - the second indicates whether the property exists; * This also means that original condition operations for string and number comparison is slightly changed: if property doesn't exist, then the condition is not evaluated. ### Context https://git.frostfs.info/TrueCloudLab/frostfs-node/issues/1406
aarifullin added the
discussion
label 2024-10-02 10:41:44 +00:00
aarifullin added 1 commit 2024-10-02 10:41:44 +00:00
[#XX] chain: Introduce new condition operations
Some checks failed
DCO action / DCO (pull_request) Failing after 31s
Tests and linters / Staticcheck (pull_request) Successful in 1m2s
Tests and linters / Tests (pull_request) Successful in 1m27s
Tests and linters / Tests with -race (pull_request) Successful in 1m44s
Tests and linters / Lint (pull_request) Successful in 2m28s
fc750ccedd
* Introduce new operation with suffix "IfExists" for all
  string and numeric condition operations;
* An operation with "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;
* Change the signature for interface method `Property()`: since it returns two
  values - the second indicates whether the property exists;
* This also means that original condition operations for string and number comparison
  is slightly changed: if property doesn't exist, then the condition is not evaluated.

Signed-off-by: Airat Arifullin <aarifullin@yadro.com>
aarifullin requested review from storage-core-committers 2024-10-02 10:42:22 +00:00
aarifullin requested review from storage-core-developers 2024-10-02 10:42:23 +00:00
aarifullin requested review from storage-services-committers 2024-10-02 10:42:26 +00:00
aarifullin requested review from storage-services-developers 2024-10-02 10:42:26 +00:00
aarifullin force-pushed feat/cond_if_exists from fc750ccedd to 687e18c50d 2024-10-02 10:42:38 +00:00 Compare
fyrchik reviewed 2024-10-02 11:12:23 +00:00
@ -175,1 +212,3 @@
return val != c.Value
return exists && val != c.Value
case CondStringNotEqualsIfExists:
return !exists || val != c.Value
Owner

@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:

If you are using an "Effect": "Deny" element with a negated condition operator like StringNotEqualsIfExists, the request is still denied even if the condition key is not present.

It seems we behave differently. What am I missing?

@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: > If you are using an "Effect": "Deny" element with a negated condition operator like StringNotEqualsIfExists, the request is still denied even if the condition key is not present. It seems we behave differently. What am I missing?
Author
Member
   {
     "Effect": "Deny",
     "Action": "s3:PutObject",
     "Resource": "*",
     "Condition": {
       "StringNotEquals": {
         "s3:x-amz-server-side-encryption": "AES256"
       }
     }
   }

In current implementation:

  1. If the property s3:x-amz-server-side-encryption is not set, then val_found && StringNotEquals(val, "s3:x-amz-server-side-encryption") -> false that seems fair;
  2. If the property s3:x-amz-server-side-encryption is set, then val_found && StringNotEquals(val, "s3:x-amz-server-side-encryption") -> true/false that seems fair;
  3. If the property s3:x-amz-server-side-encryption is not set, then StringNotEqualsIfExists(...) -> !val_found || StringNotEquals(val, "s3:x-amz-server-side-encryption") -> true
  4. If the property s3:x-amz-server-side-encryption is set, then StringNotEqualsIfExists(...) -> !val_found || StringNotEquals(val, "s3:x-amz-server-side-encryption") -> true/false. If "s3:x-amz-server-side-encryption" is set to "", then it returns true

Anyway, the current implementation fits to this requirement even for StringNotEquals:

If you are using an "Effect": "Deny" element with a negated condition operator like StringNotEqualsIfExists, the request is still denied even if the condition key is not present.

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

```json { "Effect": "Deny", "Action": "s3:PutObject", "Resource": "*", "Condition": { "StringNotEquals": { "s3:x-amz-server-side-encryption": "AES256" } } } ``` In current implementation: 1. If the property `s3:x-amz-server-side-encryption` is _not_ set, then `val_found && StringNotEquals(val, "s3:x-amz-server-side-encryption") -> false` that seems fair; 2. If the property `s3:x-amz-server-side-encryption` is set, then `val_found && StringNotEquals(val, "s3:x-amz-server-side-encryption") -> true/false` that seems fair; 3. If the property `s3:x-amz-server-side-encryption` is **not** set, then `StringNotEqualsIfExists(...) -> !val_found || StringNotEquals(val, "s3:x-amz-server-side-encryption") -> true` 4. If the property `s3:x-amz-server-side-encryption` is set, then `StringNotEqualsIfExists(...) -> !val_found || StringNotEquals(val, "s3:x-amz-server-side-encryption") -> true/false`. If `"s3:x-amz-server-side-encryption"` is set to `""`, then it returns `true` Anyway, the current implementation fits to this requirement even for `StringNotEquals`: ``` If you are using an "Effect": "Deny" element with a negated condition operator like StringNotEqualsIfExists, the request is still denied even if the condition key is not present. ``` 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`
Member

If the property s3:x-amz-server-side-encryption is not set, then val_found && StringNotEquals(val, "s3:x-amz-server-side-encryption") -> false that seems fair;

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

{
  "Version": "2012-10-17",
  "Statement": {
    "Effect": "Deny",
    "Principal": "*",
    "Action": "s3:PutObject",
    "Resource": "arn:aws:s3:::bucket/*",
    "Condition": {
      "StringNotEquals": {
        "s3:RequestObjectTag/environment": "production"
      }
    }
  }
}

I can put object only if provide tag environment=production in request

> If the property s3:x-amz-server-side-encryption is not set, then val_found && StringNotEquals(val, "s3:x-amz-server-side-encryption") -> false that seems fair; 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 ```json { "Version": "2012-10-17", "Statement": { "Effect": "Deny", "Principal": "*", "Action": "s3:PutObject", "Resource": "arn:aws:s3:::bucket/*", "Condition": { "StringNotEquals": { "s3:RequestObjectTag/environment": "production" } } } } ``` I can put object only if provide tag `environment=production` in request
Owner

It it difficult to add a test here and check with other implementations to be sure?
aa82bd16ae/s3tests_boto3/functional/test_s3.py

It it difficult to add a test here and check with other implementations to be sure? https://git.frostfs.info/TrueCloudLab/s3-tests/src/commit/aa82bd16aef7c8b37cbc5e95ee06ff198d6270a8/s3tests_boto3/functional/test_s3.py
Author
Member

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)

Probably, I incorrectly pointed to "current implementation". I meant - "current" like in this PR, not master

> 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) Probably, I incorrectly pointed to "current implementation". I meant - "current" like in this PR, not `master`
Member

It it difficult to add a test here and check with other implementations to be sure?
aa82bd16ae/s3tests_boto3/functional/test_s3.py

I suppose it's possible, but first we need to know what policies we are about to test

> It it difficult to add a test here and check with other implementations to be sure? aa82bd16ae/s3tests_boto3/functional/test_s3.py I suppose it's possible, but first we need to know what policies we are about to test
Owner
TEST 1
        {"Version": "2012-10-17","Statement": {"Effect": "Allow","Principal": "*","Action": "s3:PutObject","Resource": "arn:aws:s3:::bucket/*","Condition":{"StringNotEquals":{"s3:RequestObjectTag/environment": "production"}}}}
        ENV: s3:RequestObjectTag/environment: production
TEST 2
        {"Version": "2012-10-17","Statement": {"Effect": "Allow","Principal": "*","Action": "s3:PutObject","Resource": "arn:aws:s3:::bucket/*","Condition":{"StringNotEquals":{"s3:RequestObjectTag/environment": "production"}}}}
        ENV: s3:RequestObjectTag/environment: development
TEST 3
        {"Version": "2012-10-17","Statement": {"Effect": "Allow","Principal": "*","Action": "s3:PutObject","Resource": "arn:aws:s3:::bucket/*","Condition":{"StringNotEquals":{"s3:RequestObjectTag/environment": "production"}}}}
        ENV: s3:RequestObjectTag/environment: <MISSING>
TEST 4
        {"Version": "2012-10-17","Statement": {"Effect": "Allow","Principal": "*","Action": "s3:PutObject","Resource": "arn:aws:s3:::bucket/*","Condition":{"StringEquals":{"s3:RequestObjectTag/environment": "production"}}}}
        ENV: s3:RequestObjectTag/environment: production
TEST 5
        {"Version": "2012-10-17","Statement": {"Effect": "Allow","Principal": "*","Action": "s3:PutObject","Resource": "arn:aws:s3:::bucket/*","Condition":{"StringEquals":{"s3:RequestObjectTag/environment": "production"}}}}
        ENV: s3:RequestObjectTag/environment: development
TEST 6
        {"Version": "2012-10-17","Statement": {"Effect": "Allow","Principal": "*","Action": "s3:PutObject","Resource": "arn:aws:s3:::bucket/*","Condition":{"StringEquals":{"s3:RequestObjectTag/environment": "production"}}}}
        ENV: s3:RequestObjectTag/environment: <MISSING>
TEST 7
        {"Version": "2012-10-17","Statement": {"Effect": "Deny","Principal": "*","Action": "s3:PutObject","Resource": "arn:aws:s3:::bucket/*","Condition":{"StringNotEquals":{"s3:RequestObjectTag/environment": "production"}}}}
        ENV: s3:RequestObjectTag/environment: production
TEST 8
        {"Version": "2012-10-17","Statement": {"Effect": "Deny","Principal": "*","Action": "s3:PutObject","Resource": "arn:aws:s3:::bucket/*","Condition":{"StringNotEquals":{"s3:RequestObjectTag/environment": "production"}}}}
        ENV: s3:RequestObjectTag/environment: development
TEST 9
        {"Version": "2012-10-17","Statement": {"Effect": "Deny","Principal": "*","Action": "s3:PutObject","Resource": "arn:aws:s3:::bucket/*","Condition":{"StringNotEquals":{"s3:RequestObjectTag/environment": "production"}}}}
        ENV: s3:RequestObjectTag/environment: <MISSING>
TEST 10
        {"Version": "2012-10-17","Statement": {"Effect": "Deny","Principal": "*","Action": "s3:PutObject","Resource": "arn:aws:s3:::bucket/*","Condition":{"StringEquals":{"s3:RequestObjectTag/environment": "production"}}}}
        ENV: s3:RequestObjectTag/environment: production
TEST 11
        {"Version": "2012-10-17","Statement": {"Effect": "Deny","Principal": "*","Action": "s3:PutObject","Resource": "arn:aws:s3:::bucket/*","Condition":{"StringEquals":{"s3:RequestObjectTag/environment": "production"}}}}
        ENV: s3:RequestObjectTag/environment: development
TEST 12
        {"Version": "2012-10-17","Statement": {"Effect": "Deny","Principal": "*","Action": "s3:PutObject","Resource": "arn:aws:s3:::bucket/*","Condition":{"StringEquals":{"s3:RequestObjectTag/environment": "production"}}}}
        ENV: s3:RequestObjectTag/environment: <MISSING>
``` TEST 1 {"Version": "2012-10-17","Statement": {"Effect": "Allow","Principal": "*","Action": "s3:PutObject","Resource": "arn:aws:s3:::bucket/*","Condition":{"StringNotEquals":{"s3:RequestObjectTag/environment": "production"}}}} ENV: s3:RequestObjectTag/environment: production TEST 2 {"Version": "2012-10-17","Statement": {"Effect": "Allow","Principal": "*","Action": "s3:PutObject","Resource": "arn:aws:s3:::bucket/*","Condition":{"StringNotEquals":{"s3:RequestObjectTag/environment": "production"}}}} ENV: s3:RequestObjectTag/environment: development TEST 3 {"Version": "2012-10-17","Statement": {"Effect": "Allow","Principal": "*","Action": "s3:PutObject","Resource": "arn:aws:s3:::bucket/*","Condition":{"StringNotEquals":{"s3:RequestObjectTag/environment": "production"}}}} ENV: s3:RequestObjectTag/environment: <MISSING> TEST 4 {"Version": "2012-10-17","Statement": {"Effect": "Allow","Principal": "*","Action": "s3:PutObject","Resource": "arn:aws:s3:::bucket/*","Condition":{"StringEquals":{"s3:RequestObjectTag/environment": "production"}}}} ENV: s3:RequestObjectTag/environment: production TEST 5 {"Version": "2012-10-17","Statement": {"Effect": "Allow","Principal": "*","Action": "s3:PutObject","Resource": "arn:aws:s3:::bucket/*","Condition":{"StringEquals":{"s3:RequestObjectTag/environment": "production"}}}} ENV: s3:RequestObjectTag/environment: development TEST 6 {"Version": "2012-10-17","Statement": {"Effect": "Allow","Principal": "*","Action": "s3:PutObject","Resource": "arn:aws:s3:::bucket/*","Condition":{"StringEquals":{"s3:RequestObjectTag/environment": "production"}}}} ENV: s3:RequestObjectTag/environment: <MISSING> TEST 7 {"Version": "2012-10-17","Statement": {"Effect": "Deny","Principal": "*","Action": "s3:PutObject","Resource": "arn:aws:s3:::bucket/*","Condition":{"StringNotEquals":{"s3:RequestObjectTag/environment": "production"}}}} ENV: s3:RequestObjectTag/environment: production TEST 8 {"Version": "2012-10-17","Statement": {"Effect": "Deny","Principal": "*","Action": "s3:PutObject","Resource": "arn:aws:s3:::bucket/*","Condition":{"StringNotEquals":{"s3:RequestObjectTag/environment": "production"}}}} ENV: s3:RequestObjectTag/environment: development TEST 9 {"Version": "2012-10-17","Statement": {"Effect": "Deny","Principal": "*","Action": "s3:PutObject","Resource": "arn:aws:s3:::bucket/*","Condition":{"StringNotEquals":{"s3:RequestObjectTag/environment": "production"}}}} ENV: s3:RequestObjectTag/environment: <MISSING> TEST 10 {"Version": "2012-10-17","Statement": {"Effect": "Deny","Principal": "*","Action": "s3:PutObject","Resource": "arn:aws:s3:::bucket/*","Condition":{"StringEquals":{"s3:RequestObjectTag/environment": "production"}}}} ENV: s3:RequestObjectTag/environment: production TEST 11 {"Version": "2012-10-17","Statement": {"Effect": "Deny","Principal": "*","Action": "s3:PutObject","Resource": "arn:aws:s3:::bucket/*","Condition":{"StringEquals":{"s3:RequestObjectTag/environment": "production"}}}} ENV: s3:RequestObjectTag/environment: development TEST 12 {"Version": "2012-10-17","Statement": {"Effect": "Deny","Principal": "*","Action": "s3:PutObject","Resource": "arn:aws:s3:::bucket/*","Condition":{"StringEquals":{"s3:RequestObjectTag/environment": "production"}}}} ENV: s3:RequestObjectTag/environment: <MISSING> ```
Member

Currently I have results only for deny policies. In AWS there are some problem make bucket public.

  • TEST 7: not matched
  • TEST 8: matched
  • TEST 9: matched
  • TEST 10: matched
  • TEST 11: not matched
  • TEST 12: not matched

These results for AWS.
Yandex badly support policies and we always get AccessDenied

Currently I have results only for deny policies. In AWS there are some problem make bucket public. * TEST 7: not matched * TEST 8: matched * TEST 9: matched * TEST 10: matched * TEST 11: not matched * TEST 12: not matched These results for AWS. Yandex badly support policies and we always get AccessDenied
Member

I couldn't validate policies with Allow statement and PutObject but I could validate it with PutObjectTagging.
So policy was

{
  "Version": "2012-10-17",
  "Statement": {
    "Effect": "Allow",
    "Principal": "*",
    "Action": "s3:PutObjectTagging",
    "Resource": [
        "arn:aws:s3:::bucket",
        "arn:aws:s3:::bucket/*"
    ],
    "Condition":{
        "StringEquals": {
            "s3:RequestObjectTag/Environment": "production"
        }
    }
  }
}

Results:

  • TEST 1: not matched -> AccessDenied
  • TEST 2: matched -> Allow
  • TEST 3: matched -> Allow
  • TEST 4: matched -> Allow
  • TEST 5: not matched -> AccessDenied
  • TEST 6: not matched -> AccessDenied
I couldn't validate policies with Allow statement and `PutObject` but I could validate it with `PutObjectTagging`. So policy was ```json { "Version": "2012-10-17", "Statement": { "Effect": "Allow", "Principal": "*", "Action": "s3:PutObjectTagging", "Resource": [ "arn:aws:s3:::bucket", "arn:aws:s3:::bucket/*" ], "Condition":{ "StringEquals": { "s3:RequestObjectTag/Environment": "production" } } } } ``` Results: * TEST 1: not matched -> AccessDenied * TEST 2: matched -> Allow * TEST 3: matched -> Allow * TEST 4: matched -> Allow * TEST 5: not matched -> AccessDenied * TEST 6: not matched -> AccessDenied
Member
branch with tests https://git.frostfs.info/dkirillov/s3-tests/src/branch/feature/ape_if_exists_conditions
Owner

This clarifies the quoted sentence from the IAM doc:

  1. TEST 3: matched -> Allow, this means that if the attribute is missing then NotEquals 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 and StringNotEqualsIfExists?
Probably IfExists will NOT match in the test 3?

Here are the same tests, but for IfExists, could you check them too?

TEST 1
{ "Version": "2012-10-17", "Statement": { "Effect": "Allow", "Principal": "*", "Action": "s3:PutObject", "Resource": "arn:aws:s3:::bucket/*", "Condition": { "StringNotEqualsIfExists": { "s3:RequestObjectTag/environment": "production" } } } }
        ENV: s3:RequestObjectTag/environment: production
TEST 2
{ "Version": "2012-10-17", "Statement": { "Effect": "Allow", "Principal": "*", "Action": "s3:PutObject", "Resource": "arn:aws:s3:::bucket/*", "Condition": { "StringNotEqualsIfExists": { "s3:RequestObjectTag/environment": "production" } } } }
        ENV: s3:RequestObjectTag/environment: development
TEST 3
{ "Version": "2012-10-17", "Statement": { "Effect": "Allow", "Principal": "*", "Action": "s3:PutObject", "Resource": "arn:aws:s3:::bucket/*", "Condition": { "StringNotEqualsIfExists": { "s3:RequestObjectTag/environment": "production" } } } }
        ENV: s3:RequestObjectTag/environment: <MISSING>
TEST 4
{ "Version": "2012-10-17", "Statement": { "Effect": "Allow", "Principal": "*", "Action": "s3:PutObject", "Resource": "arn:aws:s3:::bucket/*", "Condition": { "StringEqualsIfExists": { "s3:RequestObjectTag/environment": "production" } } } }
        ENV: s3:RequestObjectTag/environment: production
TEST 5
{ "Version": "2012-10-17", "Statement": { "Effect": "Allow", "Principal": "*", "Action": "s3:PutObject", "Resource": "arn:aws:s3:::bucket/*", "Condition": { "StringEqualsIfExists": { "s3:RequestObjectTag/environment": "production" } } } }
        ENV: s3:RequestObjectTag/environment: development
TEST 6
{ "Version": "2012-10-17", "Statement": { "Effect": "Allow", "Principal": "*", "Action": "s3:PutObject", "Resource": "arn:aws:s3:::bucket/*", "Condition": { "StringEqualsIfExists": { "s3:RequestObjectTag/environment": "production" } } } }
        ENV: s3:RequestObjectTag/environment: <MISSING>
TEST 7
{ "Version": "2012-10-17", "Statement": { "Effect": "Deny", "Principal": "*", "Action": "s3:PutObject", "Resource": "arn:aws:s3:::bucket/*", "Condition": { "StringNotEqualsIfExists": { "s3:RequestObjectTag/environment": "production" } } } }
        ENV: s3:RequestObjectTag/environment: production
TEST 8
{ "Version": "2012-10-17", "Statement": { "Effect": "Deny", "Principal": "*", "Action": "s3:PutObject", "Resource": "arn:aws:s3:::bucket/*", "Condition": { "StringNotEqualsIfExists": { "s3:RequestObjectTag/environment": "production" } } } }
        ENV: s3:RequestObjectTag/environment: development
TEST 9
{ "Version": "2012-10-17", "Statement": { "Effect": "Deny", "Principal": "*", "Action": "s3:PutObject", "Resource": "arn:aws:s3:::bucket/*", "Condition": { "StringNotEqualsIfExists": { "s3:RequestObjectTag/environment": "production" } } } }
        ENV: s3:RequestObjectTag/environment: <MISSING>
TEST 10
{ "Version": "2012-10-17", "Statement": { "Effect": "Deny", "Principal": "*", "Action": "s3:PutObject", "Resource": "arn:aws:s3:::bucket/*", "Condition": { "StringEqualsIfExists": { "s3:RequestObjectTag/environment": "production" } } } }
        ENV: s3:RequestObjectTag/environment: production
TEST 11
{ "Version": "2012-10-17", "Statement": { "Effect": "Deny", "Principal": "*", "Action": "s3:PutObject", "Resource": "arn:aws:s3:::bucket/*", "Condition": { "StringEqualsIfExists": { "s3:RequestObjectTag/environment": "production" } } } }
        ENV: s3:RequestObjectTag/environment: development
TEST 12
{ "Version": "2012-10-17", "Statement": { "Effect": "Deny", "Principal": "*", "Action": "s3:PutObject", "Resource": "arn:aws:s3:::bucket/*", "Condition": { "StringEqualsIfExists": { "s3:RequestObjectTag/environment": "production" } } } }
        ENV: s3:RequestObjectTag/environment: <MISSING>
This clarifies the quoted sentence from the IAM doc: 1. `TEST 3: matched -> Allow`, this means that if the attribute is missing then `NotEquals` 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` and `StringNotEqualsIfExists`? Probably `IfExists` will NOT match in the test 3? Here are the same tests, but for `IfExists`, could you check them too? ``` TEST 1 { "Version": "2012-10-17", "Statement": { "Effect": "Allow", "Principal": "*", "Action": "s3:PutObject", "Resource": "arn:aws:s3:::bucket/*", "Condition": { "StringNotEqualsIfExists": { "s3:RequestObjectTag/environment": "production" } } } } ENV: s3:RequestObjectTag/environment: production TEST 2 { "Version": "2012-10-17", "Statement": { "Effect": "Allow", "Principal": "*", "Action": "s3:PutObject", "Resource": "arn:aws:s3:::bucket/*", "Condition": { "StringNotEqualsIfExists": { "s3:RequestObjectTag/environment": "production" } } } } ENV: s3:RequestObjectTag/environment: development TEST 3 { "Version": "2012-10-17", "Statement": { "Effect": "Allow", "Principal": "*", "Action": "s3:PutObject", "Resource": "arn:aws:s3:::bucket/*", "Condition": { "StringNotEqualsIfExists": { "s3:RequestObjectTag/environment": "production" } } } } ENV: s3:RequestObjectTag/environment: <MISSING> TEST 4 { "Version": "2012-10-17", "Statement": { "Effect": "Allow", "Principal": "*", "Action": "s3:PutObject", "Resource": "arn:aws:s3:::bucket/*", "Condition": { "StringEqualsIfExists": { "s3:RequestObjectTag/environment": "production" } } } } ENV: s3:RequestObjectTag/environment: production TEST 5 { "Version": "2012-10-17", "Statement": { "Effect": "Allow", "Principal": "*", "Action": "s3:PutObject", "Resource": "arn:aws:s3:::bucket/*", "Condition": { "StringEqualsIfExists": { "s3:RequestObjectTag/environment": "production" } } } } ENV: s3:RequestObjectTag/environment: development TEST 6 { "Version": "2012-10-17", "Statement": { "Effect": "Allow", "Principal": "*", "Action": "s3:PutObject", "Resource": "arn:aws:s3:::bucket/*", "Condition": { "StringEqualsIfExists": { "s3:RequestObjectTag/environment": "production" } } } } ENV: s3:RequestObjectTag/environment: <MISSING> TEST 7 { "Version": "2012-10-17", "Statement": { "Effect": "Deny", "Principal": "*", "Action": "s3:PutObject", "Resource": "arn:aws:s3:::bucket/*", "Condition": { "StringNotEqualsIfExists": { "s3:RequestObjectTag/environment": "production" } } } } ENV: s3:RequestObjectTag/environment: production TEST 8 { "Version": "2012-10-17", "Statement": { "Effect": "Deny", "Principal": "*", "Action": "s3:PutObject", "Resource": "arn:aws:s3:::bucket/*", "Condition": { "StringNotEqualsIfExists": { "s3:RequestObjectTag/environment": "production" } } } } ENV: s3:RequestObjectTag/environment: development TEST 9 { "Version": "2012-10-17", "Statement": { "Effect": "Deny", "Principal": "*", "Action": "s3:PutObject", "Resource": "arn:aws:s3:::bucket/*", "Condition": { "StringNotEqualsIfExists": { "s3:RequestObjectTag/environment": "production" } } } } ENV: s3:RequestObjectTag/environment: <MISSING> TEST 10 { "Version": "2012-10-17", "Statement": { "Effect": "Deny", "Principal": "*", "Action": "s3:PutObject", "Resource": "arn:aws:s3:::bucket/*", "Condition": { "StringEqualsIfExists": { "s3:RequestObjectTag/environment": "production" } } } } ENV: s3:RequestObjectTag/environment: production TEST 11 { "Version": "2012-10-17", "Statement": { "Effect": "Deny", "Principal": "*", "Action": "s3:PutObject", "Resource": "arn:aws:s3:::bucket/*", "Condition": { "StringEqualsIfExists": { "s3:RequestObjectTag/environment": "production" } } } } ENV: s3:RequestObjectTag/environment: development TEST 12 { "Version": "2012-10-17", "Statement": { "Effect": "Deny", "Principal": "*", "Action": "s3:PutObject", "Resource": "arn:aws:s3:::bucket/*", "Condition": { "StringEqualsIfExists": { "s3:RequestObjectTag/environment": "production" } } } } ENV: s3:RequestObjectTag/environment: <MISSING> ```
Member

The same as previous (TEST 1-6 used PutObjectTagging instead of PutObject, TEST 7-12 used PubObject)

  • TEST 1: not matched -> AccessDenied
  • TEST 2: matched -> Allow
  • TEST 3: matched -> Allow
  • TEST 4: matched -> Allow
  • TEST 5: not matched -> AccessDenied
  • TEST 6: matched -> Allow
  • TEST 7: not matched -> Allow
  • TEST 8: matched -> AccessDenied
  • TEST 9: matched -> AccessDenied
  • TEST 10: matched -> AccessDenied
  • TEST 11: not matched -> Allow
  • TEST 12: matched -> AccessDenied

P.S. For the history tests 1-6 for the PutObject (It looks weird, probably I missed something)

  • TEST 1: not matched -> AccessDenied
  • TEST 2: not matched -> AccessDenied
  • TEST 3: matched -> Allow
  • TEST 4: not matched -> AccessDenied
  • TEST 5: not matched -> AccessDenied
  • TEST 6: not matched -> AccessDenied
The same as previous (TEST 1-6 used PutObjectTagging instead of PutObject, TEST 7-12 used PubObject) * TEST 1: not matched -> AccessDenied * TEST 2: matched -> Allow * TEST 3: matched -> Allow * TEST 4: matched -> Allow * TEST 5: not matched -> AccessDenied * TEST 6: matched -> Allow * * TEST 7: not matched -> Allow * TEST 8: matched -> AccessDenied * TEST 9: matched -> AccessDenied * TEST 10: matched -> AccessDenied * TEST 11: not matched -> Allow * TEST 12: matched -> AccessDenied P.S. For the history tests 1-6 for the PutObject (It looks weird, probably I missed something) * TEST 1: not matched -> AccessDenied * TEST 2: not matched -> AccessDenied * TEST 3: matched -> Allow * TEST 4: not matched -> AccessDenied * TEST 5: not matched -> AccessDenied * TEST 6: not matched -> AccessDenied
Owner

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?

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?
Member

No, there shouldn't be any mistakes. As for me we got the same match result for pair tests

  • TEST 1,7 - not matched
  • TEST 2,8 - matched
  • TEST 3,9 - matched
  • TEST 4,10 - matched
  • TEST 5,11 - not matched
  • TEST 6,12 - matched

and it's expected in general (I means symmetric results for Allow/Deny Equals/NotEquals).

No, there shouldn't be any mistakes. As for me we got the same match result for pair tests * TEST 1,7 - not matched * TEST 2,8 - matched * TEST 3,9 - matched * TEST 4,10 - matched * TEST 5,11 - not matched * TEST 6,12 - matched and it's expected in general (I means symmetric results for Allow/Deny Equals/NotEquals).
Owner

So NotEquals doesn't differ from NotEqualsIfExists, right?
Equals differs from EqualsIfExists, though.

So `NotEquals` doesn't differ from `NotEqualsIfExists`, right? `Equals` differs from `EqualsIfExists`, though.
Owner

Hm, I think I have found the discriminator:

{ "Version": "2012-10-17", "Statement": { "Effect": "Allow", "Principal": "*", "Action": "s3:PutObject", "Resource": "arn:aws:s3:::bucket/*",
  "Condition": {
    "StringNotEqualsIfExists": { "s3:RequestObjectTag/environment": "production" },
    "StringNotEqualsIfExists": { "s3:RequestObjectTag/anotherparameter": "xxx" }
   }}}
        ENV:
        s3:RequestObjectTag/environment: development
        s3:RequestObjectTag/anotherparameter: xxx

DISCRIMINATOR 2:
{ "Version": "2012-10-17", "Statement": { "Effect": "Allow", "Principal": "*", "Action": "s3:PutObject", "Resource": "arn:aws:s3:::bucket/*",
  "Condition": {
    "StringNotEqualsIfExists": { "s3:RequestObjectTag/environment": "production" },
    "StringNotEqualsIfExists": { "s3:RequestObjectTag/anotherparameter": "xxx" }
   }}}
        ENV:
        s3:RequestObjectTag/anotherparameter: xxx

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:

DISCRIMINATOR 1: false (both values exist, one of them is equal and thus unmatched)
DISCRIMINATOR 2: true (only 1 value exists. It is unmatched, but the second IfExists makes ALL condition true)
Hm, I think I have found the discriminator: ```DISCRIMINATOR 1: { "Version": "2012-10-17", "Statement": { "Effect": "Allow", "Principal": "*", "Action": "s3:PutObject", "Resource": "arn:aws:s3:::bucket/*", "Condition": { "StringNotEqualsIfExists": { "s3:RequestObjectTag/environment": "production" }, "StringNotEqualsIfExists": { "s3:RequestObjectTag/anotherparameter": "xxx" } }}} ENV: s3:RequestObjectTag/environment: development s3:RequestObjectTag/anotherparameter: xxx DISCRIMINATOR 2: { "Version": "2012-10-17", "Statement": { "Effect": "Allow", "Principal": "*", "Action": "s3:PutObject", "Resource": "arn:aws:s3:::bucket/*", "Condition": { "StringNotEqualsIfExists": { "s3:RequestObjectTag/environment": "production" }, "StringNotEqualsIfExists": { "s3:RequestObjectTag/anotherparameter": "xxx" } }}} ENV: s3:RequestObjectTag/anotherparameter: xxx ``` 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: ``` DISCRIMINATOR 1: false (both values exist, one of them is equal and thus unmatched) DISCRIMINATOR 2: true (only 1 value exists. It is unmatched, but the second IfExists makes ALL condition true) ```
Owner

or no, my brain has melted

or no, my brain has melted
Member

It seems no.
I have the following policy (StringNotEqualsIfExists can be added only once):

{
  "Version": "2012-10-17",
  "Statement": {
    "Effect": "Allow",
    "Principal": "*",
    "Action": "s3:PutObjectTagging",
    "Resource": [
        "arn:aws:s3:::yvo6fckpk92rhcsz",
        "arn:aws:s3:::yvo6fckpk92rhcsz/*"
    ],
    "Condition":{
        "StringNotEqualsIfExists": {
                "s3:RequestObjectTag/environment": "production" ,
                "s3:RequestObjectTag/anotherparameter": "xxx"
         }
    }
  }
}

And I get the following results:

$ aws s3api put-object-tagging --bucket yvo6fckpk92rhcsz --key tmp --no-sign-request --tagging '{"TagSet": [ { "Key": "anotherparameter", "Value": "xxx" } ]}'

An error occurred (AccessDenied) when calling the PutObjectTagging operation (reached max retries: 0): Access Denied



$ aws s3api put-object-tagging --bucket yvo6fckpk92rhcsz --key tmp --no-sign-request --tagging '{"TagSet": [ { "Key": "anotherparameter", "Value": "xxx2" } ]}'

{
    "VersionId": "null"
}



$ aws s3api put-object-tagging --bucket yvo6fckpk92rhcsz --key tmp --no-sign-request --tagging '{"TagSet": [ { "Key": "anotherparameter", "Value": "xxx2" }, {"Key": "environment", "Value": "production"} ]}'

An error occurred (AccessDenied) when calling the PutObjectTagging operation (reached max retries: 0): Access Denied



$ aws s3api put-object-tagging --bucket yvo6fckpk92rhcsz --key tmp --no-sign-request --tagging '{"TagSet": [  ]}'

{
    "VersionId": "null"
}
It seems no. I have the following policy (`StringNotEqualsIfExists` can be added only once): ```json { "Version": "2012-10-17", "Statement": { "Effect": "Allow", "Principal": "*", "Action": "s3:PutObjectTagging", "Resource": [ "arn:aws:s3:::yvo6fckpk92rhcsz", "arn:aws:s3:::yvo6fckpk92rhcsz/*" ], "Condition":{ "StringNotEqualsIfExists": { "s3:RequestObjectTag/environment": "production" , "s3:RequestObjectTag/anotherparameter": "xxx" } } } } ``` And I get the following results: ```shell $ aws s3api put-object-tagging --bucket yvo6fckpk92rhcsz --key tmp --no-sign-request --tagging '{"TagSet": [ { "Key": "anotherparameter", "Value": "xxx" } ]}' An error occurred (AccessDenied) when calling the PutObjectTagging operation (reached max retries: 0): Access Denied $ aws s3api put-object-tagging --bucket yvo6fckpk92rhcsz --key tmp --no-sign-request --tagging '{"TagSet": [ { "Key": "anotherparameter", "Value": "xxx2" } ]}' { "VersionId": "null" } $ aws s3api put-object-tagging --bucket yvo6fckpk92rhcsz --key tmp --no-sign-request --tagging '{"TagSet": [ { "Key": "anotherparameter", "Value": "xxx2" }, {"Key": "environment", "Value": "production"} ]}' An error occurred (AccessDenied) when calling the PutObjectTagging operation (reached max retries: 0): Access Denied $ aws s3api put-object-tagging --bucket yvo6fckpk92rhcsz --key tmp --no-sign-request --tagging '{"TagSet": [ ]}' { "VersionId": "null" } ```
Author
Member

So NotEquals doesn't differ from NotEqualsIfExists, right?

Currently, in this pull-request - it doesn't, but I believe that's why the doc clarifies:

If you are using an "Effect": "Deny" element with a negated condition operator like StringNotEqualsIfExists, the request is still denied even if the condition key is not present.

So, status should be propagated to Match function to check ...NotEqualsIfExists operators

> So `NotEquals` doesn't differ from `NotEqualsIfExists`, right? Currently, in this pull-request - it doesn't, but I believe that's why the [doc](https://docs.aws.amazon.com/IAM/latest/UserGuide/reference_policies_elements_condition_operators.html#Conditions_IfExists) clarifies: > If you are using an "Effect": "Deny" element with a negated condition operator like StringNotEqualsIfExists, the request is still denied even if the condition key is not present. So, status should be propagated to `Match` function to check `...NotEqualsIfExists` operators
Owner

So, status should be propagated

Hm, 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.

>So, status should be propagated Hm, 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.
Author
Member

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

Could you give an example of policy, where notequal and notequalifexists behave differently?

I mean, I confirmed that you was right - these both operators, actually, had had no difference if key is present.

  • StringNotEquals returns false if key is not present (seems OK)
  • StringEqualsIfExists returns true if key is not present ("If the key is not present, evaluate the condition element as true.")
  • StringNotEqualsIfExists: returns true 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 for
StringNotEqualsIfExists(x, y) = ¬StringEqualsIfExists(x, y) = 
= ¬(¬Present(Properties, Key_X) OR Value(Key_X) == Y)) =
= Present(Properties, Key_X) && Value(Key_X) != Y = StringNotEquals(x,y)-> condition won't work out if key is not present

So, it must be interpreted only as (corresponding to the specification):

StringNotEqualsIfExists(x, y) = ¬Present(Properties, Key_X) || Value(Key_X) != Y

while

StringNotEquals(x,y) = Present(Properties, Key_X) && Value(Key_X) != Y

Anyway, let's keep these negated operators far away for the goodness sake

> 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 > Could you give an example of policy, where notequal and notequalifexists behave differently? I mean, I confirmed that you was right - these both operators, actually, had had no difference if key is present. - `StringNotEquals` returns `false` if key is not present (seems OK) - `StringEqualsIfExists` returns `true` if key is not present ("If the key is not present, evaluate the condition element as true.") - `StringNotEqualsIfExists`: returns `true` 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** for ``` StringNotEqualsIfExists(x, y) = ¬StringEqualsIfExists(x, y) = = ¬(¬Present(Properties, Key_X) OR Value(Key_X) == Y)) = = Present(Properties, Key_X) && Value(Key_X) != Y = StringNotEquals(x,y)-> condition won't work out if key is not present ``` So, it must be interpreted only as (corresponding to the specification): ``` StringNotEqualsIfExists(x, y) = ¬Present(Properties, Key_X) || Value(Key_X) != Y ``` while ``` StringNotEquals(x,y) = Present(Properties, Key_X) && Value(Key_X) != Y ``` **Anyway**, let's keep these negated operators far away for the goodness sake
dkirillov reviewed 2024-10-02 13:49:14 +00:00
@ -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]
Member

Why don't we write just return r.properties[name] ?

Why don't we write just `return r.properties[name]` ?
Author
Member

Because compiler doesn't allow this:

not enough return values
	have (string)
	want (string, bool)
Because compiler doesn't allow this: ``` not enough return values have (string) want (string, bool) ```
dkirillov marked this conversation as resolved
aarifullin force-pushed feat/cond_if_exists from 687e18c50d to ab67b9028e 2024-10-21 09:39:55 +00:00 Compare
Author
Member

Re-review, please. Negated operators have been factored out

cc @fyrchik @dkirillov

Re-review, please. Negated operators have been factored out cc @fyrchik @dkirillov
fyrchik reviewed 2024-10-22 07:04:24 +00:00
@ -174,2 +202,3 @@
return !exists || val == c.Value
case CondStringNotEquals:
return val != c.Value
return exists && val != c.Value
Owner

Why have you changed this?

Why have you changed this?
Author
Member

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 empty

The [reason](https://git.frostfs.info/TrueCloudLab/frostfs-node/issues/1406#issuecomment-53462) 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 empty
Owner

We need this change for positive conditions, not the negative ones.
As you can see below, negative conditions are handled differently.

We need this change for positive conditions, not the negative ones. As you can see below, negative conditions are handled differently.
Author
Member

Oh, sorry! I misunderstood this - have been keeping the idea about ...IfExists operators in my mind

Oh, sorry! I misunderstood this - have been keeping the idea about `...IfExists` operators in my mind
dkirillov reviewed 2024-10-22 07:49:54 +00:00
@ -171,3 +198,3 @@
panic(fmt.Sprintf("unimplemented: %d", c.Op))
case CondStringEquals:
return val == c.Value
return exists && val == c.Value
Member

Why do we change this?

Why do we change this?
Author
Member
https://git.frostfs.info/TrueCloudLab/policy-engine/pulls/93#issuecomment-55390
aarifullin changed title from chain: Introduce new condition operations to WIP: chain: Introduce new condition operations 2024-10-22 13:14:07 +00:00
Author
Member

The 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

The issue [#1406](https://git.frostfs.info/TrueCloudLab/frostfs-node/issues/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
aarifullin closed this pull request 2024-10-30 07:02:45 +00:00
All checks were successful
Tests and linters / Staticcheck (pull_request) Successful in 1m4s
DCO action / DCO (pull_request) Successful in 1m13s
Tests and linters / Tests (pull_request) Successful in 1m14s
Tests and linters / Tests with -race (pull_request) Successful in 1m28s
Tests and linters / Lint (pull_request) Successful in 1m50s

Pull request closed

Sign in to join this conversation.
No reviewers
TrueCloudLab/storage-core-committers
TrueCloudLab/storage-core-developers
TrueCloudLab/storage-services-committers
TrueCloudLab/storage-services-developers
No milestone
No project
No assignees
3 participants
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference: TrueCloudLab/policy-engine#93
No description provided.