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

[Defend workflows] Osquery license check + display errors #156738

Merged
merged 9 commits into from
May 8, 2023

Conversation

tomsonpl
Copy link
Contributor

@tomsonpl tomsonpl commented May 4, 2023

Whenever using osquery response actions after downgrading the license, we save actions in index, but never send them to fleet. Now, next to the error of 'paramater not found' (when we use invalid parameter in {{ }} syntax ) we also provide a 'at least platinum license' error.

Zrzut ekranu 2023-05-4 o 22 09 25

@tomsonpl tomsonpl added release_note:skip Skip the PR/issue when compiling release notes Team:Defend Workflows “EDR Workflows” sub-team of Security Solution Feature:Osquery Security Solution Osquery feature v8.9.0 labels May 4, 2023
@tomsonpl tomsonpl self-assigned this May 4, 2023
@tomsonpl tomsonpl requested review from a team as code owners May 4, 2023 20:12
@elasticmachine
Copy link
Contributor

Pinging @elastic/security-defend-workflows (Team:Defend Workflows)

@tomsonpl tomsonpl marked this pull request as draft May 5, 2023 06:16
Copy link
Contributor

@paul-tavares paul-tavares left a comment

Choose a reason for hiding this comment

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

Left a few suggestions (optional).

👍

Comment on lines 24 to 26
const platinumLicenseRequired = 'At least Platinum license is required to use Response Actions.';
const parametersNotFound =
"This query hasn't been called due to parameter used and its value not found in the alert.";
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it possible for these to be placed in a area where both the server-side and UI side can use?

Keeping your version of these messages here feels fragile and prone to bugs if the server side errors are ever changed (by us, or a "future" us 😄 ).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Touche, I was under the impression that we cannot use i18n on the server, but actually we can! I'll just move the logic in there.

const parametersNotFound =
"This query hasn't been called due to parameter used and its value not found in the alert.";

export const getSkippedQueryError = (error: string) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

can you add a return type? based on what I see in the code below, I think it should be string | null

Copy link
Contributor Author

Choose a reason for hiding this comment

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

After applying the comment from above, this function got redundant :)

@@ -68,12 +68,19 @@ interface CreateActionMetadata {
enableActionsWithErrors?: boolean;
}

export interface ActionCreateService {
Copy link
Contributor

Choose a reason for hiding this comment

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

thank you 🙏

@tomsonpl tomsonpl marked this pull request as ready for review May 8, 2023 12:40
@tomsonpl tomsonpl changed the title Osquery licensing [Defend workflows] Osquery license check + display errors May 8, 2023
@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

Public APIs missing comments

Total count of every public API that lacks a comment. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats comments for more detailed information.

id before after diff
osquery 24 22 -2

Async chunks

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

id before after diff
osquery 1.1MB 1.1MB -187.0B
Unknown metric groups

API count

id before after diff
osquery 24 22 -2

ESLint disabled line counts

id before after diff
enterpriseSearch 19 21 +2
securitySolution 398 401 +3
total +5

References to deprecated APIs

id before after diff
osquery 41 42 +1

Total ESLint disabled count

id before after diff
enterpriseSearch 20 22 +2
securitySolution 478 481 +3
total +5

History

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

cc @tomsonpl

@tomsonpl tomsonpl merged commit 952489f into elastic:main May 8, 2023
@kibanamachine kibanamachine added the backport:skip This commit does not require backporting label May 8, 2023
@tomsonpl
Copy link
Contributor Author

💚 All backports created successfully

Status Branch Result
8.8

Note: Successful backport PRs will be merged automatically after passing CI.

Questions ?

Please refer to the Backport tool documentation

tomsonpl added a commit to tomsonpl/kibana that referenced this pull request May 15, 2023
…6738)

(cherry picked from commit 952489f)

# Conflicts:
#	x-pack/plugins/security_solution/common/endpoint/constants.ts
#	x-pack/plugins/security_solution/server/endpoint/routes/actions/list.ts
#	x-pack/plugins/security_solution/server/endpoint/routes/actions/response_actions.test.ts
#	x-pack/plugins/security_solution/server/endpoint/services/actions/create/index.ts
#	x-pack/plugins/security_solution/server/plugin.ts
@tomsonpl
Copy link
Contributor Author

I have created a backport, that backports just one file - e2e utils change. To unblock 8.8 backport merges because pipelines e2e job tends to fail.

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:Osquery Security Solution Osquery feature release_note:skip Skip the PR/issue when compiling release notes Team:Defend Workflows “EDR Workflows” sub-team of Security Solution v8.9.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants