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.

Sign up for GitHub

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

Jump to bottom

update: add extra CI checks for rules changes #67

Merged
merged 18 commits into from
Jun 1, 2023
Merged

update: add extra CI checks for rules changes #67

merged 18 commits into from
Jun 1, 2023

Conversation

jasondellaluce
Copy link
Contributor

What type of PR is this?

/kind feature

Any specific area of the project related to this PR?

/area build

What this PR does / why we need it:

Which issue(s) this PR fixes:

This PR serves to experiment options for making our CI stronger.

Special notes for your reviewer:

@jasondellaluce @loresuso
new(build): add tool for checking and validating rules files
615c8ee 
Co-authored-by: Lorenzo Susini <[email protected]>
Signed-off-by: Jason Dellaluce <[email protected]>
@poiana poiana requested review from darryk10 and Kaizhe May 25, 2023 13:41
@jasondellaluce jasondellaluce force-pushed the experimental/ci-checks branch 15 times, most recently from 64e3a22 to fb5e35b Compare May 25, 2023 14:56
@github-actions
Copy link

github-actions bot commented May 25, 2023 •
edited
Loading

rules/falco_rules.yaml

Comparing f00118b4d11cc7edf6e7ade532aead908e088291 with latest tag falco-rules-0.1.0

Major changes:

  • List white_listed_modules has been removed
  • Rule Contact EC2 Instance Metadata Service From Container has been disabled at default
  • Rule Outbound Connection to C2 Servers has been disabled at default
  • Rule Java Process Class File Download has been disabled at default

Minor changes:

  • Required engine version was incremented from 13 to 17
  • Rule PTRACE anti-debug attempt has been added
  • Rule Drop and execute new binary in container has been added
  • Macro ptrace_attach_or_injection has been added
  • Macro known_aks_mount_in_privileged_containers has been added
  • Macro kernel_module_load has been added
  • List allowed_container_images_loading_kernel_module has been added
  • List authorized_server_binary has been added
  • List known_drop_and_execute_containers has been added
  • List python_package_managers has been added

Patch changes:

  • Rule Linux Kernel Module Injection Detected changed its output fields
  • Rule PTRACE attached to process matches more events than before
  • List docker_binaries has some item added or removed
  • List rpm_binaries has some item added or removed
  • List package_mgmt_binaries has some item added or removed
  • List safe_etc_dirs has some item added or removed
  • List falco_privileged_images has some item added or removed
  • List network_tool_binaries has some item added or removed
  • List user_known_k8s_ns_kube_system_images has some item added or removed

@jasondellaluce jasondellaluce force-pushed the experimental/ci-checks branch from fb5e35b to 94a9790 Compare May 25, 2023 15:02
@jasondellaluce
update(cmd): add tests
bfdc712 
Signed-off-by: Jason Dellaluce <[email protected]>
@jasondellaluce
chore: add copyright notice
ff95410 
Signed-off-by: Jason Dellaluce <[email protected]>
@jasondellaluce jasondellaluce force-pushed the experimental/ci-checks branch 2 times, most recently from ecf0d21 to d393be5 Compare May 30, 2023 15:32
@maxgio92
Copy link
Member

maxgio92 commented May 31, 2023 •
edited
Loading

Awesome work @jasondellaluce!

@jasondellaluce
Copy link
Contributor Author

Awesome work @jasondellaluce!

Shoutout to @loresuso as well!

@jasondellaluce jasondellaluce changed the title wip: update: add extra CI checks for rules changes update: add extra CI checks for rules changes May 31, 2023
@jasondellaluce
Copy link
Contributor Author

Now ready for review! @falcosecurity/rules-maintainers

FedeDP
FedeDP reviewed May 31, 2023
View reviewed changes
.github/workflows/ci.yaml Outdated
echo "comment_file=result.txt" >> $GITHUB_OUTPUT
fi

- uses: mshick/add-pr-comment@v2
Copy link
Contributor

Choose a reason for hiding this comment

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

This is great! However, i am a bit worried if this will work when run on a PR coming from a fork. I had similar issues trying to post a comment on libs repo and had to move to pull_request_target trigger that is unsafe: falcosecurity/libs#977

Copy link
Contributor

Choose a reason for hiding this comment

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

That's it: https://github.com/mshick/add-pr-comment#proxy-for-fork-based-prs

@jasondellaluce jasondellaluce force-pushed the experimental/ci-checks branch from 24d26dc to 855b3c8 Compare May 31, 2023 11:16
@jasondellaluce jasondellaluce force-pushed the experimental/ci-checks branch from 248291a to ecdaa6f Compare May 31, 2023 13:21
@jasondellaluce
Copy link
Contributor Author

@FedeDP @LucaGuerra I implemented the more complex solution presented in see: https://securitylab.github.com/research/github-actions-preventing-pwn-requests/.
The idea is to have the checks happen with the pull_request trigger, which is supposed to run untrusted code. Then, a secondary workflow is triggered with workflow_run with the sole responsibility of retrieving text from a stored artifact and opening a comment automatically with its content.

The workflow_run trigger will need the workflow file to be present in the main branch, so the very final test will happen once this PR is merged.

leogr
leogr reviewed May 31, 2023
View reviewed changes
Copy link
Member

@leogr leogr left a comment

Choose a reason for hiding this comment

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

Overall, looks great!

Just a few questions. Please take a look at my comments below.

@jasondellaluce @leogr
docs(RELEASE.md): apply review suggestions
802db27 
Co-authored-by: Leonardo Grasso <[email protected]>
Signed-off-by: Jason Dellaluce <[email protected]>
leogr
leogr approved these changes Jun 1, 2023
View reviewed changes
Copy link
Member

@leogr leogr left a comment

Choose a reason for hiding this comment

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

Looks very good to me know.

Great job, thank you 🙏

@poiana poiana added the lgtm label Jun 1, 2023
@poiana
Copy link

poiana commented Jun 1, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jasondellaluce, leogr

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:
  • OWNERS [jasondellaluce,leogr]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@poiana
Copy link

poiana commented Jun 1, 2023

LGTM label has been added.

Git tree hash: 3bd66d5371a01d27260256d5dac2cef52e8c103f

@poiana poiana merged commit c45969d into main Jun 1, 2023
@poiana poiana deleted the experimental/ci-checks branch June 1, 2023 10:13
@jasondellaluce jasondellaluce added this to the falco-rules-1.0.0 milestone Jun 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@FedeDP FedeDP FedeDP left review comments

@leogr leogr leogr approved these changes

@darryk10 darryk10 Awaiting requested review from darryk10

@Kaizhe Kaizhe Awaiting requested review from Kaizhe

Assignees

@leogr leogr

Labels
approved area/build dco-signoff: yes kind/feature New feature or request lgtm size/XXL
Projects
None yet
Milestone
falco-rules-1.0.0
Development

Successfully merging this pull request may close these issues.
6 participants
@jasondellaluce @maxgio92 @poiana @leogr @FedeDP @loresuso