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

Enable bandit scanning #954

Merged
merged 4 commits into from
Mar 10, 2023
Merged

Enable bandit scanning #954

merged 4 commits into from
Mar 10, 2023

Conversation

yunchu
Copy link
Contributor

@yunchu yunchu commented Mar 9, 2023

Description

  • Enabled bandit code scanning by adding jobs in code_scan workflow
  • Configuration file for the bandit was imported/added from IPAS as a default
  • Corresponding tox testenv also added
  • The report will be posted as the artifact to the workflow

Changes

  • Bug fix (non-breaking change which fixes an issue)
  • Refactor (non-breaking change which refactors the code base)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

Checklist

  • My code follows the pre-commit style and check guidelines of this project.
  • I have performed a self-review of my code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing tests pass locally with my changes
  • I have added a summary of my changes to the CHANGELOG (not for minor changes, docs and tests).

@codecov
Copy link

codecov bot commented Mar 9, 2023

Codecov Report

Patch and project coverage have no change.

Comparison is base (522858f) 81.34% compared to head (cff2ac8) 81.34%.

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.
📢 Do you have feedback about the report comment? Let us know in this issue.

@yunchu yunchu enabled auto-merge (squash) March 9, 2023 05:21
Copy link
Collaborator

@ashwinvaidya17 ashwinvaidya17 left a 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
Copy link
Collaborator

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"

Copy link
Contributor Author

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Alright. Makes sense.

@@ -0,0 +1,410 @@

Copy link
Collaborator

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?

Copy link
Contributor

@samet-akcay samet-akcay Mar 9, 2023

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 .

Copy link
Contributor Author

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

tox.ini Outdated
allowlist_externals =
bandit
commands =
- bandit -r -c {toxinidir}/ipas_default.config {toxinidir}/ -f txt -o {toxworkdir}/bandit-report.txt
Copy link
Collaborator

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

Copy link
Contributor Author

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.

Copy link
Contributor

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 ...

Today Bandit updated and some items were removed so that need to update configuration file
Copy link
Collaborator

@ashwinvaidya17 ashwinvaidya17 left a comment

Choose a reason for hiding this comment

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

Thanks

@yunchu yunchu merged commit c6bd757 into main Mar 10, 2023
@yunchu yunchu deleted the yunchule/enable-bandit branch March 10, 2023 15:45
samet-akcay added a commit that referenced this pull request Mar 22, 2023
* 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 <yunchu.lee@intel.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants