Update Azure secretPatFmt

[justanothernate]

We had a secret leak through and looks like the existing detector doesn't match Azure docs:

https://learn.microsoft.com/en-us/purview/sit-defn-client-secret-api-key

This changed regex should match what is found in the above docs:

A combination of 24 characters consisting of letters, digits, and special characters.

or

A combination of 32 characters consisting of letters and digits.

or

A combination of 40 characters consisting of letters and digits.

or

A combination of 44 characters consisting of letters, digits, and special characters.

or

A combination of 56 characters consisting of letters, digits, and special characters

or

A combination of 88 characters consisting of letters, digits, and special characters.

Description:

Explain the purpose of the PR.

Checklist:

• Tests passing (make test-community)?
• Lint passing (make lint this requires golangci-lint)?

[CLAassistant]

All committers have signed the CLA.

[rgmz]

We had a secret leak through and looks like the existing detector doesn't match Azure docs:

https://learn.microsoft.com/en-us/purview/sit-defn-client-secret-api-key

This appears to be a generic reference for any client secret. Entra/Azure AD service principal secret guidelines are here.

Out of curiosity, how old was the secret in question? If it was generated within the last few years, it should match this pattern (...Q~...).

trufflehog/pkg/detectors/azure_entra/serviceprincipal/v2/spv2.go

Line 34 in 4a99e74

SecretPat = regexp.MustCompile(`(?:[^a-zA-Z0-9_~.-]|\A)([a-zA-Z0-9_~.-]{3}\dQ~[a-zA-Z0-9_~.-]{31,34})(?:[^a-zA-Z0-9_~.-]|\z)`)

[justanothernate]

Got it, thanks for the link.

The secret we had that slipped through was new but it has 40 chars so didn't match the existing filter which was set for 34 chars. Based on the linked doc you added, we could just leave the existing regex pattern as-is but change it to match a length of 40 chars. We could also do 34 - 40 chars long if the original value of 34 was something that was used previously and you want to retain matches of that or any intermediary length as well.

[rosecodym]

@abmussani

[abmussani]

@justanothernate If old secrets is still working (not revoked) and new format has been introduced, Then as per the guideline, a newer version of Detector should be implemented. You can check for github V1 and V2 or Twitter V1 and V2 as an example.

[rgmz]

If old secrets is still working (not revoked) and new format has been introduced

FWIW, the current v1 pattern doesn't match a large chunk of old secrets. @justanothernate highlights an important issue, although I don't know if the proposed changes are correct.

[abmussani]

@rgmz definitely, I do agree. but Right now, I believe we are not sure that the old formatted secrets are still acceptable by azure or not. Versioning is good way to support both formats if we are not 100% confirmed.

Typically services keep allows to support old ones. @justanothernate Do we have any reference about the acceptance of old format of secrets?

[rgmz]

but Right now, I believe we are not sure that the old formatted secrets are still acceptable by azure or not.

I can confirm that they are.

[justanothernate]

Not sure on that, this particular Azure account as a whole is only 6 months old and the secret was generated last week so I don't have much exposure to any old secrets but it didn't seem to match the newly generated, 40 character long one.

[ankushgoel27]

your suggested regex still doesnt match 40 chars containing special chars including a period .

[justanothernate]

The linked Microsoft doc about secrets says the 40 character secretes are only letters and digits so it's not supposed to match on special chars.

Any other changes that would need to be made to merge this in?

[ankushgoel27]

The linked Microsoft doc about secrets says the 40 character secretes are only letters and digits so it's not supposed to match on special chars.

Any other changes that would need to be made to merge this in?

i don't know about the doc but have you tried running/testing your changes against actual/live secrets. the actual secrets generated by azure contain special chars.

[justanothernate]

Ahhh, I see, the proposed change wouldn't match the special chars in the longer tokens.

That doc says there shouldn't be any 40 character keys with special characters but that they would be in the 44, 56 and 88 character keys so I can add that.

As to testing, I only noticed we had a key slip past the detector. We don't use Azure heavily so I don't have much of a test set.

To accommodate the special characters, I'd propose to break it into something like this so it's a bit more readable but not sure what people would prefer.

secretPatFmt = `(?i)(%s)\s*(` +
    `[\w!@#$%^&*()_+\-=\[\]{};':"\\|,.<>/?~\x60]{24}|` +
    `[a-zA-Z0-9]{32}|` +
    `[a-zA-Z0-9]{40}|` +
    `[\w!@#$%^&*()_+\-=\[\]{};':"\\|,.<>/?~\x60]{44}|` +
    `[\w!@#$%^&*()_+\-=\[\]{};':"\\|,.<>/?~\x60]{56}|` +
    `[\w!@#$%^&*()_+\-=\[\]{};':"\\|,.<>/?~\x60]{88}` +
`)`

[ankushgoel27]

There is already a PR out there - #2985 that works well.

[ankushgoel27]

Well, I have these azure client secret keys from the live environment. one is 32 char and one is 40 char and both contains special chars

7KOCfo/BxD3iyWpgO9qL8ZMEinucw?.[

xLQ7Q~CFpA3Dusgj9wDcY6MQ3kYybrxUj.5tUbAG

[rgmz]

7KOCfo/BxD3iyWpgO9qL8ZMEinucw?.[

This is the old format. It has no predictable elements and seems to use all special characters.

xLQ7Q~CFpA3Dusgj9wDcY6MQ3kYybrxUj.5tUbAG

This is the new format. It has \dQ~ at the beginning and only uses ~_-. (I could be forgetting others)