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

feat: security ci to check vulnerability in superset added for superset #24491

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

hash-data
Copy link

SUMMARY

Security ci contains the following modules which detect the vulnerabilities
-> Bandit
-> PIP Audit
-> safety

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

  • Has associated issue:
  • Required feature flags:
  • Changes UI
  • Includes DB Migration (follow approval process in SIP-59)
    • Migration is atomic, supports rollback & is backwards-compatible
    • Confirm DB migration upgrade and downgrade tested
    • Runtime estimates and downtime expectations provided
  • Introduces new feature or API
  • Removes existing feature or API

Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

Congrats on making your first PR and thank you for contributing to Superset! 🎉 ❤️

We hope to see you in our Slack community too! Not signed up? Use our Slack App to self-register.

@hash-data
Copy link
Author

hash-data commented Jun 23, 2023

@dpgaspar I have created a workflow that provide information about the vulnerability. Mostly uses open source projects and database.
Mentioning the issue here #23621

Allow the workflow to run to check the vulnerable issues.

Also thanks for the great project ❤️

@hash-data
Copy link
Author

@villebro have you guys checked ?

@rusackas
Copy link
Member

It seems this PR slipped off our collective radar. It's still an interesting idea, but seems to have failed CI because of the added test itself. Maybe we can set some stuff to warn instead of error, or clean up the errors to get it through? In any case, I'm closing and reopening to reboot CI, since the logs have vanished.

@rusackas rusackas closed this Aug 19, 2024
@rusackas rusackas reopened this Aug 19, 2024
@github-actions github-actions bot added the github_actions Pull requests that update GitHub Actions code label Aug 19, 2024
Copy link
Member

@mistercrunch mistercrunch left a comment

Choose a reason for hiding this comment

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

Should we use something like this reusable action to simplify things? -> https://github.com/PyCQA/bandit-action

Also I think those checks are not deterministic, meaning as security report roll in, it can break master and unrelated/open PRs semi-randomly which has pros/cons.

My personal vote would be to trigger:

  • on PRs that touch dependencies (package-lock or requirements/)
  • maybe on a nightly or weekly schedule - though we'd need to raise an alert, ideally would email to dev@superset.apache.org or similar

@hash-data
Copy link
Author

Hi, @rusackas thanks for looking at it.
@mistercrunch That is a great idea for sending alerts,
for the second one: it must run on every pr as there can be vulnerabilities that are found later not when the dependency gets upgraded.

@mistercrunch
Copy link
Member

mistercrunch commented Aug 21, 2024

it must run on every pr as there can be vulnerabilities that are found later not when the dependency gets upgraded.

So say if I open a super simple PR fixing a typo, it shouldn't be held back because a vulnerability was found in some package that's completely unrelated to my PR. If we run bandit on every PR it means all of the open PRs (say all 300+ of them open right now) would start failing CI for things totally out of scope/knowledge for all these contributors. Also, once it's fixed in master by some PR, all other 300+ PRs now need to be rebased to pass the checks. I also don't think we want master CI (happens every time we merge a PR) to start failing semi-randomly, in a way that's unrelated to what was merged.

That's why I'm suggesting a workflow on a schedule (say @daily, which is supported by GHA) that notifies committers through email (dev@supserset.apache.org), but doesn't break ongoing unrelated to PRs or the builds on master

@hash-data
Copy link
Author

hash-data commented Aug 21, 2024

@mistercrunch agreed with it, a scheduled workflow is fine.
So to summarize
-> Must run on PRs that are doing some changes in dependencies
-> A Scheduled action that checks on a daily or weekly basis for vulnerabilities and send mail or alerts.

I will do these changes

Should we use something like this reusable action to simplify things? -> https://github.com/PyCQA/bandit-action

@mistercrunch
Copy link
Member

Yes exactly!

Here's an example of using paths: in GitHub actions https://github.com/apache/superset/blob/master/.github/workflows/check_db_migration_confict.yml#L4-L5

Given bandit is python-only, I think the path you want to add is requirements/**

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
github_actions Pull requests that update GitHub Actions code size/M
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants