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

Update AzureSearchAdminKey detector #3634

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

rgmz
Copy link
Contributor

@rgmz rgmz commented Nov 20, 2024

Description:

There are a number of problems with this detector. This Pr attempts to fix some of the glaring issues.

e.g.,

Verification issue: unexpected HTTP response status 400 | {"error":{"code":"","message":"Service name must only contain lowercase letters, digits or dashes, cannot start or end with or contain consecutive dashes and is limited to 60 characters."}}

Checklist:

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

@rgmz rgmz requested a review from a team as a code owner November 20, 2024 02:31
@rgmz rgmz force-pushed the feat/azure-search branch from 8d56655 to 1dd64ad Compare November 20, 2024 03:52
Copy link
Collaborator

@ahrav ahrav left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Single blocking change as it pertains to reformatting RawV2, otherwise LGTM!

Raw: []byte(resMatch),
RawV2: []byte(resMatch + resServiceMatch),
Raw: []byte(key),
RawV2: []byte(`{"service":"` + service + `","key":"` + key + `"}`),
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changing the RawV2 value will break some enterprise customers, as we currently have secrets verified using the old key+service format. This change would orphan previously verified credentials and create duplicates. Is there a reason we can't continue supporting the old format?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a reason we can't continue supporting the old format?

Obviously you get the final say here, but #2128 has been a PITA for me and others using OSS.

TL;DR Most of the existing RawV2 values are suboptional because they smash together text (sometimes without a delimiter) and are devoid of context/keywords. This makes it impossible to programmatically re-construct and re-verify results.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My guess is that when RawV2 was proposed, we didn't account for its visibility in OSS reporting. Wonder if we can find a solution that preserves readability for OSS while avoiding duplicates on the enterprise side?

This likely requires a broader discussion, and I think @zricethezav will have the most input, but I agree that combining multiple credentials makes them difficult to interpret and largely ineffective

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My guess is that when RawV2 was proposed, we didn't account for its visibility in OSS reporting. Wonder if we can find a solution that preserves readability for OSS while avoiding duplicates on the enterprise side?

I can't speak to the impact on enterprise. I can revert the change to RawV2, though I'd prefer to improve it (eventually).

@rgmz rgmz force-pushed the feat/azure-search branch from 1dd64ad to 91e6661 Compare November 20, 2024 21:50
@rgmz

This comment was marked as outdated.

@rgmz rgmz force-pushed the feat/azure-search branch from 91e6661 to 549eed7 Compare December 2, 2024 14:10
@rgmz rgmz force-pushed the feat/azure-search branch from 549eed7 to c3e1d7a Compare December 15, 2024 15:27
@rgmz rgmz mentioned this pull request Dec 19, 2024
2 tasks
@rgmz rgmz force-pushed the feat/azure-search branch from c3e1d7a to fda5209 Compare December 21, 2024 16:00
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