ZAP active scanning for source and journalist interfaces #6617
Conversation
|
This pull request introduces 3 alerts when merging 7834fe2 into cc3ed8a - view on LGTM.com new alerts:
|
ghost
force-pushed
the
5946-zap-rewrite
branch
from
7834fe2
to
26214bc
Compare
|
This pull request introduces 3 alerts when merging 26214bc into cc3ed8a - view on LGTM.com new alerts:
|
ghost
force-pushed
the
5946-zap-rewrite
branch
6 times, most recently
from
a8120d5
to
6c5ab56
Compare
|
This pull request introduces 3 alerts when merging 6c5ab56 into cc3ed8a - view on LGTM.com new alerts:
|
ghost
force-pushed
the
5946-zap-rewrite
branch
from
6c5ab56
to
cf933b1
Compare
|
This pull request introduces 3 alerts when merging cf933b1 into cc3ed8a - view on LGTM.com new alerts:
|
ghost
force-pushed
the
5946-zap-rewrite
branch
from
cf933b1
to
cf67970
Compare
|
This pull request introduces 3 alerts when merging cf67970 into 3288be8 - view on LGTM.com new alerts:
|
ghost
force-pushed
the
5946-zap-rewrite
branch
from
cf67970
to
5b032cd
Compare
|
This pull request introduces 2 alerts when merging 5b032cd into 3288be8 - view on LGTM.com new alerts:
|
ghost
force-pushed
the
5946-zap-rewrite
branch
4 times, most recently
from
d789ec4
to
ee7f214
Compare
ghost
changed the title
ghost
marked this pull request as ready for review
|
@L3th3 Just checking in, is this formally ready for review now? (The description still says WIP.) If so, it looks like the |
ghost
force-pushed
the
5946-zap-rewrite
branch
3 times, most recently
from
c477cbb
to
312ef77
Compare
ghost
force-pushed
the
5946-zap-rewrite
branch
from
312ef77
to
60a89c4
Compare
ghost
force-pushed
the
5946-zap-rewrite
branch
from
60a89c4
to
c8ab312
Compare
|
Yes, it's ready for review. The bandit check is fixed |
There was a problem hiding this comment.
Thanks for this, @L3th3! I can confirm:
- Verify that zap scans run in CircleCI and produce the expected HTML reports as artifacts
I've read through the diff and left some comments inline, with the goal of integrating this script and CI job as much as possible into our current conventions. Let me know what you think.
And a question for follow-up work once this is merged: In the scan report (e.g.) https://output.circle-artifacts.com/output/job/121fb14c-4b31-42b5-acc0-cda447097bb2/artifacts/0/~/project/src_report.html, I note a number of alerts that are by-products of how the applications are served in the development environment. Could these alerts be silenced, so that a maintainer can tell at a glance whether a pull request introduce new alerts at the application-code rather than the make dev level?
| urllib3<1.25,>=1.21.1 | ||
| zapcli | ||
| pyotp | ||
| selenium |
There was a problem hiding this comment.
How would you feel about moving this to something like securedrop/requirements/python3/scan-requirements.txt (with or without a pre-processed scan-requirements.in)?
scans/zapscan.py
Outdated
| @@ -0,0 +1,188 @@ | |||
| from time import sleep | |||
There was a problem hiding this comment.
How would you feel about moving this script to either of the devops or securedrop/bin directories?
There was a problem hiding this comment.
Thanks for updating this, @L3th3! A couple questions from my previous review are still outstanding, and it looks like you'll need to rebase from develop to resolve conflicts with .circleci/config.yml. Let me know if you'd like to pair on any of this.
| .PHONY: dev-detatched | ||
| dev-detatched: ## Run the development server in a Docker container without attatching tty. |
There was a problem hiding this comment.
Typos:
| .PHONY: dev-detatched | |
| dev-detatched: ## Run the development server in a Docker container without attatching tty. | |
| .PHONY: dev-detached | |
| dev-detached: ## Run the development server in a Docker container without attaching tty. |
Status
Ready for review
Description of Changes
Changes proposed in this pull request:
Adds zap proxy active scanning for journalist and source interfaces, both authenticated and unauthenticated.
Testing
How should the reviewer test this PR?
Verify that zap scans run in CircleCI and produce the expected HTML reports as artifacts
Deployment
Should not disrupt deployment
Checklist
Choose one of the following: