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

Implement policy remediation support in the engine and add a REST remediator #1057

Merged
merged 8 commits into from
Oct 2, 2023

Conversation

jhrozek
Copy link
Contributor

@jhrozek jhrozek commented Sep 29, 2023

This PR adds the first usable part of policy remediations. Currently there is
remediation only for one rule type to enable testing, but the rest should be
very easy to add.

There are no tests yet in this PR - I do think we should block on that but I
wanted to feedback and write the tests in the meantime. Otherwise I think the
PR is in decent shape and (review permitting and after adding tests) can be
merged.

The only change to make with respect to policies is to enable the remediations in your policy object:

---
# sample policy for validating repositories
version: v1
type: policy
name: acme-github-policy
context:
  group: Root Group
  provider: github
remediate: on   <----- HERE
repository:
      - type: secret_scanning

Maybe this is something we want to enable globally, but I wasn't sure.
Currently the default is controlled by Mediator and defaults to "do nothing".

To test, make sure the policies are loaded, then put the repository into
the non-compliant state:

curl -L -X PUT \
 -H "Accept: application/vnd.github+json" \
 -H "Authorization: Bearer $TOKEN" \
 -H "X-GitHub-Api-Version: 2022-11-28" \
 https://api.github.com/repos/jakubtestorg/bad-go/actions/permissions \
 -d '{ "enabled": true, "allowed_actions": "all" }'

Then trigger policy evaluation either with medev or by sending a repo webhook.

Afterwards, verify that the repo is now compliant:

curl -L \
 -H "Accept: application/vnd.github+json" \
 -H "Authorization: Bearer $TOKEN" \
 -H "X-GitHub-Api-Version: 2022-11-28" \
 https://api.github.com/repos/jakubtestorg/testrepo/actions/permissions

You should see something like:

{
  "enabled": true,
  "allowed_actions": "selected",
  "selected_actions_url": "https://api.github.com/repositories/678351736/actions/permissions/selected-actions"
}

Fixes: #1040
Fixes: #1055
Fixes: #1056

@@ -198,6 +198,7 @@ CREATE TABLE policies (
name TEXT NOT NULL,
provider TEXT NOT NULL,
group_id INTEGER NOT NULL,
remediate TEXT,
Copy link
Contributor

Choose a reason for hiding this comment

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

should remediate be an enum instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I guess yes, on the database level it could be. I had trouble with enums in the grpc code though, they are generated into integers and become a bit awkward to use.

Copy link
Contributor

Choose a reason for hiding this comment

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

Just enums in the db is fine

Copy link
Contributor

@JAORMX JAORMX left a comment

Choose a reason for hiding this comment

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

small comments and we need some tests. Otherwise I think the direction is good.

Remediate: sql.NullString{},
}

if in.Remediate != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

This requires validation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done as part of moving to db enums

}

return rest.NewRestRemediate(rem.GetRest(), pbuild)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

will we need a NOOP remediator for cases where it's not set yet?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

noop remediator added

rrem, err := remediate.NewRuleRemediator(rt, cli)
if errors.Is(err, remediate.ErrNoRemediation) {
// we should be graceful about not having a remediator
// TODO: return a noop remediator instead that would log that there's nothing configured?
Copy link
Contributor

Choose a reason for hiding this comment

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

ah, I see you already had the idea of a noop remediator.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

noop remediator added

@jhrozek
Copy link
Contributor Author

jhrozek commented Oct 2, 2023

oops sorry looks like I need one more rebase

@jhrozek
Copy link
Contributor Author

jhrozek commented Oct 2, 2023

oops sorry looks like I need one more rebase

done

JAORMX
JAORMX previously approved these changes Oct 2, 2023
This will mostly be used for dry-run of the remediations as we want to
be able to include the URL of the REST server we are talking to.
…emediate toggle to the Policy message

Extends RuleType with a new Remediate message that holds the supported
remediation methods as well as the selected type.

Adds a strings to the Policy message that will be toggled in order to
enable remediations.
Since we extended a top-level attribute of Policy, we need to also make
sure we persist the attribute in the DB and are able to load it.

I chose a NULLable string in order for the server to decide what value
is the default.
The REST remediator and the REST ingestor would share a bunch of code.
Let's reduce the code duplication by providing several utility
functions.
In addition to the current Ingest and Eval, this adds a third step
Remediate that can be used to fix a policy finding. No remediator is
currently provided, this will come in a separate patch.

Fixes: #1040
Implements a generic REST remediator that constructs a message based on
the policy input and sends it using the configured method, typically PUT
or PATCH.

Fixes: #1056
Adds a first example for testing remediations.
@jhrozek
Copy link
Contributor Author

jhrozek commented Oct 2, 2023

just rebased atop recent changes again

@jhrozek
Copy link
Contributor Author

jhrozek commented Oct 2, 2023

thanks for the quick review!

@jhrozek jhrozek merged commit fb747cd into mindersec:main Oct 2, 2023
13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants