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

[Fleet] Add reserved privilege for Fleet setup #112647

Closed
Tracked by #112648
joshdover opened this issue Sep 21, 2021 · 5 comments · Fixed by #113932
Closed
Tracked by #112648

[Fleet] Add reserved privilege for Fleet setup #112647

joshdover opened this issue Sep 21, 2021 · 5 comments · Fixed by #113932
Assignees
Labels
enhancement New value added to drive a business result Team:Fleet Team label for Observability Data Collection Fleet team

Comments

@joshdover
Copy link
Contributor

joshdover commented Sep 21, 2021

As part of #112648, we need to grant the elastic/fleet-server service account privileges that would allow it to call the Fleet setup API without giving it access to anything else in Kibana.

Broadly, this involves:

  • Adding a new privilege definition registered by Fleet plugin in Kibana (see below)
  • Granting the elastic/fleet-server user access to the kibana-* application (ES issue to be opened for this)
  • Updating the setup route to require this privilege
  • Adding tests that use the elastic/fleet-server user to call the setup API (in addition to allow superuser)

Notes on how to do this from @legrego, copied from an internal ticket:

I wonder if we can take advantage of Kibana's "reserved privileges" here. With this setup, Kibana would register a privilege similar to the following:

{
	"kibana-.kibana": {
		"reserved_fleet-setup": {
			"actions": [
	          "setup:fleet"
	        ],
	        "metadata": {
	          "description": "Setup access to Fleet Setup API"
	        }
		}
	}
}

And then we could update the Fleet service account in ES to grant this privilege to the kibana-* application, which would give these tokens the appropriate access regardless of the current kibana.index setting. This is what we do for some of the built-in roles today, such as machine_learning_user:

https://github.com/elastic/elasticsearch/blob/7.x/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/authz/store/ReservedRolesStore.java#L197-L209


If we take this route, then Fleet could register this privilege itself, similar to the following:

public setup(core, { features }) {
   features.registerKibanaFeature({
     ....,
	 reserved: {
        description: 'Privilege to setup fleet integrations. Intended for use by the Fleet Service account only.',
        privileges: [{
          id: 'fleet-setup',
          privilege: {
            excludeFromBasePrivileges: true,
            api: ['fleet-setup'],
            savedObject: {
              all: [],
              read: [],
            },
            ui: [],
	      },
        }],
      },
   })
}

Note the api property within the privilege. This should allow us to take advantage of the existing authorization checks, so if you tag your route with access:fleet-setup, then you shouldn't need to make your own _has_privileges check.

We might have to tweak the Role Management UI a to make sure the Fleet feature still renders as expected, but that shouldn't take much effort on our part if needed.

@joshdover joshdover added enhancement New value added to drive a business result Team:Fleet Team label for Observability Data Collection Fleet team labels Sep 21, 2021
@elasticmachine
Copy link
Contributor

Pinging @elastic/fleet (Team:Fleet)

@joshdover
Copy link
Contributor Author

joshdover commented Oct 5, 2021

@legrego I've gotten pretty far with this, but I could some advice on how to proceed here.

I have implemented the suggested changes, however, the built-in privilege check using the tags: ['access:fleet-setup'] feature continues to fail due to the service account not having the login: application privilege (which I don't believe it should need?).

This is the request being executed by checkPrivilegesWithRequest:

GET http://localhost:9200/_security/user/_has_privileges
Authorization: Bearer <elastic/fleet-server service account token>
content-type: application/json

{
  "index": [],
  "application": [
    {
      "application": "kibana-.kibana",
      "resources": [
        "space:default"
      ],
      "privileges": [
        "version:8.0.0",
        "login:",
        "api:8.0.0:fleet-setup"
      ]
    }
  ]
}

Which returns:

{
  "username": "elastic/fleet-server",
  "has_all_requested": false,
  "cluster": {},
  "index": {},
  "application": {
    "kibana-.kibana": {
      "space:default": {
        "api:8.0.0:fleet-setup": true,
        "login:": false,
        "version:8.0.0": true
      }
    }
  }
}

It seems to me that we shouldn't grant this user the login: privilege, so I am tyring to find a way to skip this privilege on this specific API. I haven't found a way to get around this without reimplementing the entire checkPrivilegesWithRequest so I experimented with adding support for a !access:login route tag that can be used to exclude this privilege which seems to work, but I want to know if you have any other advice or ideas for this.

It's also worth noting that we plan to remove this in 8.0, so this could be seen as a temporary measure which may make this more acceptable.

Here is the PR that adds the privileges to the service account in Elasticsearch: elastic/elasticsearch#78192
In this draft PR, the last commit adds the privilege registration in Kibana and support for the !access:login tag: #112808

@legrego
Copy link
Member

legrego commented Oct 5, 2021

I have implemented the suggested changes, however, the built-in privilege check using the tags: ['access:fleet-setup'] feature continues to fail due to the service account not having the login: application privilege (which I don't believe it should need?).

@joshdover ah this was an oversight in my initial proposal, sorry about that.

It seems to me that we shouldn't grant this user the login: privilege, so I am tyring to find a way to skip this privilege on this specific API.

I agree, the fleet service account should not have this action granted to it.

I haven't found a way to get around this without reimplementing the entire checkPrivilegesWithRequest so I experimented with adding support for a !access:login route tag that can be used to exclude this privilege which seems to work, but I want to know if you have any other advice or ideas for this.

I only have 1 other idea. Instead of relying on the route's access tags, we could add an optional options parameter to checkPrivilegesWithRequest which could allow you to bypass the login requirement. Then, the fleet endpoint could check privileges manually instead of relying on the built-in mechanism. Roughly something like:

diff --git a/x-pack/plugins/security/server/authorization/check_privileges.ts b/x-pack/plugins/security/server/authorization/check_privileges.ts
index 3a35cf164ad..adfb5f97aea 100644
--- a/x-pack/plugins/security/server/authorization/check_privileges.ts
+++ b/x-pack/plugins/security/server/authorization/check_privileges.ts
@@ -25,6 +25,18 @@ interface CheckPrivilegesActions {
   version: string;
 }
 
+/**
+ * Options to influence the privilege checks.
+ */
+export interface CheckPrivilegesOptions {
+  /**
+   * Whether or not the `login` action should be required (default: true).
+   * Setting this to false is not advised except for special circumstances, when you do not require
+   * the request to belong to a user capable of logging into Kibana.
+   */
+  requireLoginAction?: boolean;
+}
+
 export function checkPrivilegesWithRequestFactory(
   actions: CheckPrivilegesActions,
   getClusterClient: () => Promise<IClusterClient>,
@@ -38,7 +50,10 @@ export function checkPrivilegesWithRequestFactory(
     );
   };
 
-  return function checkPrivilegesWithRequest(request: KibanaRequest): CheckPrivileges {
+  return function checkPrivilegesWithRequest(
+    request: KibanaRequest,
+    { requireLoginAction = true }: CheckPrivilegesOptions = {}
+  ): CheckPrivileges {
     const checkPrivilegesAtResources = async (
       resources: string[],
       privileges: CheckPrivilegesPayload
@@ -48,7 +63,11 @@ export function checkPrivilegesWithRequestFactory(
         : privileges.kibana
         ? [privileges.kibana]
         : [];
-      const allApplicationPrivileges = uniq([actions.version, actions.login, ...kibanaPrivileges]);
+      const allApplicationPrivileges = uniq([
+        actions.version,
+        ...[requireLoginAction && actions.login],
+        ...kibanaPrivileges,
+      ]);
 
       const clusterClient = await getClusterClient();
       const { body } = await clusterClient.asScoped(request).asCurrentUser.security.hasPrivileges({

And then in your route definition, remove the access tag, and instead do something like:

  router.post(
    {
      path: '/internal/fleet/my-special-endpoint',
      validate: false,
    },
    (async (context, request, response) => {
      if (securityStart.authz.mode.useRbacForRequest(request)) {
        const checkPrivileges = securityStart.checkPrivilegesDynamicallyWithRequest(request);
        const { hasAllRequested } = await checkPrivileges(
          {
            kibana: [securityStart.authz.actions.api.get('fleet-setup')],
          },
          {
            requireLoginAction: false,
          }
        );

        if (!hasAllRequsted) {
          return response.forbidden();
        }
      }
    })
  );

I slightly prefer this approach over the proposal in https://github.com/elastic/kibana/pull/112808/files#diff-f258f464e2091c78a6081cc606d85949c1009b38616069449eccc197594c8d30 as #112808 forces checkPrivileges to be aware of route tags/API authorization, which isn't something that it should have to care about.

It's also worth noting that we plan to remove this in 8.0, so this could be seen as a temporary measure which may make this more acceptable.

Agreed. If my proposal here isn't workable or palatable for some reason, then I can personally get behind your proposal as a short-term solution.

cc @jportner / @elastic/kibana-security as I shouldn't be making these types of technical decisions unilaterally at this point 🙂

@joshdover
Copy link
Contributor Author

Instead of relying on the route's access tags, we could add an optional options parameter to checkPrivilegesWithRequest which could allow you to bypass the login requirement. Then, the fleet endpoint could check privileges manually instead of relying on the built-in mechanism.

Makes sense to me, I'll implement and tests for that now and we can review the changes in the PR.

@jportner
Copy link
Contributor

jportner commented Oct 5, 2021

cc @jportner / @elastic/kibana-security as I shouldn't be making these types of technical decisions unilaterally at this point 🙂

Thanks for tagging me, I agree this sounds like the best path forward.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New value added to drive a business result Team:Fleet Team label for Observability Data Collection Fleet team
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants