Skip to content

BucketPolicy: add test for sse-c in conditions#574

Merged
cbodley merged 2 commits intoceph:masterfrom
clwluvw:sse-c-policy
Jul 26, 2024
Merged

BucketPolicy: add test for sse-c in conditions#574
cbodley merged 2 commits intoceph:masterfrom
clwluvw:sse-c-policy

Conversation

@clwluvw
Copy link
Copy Markdown
Member

@clwluvw clwluvw commented Jul 19, 2024

Ref. ceph/ceph#58689

Signed-off-by: Seena Fallah <seenafallah@gmail.com>
Comment thread s3tests_boto3/functional/test_s3.py Outdated
Comment on lines +10087 to +10095
deny_incorrect_algo = {
"StringNotEquals": {
"s3:x-amz-server-side-encryption-customer-algorithm": "AES256"
}
}

deny_unencrypted_obj = {
"Null" : {
"s3:x-amz-server-side-encryption-customer-algorithm": "true"
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this first condition relevant to the test? it looks like the check_access_denied part only depends on the second

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's my confusion part as well... Apparantly the stringNotEqual doesn't deny if the header is not specified at all (IIRC same for sse-s3) and the string match happens when it's there (but the only option is AES256). Maybe I can decouple it via status code 403 vs 400.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i guess that's the difference between StringNotEquals and StringNotEqualsIfExists? 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 tag is missing.

for the purposes of testing ceph/ceph#58689, i tend to think the Null condition is sufficient to verify that we understand that condition key. what do you think?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@cbodley Can you please check my last commit? I guess this can cover the case to make sure the value is also respected by RGW. But if you think it's unnecessary I'll drop it. np.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

update looks great

…pted

Signed-off-by: Seena Fallah <seenafallah@gmail.com>
@cbodley cbodley merged commit 218f900 into ceph:master Jul 26, 2024
@cbodley
Copy link
Copy Markdown
Contributor

cbodley commented Jul 26, 2024

cherry-picked to ceph-master

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants