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

Add set-bucket-replication action #5387

Merged
merged 65 commits into from Mar 19, 2020
Merged

Add set-bucket-replication action #5387

merged 65 commits into from Mar 19, 2020

Conversation

JohnHillegass
Copy link
Collaborator

@JohnHillegass JohnHillegass commented Feb 28, 2020

Add set replication action for s3 so we can take action on data exfil threats. A use case for this is:

            policies:
              - name: s3-unapproved-account-replication
                resource: s3
                filters:
                  - type: value
                    key: Replication.ReplicationConfiguration.Rules[].Destination.Account
                    value: present
                  - type: value
                    key: Replication.ReplicationConfiguration.Rules[].Destination.Account
                    value_from:
                      url: 's3:///path/to/file.json'
                      format: json
                      expr: "approved_accounts.*"
                    op: ni
                actions:
                  - type: set-replication
                    state: remove

Add remove replication action for s3 so we can take action on data exfil threats. A use case for this is:

```
            policies:
              - name: s3-unapproved-account-replication
                resource: s3
                filters:
                  - type: value
                    key: Replication.ReplicationConfiguration.Rules[].Destination.Account
                    value: present
                  - type: value
                    key: Replication.ReplicationConfiguration.Rules[].Destination.Account
                    value_from:
                      url: 's3:///path/to/file.json'
                      format: json
                      expr: "approved_accounts.*"
                    op: ni
                actions:
                  - type: remove-bucket-replication
```
@JohnHillegass
Copy link
Collaborator Author

Hey @kapilt, I wanted to get this in front of you early. I'll be adding test updates in a few moments. How do you feel about the general approach? This is my first action addition so want to make sure I am considering all of the things.

c7n/resources/s3.py Outdated Show resolved Hide resolved
use the set/state pattern as discussed in the comments
Fix some lint issues
Got schema right, tested all enums and ready to go
@kapilt
Copy link
Collaborator

kapilt commented Feb 28, 2020

your getting a ci failure because the permissions you've got on the action aren't valid, see the iam policy generator its s3:GetReplicationConfiguration etc

fix lint issues
add the right permissions, whoops, read the boto docs wrong
@JohnHillegass
Copy link
Collaborator Author

your getting a ci failure because the permissions you've got on the action aren't valid, see the iam policy generator its s3:GetReplicationConfiguration etc

Thanks @kapilt! That was a real head scratcher. Looks like I misread the boto docs.

fix another lint issue
@JohnHillegass JohnHillegass changed the title Update s3.py Add set-bucket-replication action Feb 28, 2020
pass docs test, so much to learn about the CI runs :)
@kapilt
Copy link
Collaborator

kapilt commented Feb 28, 2020

your getting a ci failure because the permissions you've got on the action aren't valid, see the iam policy generator its s3:GetReplicationConfiguration etc

Thanks @kapilt! That was a real head scratcher. Looks like I misread the boto docs.

iam stuff isn't actually documented in the sdks, its sometimes the method name, sometimes its oddball, been trying to work with some upstream folks to get this stuff in a machine readable format.

@JohnHillegass
Copy link
Collaborator Author

JohnHillegass commented Feb 28, 2020

For the codecov check 20.69% of diff hit (target 89.69%), does this mean that only 20% of what I added is covered in tests and it should be 90+?

@kapilt
Copy link
Collaborator

kapilt commented Feb 28, 2020

In a nutshell yes, atm there aren’t any tests in this pr

@JohnHillegass
Copy link
Collaborator Author

JohnHillegass commented Feb 29, 2020

@kapilt I was unfamiliar with how placebo works but was able to find some good guidance in the custodian docs. All tests pass now. Thanks for the guidance on this!

@kapilt
Copy link
Collaborator

kapilt commented Mar 2, 2020

please sanitize recorded data of any account ids, for s3 in particular it maybe useful to use a different source to minimize the amount of recorded data.

tests/test_s3.py Outdated Show resolved Hide resolved
@kapilt
Copy link
Collaborator

kapilt commented Mar 14, 2020

@kapilt I believe this is good to go now. Thanks for your feedback along the way. I'll make sure to incorporate it into future contributions.

@JohnHillegass if you have a chance, given you've got a fresh perspective on it, i'd appreciate if you could have a look at the developer docs (https://github.com/cloud-custodian/cloud-custodian/tree/master/docs/source/developer) to see if there's any improvements/additions that should be made to make the contributing/onboarding process easier. thanks

@JohnHillegass
Copy link
Collaborator Author

@kapilt I believe this is good to go now. Thanks for your feedback along the way. I'll make sure to incorporate it into future contributions.

@JohnHillegass if you have a chance, given you've got a fresh perspective on it, i'd appreciate if you could have a look at the developer docs (https://github.com/cloud-custodian/cloud-custodian/tree/master/docs/source/developer) to see if there's any improvements/additions that should be made to make the contributing/onboarding process easier. thanks

Sure thing! It has been a journey with many learnings for me :) I'd be glad to take a look and fill in any areas that might need additional clarity.

Copy link
Collaborator

@kapilt kapilt left a comment

Choose a reason for hiding this comment

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

lgtm, thanks

c7n/resources/s3.py Outdated Show resolved Hide resolved
@kapilt kapilt merged commit 8cbaffa into cloud-custodian:master Mar 19, 2020
@kapilt kapilt mentioned this pull request Apr 7, 2020
3 tasks
fidelito pushed a commit to fidelito/cloud-custodian that referenced this pull request May 29, 2020
@Ykamdar
Copy link

Ykamdar commented Jul 5, 2023

@kapilt finding it difficult to wrap my head around this policy definition.

What does this part in policy mean -

- type: value key: Replication.ReplicationConfiguration.Rules[].Destination.Account value: present - type: value key: Replication.ReplicationConfiguration.Rules[].Destination.Account value_from: url: 's3:///path/to/file.json' format: json expr: "approved_accounts.*" op: ni

Is there a way to get this policy for a specific bucket only, i am trying to implement this in one of my AWS accounts wherein i only want to enable replication for certain buckets but i dont see the option to specify the bucket name anywhere here in the policy file.

Can you please guide ? This is crucial for me and i have really tried and failed to understand this particular thing.

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.

None yet

3 participants