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] Make Osquery All with All base privillege #130523

Merged
merged 10 commits into from
May 17, 2022

Conversation

tomsonpl
Copy link
Contributor

Summary

This change will turn on Osquery Privillege to All, when we set the Global Privillege button to All.

@tomsonpl tomsonpl added bug Fixes for quality problems that affect the customer experience release_note:skip Skip the PR/issue when compiling release notes Team:Asset Management Security Asset Management Team Feature:Osquery Security Solution Osquery feature v8.3.0 labels Apr 19, 2022
@tomsonpl tomsonpl self-assigned this Apr 19, 2022
@tomsonpl tomsonpl requested a review from a team as a code owner April 19, 2022 06:08
@elasticmachine
Copy link
Contributor

Pinging @elastic/security-asset-management (Team:Asset Management)

@tomsonpl
Copy link
Contributor Author

@elasticmachine merge upstream

@tomsonpl tomsonpl requested a review from a team as a code owner April 19, 2022 08:21
@tomsonpl
Copy link
Contributor Author

@elasticmachine merge upstream

@@ -45,7 +45,6 @@ const registerFeatures = (features: SetupPlugins['features']) => {
app: [PLUGIN_ID, 'kibana'],
catalogue: [PLUGIN_ID],
order: 2300,
excludeFromBasePrivileges: true,
Copy link
Member

Choose a reason for hiding this comment

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

This was an intentional design decision when this feature was introduced in #106669 (cc @patrykkopycinski). I don't remember the full context, but I believe the team wanted to ensure that osquery was explicitly granted to end users, rather than "rolling up" into the preconfigured all/read base privileges.

Altering this behavior has historically been considered a breaking change, and as such cannot be made in a minor release.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @legrego thanks for pointing this out :)

I think that the teams consensus (in here: #128768) is that we want to change this.
However, I will forward your concerns to @melissaburpo @patrykkopycinski @james-elastic for a second round of thoughts ;)

IF we decide to still go with the change, how do you suggest going forward with this ? Are you suggesting waiting for post-v8 version?

Big thanks!

Copy link
Member

@legrego legrego Apr 19, 2022

Choose a reason for hiding this comment

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

I think that the teams consensus (in here: #128768) is that we want to change this.

Ok, great!

IF we decide to still go with the change, how do you suggest going forward with this ? Are you suggesting waiting for post-v8 version?

Yes, my recommendation is to wait for the next major version. Otherwise we will grant access to osquery to users who previously didn't have access, and this lack of access may have been done intentionally by an administrator. Granting this additional access simply by upgrading their stack constitutes a "surprise on upgrade", and when it comes to authorization/access controls, that's not a good thing.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hi @legrego, apologies for the slow reply on this one. The team wanted to discuss it as a group before getting back to you, and I was out of office for a bit.

We’d like to move forward with this during the 8.x release period, for a couple reasons. 1) After a customer raised this issue, it became clear that the current setting can be experienced as a bug. If an administrator is setting up a user role that has All Kibana access, it would make sense that would apply to Osquery as well. 2) It’s not clear yet what the plans are for 9.x. So if possible, we’d like to fix this sooner rather than later.

While surprising people on upgrade is not ideal, in this case, we would plan to clearly document this change so that people know what to expect. And for full Osquery access, Administrators would still need to take additional steps so that people can use it: to view osquery data, users need Read access to the logs-osquery_manager.result* index, and to use Osquery itself, the Osquery Manager integration must be deployed to Elastic Agents. So while users with All may get access to a new nav item after this change, they wouldn't be able to see any sensitive data without fulfilling the other Osquery requirements.

If it would be easier to discuss this one live, I’m happy to set up a meeting, just let me know. Thanks!

Copy link
Member

Choose a reason for hiding this comment

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

Hey @melissaburpo, thanks for taking the time to explain your reasoning, and the mitigating factors that are in place to prevent unintended usage of this feature. That makes me feel quite a bit better, but I would like to discuss with @jportner before we proceed. He's on PTO for the next few days, but I'll try to follow up with him towards the end of the week.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hi folks, I spoke with Larry about this --
I agree it's not ideal but it's OK to make this particular change.

My one suggestion is to relabel this issue with release_note:fix (instead of release_note:skip) so that we can capture this change in our release notes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@legrego @jportner @melissaburpo @patrykkopycinski Given we came to a conclusion here to merge this. Could I get the green light (approvals)? Big, big thanks in advance! :)

@tomsonpl
Copy link
Contributor Author

tomsonpl commented May 8, 2022

@elasticmachine merge upstream

@tomsonpl tomsonpl added release_note:fix and removed release_note:skip Skip the PR/issue when compiling release notes labels May 12, 2022
@tomsonpl
Copy link
Contributor Author

@elasticmachine merge upstream

@tomsonpl
Copy link
Contributor Author

@elasticmachine merge upstream

@tomsonpl tomsonpl requested review from melissaburpo and jportner May 16, 2022 12:27
@tomsonpl tomsonpl requested a review from legrego May 16, 2022 12:27
@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

✅ unchanged

History

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

cc @tomsonpl

@tomsonpl tomsonpl merged commit e9c1c39 into elastic:main May 17, 2022
@kibanamachine kibanamachine added the backport:skip This commit does not require backporting label May 17, 2022
academo pushed a commit to XavierM/kibana that referenced this pull request May 17, 2022
XavierM added a commit that referenced this pull request May 17, 2022
* wip I

* add alert table state in case

* [CI] Auto-commit changed files from 'node scripts/eslint --no-cache --fix'

* add new API to get FeatureID form registrationContext and update UI to use this new API

* rm dead code

* [CI] Auto-commit changed files from 'node scripts/eslint --no-cache --fix'

* remove unnecessary memo

* adds tests for case view helpers

* Move http call to API and add tests for getFeatureIds

* fix type + unit test

* add unit tests + cleanup

* add new api integration test for _feature_ids

* [CI] Auto-commit changed files from 'node scripts/eslint --no-cache --fix'

* Fix small type creating typescript slowness

* remove console log

* use import type for validfeatureId

* force any to improve typescript performance

