-
Notifications
You must be signed in to change notification settings - Fork 709
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
Enable bandit scanning #954
Conversation
Codecov ReportPatch and project coverage have no change.
Additional details and impacted files@@ Coverage Diff @@
## main #954 +/- ##
=======================================
Coverage 81.34% 81.34%
=======================================
Files 176 176
Lines 6812 6812
=======================================
Hits 5541 5541
Misses 1271 1271 Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks. I have a few comments
@@ -7,6 +7,26 @@ on: | |||
- cron: "0 18 * * 1-5" | |||
|
|||
jobs: | |||
Bandit: | |||
runs-on: ubuntu-20.04 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need this? The CI already runs on ubuntu 20 image. If we add this it might unnecessarily pull the image and given the proxy settings, pulling images generally fails stating "too many requests"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it's about using the Github-hosted runner instance instead of Self-hosted one. so we don't need to worry about the image pulling burden at all.
in case of Snyk, scanning request should be passed to the internal resource but it's not possible to access it from external (GH-hosted one) without having something like VPN. so that's the reason two jobs run on different machine at this point.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alright. Makes sense.
ipas_default.config
Outdated
@@ -0,0 +1,410 @@ | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it possible to move this to some other folder instead of root?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rather than having this config in the root or somewhere else, would it be possible to move this to the pyproject.toml
file?
We ideally want to collect all the configuration within the pyproject.toml
file. I can see from the bandit documentation, it is possible:
[tool.bandit]
exclude_dirs = ["tests", "path/to/file"]
tests = ["B201", "B301"]
skips = ["B101", "B601"]
[tool.bandit.any_other_function_with_shell_equals_true]
no_shell = [
"os.execl",
...
And to run bandir based on the configurations from pyproject.toml
file
bandit -c pyproject.toml -r .
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@samet-akcay Yes, I know that but my concern is on that I've copied the bandit config file from IPAS (Intel Product Assurance and Security) which contains minimum requirements for specific task in the SDL (internal release process), so I think, it would be better to keep it as same as original to track the updating their side and applying them to our projects efficiently. how do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
here is the internal repo which has configuration: https://github.com/intel-innersource/applications.security.bandit-config
tox.ini
Outdated
allowlist_externals = | ||
bandit | ||
commands = | ||
- bandit -r -c {toxinidir}/ipas_default.config {toxinidir}/ -f txt -o {toxworkdir}/bandit-report.txt |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we can place the config in .ci
. We can then use {toxinidir}/.ci/ipas_default.config
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that's clear. will update.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as the above comment. I think we should have something like
bandit -r -c {toxinidir}/pyproject.toml ...
76c1a0c
to
0b0406f
Compare
Today Bandit updated and some items were removed so that need to update configuration file
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks
* Enable bandit scanning (#954) * Temporarily add unit env to tox * Add pytest xdist n flag * Add base, image, mvtec, btech and folder datasets * Add base video tests * Add Avenue datasets tests * Add Visa dataset * Add shanghaitech dataset * Add ucsd ped dataset * Add base depth datamodule tests * Add MVTec3D datamodule tests * Rename mvtec3d tests * Add 3d depth dataset tests * Remove commented code * Uncomment jupyter notebook tests * Remove random subset * Add get_dataset_path` * Add __init__ * isort * Revert "isort" This reverts commit 9083f0c. --------- Co-authored-by: Yunchu Lee <[email protected]>
Description
Changes
Checklist