-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
Get Bucket Encryption Fails When No Encryption Configuration Is Present but KMS Bucket Key is Enabled #7592
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice catch finding this issue, and thanks for submitting a fix! 👍
c7n/resources/s3.py
Outdated
@@ -3283,7 +3283,10 @@ def filter_bucket(self, b, sse): | |||
key = self.get_key(b) | |||
crypto = self.data.get('crypto') | |||
rule = sse.get('ApplyServerSideEncryptionByDefault') | |||
algo = rule.get('SSEAlgorithm') | |||
try: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: we may be able to cover the problem case here a little more explicitly by using if not algo
or if algo is None
rather than catching an exception on .get()
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a good point. I suppose I could have done that too. Would you like me to modify the PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I think the tweak would accomplish the same thing (covering a non-existent default encryption algorithm) and be a bit clearer. I think that would address @kapilt's question too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree. I'll submit the updated PR tomorrow. As far as the traceback is concerned, there wasn't anything generated except for the following message for a resource experiencing the issue:
2022-07-31 00:11:45,319 - custodian.filters - ERROR - Message: 'NoneType' object has no attribute 'get' Bucket: xcc-services-alb-access-logs-prod-us-east-1
Thanks again @ajkerrigan
what's the actual traceback for the error? the fix here looks odd, try/except around a dict get, if its not a dict then it would be good to have the traceback to understand that a bit more. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good to me, thanks again! 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm, thanks
An account within one of our AWS Organizations has three buckets that are failing with the following error:
'NoneType' object has no attribute 'get' Bucket: <BUCKET_NAME>
After researching it, it appears that there is no encryption configuration, but the use of KMS bucket keys is enabled. Using the AWS CLI, the following output is displayed:
{
"ServerSideEncryptionConfiguration": {
"Rules": [
{
"BucketKeyEnabled": true
}
]
}
}
Currently there is no exception handling for this scenario, so the call by c7n fails. This PR is a simple fix to ensure that it fails gracefully and the resource is included as one that does not have a default encryption configuration.