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.

Sign up for GitHub

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

Jump to bottom

Block public access to S3 bucket as per standard compliance rules #581

Merged
merged 3 commits into from
May 3, 2023
Merged

Block public access to S3 bucket as per standard compliance rules #581

merged 3 commits into from
May 3, 2023

Conversation

shreyas-s-rao
Copy link
Contributor

How to categorize this PR?

/area backup
/area security
/area compliance
/kind task
/kind test
/platform aws

What this PR does / why we need it:
This PR configures the S3 buckets created by e2e tests to be blocked from public access as per standard practices. This is achieved by putting a public-access-block on the created bucket, as per AWS documentation.

Which issue(s) this PR fixes:
Fixes #

Special notes for your reviewer:
cc @vlerenc

Release note:

Block public access for S3 buckets created by e2e tests.

@shreyas-s-rao shreyas-s-rao added this to the v0.18.0 milestone Apr 19, 2023
@shreyas-s-rao shreyas-s-rao requested a review from a team as a code owner April 19, 2023 10:30
@gardener-robot gardener-robot added area/backup Backup related area/compliance Compliance related area/security Security related kind/task General task kind/test Test platform/aws Amazon web services platform/infrastructure needs/rebase Needs git rebase labels Apr 19, 2023
@gardener-robot
Copy link

@shreyas-s-rao You need rebase this pull request with latest master branch. Please check.

@gardener-robot gardener-robot added needs/review Needs review size/xs Size of pull request is tiny (see gardener-robot robot/bots/size.py) labels Apr 19, 2023
@gardener-robot-ci-2 gardener-robot-ci-2 added reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) needs/ok-to-test Needs approval for testing (check PR in detail before setting this label because PR is run on CI/CD) and removed reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) labels Apr 19, 2023
@gardener-robot-ci-2 gardener-robot-ci-2 added reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) and removed reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) labels Apr 20, 2023
@gardener-robot-ci-1 gardener-robot-ci-1 added reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) and removed reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) labels Apr 21, 2023
@shreyas-s-rao
Copy link
Contributor Author

@abdasgupta I have addressed your comment gardener/etcd-backup-restore#615 (comment) in this PR as well. Please confirm that I have not added the bucket policy for localstack S3 buckets since they are exposed on http endpoint instead of https, so setting this policy on such buckets will fail the localstack-based e2e tests

@gardener-robot-ci-1 gardener-robot-ci-1 added the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Apr 21, 2023
@gardener-robot-ci-3 gardener-robot-ci-3 removed the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Apr 21, 2023
@abdasgupta
Copy link
Contributor

abdasgupta commented Apr 24, 2023 •
edited
Loading

It looks fine, can you verify if you can run make ci-e2e-kind following docs/development/local-e2e-tests.md? @shreyas-s-rao

@ishan16696
Copy link
Member

It looks fine, can you verify if you can run make ci-e2e-kind following docs/development/local-e2e-tests.md?

I don't get it, by running make ci-e2e-kind it will create a bucket using localstack right ? so, to tests this better to run druid e2e tests with AWS S3 and check whether the bucket created by public access or not

make \
  AWS_ACCESS_KEY_ID="abc" \
  AWS_SECRET_ACCESS_KEY="xyz" \
  AWS_REGION="eu-central-1" \
  KUBECONFIG="$HOME/.kube/config" \
  PROVIDERS="aws" \
  TEST_ID="some-test-id" \
  STEPS="setup,deploy,test,undeploy,cleanup" \
test-e2e

@abdasgupta
Copy link
Contributor

It looks fine, can you verify if you can run make ci-e2e-kind following docs/development/local-e2e-tests.md?

I don't get it, by running make ci-e2e-kind it will create a bucket using localstack right ? so, to tests this better to run druid e2e tests with AWS S3 and check whether the bucket created by public access or not

make \
  AWS_ACCESS_KEY_ID="abc" \
  AWS_SECRET_ACCESS_KEY="xyz" \
  AWS_REGION="eu-central-1" \
  KUBECONFIG="$HOME/.kube/config" \
  PROVIDERS="aws" \
  TEST_ID="some-test-id" \
  STEPS="setup,deploy,test,undeploy,cleanup" \
test-e2e

As Shreyas was concerned that if the addition of policy would make any problem with localstack or not. That's why I asked him to run the PR once with make ci-e2e-kind.

@shreyas-s-rao
Copy link
Contributor Author

I have already run the e2e tests that use AWS S3 bucket (test-e2e) as well as localstack S3 bucket (ci-e2e-kind), and both worked fine. The AWS S3 bucket was created with the expected bucket policy and blocked public access

ishan16696
ishan16696 approved these changes May 1, 2023
View reviewed changes
Copy link
Member

@ishan16696 ishan16696 left a comment

Choose a reason for hiding this comment

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

ok, thanks @shreyas-s-rao for PR and for testing it.

@shreyas-s-rao shreyas-s-rao merged commit 03618c9 into gardener:master May 3, 2023
@shreyas-s-rao shreyas-s-rao deleted the fix/s3-bucket-access branch May 3, 2023 03:37
@gardener-robot gardener-robot added the status/closed Issue is closed (either delivered or triaged) label May 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ishan16696 ishan16696 ishan16696 approved these changes

Assignees
No one assigned
Labels
area/backup Backup related area/compliance Compliance related area/security Security related kind/task General task kind/test Test needs/ok-to-test Needs approval for testing (check PR in detail before setting this label because PR is run on CI/CD) needs/rebase Needs git rebase needs/review Needs review platform/aws Amazon web services platform/infrastructure size/xs Size of pull request is tiny (see gardener-robot robot/bots/size.py) status/closed Issue is closed (either delivered or triaged)
Projects
None yet
Milestone
v0.18.0
Development

Successfully merging this pull request may close these issues.
7 participants
@shreyas-s-rao @gardener-robot @abdasgupta @ishan16696 @gardener-robot-ci-1 @gardener-robot-ci-2 @gardener-robot-ci-3