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

[Osquery] Add Osquery results to Case #139909

Merged
merged 70 commits into from Sep 16, 2022
Merged

Conversation

tomsonpl
Copy link
Contributor

@tomsonpl tomsonpl commented Sep 1, 2022

This PR adds a functionality to add Osquery Results to Cases with help of CasesUi.hooks.

All types of Cases are supported - Security, Observability, Stack Management.

To Do:
[+] - Live queries + live query details
[+] - O11y Host metrics flyout
[+] - Alert flyout
[+] - Comment should include agentId details and query results
[+] - Change icon on the left side of comments
[+] - Adding packs
[] - Action item on the right side of the comment ( arrow-right ) should add whole result to Timeline
[+] - Add to timeline in case comment's result

Zrzut ekranu 2022-09-1 o 16 08 32
Zrzut ekranu 2022-09-1 o 16 08 13

# Conflicts:
#	x-pack/plugins/osquery/public/plugin.ts
#	x-pack/plugins/osquery/public/routes/live_queries/details/index.tsx
#	x-pack/plugins/osquery/public/routes/saved_queries/edit/tabs.tsx
#	x-pack/plugins/osquery/public/types.ts
@tomsonpl tomsonpl self-assigned this Sep 1, 2022
@tomsonpl tomsonpl added v8.5.0 release_note:feature Makes this part of the condensed release notes Team:Asset Mgmt Osquery labels Sep 1, 2022

return getAddToTimelineButton({
dataProvider: providerA,
field: value,
dataProvider: providers,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This change is required for us to add multiple actions to time at once. Is this a proper approach?
The functionality itself is not used yet, but I am hoping to use it still for 8.5

@tomsonpl
Copy link
Contributor Author

tomsonpl commented Sep 6, 2022

@elasticmachine merge upstream

@tomsonpl tomsonpl marked this pull request as ready for review September 6, 2022 07:05
@tomsonpl tomsonpl requested a review from a team as a code owner September 6, 2022 07:05
},
async (context, request, response) => {
// this is to skip validation eg. for analysts in cases attachments so they can see the results despite not having permissions
const { isSystemRequest } = request;
if (!isSystemRequest) {
Copy link
Contributor

Choose a reason for hiding this comment

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

@elastic/kibana-security please keep me honest here. The system request header should NOT be used to skip authorization checks. The existence of the system request header is only intended to allow us to determine whether or not the user's session should be extended as a result of a HTTP request being made.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, the inner circle agreed to revert this changes and just show permission denied info.
Thanks @kobelb for your input 👍

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for pinging us @kobelb! Yes, you're absolutely correct, isSystemRequest == request that wasn't initiated by the user and shouldn't be treated as the user activity.

Copy link
Contributor

@jonathan-buttner jonathan-buttner left a comment

Choose a reason for hiding this comment

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

Great progress! Few other nit comments

const { cases } = useKibana().services;

const casePermissions = cases.helpers.canUseCases();
const hasReadPermissions = casePermissions.read && casePermissions.update && casePermissions.push;
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: maybe rename this to hasCasesPermissions, the permissions being checked here indicate there are more than just read.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍

<EuiToolTip
content={
<EuiFlexItem>
{i18n.translate('xpack.osquery.cases.addToCaseText', {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we use the ADD_TO_CASE variable here?

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, thx!

import OsqueryLogo from '../../components/osquery_icon/osquery.svg';

// TODO waiting for Metadata to add "add to timeline" in here
// const AttachmentActions: React.FC = () => (
Copy link
Contributor

Choose a reason for hiding this comment

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

Reminder to remove commented out code

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍

type: 'regular',
event: 'attached Osquery results',
timelineAvatar: <EuiAvatar name="osquery" color="subdued" iconType={OsqueryLogo} />,
// actions: <AttachmentActions />,
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍

const addToTimeline = useCallback(
(payload) => {
if (!actionId || !addToTimelineButton) {
return <></>;
Copy link
Contributor

Choose a reason for hiding this comment

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

Just checking, can this be null? or does that mess with how the parent component is rendered?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Null would mess up. I will refactor these 2 - addToTimeline and addToCases buttons within OSquery during FF, or for 8.6. There is a few new things we learnt during preparation for 8.5.

},
async (context, request, response) => {
let agent;
// this is to skip validation eg. for analysts in cases attachments so they can see the results despite not having permissions
const { isSystemRequest } = request;
if (!isSystemRequest) {
Copy link
Member

Choose a reason for hiding this comment

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

Hey! Don't forget to remove this logic from all routes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍

@jonathan-buttner
Copy link
Contributor

Great progress on this feature! Here are a few things I noted while testing before the instance went down 😢 :

There is empty space on the overflow actions button

Extra padding in actions menu image

Rerendering the osquery attachment causes the case to flicker and jump. I'm not sure if this is related to how Cases performs the rerendering, or maybe the animation of populating results in the osquery component. This appears to only happen in the Stack Cases, so maybe it's our fault 😆

Flickering
Screen.Recording.2022-09-15.at.1.03.30.PM.mov

@tomsonpl
Copy link
Contributor Author

@elasticmachine merge upstream

1 similar comment
@tomsonpl
Copy link
Contributor Author

@elasticmachine merge upstream

Copy link
Contributor

@jonathan-buttner jonathan-buttner left a comment

Choose a reason for hiding this comment

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

Nice work! I left a few minor questions and text changes.

I tested:

  • user with no osquery permissions cannot view the attachment within cases ✅
  • user with no cases permissions results in the add to case button being disabled ✅
  • user without superuser permissions can view the attachment within cases (this works after adding read to the all feature` ✅
  • user without indices permissions cannot view the attachment within cases ✅
  • attaching to timeline, viewing in lens, and viewing in discover from within cases works ✅

body={
<FormattedMessage
id="xpack.osquery.cases.permissionDenied"
defaultMessage=" To access this results, ask your administrator for {osquery} Kibana
Copy link
Contributor

Choose a reason for hiding this comment

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

Minor change: To access these...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍

<CasesContext owner={CASES_OWNER} permissions={casePermissions}>
<EuiTabbedContent
// TODO: extend the EuiTabbedContent component to support EuiTabs props
// bottomBorder={false}
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: remove commented out code

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'd leave the TODO, maybe there's gonna be time to take care of it soon

<AddToCaseButton
queryId={payload.queryId}
agentIds={agentIds}
actionId={liveQueryActionId || ''}
Copy link
Contributor

Choose a reason for hiding this comment

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

How likely is it for the liveQueryActionId to be undefined? aka setting actionId to an empty string? Should we disable the button if it undefined?

I think I tracked the code and it'll just result in the live query not running so maybe we'll just show an empty component within cases.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're right, empty string doesn't make sense. I'll add a check if we have livequeryActionId before rendering Button. We should have it after running a query, before getting results - I would risk a statement that it's obligatory.

@jonathan-buttner
Copy link
Contributor

jonathan-buttner commented Sep 15, 2022

Also I tested closing the osquery indices and that results in the attachments within cases looking like:

image

Seems fine to me 👍

Or maybe we could say something like Unable to find results or Index does not exist. Basically this is what a user would see if they exported a case and imported it into a different cluster. We can improve upon that later though.

@tomsonpl
Copy link
Contributor Author

The indices error is definitely something we should take a closer look into, thanks for the suggestion. 👍 and the whole review thing, you were very helpful @jonathan-buttner and @cnasikas thanks 👍

@tomsonpl tomsonpl removed request for a team September 16, 2022 06:47
@kibana-ci
Copy link
Collaborator

kibana-ci commented Sep 16, 2022

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
osquery 288 295 +7

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
cases 68 71 +3

Async chunks

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

id before after diff
osquery 1005.7KB 1009.7KB +4.0KB

Public APIs missing exports

Total count of every type that is part of your API that should be exported but is not. This will cause broken links in the API documentation system. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats exports for more detailed information.

id before after diff
cases 29 28 -1

Page load bundle

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

id before after diff
cases 124.4KB 124.6KB +205.0B
osquery 85.5KB 89.9KB +4.4KB
total +4.6KB
Unknown metric groups

API count

id before after diff
cases 84 87 +3

async chunk count

id before after diff
osquery 7 9 +2

ESLint disabled line counts

id before after diff
osquery 102 106 +4

Total ESLint disabled count

id before after diff
osquery 104 108 +4

History

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

cc @tomsonpl

Copy link
Contributor

@patrykkopycinski patrykkopycinski left a comment

Choose a reason for hiding this comment

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

LGTM

@tomsonpl tomsonpl merged commit b744e7c into elastic:main Sep 16, 2022
@kibanamachine kibanamachine added the backport:skip This commit does not require backporting label Sep 16, 2022
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 ci:cloud-deploy Create or update a Cloud deployment ci:cloud-redeploy Always create a new Cloud deployment Osquery release_note:feature Makes this part of the condensed release notes Team:Asset Mgmt v8.5.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants