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 SentinelOne-V2.yml #35273

Closed
wants to merge 6 commits into from
Closed

Conversation

ShirleyDenkberg
Copy link
Contributor

@ShirleyDenkberg ShirleyDenkberg commented Jul 7, 2024

Contributing to Cortex XSOAR Content

Make sure to register your contribution by filling the contribution registration form

The Pull Request will be reviewed only after the contribution registration form is filled.

Status

  • In Progress
  • Ready
  • In Hold - (Reason for hold)

Related Issues

https://jira-dc.paloaltonetworks.com/browse/CRTXDOC-3017

Description

Initial review of SentinelOne pack.

Must have

  • Tests
  • Documentation

@ShirleyDenkberg
Copy link
Contributor Author

Comments/edits on yml file:

  • There is a command - sentinelone-get-white-list
    The description is: Lists all exclusion items that match the specified input filter.
    Shouldn't this command list all the included items? Not the excluded items?
    Can you please check with the partner to see if this description is correct? (@kgal-pan is checking on this)

  • Line 104 - I don't see that there is an API Token (recommended) parameter.
    additionalinfo: Use the "API Token (Recommended)" parameter instead.

  • Line 386
    The example does not match the predefined list.

  • Line 1400, 1402, 1406, 1862
    Should the description be: A comma-separated list of....

Copy link

github-actions bot commented Jul 7, 2024

Coverage

Coverage Report
FileStmtsMissCoverMissing
Packs/SentinelOne/Integrations/SentinelOne-V2
   SentinelOne-V2.py132465950%108, 232, 249–250, 265, 269, 271, 299, 301, 310–311, 314, 316, 322–323, 326, 328, 334–335, 338–339, 342–344, 347–348, 351, 353, 359–360, 367, 369, 373–374, 377–379, 382–383, 389–390, 393–394, 398–399, 402–403, 414, 416, 422–423, 426, 428, 434–435, 438, 440, 446, 448, 451–452, 460–461, 464–465, 473–474, 477–478, 484–485, 511–512, 517–518, 524–526, 529–531, 537–539, 571–573, 588, 601–604, 683–685, 689–690, 695–696, 714–715, 748–749, 757–758, 761–763, 766–767, 784–785, 788–789, 795–796, 799–803, 806–807, 810–811, 817–818, 821–823, 854, 856–857, 860, 862, 870–871, 874–875, 880, 882–892, 895, 897–900, 903, 976–981, 983–984, 991–993, 995–996, 1016, 1030, 1032, 1042, 1044, 1057–1063, 1076–1077, 1079, 1082–1084, 1090, 1104–1107, 1109, 1111–1113, 1121, 1123, 1138, 1141–1142, 1144, 1161–1164, 1166–1168, 1173, 1186, 1188–1189, 1192, 1195–1197, 1199–1202, 1207, 1220–1221, 1224–1225, 1228, 1231–1233, 1235–1238, 1243, 1251, 1278–1279, 1322–1323, 1367, 1373, 1399–1400, 1415, 1417, 1419–1420, 1434, 1448, 1450–1462, 1464–1465, 1468, 1470–1471, 1485, 1510–1511, 1543–1544, 1576–1577, 1596, 1599–1600, 1603, 1606–1608, 1610–1613, 1618, 1631, 1633–1637, 1647, 1660, 1663–1669, 1671–1672, 1675, 1677–1678, 1691, 1703, 1706–1707, 1710, 1713–1715, 1717–1718, 1720–1721, 1725, 1738–1739, 1762, 1764–1765, 1767, 1769–1770, 1783, 1797, 1800–1803, 1806, 1808–1809, 1812, 1814–1819, 1821, 1823, 1835–1836, 1840–1847, 1855, 1879, 1917, 1939, 1942, 1945, 1947–1948, 1962, 1974, 1977–1978, 1980, 1982–1984, 1986–1996, 2001, 2015–2016, 2018–2019, 2023–2024, 2026–2027, 2029, 2041, 2043–2044, 2046–2054, 2086, 2101, 2103, 2106, 2109–2111, 2113–2114, 2116–2117, 2122, 2173–2174, 2193, 2196–2201, 2204, 2209–2210, 2223, 2251, 2267, 2274, 2278, 2303–2304, 2306–2313, 2315–2316, 2319, 2323–2324, 2326, 2332, 2344, 2346, 2363–2364, 2367–2368, 2384, 2399, 2402, 2405, 2408–2409, 2427, 2443, 2446–2448, 2450–2451, 2454–2455, 2458–2459, 2462–2463, 2466, 2469–2470, 2475, 2488, 2490–2491, 2494, 2497–2498, 2510, 2527–2534, 2544–2545, 2548, 2550, 2565, 2568, 2571, 2573, 2620, 2623–2624, 2627–2628, 2632, 2634, 2646, 2649–2650, 2652–2653, 2658, 2670–2672, 2679, 2681–2682, 2685–2686, 2688–2689, 2701, 2703–2706, 2708–2711, 2713, 2720–2721, 2723–2726, 2728–2730, 2733, 2735, 2746–2748, 2750, 2752, 2758, 2796–2797, 2826, 2852, 2854–2855, 2857–2859, 2879, 2895, 2913, 2919–2921, 2924, 2928, 2930, 2949–2951, 2960, 2976, 2981, 2985, 3082, 3111, 3113, 3115, 3117–3119, 3130, 3189–3193, 3195–3196, 3198, 3200–3204, 3206–3207, 3209–3215, 3217, 3252–3253, 3262–3263, 3273, 3277–3278, 3285, 3332, 3336–3337, 3342–3344, 3346, 3426–3427, 3429, 3436–3438, 3440–3445, 3447–3448, 3450, 3459–3462, 3472–3473, 3475, 3477, 3480, 3498–3500, 3513, 3626, 3643, 3653, 3655–3657
TOTAL132465950% 

Tests Skipped Failures Errors Time
87 0 💤 0 ❌ 0 🔥 7.173s ⏱️

@kgal-pan
Copy link
Contributor

kgal-pan commented Jul 7, 2024

@munna-metron @saurabh-metron - Can you please review and approve these changes?

@ShirleyDenkberg
Copy link
Contributor Author

In the pack readme, the last line is:
Please contact the partner directly via the support link on the right.
But there is no link. Either provide a link if it is needed or remove this line (and perhaps the line before it).

@munna-metron
Copy link
Contributor

munna-metron commented Jul 10, 2024

Hi @ShirleyDenkberg ,
These are my comments on the PR:

  • SentinelOne-get-white-list: This command was not added by us and is a bit older. When I cross-checked the endpoint details in the API documentation provided by SentinelOne, they were correct. I suggest changing the name of this command to something like SentinelOne-get-exclusions-list
    -- Attached a screenshot of API doc for the same.
  • Additional Info: Use the "API Token (Recommended)" parameter instead. Feel free to remove the "(Recommended)" string from the additional info.
  • Classification Query Parameter in sentinelone-get-threats: Correct, the example and the predefined options are not matching. Cross-checked the classification query parameter in the official SentinelOne API docs. The classification field is an array and this needs to be a text box that will accept comma-separated values instead of a dropdown.
    -- Attached screenshot for the same.
  • Support Link in README.md: You can find the support link in the pack-metadata.json file. Additionally, update the README with this line in #L16: Please contact the partner directly via the support [link](https://www.sentinelone.com/support/).
  • README Update: Also please update the SentinelOne-V2/README.md file according to the changes made in the YAML files.
    Thanks!
    image (55)
    image (54)

## How does this pack work?
Create an instance of the **SentinelOne v2** integration and start fetching information from the SentinelOne database.

Note: Support for this Pack moved to the partner on March, 23, 2023.
Note: Support for this pack moved to the partner on March, 23, 2023.
Please contact the partner directly via the support link on the right.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Suggested change
Please contact the partner directly via the support link on the right.
Please contact the partner directly via the support [link](https://www.sentinelone.com/support/).

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, I hope this will suffice.

@ShirleyDenkberg
Copy link
Contributor Author

@thefrieddan1 @kgal-pan @munna-metron
Not sure what to do with this comment:
I asked: Line 386 - The example does not match the predefined list.
Partner said: Correct, the example and the predefined options are not matching. Cross-checked the classification query parameter in the official SentinelOne API docs. The classification field is an array and this needs to be a text box that will accept comma-separated values instead of a dropdown.
-- Attached screenshot for the same. (see partner's comment above).

I can change the description to: List of threat classifications to search.
But should the predefined values be removed? or Should the example be removed?

And I did not get a response to:
Line 1400, 1402, 1406, 1862
Should the description be: A comma-separated list of....

@munna-metron
Copy link
Contributor

Hi @ShirleyDenkberg ,

  1. Since this one of first commands built for this integration, a lot has changed from then, not sure of this API used to contain some predefined values earlier but now it supports comma-separated values, which can be of any combination (the list keeps evolving, it shouldn't be static, imo), the recommendation is to remove the predefined options and make the classification a text field where the user can provide comma-separated values, which we will then parse into a list. This should do the trick, and there is no need to update the examples. I hope this makes sense.
  2. About lines 1400, 1402, 1406, and 1862, the current descriptions will suffice.
    Thanks

@ShirleyDenkberg
Copy link
Contributor Author

@munna-metron Thanks for your response. I removed the predefined values.

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

Successfully merging this pull request may close these issues.

3 participants