Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

change bucket policy schema to fit AWS json #7389

Merged
merged 1 commit into from
Aug 22, 2023

Conversation

nadavMiz
Copy link
Contributor

@nadavMiz nadavMiz commented Jul 10, 2023

Explain the changes

  1. change bucket policy schema to match AWS schema, according to what we currently support. passing the json as is from the request
  2. change all calling functions to use correct arguments.
  3. add an upgrade script for the bucket policy
  4. add unit tests for upgrade script

Issues: Fixed #xxx / Gap #xxx

  1. gap - anyOf schema changes aren't dealt by decode_json / encode_json functions. since we only use string and sensitiveString in our case it works fine. issue: decode_json / encode_json functions should work with oneOf / anyOf schema options  #7448

Testing Instructions:

  • Doc added/updated
  • Tests added

@nadavMiz nadavMiz requested review from guymguym, dannyzaken, a team and aspandey and removed request for a team July 10, 2023 12:54
@nadavMiz nadavMiz force-pushed the bucket_policy_schema_change branch 2 times, most recently from 34ba9eb to 7a8beb9 Compare July 11, 2023 06:23
@nadavMiz nadavMiz force-pushed the bucket_policy_schema_change branch from 7a8beb9 to 2c0e8a9 Compare July 13, 2023 13:07
@nadavMiz nadavMiz force-pushed the bucket_policy_schema_change branch 2 times, most recently from ae40d7c to ef37a74 Compare July 16, 2023 06:20
@nadavMiz nadavMiz requested a review from dannyzaken July 16, 2023 07:28
@nimrod-becker nimrod-becker requested review from romayalon, a team and achouhan09 and removed request for a team July 18, 2023 09:01
Copy link
Contributor

@aspandey aspandey left a comment

Choose a reason for hiding this comment

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

Looks good to me. I observe that most of the changes are related to lower case to upper case. We need to make sure that this does not break other code wherever it might be using lowercase.

src/endpoint/s3/s3_utils.js Outdated Show resolved Hide resolved
@@ -252,41 +252,66 @@ module.exports = {
}
},

bucket_policy_principal: {
anyOf: [{
Copy link
Contributor

Choose a reason for hiding this comment

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

I recall we didn't support anyOf/oneOf in postgresClient encode_json/decode_json, @dannyzaken is this valid now?

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

I guess you're right. this can be an issue since the values underneath are SensitiveString and will probably not be unwrapped properly when stored. @nadavMiz how does it look in the DB when you store a policy?

in general we should probably handle it. I think there are other places we use anyOf/oneOf that are stores in the DB

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@dannyzaken @romayalon

in the database the prefix appears clearly in all case:

{"_id": "64ca177180d3ca00289e9937", "tag": "", "name": "first.bucket", "system": "64ca177180d3ca00289e992f", "tiering": "64ca177180d3ca00289e9936", "s3_policy": {"Version": "20
12-10-17", "Statement": [{"Action": ["s3:PutBucketPolicy", "s3:*"], "Effect": "Allow", "Resource": ["arn:aws:s3:::first.bucket"], "Principal": {"AWS": "*"}}, {"Action": ["s3:PutObject"], *"Effect": "Allow"
, "Resource": ["arn:aws:s3:::first.bucket/*"], "Principal": "*"}, {"Sid": "id-1", "Action": ["s3:GetObject"], "Effect": "Allow", "Resource": ["arn:aws:s3:::first.bucket/*"], "Principal": {"AWS": ["nadav",
 "*"]}}]}, "versioning": "DISABLED", "last_update": 1690977631534, "master_key_id": "64ca177180d3ca00289e9938", "owner_account": "64ca177180d3ca00289e992e", "storage_stats": {"pools": {}, "blocks_size": 0
, "last_update": 1690977537545, "objects_hist": [], "objects_size": 0, "objects_count": 0, "chunks_capacity": 0, "stats_by_content_type": []}, "lambda_triggers": []}

also in logs we print for bucket policy we get the prefix is a sensitive string. for example in policy validation:

principal fit? SENSITIVE-7846cdd4c2b90527 nadav

looks like it is fine in this case

Copy link
Contributor

Choose a reason for hiding this comment

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

As we discussed, this anyOf doesn't really gets encoded, but since all we have inside the any of is string/sensitive string it works.
@dannyzaken Are you ok with merging it and opening a gap for the anyOf issue?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

opened an issue for the anyOf: #7448

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm fine with the gap.

src/api/common_api.js Show resolved Hide resolved
src/endpoint/s3/ops/s3_put_bucket_policy.js Outdated Show resolved Hide resolved
@nadavMiz nadavMiz force-pushed the bucket_policy_schema_change branch from e7817c8 to 147f72c Compare July 26, 2023 15:02
@nadavMiz nadavMiz force-pushed the bucket_policy_schema_change branch 2 times, most recently from 62f670e to 7272b34 Compare July 26, 2023 15:15
@nadavMiz nadavMiz force-pushed the bucket_policy_schema_change branch 4 times, most recently from b0eac9c to 9d99b30 Compare July 27, 2023 11:41
@nadavMiz nadavMiz requested a review from romayalon July 30, 2023 07:50
Signed-off-by: nadav mizrahi <nadav.mizrahi16@gmail.com>
@nadavMiz nadavMiz merged commit 06d01f2 into noobaa:master Aug 22, 2023
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants