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 a YQ-powered remediation function #4830

Merged
merged 2 commits into from
Oct 29, 2024

Conversation

jhrozek
Copy link
Contributor

@jhrozek jhrozek commented Oct 27, 2024

Summary

  • Extend the PR evaluator with generic params - The PR evaluator used to have typed params for every function we'd add - there's one for frizbee, one for the pull_request_content function etc. This is not great, as we tie the functions to our protobuf API and every new function requires a client release to be done or else the clients can't even add ruletypes with the new functions. Let's just use a generic structpb.Struct going forward. This time it is still a change that needs client support, but going forward we'll just have to change the server code to add a new function.
  • Add YQ-evaluating remediator - In order to be able to change YAML files such as github workflows safely and with minimal amount of changes, we need to add a new remediation function in addition to the put-a-content-somewhere and call-frizbee ones we have now. I chose to add one based on libyq which does a decent job at retaining comments and general YAML structure.

Fixes: #4815

Change Type

  • Bug fix (resolves an issue without affecting existing features)
  • Feature (adds new functionality without breaking changes)
  • Breaking change (may impact existing functionalities or require documentation updates)
  • Documentation (updates or additions to documentation)
  • Refactoring or test improvements (no bug fixes or new functionality)

Testing

a mix of manual testing and unit tests. You can see one such PR created by this function here: jakubtestorg/bad-workflows@4b2863b

Review Checklist:

  • Reviewed my own code for quality and clarity.
  • Added comments to complex or tricky code sections.
  • Updated any affected documentation.
  • Included tests that validate the fix or feature.
  • Checked that related changes are merged.

@jhrozek jhrozek requested a review from a team as a code owner October 27, 2024 12:49
jhrozek added a commit to jhrozek/minder-rules-and-profiles that referenced this pull request Oct 27, 2024
This PR depends on having mindersec/minder#4830
merged first as it takes the remediation function added there into
effect.

The remediation works as follows:
 - if there are any instances of pull_request target objects those are
   removed
 - else if there are any instances of pull_request strings in an array
   those are removed
 - if the resulting array of array of objects would have length 0,
   `workflow_dispatch` is added instead

Fixes: mindersec#201
jhrozek added a commit to mindersec/minder-rules-and-profiles that referenced this pull request Oct 27, 2024
This PR depends on having mindersec/minder#4830
merged first as it takes the remediation function added there into
effect.

The remediation works as follows:
 - if there are any instances of pull_request target objects those are
   removed
 - else if there are any instances of pull_request strings in an array
   those are removed
 - if the resulting array of array of objects would have length 0,
   `workflow_dispatch` is added instead

Fixes: #201
@jhrozek jhrozek force-pushed the pull_request_target_remediation branch from 45fb1d6 to 15e95fe Compare October 27, 2024 13:28
@jhrozek
Copy link
Contributor Author

jhrozek commented Oct 27, 2024

The tests seem to be failing with:

2024-10-27T13:38:09.2011051Z ##[group]�[0;31m❌ TestExampleRulesAreValidatedCorrectly/secret_scanning.test.yaml�[0;37m (10ms)�[0m
2024-10-27T13:38:09.2012941Z     rule_validator_test.go:46: parsing rule type ../../examples/rules-and-profiles/rule-types/github/secret_scanning.test.yaml
2024-10-27T13:38:09.2014030Z     rule_validator_test.go:48: 
2024-10-27T13:38:09.2015375Z         	Error Trace:	/home/runner/work/minder/minder/pkg/profiles/rule_validator_test.go:48
2024-10-27T13:38:09.2016741Z         	Error:      	Received unexpected error:
2024-10-27T13:38:09.2018774Z         	            	error validating resource meta: invalid resource type: invalid resource type: 
2024-10-27T13:38:09.2020305Z         	Test:       	TestExampleRulesAreValidatedCorrectly/secret_scanning.test.yaml
2024-10-27T13:38:09.2022201Z         	Messages:   	failed to parse rule type ../../examples/rules-and-profiles/rule-types/github/secret_scanning.test.yaml

which sounds unrelated. I can take into this tomorrow, just going to tag @blkt who was looking into skipping tests lately in case it is a known issue.

The PR evaluator used to have typed params for every function we'd add -
there's one for frizbee, one for the pull_request_content function etc.
This is not great, as we tie the functions to our protobuf API and every
new function requires a client release to be done or else the clients
can't even add ruletypes with the new functions.

Let's just use a generic `structpb.Struct` going forward. This time it
is still a change that needs client support, but going forward we'll
just have to change the server code to add a new function.
In order to be able to change YAML files such as github workflows safely and
with minimal amount of changes, we need to add a new remediation
function in addition to the put-a-content-somewhere and call-frizbee
ones we have now.

I chose to add one based on `libyq` which does a decent job at retaining
comments and general YAML structure.

Fixes: mindersec#4815
@jhrozek jhrozek force-pushed the pull_request_target_remediation branch from 15e95fe to c20ed0f Compare October 28, 2024 16:55
@jhrozek
Copy link
Contributor Author

jhrozek commented Oct 28, 2024

The tests seem to be failing with:

2024-10-27T13:38:09.2011051Z ##[group]�[0;31m❌ TestExampleRulesAreValidatedCorrectly/secret_scanning.test.yaml�[0;37m (10ms)�[0m
2024-10-27T13:38:09.2012941Z     rule_validator_test.go:46: parsing rule type ../../examples/rules-and-profiles/rule-types/github/secret_scanning.test.yaml
2024-10-27T13:38:09.2014030Z     rule_validator_test.go:48: 
2024-10-27T13:38:09.2015375Z         	Error Trace:	/home/runner/work/minder/minder/pkg/profiles/rule_validator_test.go:48
2024-10-27T13:38:09.2016741Z         	Error:      	Received unexpected error:
2024-10-27T13:38:09.2018774Z         	            	error validating resource meta: invalid resource type: invalid resource type: 
2024-10-27T13:38:09.2020305Z         	Test:       	TestExampleRulesAreValidatedCorrectly/secret_scanning.test.yaml
2024-10-27T13:38:09.2022201Z         	Messages:   	failed to parse rule type ../../examples/rules-and-profiles/rule-types/github/secret_scanning.test.yaml

which sounds unrelated. I can take into this tomorrow, just going to tag @blkt who was looking into skipping tests lately in case it is a known issue.

This was resolved by rebasing atop origin/main

@jhrozek jhrozek merged commit 800526c into mindersec:main Oct 29, 2024
26 checks passed
jhrozek added a commit to mindersec/minder-rules-and-profiles that referenced this pull request Nov 5, 2024
This PR depends on having mindersec/minder#4830
merged first as it takes the remediation function added there into
effect.

The remediation works as follows:
 - if there are any instances of pull_request target objects those are
   removed
 - else if there are any instances of pull_request strings in an array
   those are removed
 - if the resulting array of array of objects would have length 0,
   `workflow_dispatch` is added instead

Fixes: #201
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.

Add a YQ modifier as a pull request action
2 participants