-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
Added azure COSMOSDB detector #3951
base: main
Are you sure you want to change the base?
Added azure COSMOSDB detector #3951
Conversation
|
||
dbKeyPattern = regexp.MustCompile(`([A-Za-z0-9+/=]{88})`) | ||
// account name can contain only lowercase letters, numbers and the `-` character, must be between 3 and 44 characters long. | ||
accountUrlPattern = regexp.MustCompile(`(https://[a-z0-9-]{3,44}.documents\.azure\.com:[0-9]{3})`) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kashifkhan0771 Does Azure Cosmos db only supports 3 digits port number ? As per my knowledge, A port number can be between 0 to 65535, includes reserved and general use port numbers.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Apologies for that. The port is fixed at 443, and there doesn't seem to be an option to specify a custom port when creating a Cosmos NoSQL database. I will change this.
var _ detectors.Detector = (*Scanner)(nil) | ||
|
||
func (s Scanner) Type() detectorspb.DetectorType { | ||
return detectorspb.DetectorType_AzureCosmosDB |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you check how this detector is different than already CosmosDBKey one
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This one does not exist. The entry only exist in Proto file somehow.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Deprecated the old one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does it make sense to create a new entry when there's an existing (unused) one?
} | ||
|
||
if verify { | ||
verified, verificationErr := verifyCosmosDB(s.getClient(), accountUrl, key) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit**: If the host url is invalid or does not exists, then we should not be spending iteration to verify other keys on that. Richard has already implemented this in AzureContainerRegistry
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is great approach ❤️ Thanks for sharing @abmussani
return false, fmt.Errorf("failed to create request: %v", err) | ||
} | ||
|
||
dateRFC1123 := time.Now().UTC().Format("Mon, 02 Jan 2006 15:04:05 GMT") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit**: time.RFC1123 already has the similar format.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kashifkhan0771 is this the expiry time ? it is being used in creating signature.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I used the formation for timezone. x-ms-date
header require time in GMT
Docs: https://learn.microsoft.com/en-us/rest/api/cosmos-db/common-cosmosdb-rest-request-headers
var ( | ||
defaultClient = common.SaneHttpClient() | ||
|
||
dbKeyPattern = regexp.MustCompile(`([A-Za-z0-9+/=]{88})`) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you have any example of keys where it does not end without [a-zA-Z0-9]==
? I am working on another Azure service and could not generate one (ending without [a-zA-Z0-9]==
pattern)
@zricethezav @rgmz by luck, do you guyz have seen azure keys like one I mentioned above ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Personally, I've only seen keys like [a-zA-Z0-9]==
.
|
||
s1 := detectors.Result{ | ||
DetectorType: detectorspb.DetectorType_AzureCosmosDB, | ||
Raw: []byte(key), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The URL should also be added. #3938 (comment)
s1 := detectors.Result{ | ||
DetectorType: detectorspb.DetectorType_AzureCosmosDB, | ||
Raw: []byte(key), | ||
ExtraData: make(map[string]string), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is unused.
ExtraData: make(map[string]string), |
Description:
This PR adds a new detector for azure cosmosdb.

Note: This for now only work for
documents.azure.com
account urls.Checklist:
make test-community
)?make lint
this requires golangci-lint)?