-
Notifications
You must be signed in to change notification settings - Fork 10
S3_OBJECT_LOCK_ENABLED rule code #10
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
base: master
Are you sure you want to change the base?
Conversation
(actual bucket years should be greater than the parameter years) | ||
Then: Return COMPLIANT | ||
Scenario: 6 | ||
Given: S3 bucket is object lock enabled with mode, days and yeaas |
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.
yeaas? spell check
Then: Return COMPLIANT | ||
Scenario: 4 | ||
Given: S3 bucket is object lock enabled with mode and days | ||
(actual bucket days should be greater than the parameter days) |
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.
use And condition for condition check to decide Complaint or Non-Complaint- Given And Then
Then: Return COMPLIANT | ||
Scenario: 5 | ||
Given: S3 bucket is object lock enabled with mode and years | ||
(actual bucket years should be greater than the parameter years) |
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.
use And condition for condition check to decide Complaint or Non-Complaint- Given And Then
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.
Agreed, the fact that you are doing greater needs to be a statement on his own.
Then: Return NON_COMPLIANT | ||
Scenario: 8 | ||
Given: S3 bucket is object lock enabled with mode and days | ||
(actual bucket days are less than the parameter days) |
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.
use And condition for condition check to decide Complaint or Non-Complaint- Given And Then
Then: Return NON_COMPLIANT | ||
Scenario: 10 | ||
Given: S3 bucket is object lock enabled with mode, days and yeaas | ||
(actual bucket days(days + (years*365)) are less than the parameter days) |
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.
use And condition for condition check to decide Complaint or Non-Complaint- Given And Then
s3_client = client_factory.build_client("s3") # AWS S3 Client | ||
config_client = client_factory.build_client('config') # AWS Config Service Client | ||
evaluations = [] | ||
valid_rule_parameters = self.evaluate_parameters(valid_rule_parameters) |
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.
Explicit call is needed to evaluate input parameters?
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.
Not needed, you will have the output of evaluate_parameters() in valid_rule_parameters already.
try: | ||
rule_parameters['Days'] = int(rule_parameters['Days']) | ||
except ValueError: | ||
raise InvalidParametersError('The parameter "Days" must be an integer') |
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.
change message to positive integer
raise InvalidParametersError('The parameter "Days" must be an integer') | ||
|
||
if rule_parameters['Days'] < 1: | ||
raise InvalidParametersError('The parameter "Days" must be greater than 0') |
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.
change message to positive integer
raise InvalidParametersError('The parameter "Years" must be an integer') | ||
|
||
if rule_parameters['Years'] < 1: | ||
raise InvalidParametersError('The parameter "Years" must be greater than 0') |
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.
change message to positive integer
|
||
Description: | ||
Check whether S3 Buckets are Object Lock enabled. There should be a retention mode and retention | ||
period to be applied to the bucket lock. |
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.
Rewrite the description - If retention mode and period applied then its COMPLAINT.
def get_buckets(config_client): | ||
sql = "select * where resourceType = 'AWS::S3::Bucket'" | ||
next_token = True | ||
response = config_client.select_resource_config(Expression=sql, Limit=5) |
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.
Use constant for limit size (PAGE_SIZE)
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.
+1 on GLOBAL VARIABLE on top
isexception = False | ||
try: | ||
response = s3_client.get_object_lock_configuration(Bucket=s3_bucket_name) | ||
except: | ||
isexception = True | ||
if isexception or 'ObjectLockConfiguration' not in response: | ||
evaluations.append( | ||
Evaluation(ComplianceType.NON_COMPLIANT, | ||
s3_bucket_name, RESOURCE_TYPE, | ||
"No ObjectLockConfiguration exist for this bucket.") |
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.
Does it throw any exception ? if yes In what case it throws any exception can we handle the specific scenarios instead just saying NON_COMPLIANT
|
||
Rule Parameters: | ||
Mode | ||
The Object Lock retention mode you want to apply to new objects placed in the specified bucket |
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.
Mention if it is mandatory or optional
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.
Mention the valid values
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.
Remane to RetentionMode
to be more descriptive.
Mode | ||
The Object Lock retention mode you want to apply to new objects placed in the specified bucket | ||
Days | ||
The number of days that you want to specify for the default retention period. |
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.
same
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.
The description should be about the fact that it is the minimum value expected. I'd rename it MinimumRetentionDays
.
Days | ||
The number of days that you want to specify for the default retention period. | ||
Years | ||
The number of years that you want to specify for the default retention period. |
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.
same
Then: Return NON_COMPLIANT | ||
Scenario: 3 | ||
Given: S3 bucket is object lock enabled with mode | ||
Then: Return COMPLIANT |
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.
You need to mention that
And: The mode Parameter is configured and valid
And: The years Parameter is not configured
And: The days Parameter is not configured
The number of years that you want to specify for the default retention period. | ||
|
||
Scenarios: | ||
Scenario: 1 |
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.
Need to add scenario about if the parameter are not valid, return Error.
years = 0 | ||
mode = None | ||
#If 'Rule' exists in the response, then only derive 'mode' | ||
if response.get('ObjectLockConfiguration').get('Rule', 0) != 0: |
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.
Never 2 get in the rows. If the first one return None, then the second get is going to Exception
raise InvalidParametersError('The parameter "Days" must be greater than 0') | ||
|
||
# The int() function will raise an error if the string configured can't be converted to an integer | ||
if 'Years' in rule_parameters: |
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.
Do not repeat yourself. Create a function for testing Days and Years.
def get_buckets(config_client): | ||
sql = "select * where resourceType = 'AWS::S3::Bucket'" | ||
next_token = True | ||
response = config_client.select_resource_config(Expression=sql, Limit=5) |
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.
+1 on GLOBAL VARIABLE on top
days_in_param = valid_rule_parameters.get('Days', 0) | ||
years_in_param = valid_rule_parameters.get('Years', 0) | ||
#Calculate days from years | ||
days_in_param = (years_in_param * 365) + days_in_param |
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.
No. years_in_param and day_in_param should be exclusive. Maybe we should reduce to only the day parameters. then do the math if "Years" is configured in the ObjectRetention configuration. It'd be more customer friendly. Thoughts?
] | ||
rdklibtest.assert_successful_evaluation(self, response, resp_expected, 1) | ||
|
||
def test_compliance_s3_mode_years(self): |
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.
Follow naming convention
I confirm these files are made available under CC0 1.0 Universal (https://creativecommons.org/publicdomain/zero/1.0/legalcode)
Issue #, if available:
Description of changes: