Adding `authc.grantAPIKeyAsInternalUser` #60423

kobelb · 2020-03-17T17:50:48Z

I tried to find a way to prevent having to parse KibanaRequest::headers to use Elasticsearch's grant API Key API, but I was unable to find a way without breaking support for clients like cURL. So instead, I've attempted to parse the Authorization header in the "nicest way possible".

The kibana_system role doesn't have the recently added grant_api cluster privilege , so for the time being I've been creating a new role and creating a new user with the kibana_system and the new role:

### Create grant_api_key role
POST {{es}}/_xpack/security/role/grant_api_key
Content-Type: application/json
Authorization: Basic elastic changeme

{
  "cluster": ["cluster:admin/xpack/security/api_key/grant"]
}

### Create kibana_system user
PUT {{es}}/_xpack/security/user/kibana_system
Content-Type: application/json
Authorization: Basic elastic changeme

{
  "password" : "changeme",
  "roles" : [ "kibana_system", "grant_api_key" ],
  "full_name" : "Kibana"
}

And then changing my elasticsearch.username to kibana_system.

…ials

kobelb · 2020-03-18T00:21:02Z

x-pack/plugins/security/server/authentication/http_authentication/http_authorization_header.ts

+    const [scheme] = authorizationHeaderValue.split(/\s+/);
+    const credentials = authorizationHeaderValue.substring(scheme.length + 1);
+
+    return new HTTPAuthorizationHeader(scheme, credentials);


With the old implementation of getHTTPAuthenticationScheme, .toLowerCase() was being called when initially parsing the scheme from the request's headers. This felt inconsistent when I began to use the HTTPAuthorizationHeader class and .toString() to create the Authorization headers themselves. It has the effect of requiring consumers to call .toLowerCase(), or instead use .localeCompare, to treat this as being case insensitive.

@azasypkin I'm interested in how you feel about this.

It's fine by me as long as we mention this in the property JS docs. I'm curious if there is any reason we cannot do toLowerCase() for both scheme and credentials in HTTPAuthorizationHeader constructor though?

We definitely can do this in the constructor. When we were previously building the Authorization header manually within the auth providers, we were using a capitalized version, for example: Bearer ${accessToken}. Changing this behavior for the sake of making matching the scheme easier felt wrong. However, I don't want to be introducing future bugs by being pedantic... I wish there was CaseInsensitiveString in JavaScript which overrode equals, etc. but AFAIK there is no such thing, or no ability to create one ourselves which would work with things like a Map and a Set.

Changing this behavior for the sake of making matching the scheme easier felt wrong. However, I don't want to be introducing future bugs by being pedantic...

Yeah, I agree. We use and compare schemes just in a couple of places, I think the risk of accidentally breaking something in this area may be negligible.

x-pack/plugins/security/server/authentication/http_authentication/http_authorization_header.ts

…ion/http_authorization_header.ts Co-Authored-By: Aleh Zasypkin <[email protected]>

kobelb · 2020-03-18T14:17:41Z

@tvernum I've been able to make the current ES Grant API key endpoint work, but it requires that Kibana parse the Authorization header to determine the grant_type and extract the access_token or username/password. For a majority of the auth provider flows, this approach wouldn't be absolutely necessary. However, we allow users/systems to authenticate using the Authorization header directly against Kibana HTTP endpoints and we don't want to constrain their ability to grant API Keys in these scenarios.

mikecote

Tested this locally. I changed some alerting code to make the alerting APIs use this new API and everything worked based on @kobelb temporary workaround. I tested with a user who didn't have any access to API keys and Kibana allowed the creation of alerts. Tested the API key the alert created to ensure it's usable, etc and it worked! LGTMike 👍

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

mikecote · 2020-03-19T18:07:27Z

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

