Test for hardcoded credentials, CWE798 #1167
Conversation
Tests all the strings for exposed secrets. The implementation was additionally tested on noise, to ensure that in 100MB of generated noise there are no false positives.
knyazer
requested review from
ericwb,
lukehinds and
sigmavirus24
as code owners
There was a problem hiding this comment.
I'd rather see a different implementation. There is already an open source project named detect-secrets that does a good job of finding secrets in source. Rather than re-implement, I'd like to see a nice integration such that detect-secrets could be used with Bandit to get a consistent report of the issues.
https://github.com/Yelp/detect-secrets
My main concern about doing it this way is the performance impact. Using "str" as the AST node to analyze is costly as most source code has many string literals. Running regex on each string literal is likely to slow Bandit to a crawl in some cases. We do have four examples of a plugins using str AST node. I'd like to see those changed as well.
Finally, it could be said we already have an existing example of checking for secrets in the general_hardcoded_password.py plugin. But I'd also like to see that one removed in exchange for an integration with detect-secrets.
| @@ -31,17 +31,17 @@ jobs: | |||
| ref: ${{ github.event_name == 'release' && github.ref || env.RELEASE_TAG }} | |||
|
|
|||
| - name: Set up Docker Buildx | |||
| uses: docker/setup-buildx-action@988b5a0280414f521da01fcc63a27aeeb4b104db # v3 | |||
| uses: docker/setup-buildx-action@d70bba72b1f3fd22344832f00baa16ece964efeb # v3 | |||
There was a problem hiding this comment.
Undo, updates to these versions are managed by dependabot.
There was a problem hiding this comment.
I'm not sure what, but something was updating these versions forcefully on my repo... Maybe I have some conflicting dependabot config, not sure. Will fix it.
|
|
||
| - name: Log in to GitHub Container Registry | ||
| uses: docker/login-action@9780b0c442fbb1117ed29e0efdff1e18412f7567 # v3 | ||
| uses: docker/login-action@0d4c9c5ea7693da7b068278f7b52bda2a190a446 # v3 |
There was a problem hiding this comment.
Undo, updates to these versions are managed by dependabot.
| with: | ||
| registry: ghcr.io | ||
| username: ${{ github.actor }} | ||
| password: ${{ secrets.GITHUB_TOKEN }} | ||
|
|
||
| - name: Install Cosign | ||
| uses: sigstore/cosign-installer@4959ce089c160fddf62f7b42464195ba1a56d382 # v3.6.0 | ||
| uses: sigstore/cosign-installer@59acb6260d9c0ba8f4a2f9d9b48431a222b68e20 # v3.5.0 |
There was a problem hiding this comment.
Undo, updates to these versions are managed by dependabot.
| @@ -51,7 +51,7 @@ jobs: | |||
|
|
|||
| - name: Build and push Docker image | |||
| id: build-and-push | |||
| uses: docker/build-push-action@16ebe778df0e7752d2cfcbd924afdbbd89c1a755 # v6 | |||
| uses: docker/build-push-action@15560696de535e4014efeff63c48f16952e52dd1 # v6 | |||
There was a problem hiding this comment.
Undo, updates to these versions are managed by dependabot.
| @@ -155,6 +155,9 @@ bandit.plugins = | |||
| # bandit/plugins/trojansource.py | |||
| trojansource = bandit.plugins.trojansource:trojansource | |||
|
|
|||
| # bandit/plugins/exposed_secrets.py | |||
| exposed_secrets = bandit.plugins.exposed_secrets:exposed_secrets | |||
There was a problem hiding this comment.
Inconsist indenting.
| exposed_secrets = bandit.plugins.exposed_secrets:exposed_secrets | |
| exposed_secrets = bandit.plugins.exposed_secrets:exposed_secrets |
|
|
||
|
|
||
| @test.checks("Str") | ||
| @test.test_id("B510") |
There was a problem hiding this comment.
B5xx is categorized as cryptography plugins. This plugin better fits in the misc group.
https://bandit.readthedocs.io/en/latest/plugins/index.html#plugin-id-groupings
| return res | ||
|
|
||
|
|
||
| @test.checks("Str") |
There was a problem hiding this comment.
A plugin that checks on every instance of an AST str does impact performance quite a bit. I realize we already have another plugin doing the same, but we should strive to avoid this as the speed of Bandit decreases signifiantly.
|
|
||
| @test.checks("Str") | ||
| @test.test_id("B510") | ||
| def exposed_secrets(context): |
There was a problem hiding this comment.
Use the decorator @test.takes_config to signify that this plugin takes config, rather than implementing a new way to configure via the secrets.toml.
| @@ -0,0 +1,1101 @@ | |||
| # MIT License | |||
There was a problem hiding this comment.
If a plugin has some configuration, like this file, it should be part of our standard plugin configuration mechanism by defining a bandit.yaml. You can look at other plugins like weak_cryptography_key.py as an example.
|
Thanks, @ericwb! I didn't really spend a ton of time on writing the secret-detecting part overall, I just took the one from gitleaks, and modified it heavily (and mostly automatically) to be easier. But still sad that a few hours of my work were useless :( Another problem, is that when I was looking through the existing plugins, I was not able to find any that do regex search over the complete files. So, if there is one, could you direct me towards it? And if there is none, I'm happy to just implement such functionality, probably also using it in 'weak_cryptographic_key' one, to avoid the string visitor. |
|
To sum up, I will work on the integration, and I'm going to close this PR (and open a new one) as soon as I will get the integration PR ready. Thanks for the insanely fast feedback :) |
|
@knyazer then in stealing an implementation from somewhere else where you did not own the copyright you were opening this project up to a severe issue by contributing that code. |
|
@sigmavirus24, nope, I checked that the license is MIT, even included the original license in the file that I "borrowed"; And the modification is so heavy I don't think you can find more than two adjacent lines that would be unchanged. And all the "meaningful" content is modified quite a bit. No worries, I am aware how copyrights work, but that's reasonable to assume that the average developer doesn't ;) But please, if you are accusing people of something, ensure that the accusation is not wrongful. I don't really mind, but having a friendly atmosphere is always nice. |
Tests all the strings for exposed secrets.
The thing was additionally tested on noise, to ensure that in 100MB of generated noise there are no false positives.
The test file,
examples/exposed_secrets.pyshows 4 leaked keys that are correctly detected, and 4 keys that should be ignored (e.g. human-constructed example keys).Also leads to a pretty significant performance degradation of around 15% due to the use of every-string-scanning decorator. This is mostly because the decorator is slow, and seemingly unrelated to the implementation itself.
Closes #443