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

Bandit security recommendations for Cylc 8 target #2998

Merged
merged 102 commits into from
Mar 20, 2019

Conversation

MartinRyan
Copy link
Contributor

recreation of pull request for the new Cylc 8

MartinRyan and others added 30 commits December 10, 2018 12:01
sync latest changes into my fork
unit tests and tweaks
remove test code block
a bandit suggestion to prevent xmlrpc attacks
# XMLRPC is particularly dangerous as it is also concerned with
# communicating data over a network. Use defused.xmlrpc.monkey_patch()
# function to monkey-patch xmlrpclib and mitigate remote XML attacks.
# https://docs.openstack.org/developer/bandit/blacklists/blacklist_imports.html#b411-import-xmlrpclib
add import of testfixtures to travis
using nosec for test file with only static input
codacy code analysis searches for popen text.  It also thinks mockopen is popen
to require a rebuild
change to python2 to prevent bandit ast errors
wrap os.system call and change permissive permissions to 755
to trigger rebuild!
@cylc cylc deleted a comment Mar 13, 2019
@matthewrmshin matthewrmshin added this to the cylc-8-alpha-1 milestone Mar 13, 2019
Copy link
Member

@hjoliver hjoliver left a comment

Choose a reason for hiding this comment

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

@MartinRyan - this looks pretty good, but can I suggest you revert all changes to lib/jinja2/ (just for this cylc-8 PR, not the 7.8.x one)? We are going to unbundle Jinja2 very soon, so these changes are pretty much pointless and will just create spurious conflicts for the removal PR.

jinja2 will be unbundled soon
@cylc cylc deleted a comment Mar 13, 2019
@cylc cylc deleted a comment Mar 14, 2019
@cylc cylc deleted a comment Mar 14, 2019
@cylc cylc deleted a comment Mar 15, 2019
@cylc cylc deleted a comment Mar 18, 2019
Copy link
Member

@hjoliver hjoliver left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@hjoliver
Copy link
Member

@matthewrmshin - can you review or reassign at the UK end?

@hjoliver
Copy link
Member

hjoliver commented Mar 20, 2019

Sorry @MartinRyan this is now conflicted too! At this point we owe you some help with the conflicts if they are non-trivial. @matthewrmshin - can we try to get these PRs done as soon as possible - Martin has suffered long enough!

@matthewrmshin
Copy link
Contributor

(OK. On the case. This will be my top priority of the day for Cylc.)

Copy link
Contributor

@matthewrmshin matthewrmshin left a comment

Choose a reason for hiding this comment

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

Perhaps not as part of this PR, but cylc.run_get_stdout should probably be combined with cylc.cylc_subproc.

One minor comment on mock. Otherwise good to go.

lib/cylc/tests/test_cylc_subproc.py Outdated Show resolved Hide resolved
.travis/install.sh Outdated Show resolved Hide resolved
@hjoliver
Copy link
Member

@matthewrmshin - I've resolved the conflicts and pushed to @MartinRyan's branch. Should be good to merge if tests pass again.

@cylc cylc deleted a comment Mar 20, 2019
@hjoliver
Copy link
Member

(and addressed your comments ... except for the "future PR" one).

Copy link
Contributor

@matthewrmshin matthewrmshin left a comment

Choose a reason for hiding this comment

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

Good to go.

I understand (but probably still disagree) with the reason behind this change. However, the change is a good solution to what is being asked.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants