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] Update Azure Client, Tenant Ids and Client Secret Detection Regexs. #3362

Closed

Conversation

abmussani
Copy link
Contributor

Description:

This PR includes the correction of few things in Azure Detector. Here are the details:

  • Support for multiple formats of Client Secret based on microsoft Documentation.
  • Tenant Id can be part of oauth token URL and the autogenerated url has microsoftonline.com as domain. Which can be used as keyword for detector (apart from 'azure').
  • Simplified the way to pass keywords in client id, tenant id & client secret expressions. For the consistency and readability, I have used PrefixRegex for this purpose.

Thanks to @justanothernate & @rgmz, for having a fruitful discussion over PR, Helped me to fixed these issue in azure detectors.

Checklist:

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

added 'microsoftonline' for azure keywords.
Support for keywords before the credentials using prefixRegex.
modified support for client Secrets based on microsoft documentation.
@abmussani abmussani requested a review from a team as a code owner October 3, 2024 12:49
@rgmz
Copy link
Contributor

rgmz commented Oct 3, 2024

The link cited in the other discussion is not correct and does not represent Azure Client Secrets AFAIK. It is a generic set of patterns Microsoft uses for detecting client secrets (not Azure-specific ones.)

Per my comment, I am only aware of two formats. This change does not reliably detect the new format.
#3039 (comment)

@abmussani
Copy link
Contributor Author

The link cited in the other discussion is not correct and does not represent Azure Client Secrets AFAIK. It is a generic set of patterns Microsoft uses for detecting client secrets (not Azure-specific ones.)

Correct. The link depict the general client secret format used by Microsoft. But as per my understanding, it will also includes the Azure one. The service I used to test is Microsoft Entra.

This change does not reliably detect the new format.

My changes successfully detect the new format of the secret. Are you referring to the old format by any chance?

@rgmz
Copy link
Contributor

rgmz commented Oct 3, 2024

Correct. The link depict the general client secret format used by Microsoft. But as per my understanding, it will also includes the Azure one. The service I used to test is Microsoft Entra.

It might be a coincidence that one of those patterns also matches the Entra ID secrets. Personally, I don't think it makes sense to introduce several broad patterns (and thus increase false matches + network traffic) when most of them don't seem to apply to Entra ID secrets.

This change does not reliably detect the new format.

My changes successfully detect the new format of the secret. Are you referring to the old format by any chance?

Emphasis on reliably. The new format has a distinct pattern which can be matched with high confidence; a large number of Entra client IDs and secrets I've found do not contain "azure" or "microsoft" anywhere in the file.

#2985 is a rather extensive update that fixes most issues with the Azure detector, including test cases.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants