-
Notifications
You must be signed in to change notification settings - Fork 472
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 audit result view #205
Conversation
6c518be
to
ea965f8
Compare
I ran this branch against the If you look at
|
@@ -16,6 +11,7 @@ | |||
from ..stripe import StripeDetector # noqa: F401 | |||
from detect_secrets.core.log import log | |||
from detect_secrets.core.usage import PluginOptions | |||
from detect_secrets.plugins.common.util import get_mapping_from_secret_type_to_class_name |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: Since we use relative imports above, for within the plugins folder, so I think we can do from .util
in this case
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed as suggested
2ec25e4
to
7ea0a22
Compare
Updated baseline (well this should be the same as before) and output: |
New output: Note it has the SHA of the last commit of this PR! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
left some feedback/suggested changes but nothing major
detect_secrets/core/audit.py
Outdated
Attempts to read a given line from the input file. | ||
""" | ||
try: | ||
with codecs.open(filename, encoding='utf-8') as file: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Super nit: We normally use f
for file
, as the latter is a keyword in python (weird highlighting on GitHub)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point, fixed
all_secrets = _secret_generator(baseline) | ||
|
||
audit_results = { | ||
'results': defaultdict(lambda: deepcopy(EMPTY_PLUGIN_AUDIT_RESULT)), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
results
feels a little redundant, e.g.
...
},
"results": {
"Base64HighEntropyString": {
"config": {
"base64_limit": 4.5,
"name": "Base64HighEntropyString"
},
"results": {
"positive": [],
...
vs.
...
},
"Base64HighEntropyString": {
"config": {
"base64_limit": 4.5,
"name": "Base64HighEntropyString"
},
"results": {
"positive": [],
...
We could rename the key to plugins
, but removing it as in the latter snippet seems okay to me since we will mostly use this for plugin development.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great use of lambda
btw 🐑
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm OK with renaming it to plugins
, but I think there needs to be a key. If we can get git config info we dump it into a key called repo_info
on the top level. If the plugins were also at the top-level then it would be really annoying for clients to have to filter out the plugin results in iteration (assuming the client is a machine or jq
).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
++
detect_secrets/core/audit.py
Outdated
'results': defaultdict(lambda: deepcopy(EMPTY_PLUGIN_AUDIT_RESULT)), | ||
} | ||
|
||
secret_type_mapping = get_mapping_from_secret_type_to_class_name() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is more of a super nit, but maybe indicate what secret type is mapping to? similar to how the func is named e.g. secret_type_to_plugin_class
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good, done.
tests/core/audit_test.py
Outdated
) as _mock: | ||
yield _mock | ||
|
||
def get_audited_baseline(self, plugin_config={}, is_secret=None): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since plugin_config
is always passed, afaict, I don't think you need the default value. (There's also the python gotcha w/ dict's as a default arg.)
Also you can do e.g. get_audited_baseline(plugin_config=foo, is_secret=True)
rather than get_audited_baseline(foo,True)
as it's a little more readable what the args are from seeing the call.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense, fixed
tests/core/audit_test.py
Outdated
results = audit.determine_audit_results(baseline, '.secrets.baseline') | ||
|
||
if plugin_config: | ||
assert results['results']['HexHighEntropyString']['config'].items() \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure if I follow, what does this comparison do?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This checks that the config in the test baseline is actually returned from determine_audit_results
.
Also, I noticed the if plugin_config
check doesn't really do anything useful, so I took it out.
* Rename a variable to be clearer per review comment * Change test fixure signature * Remove unneeded `if` from test
8856d3a
to
69a109e
Compare
Supports git-defenders/detect-secrets-discuss#203 IBM Cloud IAM Api Key Validation (Yelp#201) Supports git-defenders/detect-secrets-discuss#203 Refactor IBM Cloud IAM verification for owner resolution reuse (Yelp#205) Related to git-defenders/detect-secrets-stream#222 Supports git-defenders/detect-secrets-discuss#203
Implements #191
As of writing this only shows hashed secrets. I'll update this once I get plaintext secrets and git repo info shown in the results.