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

TheHive Case Connector #180138

Merged
merged 23 commits into from
Jul 30, 2024
Merged

Conversation

brijesh-elastic
Copy link
Contributor

@brijesh-elastic brijesh-elastic commented Apr 5, 2024

Summary

TheHive is a new case connector, enabling users to seamlessly transfer elastic cases to TheHive Security Incident Response Platform. This connector facilitates sub-actions such as creating cases, updating cases, and adding comments and creating alerts.

create connector
thehive-connector

test connector

  1. create case

thehive-params-case-test

  1. create alert

thehive-params-alert-test

Checklist

Delete any items that are not applicable to this PR.

For maintainers

@brijesh-elastic brijesh-elastic requested a review from a team as a code owner April 5, 2024 11:06
@cnasikas cnasikas added Team:ResponseOps Label for the ResponseOps team (formerly the Cases and Alerting teams) Feature:Cases Cases feature release_note:feature Makes this part of the condensed release notes Feature:Actions/ConnectorTypes Issues related to specific Connector Types on the Actions Framework v8.14.0 labels Apr 5, 2024
@elasticmachine
Copy link
Contributor

Pinging @elastic/response-ops-cases (Feature:Cases)

@elasticmachine
Copy link
Contributor

Pinging @elastic/response-ops (Team:ResponseOps)

Copy link
Member

@cnasikas cnasikas left a comment

Choose a reason for hiding this comment

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

I did a lightweight review and left some comments. I will leave the full review and testing to the Security solution team.

PUSH_TO_SERVICE = 'pushToService',
CREATE_ALERT = 'createAlert',
}
export enum TheHiveSeverity {
Copy link
Member

Choose a reason for hiding this comment

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

I remember discussing it but just to be sure, are we sure that the severity and the TLP values are fixed and cannot be changed by users in TheHive?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Correct, users can't change or add new enum for severity and TLP in TheHive.

Choose a reason for hiding this comment

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

I confirm, the TLP, PAP and Severity are static values in TheHive

summary: schema.nullable(schema.string()),
impactStatus: schema.nullable(schema.string()),
assignee: schema.nullable(schema.string()),
customFields: schema.nullable(schema.arrayOf(schema.object({}, { unknowns: 'allow' }))),
Copy link
Member

Choose a reason for hiding this comment

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

Instead of schema.object({}, { unknowns: 'allow' }) you can use schema.recordOf(schema.stirng(), schema.any())

]);


export const TheHiveIncidentResponseSchema = schema.object(
Copy link
Member

Choose a reason for hiding this comment

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

Are we sure that we need all the fields to be listed in the schema? Do we use them all?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, we don't use all of them. However, I've included them all as a precaution for future needs.

LMK your thoughts.

Copy link
Member

@cnasikas cnasikas Apr 11, 2024

Choose a reason for hiding this comment

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

I am not sure what is best tbh. Assuming TheHIve will not introduce any breaking changes in the fields I do not see any harm in keeping them. If they do then our validation will start failing. The more field we have the more the chance for this to happen. If they are documented here https://docs.strangebee.com/thehive/api-docs/#operation/Create%20case then I think it is fine to keep them.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, They are documented.

{ unknowns: 'ignore' }
);

export const TheHiveUpdateIncidentResponseSchema = schema.any();
Copy link
Member

Choose a reason for hiding this comment

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

What does TheHive return when you update a case?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is a PATCH request and it returns 204 No Content.

Copy link
Member

Choose a reason for hiding this comment

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

Interesting. Do you know if schema.never() works? Not of this PR, but maybe the framework should skip the validation if the response is 204 No Content.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will check and update.

type: schema.number(),
message: schema.string(),
},
{ unknowns: 'allow' }
Copy link
Member

Choose a reason for hiding this comment

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

Should we ignore?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will remove it.

this.url = this.config.url;
this.organisation = this.config.organisation;
this.api_key = this.secrets.api_key;
this.urlWithoutTrailingSlash = this.url.endsWith('/') ? this.url.slice(0, -1) : this.url;
Copy link
Member

Choose a reason for hiding this comment

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

The framework will take care of that for you. You can do this.url directly in the requests.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right. But it is needed while we returning ExternalServiceIncidentResponse.

Copy link
Member

Choose a reason for hiding this comment

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

I missed that. All good!

method: 'patch',
url: `${this.url}/api/${API_VERSION}/case/${incidentId}`,
data: incident,
headers: { Authorization: `Bearer ${this.api_key}`, 'X-Organisation': this.organisation },
Copy link
Member

Choose a reason for hiding this comment

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

It seems that the same headers are being used in all requests. What about creating a util function getHeaders where it can be used in all functions?


}, [actionParams]);

useEffect(() => {
Copy link
Member

Choose a reason for hiding this comment

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

Why do we need this useEffect?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

When the eventAction changes, it sets subAction props and defaults the severity and TLP values.

Copy link
Member

@cnasikas cnasikas Apr 11, 2024

Choose a reason for hiding this comment

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

Why do we need to put the defaults when the eventAction changes? Theoretically eventAction should never change.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

When user change the toggle(for selecting a case or an alert), we are updating eventAction state accordingly and clear out all the fields and set defaults values in severity and TLP.

Copy link
Member

Choose a reason for hiding this comment

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

If that's the case then you can clear the fields inside setEventActionType when the user changes the toggle.

onChange={(e) => setEventActionType(e.target.value as SUB_ACTION)}
/>
</EuiFormRow>
{eventAction === SUB_ACTION.PUSH_TO_SERVICE ?
Copy link
Member

Choose a reason for hiding this comment

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

I think is better if you create two components one for each subAction and not having all logic in this component. Each component can manage its own state and the parent can reset the global state when the subAction changes.

errors,
messageVariables
}) => {
const [eventAction, setEventAction] = useState(actionParams.subAction ?? SUB_ACTION.PUSH_TO_SERVICE);
Copy link
Member

Choose a reason for hiding this comment

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

I was not part of the conversation so sorry if it was already discussed. Do we want the users to select if they want to create a case or an alert or we should only create alerts?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, we've previously discussed providing support for both creating a case or an alert. However, I'll double-check with the team to confirm once more.

Copy link
Member

Choose a reason for hiding this comment

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

Perfect! Thanks!

@@ -69,6 +70,7 @@ export function registerConnectorTypes({
connectorTypeRegistry.register(getTorqConnectorType());
connectorTypeRegistry.register(getTinesConnectorType());
connectorTypeRegistry.register(getD3SecurityConnectorType());
connectorTypeRegistry.register(getTheHiveConnectorType());
Copy link
Contributor

Choose a reason for hiding this comment

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

Hey @cnasikas, does the new convention to put new connectors under a feature flag apply to this PR?

Copy link
Member

Choose a reason for hiding this comment

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

Hey! Yes, we need to follow the new intermediate release process for all new connectors.

Copy link
Contributor

@semd semd left a comment

Choose a reason for hiding this comment

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

@brijesh-elastic The code looks great, we have good test coverage, and everything works well when tested locally, both sending to theHive alerts and cases.

What I have noticed is that we don't send any information about the alerts generated when the rule is triggered.
When the alerts are received in TheHive we only have the static information defined in the connector configuration, we know the rule, but nothing else.

Is this behavior intended?

I say that because most of the connectors have a body template parameter to parse the alerts and send information about them, such as the host, IP, user... So we have some details to add to the alert description in theHive.

<EuiComboBox
data-test-subj="tagsInput"
fullWidth
placeholder="Tags"
Copy link
Contributor

Choose a reason for hiding this comment

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

We need to apply translation here

<EuiComboBox
data-test-subj="tagsInput"
fullWidth
placeholder="Tags"
Copy link
Contributor

Choose a reason for hiding this comment

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

We need to apply translation here as well

@brijesh-elastic
Copy link
Contributor Author

@brijesh-elastic The code looks great, we have good test coverage, and everything works well when tested locally, both sending to theHive alerts and cases.

What I have noticed is that we don't send any information about the alerts generated when the rule is triggered. When the alerts are received in TheHive we only have the static information defined in the connector configuration, we know the rule, but nothing else.

Is this behavior intended?

I say that because most of the connectors have a body template parameter to parse the alerts and send information about them, such as the host, IP, user... So we have some details to add to the alert description in theHive.

We've decided not to set default message variables for the description parameter, allowing users instead to add their own message variables during connector configuration. This approach doesn't limit users, enabling them to include any desired information in the alert descriptions. Additionally, following discussions with the Hive Team, we've agreed to leave this as blank.

Let me know your thoughts on this.

@semd semd removed the v8.15.0 label Jul 15, 2024
Copy link
Contributor

@semd semd left a comment

Choose a reason for hiding this comment

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

Approving on behalf of Security Solution.
I desk-tested the PR with a local instance of TheHive, everything works well on the Security Solution side. I haven't been able to spot any issues.
The connector sends one alert to theHive (case or alert) per rule execution when alerts are detected, with the static title and description defined on the connector parameters when it is attached to the rule.

The code LGTM. Only a couple of minor test fixes are needed.
Thanks @brijesh-elastic

@elasticmachine
Copy link
Contributor

⏳ Build in-progress, with failures

Failed CI Steps

Test Failures

  • [job] [logs] Jest Integration Tests #7 / Connector type config checks detect connector type changes for: .thehive
  • [job] [logs] Jest Integration Tests #7 / Connector type config checks detect connector type changes for: .thehive

History

@jamiehynds
Copy link

Hey @cnasikas - now that @semd has completed his review, would you mind reviewing the PR from the ResponseOps side?

@cnasikas
Copy link
Member

Hey @jamiehynds! The team is aware and we will review the PR soon!

Copy link
Contributor

@js-jankisalvi js-jankisalvi left a comment

Choose a reason for hiding this comment

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

ResponseOps changes look good, follows the guidelines of sub actions framework and case connector. 👍

Copy link
Contributor

@js-jankisalvi js-jankisalvi left a comment

Choose a reason for hiding this comment

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

The PR is great and already approved.
However, could you please hold on merging it till we confirm whether it needs to follow an intermediate release process for serverless or not.
Putting this comment as request change to stop getting it merged accidentally.

@cnasikas
Copy link
Member

@elasticmachine merge upstream

@cnasikas
Copy link
Member

cnasikas commented Jul 26, 2024

I tested the code for the intermediate release process and is working as expected. @semd @brijesh-elastic I found a bug when I tried to use the connector with fake credentials. React is stuck in an infinite loop. I think we should fix it because this can be a valid scenario (the user puts wrong credentials or TheHive returns a 403 error).

Screenshot 2024-07-26 at 12 02 08 PM

@cnasikas
Copy link
Member

@elasticmachine merge upstream

@cnasikas
Copy link
Member

@semd @brijesh-elastic I tested the connector on the Stack management and I found a bug. Even though I set the Severity, the TLP, and the tags the UI does not show my values. The values are correctly persisted on the backend.

Screen.Recording.2024-07-29.at.12.07.52.PM.mov

Also for the tags (both for cases and alerts):

  • I think we should remove the placeholder as it has the same word with the field's label
  • When I type it shows "There aren't any options available". I found it confusing as the user is only allowed to create new options and not select from a list. I think we need to put the noSuggestions prop to the EuiCombobox to remove this.
Screenshot 2024-07-29 at 12 03 33 PM Screenshot 2024-07-29 at 12 01 21 PM

Copy link
Member

@cnasikas cnasikas left a comment

Choose a reason for hiding this comment

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

I tested @js-jankisalvi changes and the bug fixes from @brijesh-elastic. Everything is working as expected!

@kibana-ci
Copy link
Collaborator

💛 Build succeeded, but was flaky

Failed CI Steps

Test Failures

  • [job] [logs] FTR Configs #90 / Rules Management - Prebuilt Rules - Update Prebuilt Rules Package @ess @serverless @skipInServerlessMKI update_prebuilt_rules_package should allow user to install prebuilt rules from scratch, then install new rules and upgrade existing rules from the new package

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
stackConnectors 265 275 +10

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
stackConnectors 554.6KB 577.4KB +22.9KB

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id before after diff
alerting 25.5KB 25.5KB +11.0B
stackConnectors 53.7KB 56.1KB +2.3KB
total +2.3KB
Unknown metric groups

async chunk count

id before after diff
stackConnectors 88 92 +4

ESLint disabled line counts

id before after diff
stackConnectors 113 119 +6

Total ESLint disabled count

id before after diff
stackConnectors 119 125 +6

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

@cnasikas cnasikas merged commit 696190d into elastic:main Jul 30, 2024
38 checks passed
@kibanamachine kibanamachine added the backport:skip This commit does not require backporting label Jul 30, 2024
cnasikas added a commit that referenced this pull request Sep 10, 2024
## Summary

This PR implements the case support for TheHive connector.
Depends on : #180138

## Screenshots

**List of connectors**

![image](https://github.com/elastic/kibana/assets/123942796/7236eaed-d1d5-4506-80b1-c4e43c6ce36a)

**Configure thehive connector from cases**

![image](https://github.com/elastic/kibana/assets/123942796/bb4843d5-9f29-47c2-baa3-77db81a35319)


### Checklist

Delete any items that are not applicable to this PR.

- [x] Any text added follows [EUI's writing
guidelines](https://elastic.github.io/eui/#/guidelines/writing), uses
sentence case text and includes [i18n
support](https://github.com/elastic/kibana/blob/main/packages/kbn-i18n/README.md)
- [ ]
[Documentation](https://www.elastic.co/guide/en/kibana/master/development-documentation.html)
was added for features that require explanation or tutorials
- [x] [Unit or functional
tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)
were updated or added to match the most common scenarios
- [x] [Flaky Test
Runner](https://ci-stats.kibana.dev/trigger_flaky_test_runner/1) was
used on any tests changed
- [x] Any UI touched in this PR is usable by keyboard only (learn more
about [keyboard accessibility](https://webaim.org/techniques/keyboard/))
- [x] Any UI touched in this PR does not create any new axe failures
(run axe in browser:
[FF](https://addons.mozilla.org/en-US/firefox/addon/axe-devtools/),
[Chrome](https://chrome.google.com/webstore/detail/axe-web-accessibility-tes/lhdoppojpmngadmnindnejefpokejbdd?hl=en-US))
- [x] If a plugin configuration key changed, check if it needs to be
allowlisted in the cloud and added to the [docker
list](https://github.com/elastic/kibana/blob/main/src/dev/build/tasks/os_packages/docker_generator/resources/base/bin/kibana-docker)
- [x] This renders correctly on smaller devices using a responsive
layout. (You can test this [in your
browser](https://www.browserstack.com/guide/responsive-testing-on-local-server))
- [x] This was checked for [cross-browser
compatibility](https://www.elastic.co/support/matrix#matrix_browsers)

### For maintainers

- [ ] This was checked for breaking API changes and was [labeled
appropriately](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process)

---------

Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
Co-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com>
Co-authored-by: Christos Nasikas <christos.nasikas@elastic.co>
gergoabraham pushed a commit to gergoabraham/kibana that referenced this pull request Sep 13, 2024
## Summary

This PR implements the case support for TheHive connector.
Depends on : elastic#180138

## Screenshots

**List of connectors**

![image](https://github.com/elastic/kibana/assets/123942796/7236eaed-d1d5-4506-80b1-c4e43c6ce36a)

**Configure thehive connector from cases**

![image](https://github.com/elastic/kibana/assets/123942796/bb4843d5-9f29-47c2-baa3-77db81a35319)


### Checklist

Delete any items that are not applicable to this PR.

- [x] Any text added follows [EUI's writing
guidelines](https://elastic.github.io/eui/#/guidelines/writing), uses
sentence case text and includes [i18n
support](https://github.com/elastic/kibana/blob/main/packages/kbn-i18n/README.md)
- [ ]
[Documentation](https://www.elastic.co/guide/en/kibana/master/development-documentation.html)
was added for features that require explanation or tutorials
- [x] [Unit or functional
tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)
were updated or added to match the most common scenarios
- [x] [Flaky Test
Runner](https://ci-stats.kibana.dev/trigger_flaky_test_runner/1) was
used on any tests changed
- [x] Any UI touched in this PR is usable by keyboard only (learn more
about [keyboard accessibility](https://webaim.org/techniques/keyboard/))
- [x] Any UI touched in this PR does not create any new axe failures
(run axe in browser:
[FF](https://addons.mozilla.org/en-US/firefox/addon/axe-devtools/),
[Chrome](https://chrome.google.com/webstore/detail/axe-web-accessibility-tes/lhdoppojpmngadmnindnejefpokejbdd?hl=en-US))
- [x] If a plugin configuration key changed, check if it needs to be
allowlisted in the cloud and added to the [docker
list](https://github.com/elastic/kibana/blob/main/src/dev/build/tasks/os_packages/docker_generator/resources/base/bin/kibana-docker)
- [x] This renders correctly on smaller devices using a responsive
layout. (You can test this [in your
browser](https://www.browserstack.com/guide/responsive-testing-on-local-server))
- [x] This was checked for [cross-browser
compatibility](https://www.elastic.co/support/matrix#matrix_browsers)

### For maintainers

- [ ] This was checked for breaking API changes and was [labeled
appropriately](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process)

---------

Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
Co-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com>
Co-authored-by: Christos Nasikas <christos.nasikas@elastic.co>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:skip This commit does not require backporting Feature:Actions/ConnectorTypes Issues related to specific Connector Types on the Actions Framework Feature:Cases Cases feature release_note:feature Makes this part of the condensed release notes Team:ResponseOps Label for the ResponseOps team (formerly the Cases and Alerting teams) v8.16.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants