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

[ BUG ] Aggregate Notifications dict instead of list #664

Closed
JulianIntargia opened this issue May 17, 2022 · 6 comments · Fixed by #666
Closed

[ BUG ] Aggregate Notifications dict instead of list #664

JulianIntargia opened this issue May 17, 2022 · 6 comments · Fixed by #666
Assignees
Labels
bug 🐛 Something isn't working detects Detections issues and questions message center Message Center issues and questions. recon Recon issues and questions

Comments

@JulianIntargia
Copy link

Describe the bug
When making a request to the AggregateNotificationsV1 endpoint, not using the body argument, but individual keywords, the API returns:
"code": 400, "message": "Invalid aggregates request, json: cannot unmarshal object into Go value of type []*msa.AggregateQueryRequest", "message_key": "INVALID_AGGREGATES" }

The problem is that the request body is sent like this:
{ arguments... }
instead of
[ { arguments.... } ]

To Reproduce

falcon = recon.Recon(access_token=self.token, base_url=self.base_url)
filter = "string" # in my case "selector_id:['x', 'y',...]"
response = falcon.aggregate_notifications(name="Hit count per selector", type="terms", field="selector_id", filter=filter, size=10)
print(response)

Expected behavior
Should return a resources list based on the filter.

Environment (please complete the following information):

  • OS: Windows
  • Python: 3.9
  • FalconPy: 1.1.1

Additional context
When using the body argument, it's possible to make a successful request, however the IDE gives a warning as body should be a dict and not list[dict].

body = [{"field": "selector_id",
                 "filter": "string",
                 "name": "Hit count per selector",
                 "size": 10,
                 "type": "terms"}]
response = falcon.aggregate_notifications(body=body)
print(response)
@JulianIntargia JulianIntargia added the bug 🐛 Something isn't working label May 17, 2022
@jshcodes
Copy link
Member

So the payload format is being generated incorrectly, you fall back to using the body keyword and then get a warning regarding the format of the body dictionary as well. That's definitely not right. 🙈

The aggregate operations can have weird payloads, so I think you've pointed out the right place for us to start.

We will investigate this today and report back once we've identified the issue.

Thank you for reporting this!

@jshcodes jshcodes self-assigned this May 17, 2022
@jshcodes jshcodes added the recon Recon issues and questions label May 17, 2022
@jshcodes
Copy link
Member

I can confirm the first of our two issues here. The body payload handler for this method is incorrectly defined as a dict instead of a list like you mention above: https://github.com/CrowdStrike/falconpy/blob/main/src/falconpy/recon.py#L60. This will be resolved in our next release (in flight now).

While we're at it, we will check other aggregate body keyword definitions for this same error. (The first couple I've checked so far all define the keyword as a list.)

Then there's the keyword to body payload generation issue, which we are still digging into. Stay tuned. 😄

@JulianIntargia
Copy link
Author

Yes it's quite weird with the aggregates, I only understood how to use them by copying the requests from the web ui 😅. As for the payload generation issue, wrapping the return value from aggregate_payload into a list might do it: https://github.com/CrowdStrike/falconpy/blob/main/src/falconpy/recon.py#L120. But that may lead to some loss of functionality, in case a list with multiple aggregates are requested?

@jshcodes
Copy link
Member

But that may lead to some loss of functionality, in case a list with multiple aggregates are requested?

Correct, body payload abstraction will result in a singular "data update" payload being generated. This is one of the reasons keywords like body, parameters and action_parameters will always be retained, as this allows for scenarios where multiple requests are sent within the same body. (One of the samples on my "to-do list" discusses this and demonstrates the different variations.)

Since there is a stand-alone payload handler for the aggregate payloads, we might get away with just wrapping the return in a list since that's what the API is expecting. (This assumes the dictionary the handler is generating is valid, just not inside a list like the API wants.) We will get this tested and confirmed today. 🧑‍💻

@jshcodes
Copy link
Member

jshcodes commented May 17, 2022

Hi @JulianIntargia!

This was a good find, thank you again for reporting it. Three Service Classes had this issue where they were passing expected aggregate list payloads as dictionaries. (Detects, MessageCenter and Recon).

Interestingly enough, there are a couple of service classes that specify they do not want lists here (QuickScan, Quarantine), so addressing this within the payload handler was overly complex.

Instead, I've handled the issue within the affected Service Classes the same way the other classes handle it, wrapping the return from the handler in a list when it assigns it to body. For sending multiple updates via a single payload, you'll still be able to specify body directly.

FalconPy v1.1.2 will release containing this update, and has already started the process through our approval cycle. If you'd like to test things out and let us know of any feedback or issues, you can install the crowdstrike-falconpy-dev package (v1.1.2.dev1) and check it out.

Remove the current version of FalconPy from the environment before installing this version.

python3 -m pip install crowdstrike-falconpy-dev==1.1.2.dev1

@jshcodes jshcodes added detects Detections issues and questions message center Message Center issues and questions. labels May 17, 2022
@JulianIntargia
Copy link
Author

Thank you for the quick work! I tested it out and now it works as expected, and so far I've encountered no issues.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🐛 Something isn't working detects Detections issues and questions message center Message Center issues and questions. recon Recon issues and questions
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants