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

Fix audit report that verifies a secret #620

Merged
merged 3 commits into from
Sep 27, 2022

Conversation

jpdakran
Copy link
Member

Problem:

  • When running an audit report on a baseline file - we reverse engineer the detect-secrets starting from secrets hashes, filenames, and how it was detected to the actual secret itself.
  • After a recent change - we fixed a bug with respect to detect-secrets not persisting the verify result
  • As part of this - we were required to pass context in to the process where we analyze a line of code
  • Since we are reverse engineering - we must also update the audit to include the context as part of the analyze_line

Solution:

  • Add context (code_snippet) when we analyze a line in the audit process.

@jpdakran jpdakran linked an issue Sep 22, 2022 that may be closed by this pull request
@jpdakran jpdakran changed the title #611: Fix audit report that verifies a secret Fix audit report that verifies a secret Sep 23, 2022
@lorenzodb1
Copy link
Member

So just to clarify, if this doesn't fix #611, what does it fix?

@jpdakran
Copy link
Member Author

@lorenzodb1 This was uncovered while I was investigating #611. When I was investigating #611 I had some AWS secrets in my baseline. These secrets have a verify on them (We can reach out to an external to verify if they are truly secrets). After a recent change in the scan functionality for a regular scan - we pass in our context to lines we analyze so we can verify them.

Since we are reverse engineering a scan - we should pass the context in to analyze lines for an audit as well. This was not a known issue because context is a parameter for the analyze function with None default. So it worked fine until we hit the verify which occasionally requires a context (depending on the dependency injection).

So when it tried to audit a AWS secret - it was receiving a None context - causing an error.

@abdelrahman-thafeer
Copy link
Member

@lorenzodb1 This was uncovered while I was investigating #611. When I was investigating #611 I had some AWS secrets in my baseline. These secrets have a verify on them (We can reach out to an external to verify if they are truly secrets). After a recent change in the scan functionality for a regular scan - we pass in our context to lines we analyze so we can verify them.

Since we are reverse engineering a scan - we should pass the context in to analyze lines for an audit as well. This was not a known issue because context is a parameter for the analyze function with None default. So it worked fine until we hit the verify which occasionally requires a context (depending on the dependency injection).

So when it tried to audit a AWS secret - it was receiving a None context - causing an error.

Good catch there! I'm wondering if we should add or modify a test case to better detect similar issues in the future?

@lorenzodb1 lorenzodb1 merged commit b32a53f into master Sep 27, 2022
@lorenzodb1 lorenzodb1 deleted the dakranj-611-fix-audit-report-failing branch September 27, 2022 16:12
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.

None yet

3 participants