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

Compliance ingestion for minimal reports #3302

Merged
merged 19 commits into from
Apr 9, 2020
Merged

Conversation

alexpop
Copy link
Contributor

@alexpop alexpop commented Apr 7, 2020

This change allows compliance reports to be ingested without the profile metadata attached to them, as long as the backend has the profile metadata.

A new report has been added in the tests that will be rejected during ingestion because the profile ID being used is unknown to the backend.

test_data/audit_reports/2018-03-04_redhat(2)-beta-non-ingestable.json

This report has been converted into a hybrid, where one profile has the metadata in it, another is missing it:

test_data/audit_reports/2018-03-05_centos(2)-beta-nginx(f)-apache(f)-failed.json

Fixes #3087


I did an end-to-end test of this change by using the audit cookbook code in this branch:
https://github.com/chef-cookbooks/audit/compare/ap/support-metasearch

I created an Automate instance from this code. Rebuilt components/compliance-service and components/automate-gateway. Imported from Compliance > Profiles > Available the DevSec Apache Baseline v2.1.0 (w/ sha256 ID: 73eba2f2c12a6014cf0880c1e7d990bb783346a92d0d46284aaf785b8206cd39) profile.

Ran the audit cookbook with the following profiles:

  • 73eba2f2c12a6014cf0880c1e7d990bb783346a92d0d46284aaf785b8206cd39
  • 09adcbb3b9b3233d5de63cd98a5ba3e155b3aaeb66b5abed379f5fb1ff143988
  • 0704c70df5f251b31b4741e0310d8b69ccf7db05e8a97c3ca0a3422335bdff88

In the first run, the Audit cookbook Info logs printed:

[2020-04-08T10:05:59+00:00] INFO: Reporting to chef-automate
[2020-04-08T10:06:00+00:00] INFO: Automate is missing metadata for the following profile ids: ["09adcbb3b9b3233d5de63cd98a5ba3e155b3aaeb66b5abed379f5fb1ff143988", "0704c70df5f251b31b4741e0310d8b69ccf7db05e8a97c3ca0a3422335bdff88"]

and sent a report where the metadata of the apache-baseline profile was stripped away as the backend already has it. The report can be seen here: report-with-profiles-2-full-1-stripped.json.txt

I verified in Automate and the report was ingested correctly with the three profiles.

In the second run of the Audit cookbook with the same profiles, the report being sent had all three profiles now being stripped of the metadata information. The report can be seen here: report-with-profiles-3-stripped.json.txt

@alexpop alexpop added the WIP label Apr 7, 2020
@alexpop alexpop requested a review from a team April 7, 2020 16:16
@alexpop alexpop force-pushed the ap/ingestion-4-min-reports branch from 2d6a117 to efaf0fb Compare April 7, 2020 17:12
@alexpop alexpop force-pushed the ap/ingestion-4-min-reports branch from efaf0fb to 7da261b Compare April 7, 2020 19:36
Copy link
Contributor

@rickmarry rickmarry left a comment

Choose a reason for hiding this comment

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

Just did peer review of this with @alexpop.. Awesome work! thanks @alexpop!

@alexpop alexpop force-pushed the ap/ingestion-4-min-reports branch from 7da261b to 4e79968 Compare April 7, 2020 22:02
for tKey, tValue := range control.Tags.Fields {
if newStringTag := relaxting.StringTagsFromProtoFields(tKey, tValue); newStringTag != nil {
stringTags = append(stringTags, *newStringTag)
if control.Tags != nil {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Made some small fixes to helper_methods.go after trying to ingest the minimal report without any changes to ingestion. Makes ingestion more resilient to malformed or malicious reports.

@@ -577,9 +577,31 @@ def reporting
"waived" => {},
"total" => 18
}
},
Copy link
Contributor Author

Choose a reason for hiding this comment

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

When we introduced tags filtering, we manually updated some reports and added tags to them. But we didn't add the tags across all profiles with the same ID. Because of this, the minimal report that is being ingested now picked up the tags from the compliance-profiles store as they were saved there when the profile was ingested first. Because of this I had to update this test.

@@ -543,6 +543,9 @@ var ComplianceRepDate = Mapping{
"type": "keyword",
"doc_values": false,
"ignore_above": 256
},
"run_time_limit": {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a new field the audit cookbook/inspec is going to provide when minimizing reports and reducing the start_time and run_time for each result.

@@ -116,18 +121,8 @@ func insertInspecProfile(msg message.Compliance, profile *relaxting.ESInspecProf
"profile_id": profile.Sha256,
}).Debug("Ingesting inspec_profile")
start := time.Now()
profileExists, err := client.ProfileExists(profile.Sha256)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We used to check for profile existence for each profile coming in the report. Reports can contain one or multiple profiles. The logic has been changed and we only query once elasticsearch asking for all the profiles in the report at once with the ProfilesMissing method

@@ -284,18 +284,21 @@ func (backend *ES2Backend) GetReports(from int32, size int32, filters map[string
reports := make([]*reportingapi.Report, 0)
if searchResult.TotalHits() > 0 && searchResult.Hits.TotalHits > 0 {
for _, hit := range searchResult.Hits.Hits {
item := ESInSpecSummary{}
item := ESInSpecReport{}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Spotted this with Rick on peer reviewing and decided to switch to the right Struct for this Unmarshalling to avoid problems in the future.

@@ -142,6 +142,19 @@ service ProfilesService {
action: "compliance:profiles:list"
};
};

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added docs for the /metasearch API used by this improvement

"profiles": [
{
"name": "nginx-baseline",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Converted this report into a hybrid one where one of its profiles is stripped of the metadata.

@alexpop alexpop changed the title WIP: Compliance ingestion for minimal reports Compliance ingestion for minimal reports Apr 8, 2020
@alexpop alexpop removed the WIP label Apr 8, 2020
}

func complianceProfile(in <-chan message.Compliance, client *ingestic.ESClient) <-chan message.Compliance {
//ctx := context.Background()
Copy link

Choose a reason for hiding this comment

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

nitpick: seems like we can remove this comment line

Copy link

@vjeffrey vjeffrey left a comment

Choose a reason for hiding this comment

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

code looks good!
would love to get a link to the audit cookbook branch so i can test all the way thru with kitchen!
silly me! you linked the audit cb here https://github.com/chef-cookbooks/audit/compare/ap/support-metasearch -- ill test it out

@alexpop
Copy link
Contributor Author

alexpop commented Apr 8, 2020

Thank you for the reviews. Audit cookbook branch to use this is ap/support-metasearch

https://github.com/chef-cookbooks/audit/compare/ap/support-metasearch

Copy link

@vjeffrey vjeffrey left a comment

Choose a reason for hiding this comment

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

tested 3x with audit cb, with profiles that existed in automate and did not, worked great!!!

alexpop and others added 16 commits April 9, 2020 09:05
Signed-off-by: Alex Pop <[email protected]>
Signed-off-by: Alex Pop <[email protected]>
Signed-off-by: Alex Pop <[email protected]>
@alexpop alexpop force-pushed the ap/ingestion-4-min-reports branch from 4e79968 to ecee466 Compare April 9, 2020 08:05
@alexpop
Copy link
Contributor Author

alexpop commented Apr 9, 2020

Removed //ctx := context.Background() and rebased with latest master. Will merge once green

@alexpop alexpop merged commit 02011e9 into master Apr 9, 2020
@chef-expeditor chef-expeditor bot deleted the ap/ingestion-4-min-reports branch April 9, 2020 08:48
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.

Reducing the Compliance reports sent for Automate ingestion
3 participants