Update diff review process to start with GuardDog and Semgrep#289
Update diff review process to start with GuardDog and Semgrep#289legoktm merged 4 commits intofreedomofpress:mainfrom
Conversation
legoktm
left a comment
legoktm
left a comment
There was a problem hiding this comment.
Thanks, this is really great. I primarily have some process questions
| alias guarddog='docker run --rm ghcr.io/datadog/guarddog' | ||
|
|
||
| .. note:: GuardDog fails quietly, and scans that did not run produce output similar to a successful scan with no findings. For this reason, you should pass ``--log-level debug`` with every invocation. | ||
|
|
There was a problem hiding this comment.
"If there are no issues, it is safe to proceed with the dependency update. If guarddog does find issues, you should ???"
Maybe: ...you should read the source code and see if it's a false positive. Ask in the security chat for a second opinion/confirmation. ?
There was a problem hiding this comment.
I had a similar question myself about how much details the docs should provide for following up/interpreting the results. I could include an example of GuardDog output with a finding.
| * Note: we trust packages managed by the `Python Packaging Authority <https://www.pypa.io/en/latest/>`_ | ||
| (PyPA) and don't diff review them. | ||
| 3. **Explain version specifiers:** Use comments in ``.in`` or ``pyproject.toml`` | ||
| - For dependencies with lower trust or otherwise requiring heightened scrutiny, use :ref:`Semgrep <scan-semgrep>` to locally scan the updated version of the dependency. Semgrep is an all-purpose static code analysis tool. |
There was a problem hiding this comment.
Hmm, I would like us to be able to provide specific guidance on what constitutes "lower trust or otherwise requiring heightened scrutiny". Did you have anything in mind on how we evaluate that?
There was a problem hiding this comment.
The best guidance I'm aware of that we presently have is the earlier section on criteria for "Adding a dependency" https://developers.securedrop.org/en/latest/dependency_updates.html#adding-a-dependency which probably needs to be updated now that we are in an npm world.
It is mostly qualitative (as any revision would be I think). I think to update that and make it clearer we would want to loop in @redshiftzero
|
I added a small edit to reflect the convention for comments in I don't think it's necessary to document the syntax, anyone adding a comment should be able to follow the convention in the file, as with the dependency files e.g. |
legoktm
left a comment
legoktm
left a comment
There was a problem hiding this comment.
Thanks, I think this is a good start and we can iterate as we gain more experience using guarddog and see what needs further clarification / what questions arise.
2 participants
Fixes freedomofpress/securedrop-dev#34
This is an initial stab at adding instructions and guidance for running GuardDog and Semgrep on updated dependencies across SecureDrop projects. I pulled from internal notes from @adaFPF when working on this.
There might be other changes to make. The goal is to deprecate or at least de-prioritize doing manual diff reviews. I left those instructions intact, because a manual diff review may still be warranted in some cases. But perhaps some of the steps should be modified or removed (e.g. adding an entry to the wiki).
I also did not include specific instructions/guidelines for logging results of Guarddog/semgrep scans, for example in a dependency update PR. I think this would be a good idea but I'm not sure if there is a preferred convention.
Checklist
This change accounts for: