-
Notifications
You must be signed in to change notification settings - Fork 21
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 mapper for AWS Config Rules #68
Conversation
when 'INSUFFICIENT_DATA' | ||
{ | ||
'run_time': 0, | ||
'code_desc': INSUFFICIENT_DATA_MSG, | ||
'skip_message': INSUFFICIENT_DATA_MSG, | ||
'start_time': DateTime.now.strftime('%Y-%m-%dT%H:%M:%S%:z'), | ||
'status': 'skipped' | ||
} | ||
end |
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.
It may be worth another opinion on this, but wouldn't INSUFFICIENT_DATA
mean not_reviewed
and not skipped? To me insufficient data sounds like something you would need to go validate.
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 chose skipped
here based on the schema options here https://github.com/mitre/inspecjs/blob/master/schemas/exec-json.json#L124
Is not_reviewed
also a legal option for the status
field?
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 did a bit more digging, not_reviewed is not a legal option, however in Heimdall an impact of 0 && a result.status of skipped
is considered 'Not Applicable', and an impact of greater-than 0 and a result.status of skipped
is considered 'Not Reviewed.
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.
Just made a change to make NOT_APPLICABLE have impact set to 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.
Is there a sample of AWS Config data to review? (As a former assessor, I need better context to help you decide which "bucket" to put the result in.)
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 don't necessarily concur with setting whatever "insufficient data" means to "not applicable". Need some original data for context. Thanks!
hdf_result['status'] = case result.dig(:compliance_type) | ||
when 'COMPLIANT' | ||
'passed' | ||
when 'NON_COMPLIANT' | ||
'failed' | ||
else | ||
'skipped' | ||
end |
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.
If insufficient_data is updated to map to not_reviewed then mapping here should include it as well.
f328805
to
d89d44e
Compare
b9c357d
to
def5397
Compare
if possible, could we add a function to also add the
i would also love to see the lambda work you guys are doing - as we are also working on a lambda task - https://github.com/mitre/serverless-inspec |
@aaronlippold Are you requesting something different than the function that is defined here and is called here? I have it set up now so that you can tag an AWS Config Rule with Also, totally happy to collaborate on lambda work. |
I've already reviewed this, it is just waiting on a final look from @rx294 before being merged. |
@jkufro This will help with reusability and consistency of the HDFs generated by
|
@rx294 @rbclark @ejaronne and @aaronlippold meet earlier today and @rx294 will be providing some feedback on our suggested direction for the PR. Great start so far, but we have a few thoughts on some backend learning we have done but not yet put forward on a docs page about 'how to develop mappers', 'what and how you should transform data', 'do and don't on data transform assumptions', 'what to embed, what to hardcode, and what to reference' etc. all things we should have documented. The list above I think would be great sections on a 'contributing_a_mapper.md' which may come out of our collaboration on this PR. Looking forward to it. |
@jkufro some additions to the unit and functional tests as well with some representative source and expected converted data would be great as well so we keep the code coverage up to par. |
@rx294 One major aspect of our use case is continued development/implementation of custom AWS Config rules AWS Docs. My concern with hardcoding EDIT: Here are some examples of Python custom rules made by AWS: https://github.com/awslabs/aws-config-rules/tree/master/python |
a180198
to
fc0c2c3
Compare
@aaronlippold @rbclark @rx294 Please review the latest updates that address the requested changed. Thanks! |
6133b0f
to
4b11835
Compare
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 have several comments. This is the first time we've introduced an idea of end-user defined 800-53 control assertions, which may cause confusion or complaints against this tool. Most assume the mapping comes from either the original approved standard or a mapping set in the heimdall-tools repo. The heimdall-tools repo owner can own-up to these, but cannot to user-defined mappings. Note: CAAT file export does not carry over the "(user provided)" text.
region = us-gov-west-1 | ||
``` | ||
|
||
Additional Rule Mappings: |
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 would rephrase this "feature" to "Custom-rule NIST SP 800-53 Security Control mappings" and restate the paragraph as:
"For user-defined custom AWS config rules (NOT the default rules provided by AWS), users may provide their own assertion as to the NIST SP 800-53 Security Controls touched on by their user-defined custom AWS config rules. This may be provided..."
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.
Also, this is not "additional mapping". it is "--custom-mapping"
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.
pushed update that changes the description and keyword arg name
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.
For the first release, may we please not reveal in the cli help the custom-mapping feature until we've had a chance to adapt Heimdall and its CAAT export to clearly expose the (user-provided) aspect?
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.
disabled cli arg for the custom mapping for the time being
lib/heimdall_tools/cli.rb
Outdated
option :output, required: true, aliases: '-o' | ||
option :verbose, type: :boolean, aliases: '-V' | ||
def aws_config_mapper | ||
hdf = HeimdallTools::AwsConfigMapper.new(options[:addl_mapping]).to_hdf |
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.
@jkufro I think you intended it to be options[: custom_mapping]
and not options[:addl_mapping]
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.
done
def initialize(addl_mapping, verbose = false) | ||
@verbose = verbose | ||
@default_mapping = get_rule_mapping(AWS_CONFIG_MAPPING_FILE) | ||
@addl_mapping = addl_mapping.nil? ? {} : get_rule_mapping(addl_mapping) |
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.
Please unify to custom_mapping
in sync with usage in the cli.rb.
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.
done
# If there is overlap in rule names from @default_mapping and @addl_mapping, | ||
# then the tags from both will be added to the rule. | ||
def to_hdf | ||
controls = @issues.filter_map do |issue| |
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.
@jkufro filter_map
is only available in ruby 7.x which will limit heimdall_tools to ruby 7.x.
Please use and alternate implementation that doesn't not force a > 2.7 dependency.
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.
Changing back to map. I had changed this to filter_map
previously for something that I have since removed.
@@ -197,6 +198,20 @@ FLAGS: | |||
example: heimdall_tools jfrog_xray_mapper -j xray_results.json -o xray_results_hdf.json | |||
``` | |||
|
|||
## aws_config_mapper |
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.
Please include details from your excellent documentation from aws_config_mapper.md
here in the Readme as well.
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.
done
Signed-off-by: Justin Kufro <[email protected]>
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.
Looks good, Thanks!
LGTM, thanks @jkufro . Please resolve conflicts, that should be the last fix. |
Just a reminder, please add an issue to the board to revisit the issue of 'user provided' data - aka overlay data - in our workflow so we can make sure to address the capability we didn't want to try to push in v1.0 |
|
👍🏻 great job all |
Adds aws_config_mapper. This pulls Ruby AWS SDK data to translate AWS Config Rule results into HDF format json to be viewable in Heimdall. Our intended use case is to incorporate this into an AWS lambda that will pull & translate config compliance and push that up to a Heimdall server via the API.
Example execution (relies on having AWS credentials set up on your local machine):
heimdall_tools aws_config_mapper -m my_addl_rule_mappings.csv -o aws_config_results.json