Expose ability to check if API Keys are enabled #63454

legrego · 2020-04-14T11:13:10Z

Summary

Exposes the ability to check if API Keys are enabled in Elasticsearch:

New server-side function: await setup.authc.areAPIKeysEnabled()
New client-side function: await setup.authc.areAPIKeysEnabled()

Updates existing /internal/security/api_key/privileges route to use ES exception metadata to check if API Keys are enabled (instead of checking error message text).

Resolves #59576

legrego · 2020-04-14T12:25:10Z

x-pack/plugins/security/server/authentication/api_keys.ts

+      return false;
+    }
+
+    const id = `kibana-api-key-service-test-${uuid.v4()}`;


note: we don't want to accidentally invalidate a key that actually exists, but we also want the ID to be descriptive in case someone inspecting the ES audit logs wants to trace this back. By combining descriptive text with a UUID, we can guarantee uniqueness while still being descriptive.

updated note: after I wrote this comment, I realized that the key id is generated by ES, and not under the user's control (unlike the key's name). Therefore, there is no need to generate a unique id using UUID. I've updated this to always just use kibana-api-key-service-test instead.

… into security/api-keys-enabled

…keys-enabled

legrego · 2020-04-14T14:38:58Z

@elasticmachine merge upstream

… into security/api-keys-enabled

elasticmachine · 2020-04-14T18:51:34Z

Pinging @elastic/kibana-security (Team:Security)

legrego · 2020-04-15T11:42:44Z

x-pack/plugins/security/public/management/api_keys/api_keys_api_client.ts

@@ -10,6 +10,7 @@ import { ApiKey, ApiKeyToInvalidate } from '../../../common/model';
 interface CheckPrivilegesResponse {
  areApiKeysEnabled: boolean;
  isAdmin: boolean;
+  canManage: boolean;


note previously, the API Keys app had a dual purpose for areApiKeysEnabled: It was true if and only if both of the following were true:

User was authorized to retrieve their own API Keys

API Keys were in fact enabled

This separates those two concerns into dedicated properties

azasypkin · 2020-04-20T14:57:43Z

ACK: can review tomorrow if no one does it today.

azasypkin

LGTM! Tested locally, everything works as expected. Just left a few questions/nits, but nothing substantial (except for error.body. that we can potentially use instead of JSON.parse).

azasypkin · 2020-04-21T12:56:49Z

x-pack/plugins/security/public/management/api_keys/api_keys_grid/api_keys_grid_page.tsx

+      this.props.notifications.toasts.addDanger(
+        i18n.translate('xpack.security.management.apiKeys.table.fetchingApiKeysErrorMessage', {
+          defaultMessage: 'Error checking privileges: {message}',
+          values: { message: _.get(e, 'body.message', '') },


nit: while you're, let's finally remove lodash from this file:

Suggested change

values: { message: _.get(e, 'body.message', '') },

values: { message: e.body?.message || '' },

azasypkin · 2020-04-21T12:58:32Z

x-pack/plugins/security/server/routes/api_keys/get.ts

@@ -10,7 +10,7 @@ import { wrapIntoCustomErrorResponse } from '../../errors';
 import { createLicensedRouteHandler } from '../licensed_route_handler';
 import { RouteDefinitionParams } from '..';

-export function defineGetApiKeysRoutes({ router, clusterClient }: RouteDefinitionParams) {
+export function defineGetApiKeysRoutes({ router, clusterClient, authc }: RouteDefinitionParams) {


nit: looks like a leftover

Suggested change

export function defineGetApiKeysRoutes({ router, clusterClient, authc }: RouteDefinitionParams) {

export function defineGetApiKeysRoutes({ router, clusterClient }: RouteDefinitionParams) {

azasypkin · 2020-04-21T12:59:56Z

x-pack/plugins/security/server/routes/api_keys/enabled.ts

+
+export function defineEnabledApiKeysRoutes({
+  router,
+  clusterClient,


nit:

Suggested change

clusterClient,

azasypkin · 2020-04-21T13:30:00Z

x-pack/plugins/security/server/authentication/api_keys.ts

+          id,
+        },
+      });
+      return true;


question: what does it mean if ES replies with 200 to such request? Does Elasticsearch just ignores unknown API keys? It seems ES works with security tokens differently then elastic/elasticsearch#54532 (comment)

question: what does it mean if ES replies with 200 to such request? Does Elasticsearch just ignores unknown API keys? It seems ES works with security tokens differently then elastic/elasticsearch#54532 (comment)

Yeah, it returns a 200 response:

I see, thanks for confirming. Just out of curiosity, @jkakavas is there any valid reason to return different status codes for "unknown" api keys and access tokens during invalidation or we just didn't have chance to unify that yet?

It's the latter. I just opened elastic/elasticsearch#55671 to keep track of the work needed for the invalidate api keys API

azasypkin · 2020-04-21T14:19:39Z

x-pack/plugins/security/server/authentication/api_keys.ts

@@ -247,6 +276,20 @@ export class APIKeys {
    return result;
  }

+  private doesErrorIndicateAPIKeysAreDisabled(e: Record<string, any>) {
+    const responseText = e.response ?? '{}';


question: can't we just use e.body?.error?.['disabled.feature'] and eliminate try/catch here completely?

Ah, you are absolutely right! I don't know why I thought I had to manually parse response to get at this metadata 🤦

azasypkin · 2020-04-21T15:13:42Z

x-pack/plugins/security/server/routes/api_keys/privileges.test.ts

@@ -70,16 +109,21 @@ describe('Check API keys privileges', () => {

    const error = Boom.notAcceptable('test not acceptable message');
    getPrivilegesTest('returns error from cluster client', {
-      apiResponses: [
+      callAsCurrrentUserResponses: [


typo:

Suggested change

callAsCurrrentUserResponses: [

callAsCurrentUserResponses: [

gah! I hate my broken keyboard!

azasypkin · 2020-04-21T15:16:49Z

x-pack/plugins/security/server/routes/api_keys/privileges.test.ts

+            { body: { cluster: ['manage_security', 'manage_api_key', 'manage_own_api_key'] } },
+          ],
+        ],
+        callAsInternalUserAPIArguments: [


question: any reason you didn't want to mock ApiKeys instead to not expose internal impl detail (the fact that we use invalidate key under the hood)? I'm fine with keeping it as is, just curious what is main motivation here.

My only motivation here was to follow the pattern that already existed in this test suite. The changes started out small, but it might have been easier to rewrite in hindsight

Got it, makes sense to me.

azasypkin · 2020-04-21T15:25:55Z

x-pack/test/api_integration/config_security_basic.js

@@ -13,6 +13,7 @@ export default async function({ readConfigFile }) {
    config.esTestCluster.serverArgs = [
      'xpack.license.self_generated.type=basic',
      'xpack.security.enabled=true',
+      'xpack.security.authc.api_key.enabled=true',


note: it's too bad we don't have an easy way to reconfigure ES during tests and jest_integration tests are not available in xpack either. Ideally we'd like to check the case when keys are disabled so that we can catch the breaking changes in ES error format, but having a dedicated config just to test this is too much as well. No need to change/improve anything here, just grumbling about imperfect test system we live with.

I agree! I almost created a dedicated test suite for the disabled case, but like you mentioned, it's not worth the effort/overhead

…keys-enabled

legrego · 2020-04-23T15:47:45Z

@elasticmachine merge upstream

kibanamachine · 2020-04-23T17:35:21Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 5daaf30

History

💚 Build #40769 succeeded 91f468c
💔 Build #40732 failed 98bcdc4
💔 Build #40670 failed 9f33dab

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

* expose ability to check if API Keys are enabled * fix mock * Fix typo in test name * simplify key check * fix privilege check * remove unused variable * address PR feedback Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>

* expose ability to check if API Keys are enabled * fix mock * Fix typo in test name * simplify key check * fix privilege check * remove unused variable * address PR feedback Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com> Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>

* master: (70 commits) KQL removes leading zero and breaks query (elastic#62748) [FieldFormats] Cleanup: rename IFieldFormatType -> FieldFormatInstanceType (elastic#64193) [ML] Changes transforms wizard UI text (elastic#64150) [Alerting] change server log action type .log to .server-log in README (elastic#64124) [Metrics UI] Design Refresh: Inventory View, Episode 1 (elastic#64026) chore(NA): reduce siem bundle size using babel-plugin-transfor… (elastic#63269) chore(NA): use core-js instead of babel-polyfill on canvas sha… (elastic#63486) skip flaky suite (elastic#61173) skip flaky suite (elastic#62497) Renamed ilm policy for event log so it is not prefixed with dot (elastic#64262) [eslint] no_restricted_paths config cleanup (elastic#63741) Add Oil Rig Icon from @elastic/maki (elastic#64364) [Maps] Migrate Maps embeddables to NP (elastic#63976) [Ingest] Data streams list page (elastic#64134) chore(NA): add file-loader into jest moduleNameMapper (elastic#64330) [DOCS] Added images to automating report generation (elastic#64333) [SIEM][CASE] Api Integration Tests: Configuration (elastic#63948) Expose ability to check if API Keys are enabled (elastic#63454) [DOCS] Fixes formatting in alerting doc (elastic#64338) [data.search.aggs]: Create agg types function for terms agg. (elastic#63541) ...

legrego added 3 commits April 14, 2020 07:07

expose ability to check if API Keys are enabled

5b7695c

fix mock

ccdd46d

Fix typo in test name

4c93afc

legrego commented Apr 14, 2020

View reviewed changes

legrego added 3 commits April 14, 2020 08:31

simplify key check

2489634

Merge branch 'security/api-keys-enabled' of github.com:legrego/kibana…

71c88cf

… into security/api-keys-enabled

Merge branch 'master' of github.com:elastic/kibana into security/api-…

4ccbf44

…keys-enabled

elasticmachine and others added 4 commits April 14, 2020 10:39

Merge branch 'master' into security/api-keys-enabled

9f33dab

fix privilege check

6b5a98f

Merge branch 'security/api-keys-enabled' of github.com:legrego/kibana…

98bcdc4

… into security/api-keys-enabled

remove unused variable

91f468c

legrego added release_note:skip Skip the PR/issue when compiling release notes Team:Security Team focused on: Auth, Users, Roles, Spaces, Audit Logging, and more! v7.8.0 v8.0.0 labels Apr 14, 2020

legrego commented Apr 15, 2020

View reviewed changes

legrego marked this pull request as ready for review April 15, 2020 11:44

legrego requested a review from a team as a code owner April 15, 2020 11:44

legrego mentioned this pull request Apr 15, 2020

[Fleet] Enforce production mode #63637

Closed

azasypkin self-requested a review April 21, 2020 10:43

azasypkin approved these changes Apr 21, 2020

View reviewed changes

legrego added 2 commits April 23, 2020 07:42

Merge branch 'master' of github.com:elastic/kibana into security/api-…

1f78e8c

…keys-enabled

address PR feedback

6a65e6e

Merge branch 'master' into security/api-keys-enabled

5daaf30

legrego merged commit 44f9cbc into elastic:master Apr 23, 2020

legrego mentioned this pull request Apr 23, 2020

[7.x] Expose ability to check if API Keys are enabled (#63454) #64348

Merged

legrego deleted the security/api-keys-enabled branch April 23, 2020 19:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Expose ability to check if API Keys are enabled #63454

Expose ability to check if API Keys are enabled #63454

legrego commented Apr 14, 2020

legrego Apr 14, 2020

legrego Apr 14, 2020

legrego commented Apr 14, 2020

elasticmachine commented Apr 14, 2020

legrego Apr 15, 2020

azasypkin commented Apr 20, 2020

azasypkin left a comment

azasypkin Apr 21, 2020

legrego Apr 23, 2020

azasypkin Apr 21, 2020

azasypkin Apr 21, 2020

azasypkin Apr 21, 2020

legrego Apr 23, 2020

azasypkin Apr 23, 2020

jkakavas Apr 23, 2020

azasypkin Apr 21, 2020

legrego Apr 23, 2020

azasypkin Apr 21, 2020

legrego Apr 21, 2020

azasypkin Apr 21, 2020

legrego Apr 21, 2020

azasypkin Apr 23, 2020

azasypkin Apr 21, 2020

legrego Apr 21, 2020

legrego commented Apr 23, 2020

kibanamachine commented Apr 23, 2020

	values: { message: _.get(e, 'body.message', '') },
	values: { message: e.body?.message \|\| '' },

	export function defineGetApiKeysRoutes({ router, clusterClient, authc }: RouteDefinitionParams) {
	export function defineGetApiKeysRoutes({ router, clusterClient }: RouteDefinitionParams) {

Expose ability to check if API Keys are enabled #63454

Expose ability to check if API Keys are enabled #63454

Conversation

legrego commented Apr 14, 2020

Summary

Choose a reason for hiding this comment

Choose a reason for hiding this comment

legrego commented Apr 14, 2020

elasticmachine commented Apr 14, 2020

Choose a reason for hiding this comment

azasypkin commented Apr 20, 2020

azasypkin 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

legrego commented Apr 23, 2020

kibanamachine commented Apr 23, 2020

💚 Build Succeeded

History