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

AWS Schemas, Take 5 #123

Merged
merged 5 commits into from
Sep 13, 2021
Merged

AWS Schemas, Take 5 #123

merged 5 commits into from
Sep 13, 2021

Conversation

wmunyan
Copy link
Contributor

@wmunyan wmunyan commented May 4, 2021

This PR represents the addition of a new schema applicable to the collection and evaluation of AWS environments using their respective API.

The current proposal includes an enumeration for the various AWS services that may be queried using the API/SDK/CLI. I am not married to the idea that this element needs to be an enumeration, as the list services changes/expands/evolves rapidly. If this element is changed to a simple Entity(Object|State|Item)StringType, I am fine with that, and can include documentation that if a service is incorrect or unavailable, that some sort of error should occur (maybe an item status="error" or similar).

This PR includes 3 test/obj/state constructs:

  • api_test: The API test allows for the invocation of the AWS API for those operations that return JSON content, and allows collection of specific data
  • apicontent_test: The API CONTENT test allows for the invocation of the AWS API for those operations that do not return JSON content, but plain text, such as the aws s3 ls operation. This object contains a regular expression pattern similar to that of the textfilecontent object, to parse the output.
  • credentialreport_test: The Credential Report test is a very specific test with regards to generating and parsing the output of the AWS credential report. More info in the schema documentation on that.

This updates the API test according to @solind's comment regarding using an element for holding JSON data and using JSONPath expressions to evaluate collection of information.

@wmunyan
Copy link
Contributor Author

wmunyan commented May 4, 2021

Re-Adding @DavidRies comments from previous PR:

Hi @wmunyan ,

I reviewed the new tests and... generally this all looks great! I'm excited to have these capabilities in OVAL.

Here are a few minor comments...

Versioning?
Because the api_test and apicontent_test tests require content authors to encode specific AWS API methods in the content as well as expectations of specific response data structures, content may break (or yield unintended results) when the API changes.

Should we add an API version (required for all objects, optional for state, included in item)? That would allow the engine to attempt to use the API version the content author wrote against. If/when the API version is no longer available, that would also make it much easier to identify the affected objects. WDYT?

API Orientation of api_test and apicontent_test
Generally speaking, I prefer OVAL tests that are configuration/control-oriented over those that are API/protocol-oriented like these. When crafted properly, they can make the content easier to write (and read) and can allow the engines to absorb the complexities of the APIs and result data structures.

For example, it might be nicer to have a instance_security_group_test that allowed authors to make assertions about the protocols allowed for ingress/egress to instances. With the protocol-oriented tests proposed, I think this is still possible, but it might require making calls to one API method to get instances and associated security group ids which are used as a filter on the results of another object/API call to get security group ingress/egress properties.

However, I do think this protocol-orientation is the right approach right now. The API is so extensive that creating simplified, semantic OVAL tests for common use cases would take a long time. And, because the API and security controls are evolving relatively quickly, we'd need this generic/API capability in any case.

I hope that over time, we--as a community--will discover common usage patterns of these API calls and will add simplified, semantic tests for them.

credentialreport_test
I'm not familiar with this report or the types of assertions that would typically made against it. Having access_key_1_* entities along with access_key_2_* entities might be inconvenient.

For example, if I wanted to make an assertion about the last rotated date, I would need to make it twice, once for access_key_1_last_rotated and once for access_key_2_last_rotated. The same goes for the cert_1 and cert_2 entities.

Do you think it would make sense to break this into three tests as follows:

credentialreportsuser_test with state/item entities: user, arn, user_creation_time, password_enabled, password_last_used, password_last_changed, password_next_rotation, mfa_active
credentialreportskey_test with state/item entities: user, arn, access_key_active, access_key_last_rotated, access_key_last_used_date, access_key_last_used_region, access_key_last_used_service
credentialreportscert_test with state/item entities: user, arn, cert_active, cert_last_rotated
That way the credentialreportskey_object would return one or two items depending on whether there were one or two keys for the user.

Thoughts?

-David

@wmunyan
Copy link
Contributor Author

wmunyan commented May 4, 2021

Should we add an API version?

I'm certainly not opposed to the idea, but would like some other views/comments as well. I wonder if the version field could be taken to mean "at least this version" type of thing, but then that wouldn't really account for functions/methods removed by a new version.

API Orientation of api_test and apicontent_test

In general, I would agree, and when first looking at creating the schema, we tried to do configuration/control-oriented constructs. We started by looking at our AWS Foundations benchmark, and after our first pass, we discovered the need for around 20 different tests. Perhaps these few API-oriented tests are a good enough jumping off point to further expand the schema over time?

credentialreport_test

Yeah this one's an interesting one. A number of the CIS benchmark recommendations, when auditing manually, first indicate the need to generate the credential report. There is first an API/CLI command to generate the report, and then a second to retrieve it. When retrieved, the output is a base64 encoded string that, when decoded, translates to a CSV blob of text. Each of the columns referenced in the CSV are the elements in the credentialreport_item, so a single generation of that credential report will always get you all the fields. For me it doesn't necessarily make sense to break this up into multiple tests, as all the data is collected from the initial credential report generation.

Thanks again for all the feedback!

@DavidRies
Copy link
Member

Should we add an API version?

I think the required @api_version would mean that that exact version of the API has to be used (not a minimum) by the engine and if that version isn't available, then the result would be not_checked. API versions are not necessarily backwards compatible. The content will use specific API methods that exist in a specific version of the API and specify response JSON paths based on the response structure of that API version. Other API versions (earlier or later) may not have those methods and/or may have them, but may have different response structure/formats. Content written for API version 2 may be meaningless against API version 3.

I don't know how much about AWS API versioning, but they don't seem to change very often. Some AWS guidance I found seems to recommend that API consumers using an explicit version in production code:

"We recommend locking the API version for a service if you rely on it in production code. This can isolate your applications from service changes resulting from updates to the SDK." - https://docs.aws.amazon.com/sdk-for-javascript/v2/developer-guide/locking-api-versions.html

This should be a maintainable approach. When AWS deprecates a specific API version, we could easily identify all content tied to that version and upgrade it to a supported version.

Credential Report Test

For me it doesn't necessarily make sense to break this up into multiple tests, as all the data is collected from the initial credential report generation.

I assume the engine would collect the data as you described (once). The advantage of using multiple tests is that it would make some content simpler. For example, to assert that access keys have been rotated in the proposed schema may require:

  • AND:
    • OR:
      • access_key_1_last_rotated > some date
      • access_key_1 does not exist
    • OR:
    • access_key_2_last_rotated > some date
    • access_key_2 does not exist

Whereas, with a credentialreportskey_test that returns an item for each access key (if they exist):

  • NOT:
    • access_key <= some date

@wmunyan
Copy link
Contributor Author

wmunyan commented Jun 7, 2021

@DavidRies I've read your ideas around splitting the credentialreport test into the 3 separate tests, and I think I am on-board with the change. I will make an update to the schema and we can see how it looks.

@wmunyan wmunyan requested a review from DavidRies June 7, 2021 18:24
@wmunyan
Copy link
Contributor Author

wmunyan commented Jul 9, 2021

@DavidRies Any chance you can commit to a review before the next Area Supervisors meeting on 7/19?

@DavidRies
Copy link
Member

I'll do my best!

@DavidRies
Copy link
Member

Hi @wmunyan, I see you've added API version and split the credential report test as we discussed. That looks good to me--I hope it ends up making the content more reliable, maintainable and simple (as intended), :).

@solind
Copy link

solind commented Aug 3, 2021

@wmunyan don't forget to add aws and panos values to the FamilyEnumeration in oval-common-schema.xsl as well.

@wmunyan
Copy link
Contributor Author

wmunyan commented Aug 3, 2021

@solind Thank you! Updated and added to PR.

@wmunyan wmunyan merged commit 97b35ab into develop Sep 13, 2021
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.

3 participants