+   * Tries to grant an API key for the current user.
+   * @param request Request instance.
+   */
+  async grant(request: KibanaRequest): Promise<GrantAPIKeyResult | null> {


We chatted a bit about the function naming. Did you decide to keep as is or go with something like grantAsInternalUser?

azasypkin · 2020-03-20T13:19:49Z

ACK: will review today

kobelb · 2020-03-20T13:59:36Z

@elasticmachine merge upstream

azasypkin

Looks great, thanks! Tested locally with both SAML and Basic.

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

...plugins/security/server/authentication/http_authentication/http_authorization_header.test.ts

...ity/server/authentication/http_authentication/basic_http_authorization_header_credentials.ts

azasypkin · 2020-03-20T17:47:07Z

x-pack/plugins/security/server/authentication/providers/basic.ts

@@ -54,7 +57,10 @@ export class BasicAuthenticationProvider extends BaseAuthenticationProvider {
    this.logger.debug('Trying to perform a login.');

    const authHeaders = {
-      authorization: `Basic ${Buffer.from(`${username}:${password}`).toString('base64')}`,
+      authorization: new HTTPAuthorizationHeader(


note: would you mind also updating https://github.com/elastic/kibana/blob/master/x-pack/plugins/security/server/routes/users/change_password.ts#L46 to use HTTPAuthorizationHeader?

You got it!

azasypkin · 2020-03-20T18:03:45Z

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

+interface GrantAPIKeyParams {
+  grant_type: 'password' | 'access_token';
+  username?: string;
+  password?: string;
+  access_token?: string;
+}


optional nit: how do you feel about something like this instead?

Suggested change

interface GrantAPIKeyParams {

grant_type: 'password' | 'access_token';

username?: string;

password?: string;

access_token?: string;

}

type GrantAPIKeyParams =

| { grant_type: 'password'; username: string; password: string }

| { grant_type: 'access_token'; access_token: string };

question: hmm I'm surprised that Elasticsearch doesn't allow client to specify Api Key name like it does in create API key API. Do you know why? Just out of curiosity.

I don't, unfortunately. It also doesn't allow you to specify a role descriptor. It's not blocking Alerting's usage, so I :ostrich_head_in_sand:'ed it.

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

azasypkin · 2020-03-20T18:14:25Z

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

+      result = (await this.clusterClient
+        .asScoped(request)
+        .callAsInternalUser('shield.grantAPIKey', { body: params })) as GrantAPIKeyResult;


question: I guess we don't need asScoped here (as well as as GrantAPIKeyResult)?

Suggested change

result = (await this.clusterClient

.asScoped(request)

.callAsInternalUser('shield.grantAPIKey', { body: params })) as GrantAPIKeyResult;

result = await this.clusterClient.callAsInternalUser('shield.grantAPIKey', { body: params });

The conceptual equivalent to an asScoped "grant" is just createAPIKey. It's safe for us to always allow end-users to use the create API Key API in Elasticsearch and ES will enforce all of the necessary authorization for this to be safe. The ability to grant an API Key in this manner is a new "two credentials API" where the Kibana server should only be able to perform the operation when it has the user's credentials, and the result of the operation should be safe-guarded...

Yeah, but I what I meant is that you're using callAsInternalUser and callAsInternalUser on ScopedClusterClient is exactly the same as on non-scoped ClusterClient and doesn't use request headers you scoped client with. So unless I'm missing something clusterClient.asScoped(request).callAsInternalUser is a bit less performant equivalent of just clusterClient.callAsInternalUser.

I'm with you now, apologies for being dense. Fix forthcoming.

azasypkin · 2020-03-20T18:25:17Z

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

+    }
+
+    this.logger.debug('Trying to grant an API key');
+    const authorizationHeader = HTTPAuthorizationHeader.parseFromRequest(request);


note: I'm not super happy that we should do that, but I can't think of a better solution for the time being as well. So +1.

Just for the records: my main concern is that the fact that we have Authorization header within request.headers is just "temporary" backward compatibility escape hatch for the places that still rely on legacy requests. The idea was to store authHeaders returned from auth hook internally in the core so that it can add them automatically whenever authenticated requested is relayed to ES. And nothing else would have access to them.

However we (Security) register and own this hook, provide auth headers and hence should have an exclusive right to access these headers whenever we need them (e.g. via Core "auth headers accessor" returned as part of the auth hook registration). So even if Core decides to no longer populate request.headers with Authorization we'll still have access to them via some API. Migrating to this API will be an implementation detail that won't change public API we expose unless I'm missing something.

What you've said makes sense. I'm also open to alternatives which require some changes to the authentication providers. I began exploring what it'd look like to allow the auth providers to store the parameters we need to grant an API key in a WeakMap<KibanaRequest, GrantAPIKeyParams> that only the security plugin had access to, but then came on the necessity to parse the Authorization header within the http authentication provider and decided to just do the header parsing logic here.

Yeah, I think what you have now is the best option for the time being. Parsing headers because of http is where I ended up as well when we had initial discussion on that requirement.

It feels we're still missing some pieces either in Security plugin or in the Core that would have allowed us to come with a more future-proof approach, but I'm sure we'll get there and reuse the work you've done here anyway.

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

Co-Authored-By: Aleh Zasypkin <[email protected]> Co-Authored-By: Mike Côté <[email protected]>

kibanamachine · 2020-03-23T15:54:17Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 15defd3

History

💚 Build #35117 succeeded 161d6cf
💚 Build #34982 succeeded dc6c511
💔 Build #34896 failed abab5e2
💔 Build #34772 failed 7d6a76e
💔 Build #34660 failed 8a5f23c

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

* master: [Uptime] Skip failing location test temporarily (elastic#60938) [ML] Disabling datafeed editing when job is running (elastic#60751) Adding `authc.invalidateAPIKeyAsInternalUser` (elastic#60717) [SIEM] Add license check to ML Rule form (elastic#60691) Adding `authc.grantAPIKeyAsInternalUser` (elastic#60423) Support Histogram Data Type (elastic#59387) [Upgrade Assistant] Fix edge case where reindex op can falsely be seen as stale (elastic#60770) [SIEM] [Cases] Update case icons (elastic#60812) [TSVB] Fix percentiles band mode (elastic#60741)

* Parsing the Authorization HTTP header to grant API keys * Using HTTPAuthorizationHeader and BasicHTTPAuthorizationHeaderCredentials * Adding tests for grantAPIKey * Adding http_authentication/ folder * Removing test route * Using new classes to create the headers we pass to ES * No longer .toLowerCase() when parsing the scheme from the request * Updating snapshots * Update x-pack/plugins/security/server/authentication/http_authentication/http_authorization_header.ts Co-Authored-By: Aleh Zasypkin <[email protected]> * Updating another inline snapshot * Adding JSDoc * Renaming `grant` to `grantAsInternalUser` * Adding forgotten test. Fixing snapshot * Fixing mock * Apply suggestions from code review Co-Authored-By: Aleh Zasypkin <[email protected]> Co-Authored-By: Mike Côté <[email protected]> * Using new classes for changing password * Removing unneeded asScoped call Co-authored-by: Aleh Zasypkin <[email protected]> Co-authored-by: Elastic Machine <[email protected]> Co-authored-by: Mike Côté <[email protected]> Co-authored-by: Aleh Zasypkin <[email protected]> Co-authored-by: Elastic Machine <[email protected]> Co-authored-by: Mike Côté <[email protected]>

* master: (26 commits) [Alerting] Fixes flaky test in Alert Instances Details page (elastic#60893) cleanup visualizations api (elastic#59958) Inline timezoneProvider function, remove ui/vis/lib/timezone (elastic#60475) [SIEM] Adds 'Open one signal' Cypress test (elastic#60484) [UA] Upgrade assistant migration meta data can become stale (elastic#60789) [Metrics Alerts] Remove metric field from doc count on backend (elastic#60679) [Uptime] Skip failing location test temporarily (elastic#60938) [ML] Disabling datafeed editing when job is running (elastic#60751) Adding `authc.invalidateAPIKeyAsInternalUser` (elastic#60717) [SIEM] Add license check to ML Rule form (elastic#60691) Adding `authc.grantAPIKeyAsInternalUser` (elastic#60423) Support Histogram Data Type (elastic#59387) [Upgrade Assistant] Fix edge case where reindex op can falsely be seen as stale (elastic#60770) [SIEM] [Cases] Update case icons (elastic#60812) [TSVB] Fix percentiles band mode (elastic#60741) Fix formatter on range aggregation (elastic#58651) Goodbye, legacy data plugin 👋 (elastic#60449) [Metrics UI] Alerting for metrics explorer and inventory (elastic#58779) [Remote clustersadopt changes to remote info API (elastic#60795) Only run xpack siem cypress in PRs when there are siem changes (elastic#60661) ...

kobelb added 7 commits March 17, 2020 10:48

Parsing the Authorization HTTP header to grant API keys

cfc288d

Using HTTPAuthorizationHeader and BasicHTTPAuthorizationHeaderCredent…

c3146b5

…ials

Adding tests for grantAPIKey

bb1d2bd

Adding http_authentication/ folder

e387b59

Removing test route

4e0c33f

Using new classes to create the headers we pass to ES

54401f3

No longer .toLowerCase() when parsing the scheme from the request

4b3fe5e

kobelb commented Mar 18, 2020

View reviewed changes

kobelb added 3 commits March 17, 2020 17:22

Merge remote-tracking branch 'upstream/master' into grant-api-key-2

56c175e

Updating snapshots

7ae9690

Merge remote-tracking branch 'upstream/master' into grant-api-key-2

712c3b4

azasypkin reviewed Mar 18, 2020

View reviewed changes

x-pack/plugins/security/server/authentication/http_authentication/http_authorization_header.ts Outdated Show resolved Hide resolved

kobelb and others added 3 commits March 18, 2020 06:42

Update x-pack/plugins/security/server/authentication/http_authenticat…

6c584f1

…ion/http_authorization_header.ts Co-Authored-By: Aleh Zasypkin <[email protected]>

Updating another inline snapshot

5f1309d

Adding JSDoc

88b2039

kobelb marked this pull request as ready for review March 18, 2020 18:10

kobelb requested a review from a team as a code owner March 18, 2020 18:10

kobelb requested a review from mikecote March 18, 2020 18:10

kobelb added release_note:skip Skip the PR/issue when compiling release notes v8.0.0 v7.7.0 labels Mar 18, 2020

mikecote approved these changes Mar 19, 2020

View reviewed changes

peterschretlen mentioned this pull request Mar 19, 2020

Allow kibana_system to create and invalidate API keys on behalf of other users elastic/elasticsearch#53824

Merged

Renaming grant to grantAsInternalUser

8a5f23c

mikecote mentioned this pull request Mar 20, 2020

Adding authc.invalidateAPIKeyAsInternalUser #60717

Merged

Adding forgotten test. Fixing snapshot

7d6a76e

azasypkin self-requested a review March 20, 2020 13:19

elasticmachine and others added 3 commits March 20, 2020 09:59

Merge branch 'master' into grant-api-key-2

abab5e2

Merge remote-tracking branch 'upstream/master' into grant-api-key-2

9555a8f

Fixing mock

dc6c511

kobelb changed the title ~~Adding authc.grantAPIKey~~ Adding authc.grantAPIKeyAsInternalUser Mar 20, 2020

azasypkin approved these changes Mar 20, 2020

View reviewed changes

kobelb and others added 4 commits March 20, 2020 15:27

Apply suggestions from code review

f14fda0

Co-Authored-By: Aleh Zasypkin <[email protected]> Co-Authored-By: Mike Côté <[email protected]>

Using new classes for changing password

161d6cf

Removing unneeded asScoped call

13001ca

Merge remote-tracking branch 'upstream/master' into grant-api-key-2

15defd3

kobelb merged commit cca23c2 into elastic:master Mar 23, 2020

kobelb deleted the grant-api-key-2 branch March 23, 2020 16:03

kobelb mentioned this pull request Mar 23, 2020

[7.x] Adding authc.grantAPIKeyAsInternalUser (#60423) #60927

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding `authc.grantAPIKeyAsInternalUser` #60423

Adding `authc.grantAPIKeyAsInternalUser` #60423

kobelb commented Mar 17, 2020 •

edited

Loading

kobelb Mar 18, 2020

azasypkin Mar 18, 2020

kobelb Mar 18, 2020 •

edited

Loading

azasypkin Mar 18, 2020

kobelb commented Mar 18, 2020

mikecote left a comment

mikecote Mar 19, 2020

azasypkin commented Mar 20, 2020

kobelb commented Mar 20, 2020

azasypkin left a comment

azasypkin Mar 20, 2020

kobelb Mar 20, 2020

azasypkin Mar 20, 2020

azasypkin Mar 20, 2020

kobelb Mar 20, 2020

azasypkin Mar 20, 2020

kobelb Mar 20, 2020

azasypkin Mar 23, 2020

kobelb Mar 23, 2020

azasypkin Mar 20, 2020

kobelb Mar 20, 2020 •

edited

Loading

azasypkin Mar 23, 2020

kibanamachine commented Mar 23, 2020

Adding authc.grantAPIKeyAsInternalUser #60423

Adding authc.grantAPIKeyAsInternalUser #60423

Conversation

kobelb commented Mar 17, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kobelb Mar 18, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kobelb commented Mar 18, 2020

mikecote left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

azasypkin commented Mar 20, 2020

kobelb commented Mar 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

kobelb Mar 20, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kibanamachine commented Mar 23, 2020

💚 Build Succeeded

History

Adding `authc.grantAPIKeyAsInternalUser` #60423

Adding `authc.grantAPIKeyAsInternalUser` #60423

kobelb commented Mar 17, 2020 •

edited

Loading

kobelb Mar 18, 2020 •

edited

Loading

kobelb Mar 20, 2020 •

edited

Loading