PYIC-6710: remove common secrets from .scerets.baseline #2034

thebauSoftwire · 2024-06-19T10:54:16Z

Proposed changes

What changed

added regex filters for obvious test secrets
use inline exclusion as default for secrets
moving shared test secrets to TestFixtures

Why did it change

The .secrets.baseline file was being updated with new secrets every time a "secret" was detected. Instead, we should use rules to avoid false positives being added to the baseline. This means we'll have less conflicts as the only changes made to the baseline should be if the filters are updated.

Issue tracking

PYIC-6710

thebauSoftwire · 2024-06-19T11:03:53Z

.secrets.baseline

+        "^.*{[a-zA-Z0-9-_\":,]+OXt0P05ZsQcK7eYusgIPsqZdaBCIJiW4imwUtnaAthU[a-zA-Z0-9-_\":,]+}$",
+        "^.*[a-zA-Z0-9._]+KClzxkHU35ck5Wck7jECzt0_TAkiy4iXRrUg_aftDg2uUpLOC0Bnb-77lyTlhSTuotEQbqB1YZqV3X_SotEQbg",
+        "^.*{[a-zA-Z0-9-_\":,]+qoXdkYVomy6HWT6yNLqjHSmYoICs6ioUF565Btx0apw[a-zA-Z0-9-_\":,]+}$",
+        "^.*{[a-zA-Z0-9-_\":,]+wfSHxlvJlJcdS20r5PKKmqdPeW3Y4ir3WsVVeiht2iOZUreUO5O3V3o7ImvEjPS[a-zA-Z0-9-_\":,]+}$",


There's a lot of values that look like this that I wasn't sure how to capture in a single regex:

{"kty":"EC","d":"OXt0P05ZsQcK7eYusgIPsqZdaBCIJiW4imwUtnaAthU","crv":"P-256","x":"E9ZzuO..., "y": "safsW....}

I tried something like

