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

Add remediation capability for GH branch protections #1174

Merged
merged 5 commits into from
Oct 15, 2023

Conversation

jhrozek
Copy link
Contributor

@jhrozek jhrozek commented Oct 11, 2023

Adds branch protection remediations and splits the previous huge branch protection rule into smaller ones.

Fixes: #1154

@jhrozek jhrozek marked this pull request as ready for review October 11, 2023 20:46
@jhrozek
Copy link
Contributor Author

jhrozek commented Oct 11, 2023

Two things to follow up on but I don't think they block the review (should block accepting the PR though..)

  • [] tests for the new remediator
  • [] include the new rule_types in the example policy (but the example policy should work)

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.

The code LGTM, how tricky would it get to add some basic tests?

@@ -4,7 +4,7 @@ type: rule-type
name: branch_protection
context:
provider: github
description: Verifies that a branch has proper protections.
description: Verifies that a branch has a branch protection rule
Copy link
Contributor

Choose a reason for hiding this comment

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

I wold suggest renaming this to something like branch_protection_enabled to mark that it's a simple boolean rule.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sure, thanks

@jhrozek
Copy link
Contributor Author

jhrozek commented Oct 12, 2023

The code LGTM, how tricky would it get to add some basic tests?

working on the tests now, using the mock API.

@jhrozek
Copy link
Contributor Author

jhrozek commented Oct 12, 2023

btw I have trouble with the allow_fork_syncing remediation so for now I didn't include it in the profile.yaml. I'll file a separate ticket and will debug that issue further, but all that is included in this PR should work..

JAORMX
JAORMX previously approved these changes Oct 13, 2023
@jhrozek
Copy link
Contributor Author

jhrozek commented Oct 13, 2023

Thanks for the review, I will first review @rdimitrov 's PR on refactoring the engine for alerts to see which rebase is easier

lukehinds
lukehinds previously approved these changes Oct 13, 2023
Copy link
Contributor

@lukehinds lukehinds left a comment

Choose a reason for hiding this comment

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

lgtm

@jhrozek
Copy link
Contributor Author

jhrozek commented Oct 14, 2023

FWIW, I'm holding off merging this because I think merging #1192 first would make rebasing easier. I'll rebase the PR atop the current master, but I'll already work on forward-porting the PR locally atop #1192.

jhrozek and others added 5 commits October 15, 2023 16:52
We will be implementing a github-specific branch protection remediator
to make our life easier when working with the complex branch protection
API. The first step is to implement the protobuf type.

At the moment, the type only has one field which is the patch.
We'll need this to implement the GH branch protection remediator.
Remediating github branch protections using plain REST calls proved to
be very difficult because of several discrepancies between the data
returned on GET and the expected data sent on POST.

Instead, we're going to use the GH API with a bespoke GH branch
protection remediatior.
To provide better granularity, testability and enable different
remediations for different tunables, let's split the branch protection
rule-type into several.

Co-Authored-By: Juan Antonio Osorio <ozz@stacklok.com>
@jhrozek
Copy link
Contributor Author

jhrozek commented Oct 15, 2023

rebased atop Radoslav's recent alerting work

@rdimitrov rdimitrov merged commit abe69a8 into stacklok:main Oct 15, 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
Development

Successfully merging this pull request may close these issues.

Implement remediations for branch protection
4 participants