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 Bandit security linting; update requirements #562

Merged
merged 5 commits into from
Jan 10, 2018
Merged

Conversation

austinbyers
Copy link
Contributor

to: @jacknagz or @ryandeivert
cc: @airbnb/streamalert-maintainers
size: small

Background

Bandit is a Python scanner which checks for common security issues in Python source code. I've already successfully added Bandit to BinaryAlert, and now we add to StreamAlert.

Changes

Fortunately, Bandit did not flag any security issues! No code changes were necessary

  • Adds bandit to requirements
  • Removes virtualenv from requirements (you don't need to install virtualenv, you are already in a venv when installing requirements)
  • Updates frozen requirements set with latest versions

Testing

  • Travis CI: unit tests, linting, and now security checks

@coveralls
Copy link

Coverage Status

Coverage remained the same at 95.187% when pulling bcb1e98 on austin-bandit into 4e12317 on master.

Copy link
Contributor

@ryandeivert ryandeivert left a comment

Choose a reason for hiding this comment

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

Thanks for adding this and cleaning things up. ¡muy bien! 🚢

@ryandeivert
Copy link
Contributor

@austinbyers one comment: you mention this is enforced in travis ci... does it only take adding a .bandit file for travis to pick it up? Asking because I don't see any changes to the .travis.yml :)

.bandit Outdated
@@ -0,0 +1,10 @@
[bandit]
Copy link
Contributor

Choose a reason for hiding this comment

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

can this live in the setup.cfg file instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Great idea! Yes, it works

requirements.txt Outdated
@@ -1,90 +1,85 @@
Sphinx==1.6.5
alabaster==0.7.10
Copy link
Contributor

Choose a reason for hiding this comment

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

the changes this file breaks the separation that previously existed, can you follow these two steps in order to retain that?

# Installing the top-level dependencies, upgrading them if necessary
pip install –r requirements-top-level.txt --upgrade 

# Updating requirements.txt
pip freeze –r requirements-top-level.txt > requirements.txt

Copy link
Contributor

Choose a reason for hiding this comment

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

The main issue I foresee with this is that dev requirements are lost with this workflow

Copy link
Contributor

Choose a reason for hiding this comment

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

@ryandeivert how's that? developers just pip install requirements.txt as normal

Copy link
Contributor

Choose a reason for hiding this comment

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

ah sorry I didn't realize the dev/test requirements were also in the normal requirements file. we should definitely move away from that

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jacknagz, that is exactly what I did, with the exception that I didn't order the requirements with the -r flag to pip freeze. So it was just the requirements order that changed (the files have the same meaning and contents).

I've put the order back, thanks for listing the command!

@coveralls
Copy link

Coverage Status

Coverage remained the same at 95.189% when pulling 825d7c3 on austin-bandit into da7e05e on master.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 95.189% when pulling 62d2454 on austin-bandit into da7e05e on master.

@austinbyers
Copy link
Contributor Author

@ryandeivert good catch! Yep, I forgot to add bandit to .travis.yml. It's there now!

@jacknagz
Copy link
Contributor

👍

@austinbyers austinbyers merged commit cf12209 into master Jan 10, 2018
@austinbyers austinbyers deleted the austin-bandit branch January 10, 2018 19:29
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.

4 participants