* Update APM (#132270)

Co-authored-by: Renovate Bot <[email protected]>

* [ResponseOps][Docs] Updating ServiceNow docs with OAuth setup instructions (#131344)

* Updating ServiceNow docs. Need screenshots

* Adding screenshots

* Fix nested screenshots and lists

* Tweaks and screenshots

* Updates

* blergh

* Apply suggestions from code review

Co-authored-by: Lisa Cawley <[email protected]>

* Apply suggestions from code review

Co-authored-by: Mike Côté <[email protected]>

Co-authored-by: lcawl <[email protected]>
Co-authored-by: Kibana Machine <[email protected]>
Co-authored-by: Mike Côté <[email protected]>

* Show polling options when 'Data streams' option is selected in the Console Settings modal. (#132277)

* [Osquery] Make Osquery All with All base privillege (#130523)

* [XY] Add normalizeTable function to correct works with esdocs (#131917)

* Add normalizeTable function to correct works with esdocs

* Fix types

* Fix types

* Fix CI

* Fix CI

* Some fixes

* Remove fallback with min/max value for domain

* Added tests

* Some refactoring

Co-authored-by: Kibana Machine <[email protected]>
Co-authored-by: Yaroslav Kuznietsov <[email protected]>

* [Osquery] Add default osquery_saved_query objects (#129461)

* [Unified Search] Show error message for invalid date filter value (#131290)

* feat: added show error message for invalid date

* refact: move logic in HOC

* feat: refactoring code and added translation

* refact show error

* refact: show error message

* refact: remove translation

* refactor: changed menu for show FilterEdit

* fix: open/close popover

* feat: field.type => KBN_FIELD_TYPES

* feat: remove extra code with with input check and refactored filter item

* feat: added tests and refactoring code

* refact: getFieldValidityAndErrorMessage

* feat: return isInvalid checking in valur input type for string, ip

* Update navigation landing pages to use appLinks config (#132027)

* Update navigation landing pages to use appLinks config

* Please code review

* align app links changes

* Update links descriptions

* Rollback title changes

* Fix wrong links descriptions

* Fix unit tests

* Fix description

Co-authored-by: semd <[email protected]>

* [Cloud Posture] add resource findings page flyout  (#132243)

* [Discover] Add a tour for Document Explorer (#131125)

* [Discover] Add "Take a tour" button to the Document Explorer callout

* [Discover] Tmp

* [Discover] Add a first Document Explorer tour step

* [Discover] Add other Document Explorer tour steps

* [Discover] Update tour steps positioning

* [Discover] Add gifs to tour steps

* [Discover] Refactor how tour steps are registered

* [Discover] Add new step to the tour. Update tour steps text.

* [Discover] Improve steps positioning

* [Discover] Fix positioning for Add field step

* [Discover] Add icons to tour steps

* [Discover] Reorganize components

* [Discover] Skip Columns step when it's not available

* [Discover] Rename components

* [Discover] Add some tests

* [Discover] Fix positioning

* [Discover] Fix props

* [Discover] Render steps only if the tour is active

* [Discover] Update gifs

* [Discover] Add image alt text for gifs

* [Discover] Tag the Take tour button

* [Discover] Update text and tests

* [Discover] Add more tests

* [Discover] Rename assets directory

* [Discover] Fix tour in mobile view. Improve steps positioning and animation.

* [Discover] Update text in tour steps

* [Discover] Update sort.gif

* [Discover] Update image width

* Update src/plugins/discover/public/components/discover_tour/discover_tour_provider.tsx

Co-authored-by: gchaps <[email protected]>

* Update src/plugins/discover/public/components/discover_tour/discover_tour_provider.tsx

Co-authored-by: gchaps <[email protected]>

* [Discover] Update sort.gif

* [Discover] Fix code style

Co-authored-by: gchaps <[email protected]>
Co-authored-by: Kibana Machine <[email protected]>

* [XY] Add `minTimeBarInterval` arg (#128726)

* Added `xAxisInterval` arg

* Add validation

* Add tests

* Rename xAxisInterval to minTimeBarInterval and add validation

* Fix imports

* Add tests to validation

* Fix conflicts

* [CI] Auto-commit changed files from 'node scripts/precommit_hook.js --ref HEAD~1..HEAD --fix'

* Fix tests

Co-authored-by: Kibana Machine <[email protected]>

* do not use barrel imports

* do not use barrel import

* do not use barrel import

* do not use barrel imports

* do not use barrel import

* import types

* Add tests

* Fix cases bundle size

* Add more tests

* [Fleet] Add new API to get current upgrades (#132276)

* Add support of Data View switching for Agg-Based visualizations (#132184)

* Add support of Data View switching for Agg-Based visualizations

* fix CI

* add use_date_view_updates

* implement sync with state

* cleanup

* cleanup

* cleanup

* Update index.ts

* fix PR comments

* Update use_data_view_updates.ts

* Update use_data_view_updates.ts

Co-authored-by: Kibana Machine <[email protected]>

* [Security Solution] Responsive styling fixes (#131951)

* [Discover] Add Analytics No Data Page (#131965)

* [Discover] Add Analytics No Data Page

* Make showEmptyPrompt parameter optional

* Remove unused import

* Remove unnecessary test

* Fix test

* Update failing test?

* Update failing test

* Changing the order of functional tests

* Fix error handling

* Addressing PR comments

Co-authored-by: Kibana Machine <[email protected]>

* Remove barrel export from public index file

* remove barrel export

* Re-export missing exports

* Turn off feature flag

Co-authored-by: kibanamachine <[email protected]>
Co-authored-by: Esteban Beltran <[email protected]>
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Co-authored-by: Renovate Bot <[email protected]>
Co-authored-by: Ying Mao <[email protected]>
Co-authored-by: lcawl <[email protected]>
Co-authored-by: Mike Côté <[email protected]>
Co-authored-by: CJ Cenizal <[email protected]>
Co-authored-by: Tomasz Ciecierski <[email protected]>
Co-authored-by: Uladzislau Lasitsa <[email protected]>
Co-authored-by: Yaroslav Kuznietsov <[email protected]>
Co-authored-by: Nodir Latipov <[email protected]>
Co-authored-by: Pablo Machado <[email protected]>
Co-authored-by: semd <[email protected]>
Co-authored-by: Or Ouziel <[email protected]>
Co-authored-by: Julia Rechkunova <[email protected]>
Co-authored-by: gchaps <[email protected]>
Co-authored-by: Christos Nasikas <[email protected]>
Co-authored-by: Nicolas Chaulet <[email protected]>
Co-authored-by: Alexey Antonov <[email protected]>
Co-authored-by: Steph Milovic <[email protected]>
Co-authored-by: Maja Grubic <[email protected]>
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 bug Fixes for quality problems that affect the customer experience Feature:Osquery Security Solution Osquery feature release_note:fix Team:Asset Management Security Asset Management Team v8.3.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants