-
Notifications
You must be signed in to change notification settings - Fork 515
crowdstrike: provide alternate endpoint to query host data for GovCloud CIDs #16007
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
base: main
Are you sure you want to change the base?
crowdstrike: provide alternate endpoint to query host data for GovCloud CIDs #16007
Conversation
🚀 Benchmarks reportTo see the full report comment with |
|
Pinging @elastic/security-service-integrations (Team:Security-Service Integrations) |
kcreddy
left a comment
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.
@navnit-elastic, can you add screenshot how the new option would show to the users?
cc: @narph
@kcreddy - added it in the commit message. |
💚 Build Succeeded
History
|
| - version: "2.8.1" | ||
| changes: | ||
| - description: Provide an alternate endpoint to query host data for GovCloud CIDs. | ||
| type: bugfix |
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 think this is an enhancement. This is an additional feature, not a fix for an incorrect behaviour.
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.
+1
| (size(body.?errors.orValue([])) > 0) ? | ||
| { | ||
| "events": body.errors.map(error, | ||
| // This logic determines which CrowdStrike devices endpoint to call. The |
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.
If we can automagically determine the correct behaviour from the URL, why are we adding a configuration toggle? Is it the case that the known URLs are not an exhaustive enumeration of all GovCloud base URLs?
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 personally feel we should not even try to switch the request flow based on url parsing as it obfuscates the control flow and undermines the entire presence of the toggle
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.
Agree. We should do one or the other. If we can always determine the correct behaviour from the URL, we should do that, if it is less than always, we should not even try and just use a configuration.
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 approach was discussed in #15795 and since there was no alternate proposals, this was taken up. I can see that the URL check will give us correct behaviour all the time until a new GovCloud URL is launched by Crowdstrike, in such case a toggle can help quicken the SDH resolution.
I do agree that having both might be confusing and reduces each other's efficacy.
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.
If there's a chance that new GovCloud URLs can be launched in future, I feel it's best to just stick to a toggle based approach for clarity purposes. When a failure occurs (if the toggle in not configured), it would be a clear signal to indicate the real issue in every case. Also it's easier for support to resolve without creating a SDH if the behaviour is singular and known in advance. A dual mechanism, though efficient and less intrusive can obfuscate the behaviour and lead to confusion in some scenarios down the line.
| // This dual-check mechanism ensures that GovCloud users do not encounter errors | ||
| // even if they forget to enable the "gov_cloud" flag in the manifest. |
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'm not convinced of the value of this convenience.
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.
+1
| // This dual-check mechanism ensures that GovCloud users do not encounter errors | ||
| // even if they forget to enable the "gov_cloud" flag in the manifest. | ||
| ( | ||
| state.gov_cloud || state.url.trim_right("/") in ["https://api.laggar.gcw.crowdstrike.com", "https://api.us-gov-2.crowdstrike.mil"] ? |
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.
Add a comment saying that this is the GovCloud branch.
(this is a long program, so some navigation aids will be helpful)
| ) | ||
| ) | ||
| : | ||
| state.with( |
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.
Add comment that this is the non-GovCloud branch.
| "error": { | ||
| "code": string(get_resp.StatusCode), | ||
| "id": string(get_resp.Status), | ||
| "message": "GET:" + ( |
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.
| "message": "GET:" + ( | |
| "message": "GET: " + ( |
| "error": { | ||
| "code": string(post_resp.StatusCode), | ||
| "id": string(post_resp.Status), | ||
| "message": "POST:" + ( |
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.
| "message": "POST:" + ( | |
| "message": "POST: " + ( |
| type: bool | ||
| title: GovCloud | ||
| description: >- | ||
| GovCloud CIDs should enable this. When enabled, the integration will automatically use CrowdStrike’s GovCloud-supported devices endpoint (`/devices/entities/devices/v2`) instead of the standard combined endpoint (`/devices/combined/devices/v1`). |
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.
| GovCloud CIDs should enable this. When enabled, the integration will automatically use CrowdStrike’s GovCloud-supported devices endpoint (`/devices/entities/devices/v2`) instead of the standard combined endpoint (`/devices/combined/devices/v1`). | |
| GovCloud CIDs should enable this. When enabled, the integration will use CrowdStrike’s GovCloud-supported devices endpoint (`/devices/entities/devices/v2`) instead of the standard combined endpoint (`/devices/combined/devices/v1`). |
If we are enabling the toggle, then it's no longer automatic. As for the dual safety mechanism, it undermines the presence of the toggle.
Proposed commit message
Checklist
changelog.ymlfile.Author's Checklist
How to test this PR locally
System Tests:
Related issues
Screenshots