{(?:\"[a-zA-z]+\":\"[a-zA-Z0-9_]+\",)+(?:\"[a-zA-z]+\":\"[a-zA-Z0-9-_]+\")}

which got rid of a lot of secrets but I think might be too generic 😨
Another option could be specifying a combination of keys

{(?:\"(kty|x|y|d)\":\"[a-zA-Z0-9_]+\",)+(?:\"(kty|x|y|d)\":\"[a-zA-Z0-9-_]+\")}

but I think it might still let a real secret through if it had a similar structure.

So I've opted for finding common key values instead that have been re-used across the codebase. This might be less likely to catch a new false positive but I think would be more likely to prevent a real secret from being added.
I'd like to get more thoughts/ideas on this though!

We definitely don't want to exclude anything in that format - since that'll include all keys (including prod ones!)

Can we just exclude the entirety of the test key strings? I think this is basically what you've done, but including the entire JSON object.

thebauSoftwire · 2024-06-19T11:15:15Z

...ov/di/ipv/core/library/verifiablecredential/validator/VerifiableCredentialValidatorTest.java

        TEST_SIGNING_KEY = ECKey.parse(EC_PUBLIC_JWK);
        TEST_SIGNING_KEY2 =
                ECKey.parse(
-                        "{\"crv\":\"P-256\",\"d\":\"o1orSH_mS3u1zzi4wXa9C-cgY2bPyZWN5DxK78JCN6E\",\"kty\":\"EC\",\"x\":\"LziA3lV476BwPG5glvLLx8-FzMbeX2ti9wYlhwCWNhQ\",\"y\":\"NfvgSlu1TMNjjMRM3um29Tv79C4NL8x6WEY7t4BBneA\"}");
+                        "{\"crv\":\"P-256\",\"d\":\"o1orSH_mS3u1zzi4wXa9C-cgY2bPyZWN5DxK78JCN6E\",\"kty\":\"EC\",\"x\":\"LziA3lV476BwPG5glvLLx8-FzMbeX2ti9wYlhwCWNhQ\",\"y\":\"NfvgSlu1TMNjjMRM3um29Tv79C4NL8x6WEY7t4BBneA\"}"); // pragma: allowlist secret


There's a lot of common test values e.g. TEST_SIGNING_KEY or FAILED_F2F_VC_PASSPORT_SIGNATURE with unique values which spans more than one line because of the length of the string. However, I think detect-secrets processes files line by line so we can't use something like the exclude_line filter with a regex to look at the value of the line before it:

(?im)(?<=.test_signing_key =\n).*$

So I think it might be the case that we'll have to use an inline exclusion for these as they typically have unique values.

So is the 'secret' matching against TEST_SIGNING_KEY2 rather than the key value itself? That's annoying :(

If they're unique, then I think an allowlist comment is probably better anyway.

It's matching against the value itself which is typically unique. We can't match it using the name of the variable it's assigned to unless the whole thing is on one line in which case we can use something like the exclude-line filter to match against the name of the variable it's assigned to

I think we explicitly don't want to match against the variable name - there's nothing guaranteeing that a variable named 'test_key' is not actually a genuine secret!

If it's unique then I think the allowlist comment is the best approach, then someone has to explicitly decide that it's safe each time.

thebauSoftwire · 2024-06-19T13:26:48Z

.secrets.baseline

+    {
+      "path": "detect_secrets.filters.regex.should_exclude_file",
+      "pattern": [
+        ".*/TestFixtures.java$"


Excluding this file since all of these are used for testing but wondering if I should use inline exclusions here instead for added safety?

Good question. Excluding by file feels a little risky - I could certainly see a scenario where a dev replaces a test VC with a prod one and uses a unit test as a way of running the decryption.

(but if it's really painful then this might be risk we want to accept)

Joe-Edwards-GDS · 2024-06-19T14:42:21Z

.secrets.baseline

+    {
+      "path": "detect_secrets.filters.regex.should_exclude_file",
+      "pattern": [
+        ".*/TestFixtures.java$"


Good question. Excluding by file feels a little risky - I could certainly see a scenario where a dev replaces a test VC with a prod one and uses a unit test as a way of running the decryption.

(but if it's really painful then this might be risk we want to accept)

Joe-Edwards-GDS · 2024-06-19T14:45:49Z

.secrets.baseline

+        "^.*{[a-zA-Z0-9-_\":,]+OXt0P05ZsQcK7eYusgIPsqZdaBCIJiW4imwUtnaAthU[a-zA-Z0-9-_\":,]+}$",
+        "^.*[a-zA-Z0-9._]+KClzxkHU35ck5Wck7jECzt0_TAkiy4iXRrUg_aftDg2uUpLOC0Bnb-77lyTlhSTuotEQbqB1YZqV3X_SotEQbg",
+        "^.*{[a-zA-Z0-9-_\":,]+qoXdkYVomy6HWT6yNLqjHSmYoICs6ioUF565Btx0apw[a-zA-Z0-9-_\":,]+}$",
+        "^.*{[a-zA-Z0-9-_\":,]+wfSHxlvJlJcdS20r5PKKmqdPeW3Y4ir3WsVVeiht2iOZUreUO5O3V3o7ImvEjPS[a-zA-Z0-9-_\":,]+}$",


We definitely don't want to exclude anything in that format - since that'll include all keys (including prod ones!)

Can we just exclude the entirety of the test key strings? I think this is basically what you've done, but including the entire JSON object.

Joe-Edwards-GDS · 2024-06-19T14:47:31Z

.secrets.baseline

+        "x-api-key",
+        "ScnF4dGXthZYXS_5k85ObEoSU04W-H3qa_p6npv2ZUY",
+        "^.*eyJpc3MiOiJpcHYtY29yZSIsInN1YiI6Imlwdi1jb3JlIiwiYXVkIjoiZHVtbX.*$",
+        "^.*&code=dummyAuthCode.*$"


I assume there's no way of putting comments against these? It'd be nice to know what these are, and what the justification for excluding them is...

Sadly I don't think you can put comments in this file :( My main motivation for putting a rule down for a secret is that it exists in multiple places either as a whole e.g. "ScnF4dGXthZYXS_5k85ObEoSU04W-H3qa_p6npv2ZUY" or a variation of that secret e.g. "^.*eyJpc3MiOiJpcHYtY29yZSIsInN1YiI6Imlwdi1jb3JlIiwiYXVkIjoiZHVtbX.*$"

Joe-Edwards-GDS · 2024-06-19T14:48:22Z

...ov/di/ipv/core/library/verifiablecredential/validator/VerifiableCredentialValidatorTest.java

        TEST_SIGNING_KEY = ECKey.parse(EC_PUBLIC_JWK);
        TEST_SIGNING_KEY2 =
                ECKey.parse(
-                        "{\"crv\":\"P-256\",\"d\":\"o1orSH_mS3u1zzi4wXa9C-cgY2bPyZWN5DxK78JCN6E\",\"kty\":\"EC\",\"x\":\"LziA3lV476BwPG5glvLLx8-FzMbeX2ti9wYlhwCWNhQ\",\"y\":\"NfvgSlu1TMNjjMRM3um29Tv79C4NL8x6WEY7t4BBneA\"}");
+                        "{\"crv\":\"P-256\",\"d\":\"o1orSH_mS3u1zzi4wXa9C-cgY2bPyZWN5DxK78JCN6E\",\"kty\":\"EC\",\"x\":\"LziA3lV476BwPG5glvLLx8-FzMbeX2ti9wYlhwCWNhQ\",\"y\":\"NfvgSlu1TMNjjMRM3um29Tv79C4NL8x6WEY7t4BBneA\"}"); // pragma: allowlist secret


So is the 'secret' matching against TEST_SIGNING_KEY2 rather than the key value itself? That's annoying :(

If they're unique, then I think an allowlist comment is probably better anyway.

Joe-Edwards-GDS · 2024-06-19T16:08:06Z

.secrets.baseline

+        "hXYrKJ_W9YItUbZxu3T63gQgScVoSMqHZ43UPfdB8im8L4d0mZPLC6BlwMJSsfjiAyU1y3c37vm-rV8kZo2uyw",
+        "EFfq4iMeJ9ekCYJDZS8MTqxK0semEH7HRMac9Tc69zILtxzlVmJxnrhsVSgjpMNi3osCBUhWlz3Zh-jEUB4izw",
+        "^.*eyJzdWIiOiIxMjM0IiwiYXVkIjoiYWRtaW4iLCJpc3MiOiJtYXNvbi5tZXRhbXVnLm5ldCIsImV4cCI6MTU3NDUxMjc2NSwiaWF0IjoxNTY.*",
+        "x-api-key",


Can I confirm, this is excluding values that are exactly x-api-key - not just secrets that contain it?

(We don't want to exclude, e.g., x-api-key: some-very-secret-value)

…my siganture

…LIC_JWK

… contract tests and exclude from baseline

This reverts commit 6989639.

…paste to other tools

# Conflicts: # .secrets.baseline # lambdas/process-cri-callback/src/test/java/uk/gov/di/ipv/core/processcricallback/pact/addressCri/ContractTest.java # lambdas/process-cri-callback/src/test/java/uk/gov/di/ipv/core/processcricallback/pact/bavCri/ContractTest.java # lambdas/process-cri-callback/src/test/java/uk/gov/di/ipv/core/processcricallback/pact/cicCri/ContractTest.java # lambdas/process-cri-callback/src/test/java/uk/gov/di/ipv/core/processcricallback/pact/dcmawCri/ContractTest.java # lambdas/process-cri-callback/src/test/java/uk/gov/di/ipv/core/processcricallback/pact/drivingLicenceCri/CredentialTests.java # lambdas/process-cri-callback/src/test/java/uk/gov/di/ipv/core/processcricallback/pact/experianKbvCri/ContractTest.java # lambdas/process-cri-callback/src/test/java/uk/gov/di/ipv/core/processcricallback/pact/fraudCri/CredentialTests.java # lambdas/process-cri-callback/src/test/java/uk/gov/di/ipv/core/processcricallback/pact/hmrcKbvCri/ContractTest.java # lambdas/process-cri-callback/src/test/java/uk/gov/di/ipv/core/processcricallback/pact/ninoCri/ContractTest.java # lambdas/process-cri-callback/src/test/java/uk/gov/di/ipv/core/processcricallback/pact/passportCri/CredentialTests.java # libs/verifiable-credentials/src/test/java/uk/gov/di/ipv/core/library/verifiablecredential/validator/VerifiableCredentialValidatorTest.java

sonarcloud · 2024-06-24T10:22:50Z

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

Joe-Edwards-GDS

Thanks!

thebauSoftwire commented Jun 19, 2024

View reviewed changes

Joe-Edwards-GDS reviewed Jun 19, 2024

View reviewed changes

thebauSoftwire changed the title ~~PYIC-6710: remove secrets from .scerets.baseline~~ PYIC-6710: remove common secrets from .scerets.baseline Jun 20, 2024

thebauSoftwire marked this pull request as ready for review June 20, 2024 09:26

thebauSoftwire requested review from a team as code owners June 20, 2024 09:26

thebauSoftwire marked this pull request as draft June 20, 2024 09:27

thebauSoftwire force-pushed the PYIC-6710 branch from ff96559 to 5604460 Compare June 20, 2024 09:43

thebauSoftwire requested review from Joe-Edwards-GDS and DanCorderIPV June 20, 2024 13:40

thebauSoftwire added 17 commits June 20, 2024 14:43

PYIC-6710: remove test fxitures from baseline

221ab3e

PYIC-6710: exclude test-secret and dummyapikey + exclude specific dum…

5857068

…my siganture

PYIC-6710: exclude shared signing key in contract tests

0b64c88

PYIC-6710: exclude shared access token

478915b

PYIC-6710: exclude shared VALID_ADDRESS_VC_SIGNATURE

d8fe92e

PYIC-6710: exclude shared signing_key + shared CRI_RSA_ENCRYPTION_PUB…

9892d0a

…LIC_JWK

PYIC-none: exclude x-api-key

e315987

PYIC-none: exclude shared token

e87869c

PYIC-6710: exclude common client_assertion_body string

0c48b70

PYIC-6710: exclude dummy body

0632387

PYIC-6710: exclude common signing key

0f6bc1e

PYIC-6710: remove unnecessary inline exclusion

cd85fe6

PYIC-6710: use inline exclusion for TestFixtures

b088271

PYIC-6710: use common RSA_ENCRYPTION_PUBLIC_JWK from testFixtures for…

e7e95dd

… contract tests and exclude from baseline

PYIC-6710: anchoring absolute secrets

11727b7

PYIC-6710: update baseline after rebase to main + spotlessApply

37b1547

PYIC-6710: reduce regex filters for exclude_lines filter

4dc4eb5

thebauSoftwire added 7 commits June 20, 2024 14:53

PYIC-6710: re-use EC_PRIVATE_KEY_JWK in contract tests

5707591

PYIC-6710: commonise CLIENT_ASSERTION_SIGNATURE in TestFixtures

aa0ac34

Revert "PYIC-6710: commonise CLIENT_ASSERTION_SIGNATURE in TestFixtures"

8c567ed

This reverts commit 6989639.

PYIC-6710: commonise an example genereted secure token

83b16f8

PYIC-6710: replace regex in filters with inline exclusions

0664a5d

PYIC-6710: rename public jwk to make clear it is for testing

4894831

PYIC-6710: re-scan detect-secrets

1859d12

thebauSoftwire force-pushed the PYIC-6710 branch from 62e1964 to 1859d12 Compare June 20, 2024 13:55

PYIC-6710: running pre-commit detect secrets hook locally

a912817

thebauSoftwire marked this pull request as ready for review June 21, 2024 08:07

DanCorderIPV added 3 commits June 24, 2024 10:39

PYIC-6710: Exclude contract test signatures from secrets

20e9cc3

PYIC-6710: Add unescaped version of EC_PRIVATE_KEY_JWK for easy copy/…

d7c664f

…paste to other tools

DanCorderIPV approved these changes Jun 24, 2024

View reviewed changes

Joe-Edwards-GDS approved these changes Jun 24, 2024

View reviewed changes

DanCorderIPV merged commit 84697b3 into main Jun 24, 2024
6 checks passed

DanCorderIPV deleted the PYIC-6710 branch June 24, 2024 11:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

PYIC-6710: remove common secrets from .scerets.baseline #2034

PYIC-6710: remove common secrets from .scerets.baseline #2034

thebauSoftwire commented Jun 19, 2024 •

edited

Loading

thebauSoftwire Jun 19, 2024 •

edited

Loading

Joe-Edwards-GDS Jun 19, 2024

thebauSoftwire Jun 19, 2024 •

edited

Loading

Joe-Edwards-GDS Jun 19, 2024

thebauSoftwire Jun 19, 2024

Joe-Edwards-GDS Jun 19, 2024

thebauSoftwire Jun 19, 2024

Joe-Edwards-GDS Jun 19, 2024

Joe-Edwards-GDS Jun 19, 2024

Joe-Edwards-GDS Jun 19, 2024

Joe-Edwards-GDS Jun 19, 2024

thebauSoftwire Jun 19, 2024 •

edited

Loading

Joe-Edwards-GDS Jun 19, 2024

Joe-Edwards-GDS Jun 19, 2024

sonarcloud bot commented Jun 24, 2024 •

edited

Loading

Joe-Edwards-GDS left a comment

PYIC-6710: remove common secrets from .scerets.baseline #2034

PYIC-6710: remove common secrets from .scerets.baseline #2034

Conversation

thebauSoftwire commented Jun 19, 2024 • edited Loading

Proposed changes

What changed

Why did it change

Issue tracking

thebauSoftwire Jun 19, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thebauSoftwire Jun 19, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thebauSoftwire Jun 19, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sonarcloud bot commented Jun 24, 2024 • edited Loading

Quality Gate passed

Joe-Edwards-GDS left a comment

Choose a reason for hiding this comment

thebauSoftwire commented Jun 19, 2024 •

edited

Loading

thebauSoftwire Jun 19, 2024 •

edited

Loading

thebauSoftwire Jun 19, 2024 •

edited

Loading

thebauSoftwire Jun 19, 2024 •

edited

Loading

sonarcloud bot commented Jun 24, 2024 •

edited

Loading