[X-Pack] Beats centralized management: security role + licensing #30520

ycombinator · 2018-05-10T13:10:20Z

Resolves #30493.

This PR adds:

a built-in role, beats_admin that provides unfettered access to the .management-beats index. The purpose of this index is to store configuration and other peripheral information to make the Beats Centralized Management feature work.
licensing-related logic for the Beats Centralized Management feature.

pickypg

Some minor comments.

pickypg · 2018-05-10T16:17:34Z

x-pack/plugin/core/src/main/java/org/elasticsearch/license/XPackLicenseState.java

+                    case STANDARD:
+                    case GOLD:
+                    case PLATINUM:
+                        return new String[] { "Logstash will no longer poll for centrally-managed configuration" };


pickypg · 2018-05-10T16:26:43Z

x-pack/plugin/core/src/main/java/org/elasticsearch/license/XPackLicenseState.java

+     * Beats is allowed as long as there is an active license of type TRIAL, STANDARD, GOLD or PLATINUM
+     * @return {@code true} as long as there is a valid license
+     */
+    public boolean isBeatsAllowed() {


Given that the ingest team owns, I wonder if this should instead be isIngestAllowed and simply replace isLogstashAllowed? They share the same license requirements, so it seems reasonable to me that they shouldn't dupe their code too.

Personally I feel like the code should reflect the products/features and not organization structure, as much as possible. So I'd like to keep this as-is.

I'd be okay with refactoring the duplicate code in the two methods into a helper method like isNotBasic or something like that. What do you think of that?

I'd be happy with an isNotBasic method.

pickypg · 2018-05-10T16:28:19Z

x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/beats/BeatsFeatureSetUsage.java

+
+import java.io.IOException;
+
+public class BeatsFeatureSetUsage extends XPackFeatureSet.Usage {


It's a shame that we don't have any way for users to see if it's actually being used -- to help them to debug or even just to check it. We can add that with a follow-on PR though.

Whatever we do here should be reflected in the Logstash centralized management.

elasticmachine · 2018-05-11T12:18:39Z

Pinging @elastic/es-core-infra

exekias

This is looking good! I left some comments on the schema, but we can evolve it once we put our feet on the API code

exekias · 2018-05-11T13:11:47Z

x-pack/plugin/core/src/main/resources/beats-admin-template.json

+        "enrollment_token": {
+          "properties": {
+            "token": {
+              "type": "keyword"


Tokens should expire under 2 situations: when they are used or after some time if they are not used. We should add a field here to store the expiration date

exekias · 2018-05-11T13:16:16Z

x-pack/plugin/core/src/main/resources/beats-admin-template.json

+            }
+          }
+        },
+        "configuration_fragment": {


maybe block is more meaningful than fragment here, as this is not holding any random fragment of the config file, but an specific block, wdyt?

Block sounds good to me. Will change.

exekias · 2018-05-11T13:17:38Z

x-pack/plugin/core/src/main/resources/beats-admin-template.json

+            },
+            "fragment_yml": {
+              "type": "text"
+            }


We will need to add the config.type here, to let the beat know where this block belongs (module, output, processor...)

Sounds good, will add.

exekias · 2018-05-11T13:18:39Z

x-pack/plugin/core/src/main/resources/beats-agents-template.json

+            "local_configuration_yml": {
+              "type": "text"
+            },
+            "central_configuration_yml": {


Do we need to store this?

We don't have to but I think it makes sense to do it from a performance perspective. Basically we can compute this value whenever anytime changes are made in the UI (or, more accurately, via the API), which should be relatively less frequent compared to the how often a Beat agent might request this value. So I think pre-computing it and storing it means we don't have to compute it on the fly each time a Beat requests it.

Are you thinking there is some downside of storing this? We can turn indexing off on this field if storage space is a concern (it will still be available via _source then).

Got it, makes sense

We definitely need some kind of caching for confs, so we either do it in memory (one config per tag combination, no need to have one per beat) or store them here. I have one concern with this approach: Changing a tag requires updating all beats on that tag, that means going one by one to get their tag list, combine blocks and update the full central_configuration_yml.

I have one concern with this approach: Changing a tag requires updating all beats on that tag, that means going one by one to get their tag list, combine blocks and update the full central_configuration_yml.

This is true. However, I think:

This should be an infrequent operation compared to how often a Beat will request its (merged) configuration,

We can get all the Beats for a tag from ES in a single _search request, and

We can update all the Beats' central_configuration_yml (after it is recomputed) in ES with a single _bulk request.

Let's see how all of this shakes out after an initial implemention. I plan to use something like siege or httperf to simulate 10000s of Beats requesting configs while changing a tag in the midst of that scenario. We can optimize the long poles in the performance tent as we go.

ycombinator · 2018-05-11T13:31:56Z

Thanks for the feedback on the schema @exekias. Yes, I imagine this schema will change as we implement the API, hence I marked this PR as WIP for now. I plan to remove the WIP and put it into formal review once all the APIs are implemented and you've had a chance to use them.

colings86 · 2018-05-14T10:36:56Z

Given how similar this is to the existing Logstash ES plugin (in that it seems to just install some index templates) would it make sense to have a single ES plugin that covers both Logstash and Beats central configuration rather than having separate ones?

ycombinator · 2018-05-14T13:51:59Z

@colings86 Hmmm, @pickypg also brought up something along similar lines earlier in this PR. 🤔

I'm not opposed to sharing the implementation between these two plugins in the code but I'm wary of combining their interfaces. Today (prior to this PR), users can disable the Logstash plugin via the xpack.logstash.enabled: false setting in their elasticsearch.yml. Also, there is a logstash section in the GET http://localhost:9200/_xpack response body. If, as part of this PR, we were to combine the logstash and beats plugins into one, those interfaces would break.

Is there some way we could keep the interfaces distinct for the beats and logstash plugins but share their implementation? If so, I can do that in this PR. But I think that might still result in having two ES plugins then, which would not accomplish your suggestion?

ycombinator · 2018-05-14T19:08:30Z

Chatted with @kobelb about the security model proposed in this PR (two indices + two roles to grant different levels of access to these indices). Turns out we can simplify this model quite a bit. Specifically:

Have just one .management-beats index instead of the two indices that are in the current proposal.
Instead of creating a new built-in beats_agent role, expand the built-in kibana_system role to have the same privileges as detailed in this proposal for the beats_agent role.

I will update this PR shortly to make the above changes. Thanks @kobelb!

colings86 · 2018-05-15T08:16:32Z

@ycombinator I still don't really see why they need to be separate in any way. I don't think we gain much by having the ability to do xpack.logstash.enabled:false as opposed to telling the user that this combined plugin (lets call it centralised-config for now) is required for either Logstash centralised config or beats centralised config and if they don't require either Logstash or Beats centralised config they might want to do xpack.centralised-config.enabled: false. This is like us saying that we require ingest-geoIP plugin to be installed for Metricbeat, we don't have a specific plugin that the user can tell Elasticsearch that they want to use GeoIP specifically in Metricbeat, its just generally available.

I would like to understand the following:

Both this plugin and the Logstash plugin seem to just install an index template. Is that correct?
Are there other things that the plugin will need to do as well?
If the plugin is just installing index templates why is it required to do this from an ES plugin rather than something upstream installing them (i.e. whatever is using those index templates)?

The reason I ask the above is that it seems pretty heavy-weight to me to build two plugins which just effectively saves a couple of PUT index template calls and I am wondering if there is something more generic that ES could provide so its not necessary to have extra plugins here.

ycombinator · 2018-05-15T11:35:20Z

@colings86 At this point I think it's safe to say these plugins only install index templates.

A bit of history here: when the Logstash plugin was originally created, there was some thinking that we might add API endpoints to it as well. Then there was talk of Kibana middleware and it was eventually decided that we wouldn't be adding APIs to the Logstash plugin in ES; instead we're going to add them in Kibana. Of course, by that time, the Logstash ES plugin was already in existence.

Now, when it came time to provide index templates for Beats central management, I followed the Logstash path for consistency and created a Beats ES plugin (via this PR). However, we know right off the bat that we won't be adding any more functionality to the Beats plugin in ES.

So, all in all, I'd be +1 to not creating a plugin for Beats in ES now. Instead we can have Kibana install the index template for Beats. I can change this PR to remove the plugin bits and leave just the bits for installing built-in Security roles.

As for the existing Logstash plugin in ES, I propose we remove it in 7.0 and have Kibana install it's index template then. This is because there could be consumers today who are using the interfaces setup by this plugin today (the ones I mentioned in #30520 (comment) above). That way we don't introduce a breaking change in 6.x.

WDYT?

colings86 · 2018-05-15T12:05:47Z

@ycombinator that sounds good to me.

ycombinator · 2018-05-15T13:33:31Z

I've now removed the Beats plugin code from this PR and updated the PR title and description to accurate reflect the changeset.

ycombinator · 2018-05-15T15:39:18Z

Template creation has been moved into Kibana: elastic/kibana#19072

elasticmachine · 2018-05-18T02:24:56Z

Pinging @elastic/es-security

pickypg

Some minor comments. LGTM with Jay's comments.

pickypg · 2018-07-09T15:29:58Z

x-pack/plugin/core/src/main/java/org/elasticsearch/license/XPackLicenseState.java

+        switch (newMode) {
+            case BASIC:
+                if (!isBasic(currentMode)) {
+                    return new String[] { "Beats will no longer poll for centrally-managed configuration" };


This is a little misleading, which is due to how Logstash was already phrased so it may be moot. Technically Beats will continue to poll, right, but they'll see a basic license and not short circuit, then repeat later? Perhaps this should say

Beats will no longer be able to use centrally-managed configuration

TBH I'm not sure what the exact behavior here will be from the beats side. I bet @exekias has given this thought and can weigh in here?

Technically Beats will continue polling, but won't get new configs until license is back on track. @pickypg's message may be more accurate

Updated in d79e73d.

pickypg · 2018-07-09T15:34:18Z

x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/beats/BeatsFeatureSetUsage.java

+
+import java.io.IOException;
+
+public class BeatsFeatureSetUsage extends XPackFeatureSet.Usage {


FYI, the other variants of this class are not final, which I think may be because some of them get mocked for tests.

jaymode · 2018-07-09T17:08:10Z

@ycombinator I forgot in my earlier review but there should be doc updates with this change

ycombinator · 2018-07-10T13:17:13Z

I forgot in my earlier review but there should be doc updates with this change

@jaymode Hey, sorry, but could you tell me which docs in this repo need to be updated? I was thinking of adding to the "built in roles" docs but they seemed to have moved here now: https://github.com/elastic/stack-docs/blob/master/docs/en/stack/security/authorization/built-in-roles.asciidoc. I am working on a PR to update those. Are there any other docs, in this (elastic/elasticsearch) repo that I should be updating too? Thanks.

ycombinator · 2018-07-10T13:35:46Z

@jaymode The PR I mentioned in my previous comment is up now: elastic/stack-docs#80.

jaymode

LGTM. Thanks @ycombinator for the doc updates in the other repo

ycombinator · 2018-07-10T17:09:36Z

We'll be backporting this to 6.x whenever the rest of the Beats management feature is ready (or close to being ready). I'm tracking this pending backport in the Beats management meta issue: elastic/beats#7028

jaymode · 2018-07-10T18:33:51Z

@ycombinator is there an expected version? I added the v7 label to this since it went into master. Usually we also add the 6.x version label too that it is supposed to go in. Since it is not backported yet, you would also add the backport pending label and remove that label once the backport is complete

ycombinator · 2018-07-10T19:16:52Z

is there an expected version? ... Usually we also add the 6.x version label too that it is supposed to go in.

I believe we're targeting 6.5, so I've added that label now.

Since it is not backported yet, you would also add the backport pending label and remove that label once the backport is complete

Yep, added.

* master: [TEST] Mute SlackMessageTests.testTemplateRender Docs: Explain closing the high level client [ML] Re-enable memory limit integration tests (#31328) [test] disable packaging tests for suse boxes Add nio transport to security plugin (#31942) XContentTests : Insert random fields at random positions (#30867) Force execution of fetch tasks (#31974) Fix unreachable error condition in AmazonS3Fixture (#32005) Tests: Fix SearchFieldsIT.testDocValueFields (#31995) Add Expected Reciprocal Rank metric (#31891) [ML] Get ForecastRequestStats doc in RestoreModelSnapshotIT (#31973) SQL: Add support for single parameter text manipulating functions (#31874) [ML] Ensure immutability of MlMetadata (#31957) Tests: Mute SearchFieldsIT.testDocValueFields() muted tests due to #31940 Work around reported problem in eclipse (#31960) Move build integration tests out of :buildSrc project (#31961) Tests: Remove use of joda time in some tests (#31922) [Test] Reactive 3rd party tests on CI (#31919) SQL: Support for escape sequences (#31884) SQL: HAVING clause should accept only aggregates (#31872) Docs: fix typo in datehistogram (#31972) Switch url repository rest tests to new style requests (#31944) Switch reindex tests to new style requests (#31941) Docs: Added note about cloud service to installation and getting started [DOCS] Removes alternative docker pull example (#31934) Add Snapshots Status API to High Level Rest Client (#31515) ingest: date_index_name processor template resolution (#31841) Test: fix null failure in watcher test (#31968) Switch test framework to new style requests (#31939) Switch low level rest tests to new style Requests (#31938) Switch high level rest tests to new style requests (#31937) [ML] Mute test failing due to Java 11 date time format parsing bug (#31899) [TEST] Mute SlackMessageTests.testTemplateRender Fix assertIngestDocument wrongfully passing (#31913) Remove unused reference to filePermissionsCache (#31923) rolling upgrade should use a replica to prevent relocations while running a scroll HLREST: Bundle the x-pack protocol project (#31904) Increase logging level for testStressMaybeFlush Added lenient flag for synonym token filter (#31484) [X-Pack] Beats centralized management: security role + licensing (#30520) HLRest: Move xPackInfo() to xPack().info() (#31905) Docs: add security delete role to api call table (#31907) [test] port archive distribution packaging tests (#31314) Watcher: Slack message empty text (#31596) [ML] Mute failing DetectionRulesIT.testCondition() test Fix broken NaN check in MovingFunctions#stdDev() (#31888) Date: Add DateFormatters class that uses java.time (#31856) [ML] Switch native QA tests to a 3 node cluster (#31757) Change trappy float comparison (#31889) Fix building AD URL from domain name (#31849) Add opaque_id to audit logging (#31878) re-enable backcompat tests add support for is_write_index in put-alias body parsing (#31674) Improve release notes script (#31833) [DOCS] Fix broken link in painless example Handle missing values in painless (#30975) Remove the ability to index or query context suggestions without context (#31007) Ingest: Enable Templated Fieldnames in Rename (#31690) [Docs] Fix typo in the Rollup API Quick Reference (#31855) Ingest: Add ignore_missing option to RemoveProc (#31693) Add template config for Beat state to X-Pack Monitoring (#31809) Watcher: Add ssl.trust email account setting (#31684) Remove link to oss-MSI (#31844) Painless: Restructure Definition/Whitelist (#31879) HLREST: Add x-pack-info API (#31870)

…stic#30520) * Adding Beats x-pack plugin + index templates * Adding built-in roles for Beats central management * Fixing typo * Refactoring: extract common code into method * More refactoring for more code reuse * Use a single index for Beats management * Rename "fragment" to "block" * Adding configuration block type * Expand kibana_system role to include Beats management index privileges * Fixing syntax * Adding test * Adding asserting for reserved role * Fixing privileges * Updating template * Removing beats plugin * Fixing tests * Fixing role variable name * Fixing assertions * Switching to preferred syntax for boolean false checks * Making class final * Making variables final * Updating Basic license message to be more accurate

) Backport of #30520 to `6.x`. Original description: Resolves #30493. This PR adds: * a built-in role, `beats_admin` that provides unfettered access to the `.management-beats` index. The purpose of this index is to store configuration and other peripheral information to make the Beats Centralized Management feature work. * licensing-related logic for the Beats Centralized Management feature.

jpountz · 2019-01-30T11:25:22Z

@ycombinator Can the "backport pending" label be removed now?

ycombinator · 2019-01-30T11:28:05Z

Yes, sorry, removed now.

ycombinator mentioned this pull request May 10, 2018

Adding Beats x-pack plugin + index templates #30518

Closed

ycombinator added the WIP label May 10, 2018

pickypg reviewed May 10, 2018

View reviewed changes

colings86 added the :Core/Infra/Core Core issues without another label label May 11, 2018

colings86 added :Beats and removed :Core/Infra/Core Core issues without another label labels May 11, 2018

exekias reviewed May 11, 2018

View reviewed changes

ycombinator force-pushed the x-pack/management/beats branch 2 times, most recently from a0bd4c0 to f801bb3 Compare May 14, 2018 20:01

This was referenced May 14, 2018

[Beats Management] APIs: Create enrollment tokens elastic/kibana#19018

Merged

[Beats Management] APIs: Enroll beat elastic/kibana#19056

Merged

ycombinator changed the title ~~[X-Pack] Beats central management plugin~~ [X-Pack] Built-in security role for Beats central management admins May 15, 2018

ycombinator changed the title ~~[X-Pack] Built-in security role for Beats central management admins~~ [X-Pack] Beats centralized management: security role + licensing May 15, 2018

This was referenced May 15, 2018

[Beats Management] APIs: List beats elastic/kibana#19086

Merged

[Beats Management] APIs elastic/kibana#18858

Closed

[Beats Management] APIs: Verify beats elastic/kibana#19103

Merged

[Beats Management] APIs: Update beat elastic/kibana#19148

Merged

ycombinator force-pushed the x-pack/management/beats branch from f8a5f6a to 77b23f6 Compare May 17, 2018 13:15

tvernum added the :Security/Authorization Roles, Privileges, DLS/FLS, RBAC/ABAC label May 18, 2018

pickypg approved these changes Jul 9, 2018

View reviewed changes

ycombinator mentioned this pull request Jul 10, 2018

Documents built-in beats_admin role elastic/stack-docs#80

Merged

Updating Basic license message to be more accurate

d79e73d

jaymode approved these changes Jul 10, 2018

View reviewed changes

exekias mentioned this pull request Jul 10, 2018

Central Management elastic/beats#7028

Closed

26 tasks

ycombinator merged commit 3189ef4 into elastic:master Jul 10, 2018

jaymode added the v7.0.0 label Jul 10, 2018

ycombinator added backport pending v6.5.0 labels Jul 10, 2018

ycombinator mentioned this pull request Oct 4, 2018

[X-Pack] Beats centralized management: security role + licensing #34305

Merged

colings86 added >enhancement and removed :Beats labels Oct 25, 2018

ycombinator removed the backport pending label Jan 30, 2019

colings86 added v7.0.0-beta1 and removed v7.0.0 labels Feb 7, 2019

ph mentioned this pull request Jan 31, 2020

[Fleet] Add a fleet_admin user and a role with the required permission for fleet. elastic/kibana#56486

Closed

jakelandis mentioned this pull request Mar 9, 2020

Remove dead 'beats' code #53312

Merged

jen-huang mentioned this pull request Aug 11, 2021

Remove code related to Beats Central Management #76330

Open

justincr-elastic mentioned this pull request Oct 29, 2021

Remove code related to Beats Central Management #80107

Closed


		import java.io.IOException;

		public class BeatsFeatureSetUsage extends XPackFeatureSet.Usage {

[X-Pack] Beats centralized management: security role + licensing #30520

[X-Pack] Beats centralized management: security role + licensing #30520

Conversation

ycombinator commented May 10, 2018 • edited Loading

pickypg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elasticmachine commented May 11, 2018

exekias left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ycombinator May 11, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ycombinator May 11, 2018 • edited Loading

Choose a reason for hiding this comment

ycombinator commented May 11, 2018

colings86 commented May 14, 2018

ycombinator commented May 14, 2018 • edited Loading

ycombinator commented May 14, 2018

colings86 commented May 15, 2018 • edited Loading

ycombinator commented May 15, 2018 • edited Loading

colings86 commented May 15, 2018

ycombinator commented May 15, 2018

ycombinator commented May 15, 2018

elasticmachine commented May 18, 2018

pickypg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jaymode commented Jul 9, 2018

ycombinator commented Jul 10, 2018 • edited Loading

ycombinator commented Jul 10, 2018

jaymode left a comment

Choose a reason for hiding this comment

ycombinator commented Jul 10, 2018

jaymode commented Jul 10, 2018

ycombinator commented Jul 10, 2018

jpountz commented Jan 30, 2019

ycombinator commented Jan 30, 2019

ycombinator commented May 10, 2018 •

edited

Loading

ycombinator May 11, 2018 •

edited

Loading

ycombinator May 11, 2018 •

edited

Loading

ycombinator commented May 14, 2018 •

edited

Loading

colings86 commented May 15, 2018 •

edited

Loading

ycombinator commented May 15, 2018 •

edited

Loading

ycombinator commented Jul 10, 2018 •

edited

Loading