Fix is_verified not stored in PotentialSecret

[abdelrahman-thafeer]

Problem:

• is_verified flag does not get stored in PotentialSecret which results in its value being always false.
• This problem didn't affect --only-verified flag because verification was running but not persisted.

Solution:

• When a secret is found, run verify method and store the result within PotentialSecret.
• pass down CodeSnippet to analyze_line as it is required to verify secrets in some plugins.
• verify will not run in case --no-verify flag was passed.

[lorenzodb1]

I re-ran the tests as a couple were running for over an hour. Hopefully they'll all pass this time.

[jpdakran]

I think we need to mock out the verify for the detect_secrets.plugins.aws.verify_aws_secret_access_key in main_test.py::TestSlimScan::test_basic

[abdelrahman-thafeer]

It looks like this is an old issue with macOS that occurs when calling requests API within a forked process. It didn't happen before because the secret was being filtered by gibberish before it reaches to the verification filter.

This currently impacts few tests (main_test.py::TestSlimScan::test_basic and any test in audit/audit_test.py that uses test_baseline fixture). I've added an environment variable NO_PROXY as a workaround.

[jpdakran]

Is this only a testing issue or do we need to somehow enable this in a production mode as well?

[abdelrahman-thafeer]

The same issue will be in production as well if the detect-secrets is ran with python 3.6/3.7 in macOS. I didn't want to add NO_PROXY in production code as it would lead to ignoring proxy configurations in the environment and the issue doesn't impact most environment. that being said detect-secrets can still be run in impacted environments by either setting NO_PROXY env variable prior to running it or adding the --no-verify flag.

I think the issue existed prior to this PR it's just that non of the tests invoked Requests APIs from a forked process.

[jpdakran]

Can we verify that this was existing in some sort of integration test / brute force testing? I would just like to ensure we are not introducing any new issues to the repository.

[abdelrahman-thafeer]

I've created a draft PR with test data that should invoke Requests API from forked process, the same error occurred

[lorenzodb1]

If this is caused by requests, could it be the case that a newer version doesn't have this issue anymore? I see we're using 2.26.0, while the latest one is 2.28.1.

[abdelrahman-thafeer]

Seems like requests v2.28.0 dropped support for python 3.6, I tried requests v2.27.1 with no luck.

It's worth noting that the issue can be fixed if we use spawn context for multiprocessing but I think that would come at a performance cost.

[jpdakran]

Right. So python 3.8+ multiprocessing library has switched to spawn where before it was fork. This is an issue since spawn does not share any memory between the parent and child processes. So we default the multiprocessing to fork when creating a pool of processes

[jpdakran]

Can we try and use v2.28.1. Python 3.6 is end of life. We can consider dropping support for it.

[abdelrahman-thafeer]

I tried v2.28.1, still the same problem. I think this is more of a python issue than requests, the default context was changed to spawn because of crashes similar to this.

Since we are already using default multiprocessing (spawn) for python 3.8+ on macOS, what do you think of checking the current OS and if it's macOS we change to spawn?

[jpdakran]

Sorry, that was the initial attempt to fix it. We decided to try and stay away from being OS specific so instead of changing the default multiprocessing, we instead passed the shared settings to child processes from the parent. So in python 3.8+ we are using spawn which is the default.