Restrict access to hapi Request in registerAuth #38763

mshustov · 2019-06-12T08:18:38Z

Summary

In New Platform only a plugin calling registerAuth has read-access to the authorization header.
This plugin can return headers that should be used to perform a request against Elasticsearch on behalf of end-user. Those headers are stored in the core and retrieved cluster client to perform a request.
If cluster client cannot find headers returned from registerAuth, it will fallback to an incoming request headers.

Checklist

Use ~~strikethroughs~~ to remove checklist items you don't feel are applicable to this PR.

~~[ ] This was checked for cross-browser compatibility, including a check against IE11~~
~~[ ] Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support~~
~~[ ] Documentation was added for features that require explanation or tutorials~~
Unit or functional tests were updated or added to match the most common scenarios
~~[ ] This was checked for keyboard-only and screenreader accessibility~~

For maintainers

This was checked for breaking API changes and was labeled appropriately
This includes a feature addition or change that requires a release note and was labeled appropriately

Dev docs

In the previous release, we introduced experimental registerAuth hook in New Platform that allows end users to define custom authentication logic.
We changed API of the AuthenticationHandler function.
In 7.2:

export type AuthenticationHandler<T> = (
  request: Request,
  sessionStorage: SessionStorage<T>,
  t: AuthToolkit
) => Promise<AuthResult>;

In 7.3

export type AuthenticationHandler = (
  request: KibanaRequest,
  t: AuthToolkit
) => AuthResult | Promise<AuthResult>;

We provide KibanaRequest instead of hapi Request object and remove sessionStorage from arguments. Now sessionStorageFactory is returned as result of registerAuth call.
Note: as API is experimental is still a subject for further changes.

elasticmachine · 2019-06-12T15:20:43Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

mshustov · 2019-06-13T08:12:10Z

src/core/server/elasticsearch/cluster_client.ts

+      this.config.requestHeadersWhitelist
+    );
+
+    return { ...headers, ...authHeaders };


if there are no authHeaders returned from registerAuth or registerAuth wasn't called, we will use headers from the incoming request.

mshustov · 2019-06-13T08:25:52Z

src/core/server/http/router/request.ts

@@ -37,6 +37,7 @@ export interface KibanaRequestRoute {
  options: Required<RouteConfigOptions>;
 }

+const securedHeaders = ['authorization'];


authHeaders is a map and we don't know its keys. @elastic/kibana-security we can make this value configurable when needed.

question: secured can give a false sense of security especially when it's used in OSS or when security plugin is disabled, just sayin' 🙂 What does secured mean here?

Headers aren't exposed by getFilteredHeaders method. We can call it filteredHeaders but it doesn't show an intention and doesn't answer why. 🤔

How about secretHeaders

works for me

mshustov · 2019-06-13T08:28:53Z

src/core/server/http/router/request.ts

    this.url = request.url;

-    this[requestSymbol] = request;
+    // prevent Symbol exposure via Object.getOwnPropertySymbols()
+    Object.defineProperty(this, requestSymbol, {


Probably, we should make KibanaRequest an object literal created by a factory instead of the class to emulate private fields via closures. Also, the class exposes static methods are meant to be used only inside of the core.

That sounds like a smart move to me, can be done in future PR.

mshustov · 2019-06-13T08:38:31Z

src/core/server/legacy/legacy_service.ts

+        const { req, res } = request.raw;
+        const authHeaders = setupDeps.core.http.auth.getAuthHeaders(request);
+        if (authHeaders) {
+          // some plugins in Legacy relay on headers.authorization presence


for example https://github.com/restrry/kibana/blob/e751da3980f7de83a7248b080a3b876528b7e08c/x-pack/plugins/beats_management/server/lib/adapters/framework/hapi_framework_adapter.ts#L125

mshustov · 2019-06-13T08:41:49Z

src/core/server/elasticsearch/elasticsearch_service.ts

@@ -103,7 +109,7 @@ export class ElasticsearchService implements CoreService<ElasticsearchServiceSet
      dataClient$: clients$.pipe(map(clients => clients.dataClient)),

      createClient: (type: string, clientConfig: ElasticsearchClientConfig) => {
-        return this.createClusterClient(type, clientConfig);
+        return this.createClusterClient(type, clientConfig, deps.http.auth.getAuthHeaders);


should we extend all external cluster clients with this functionality or we should make it configurable?

The only example I can contrive where we may want to call asScoped on a ClusterClient with a KibanaRequest and not get the auth from the internal HTTP server is the proxy plugin which has it's own HTTPServer. That said, that plugin is not planning to use header/credential forwarding, but it may make the createNewServer API behave strangely to 3rd party consumers.

agree I want to extract all Kibana specific logic from HttpServer to HttpService in #38092

mshustov · 2019-06-13T12:17:57Z

src/core/server/elasticsearch/cluster_client.ts

   */
-  public asScoped(req: { headers?: Headers } = {}) {
+  public asScoped(request?: KibanaRequest | Request | FakeRequest) {


for now, Security can use FakeRequest to perform a request to shield.authenticate.
also we can move all logic related to headers calculation to http_service. then cluster_client just call

return new ScopedClusterClient( this.callAsInternalUser, this.callAsCurrentUser, this.getHeaders(request, this.config.requestHeadersWhitelist) );

what do you think?

should we use Legacy.Request instead of Request to emphasise it is a deprecated api?

request is optional request? for BWC with the current code. what is the case when cluster.asScoped(undefined) and ``cluster.asScoped({headers: undefined})` are valid?

should we use Legacy.Request instead of Request to emphasise it is a deprecated api?

That makes sense to me 👍

request is optional request? for BWC with the current code. what is the case when
cluster.asScoped(undefined) and ``cluster.asScoped({headers: undefined})` are valid?

Not sure I understand this issue. Both of those cases seem like they should have the same behavior. Am I missing something?

Not sure I understand this issue. Both of those cases seem like they should have the same behavior. Am I missing something?

ok, re-phrase: is it a valid case to perform a request to ES when authorization header is not set? I thought we always require authentication creating default elasticuser. don't we?

Yeah I'm not sure why request would ever be undefined, but the headers could be empty when Security is disabled or when using OSS Kibana. I believe that case would look more like cluster.asScoped({headers: {}}) (no Authorization header present).

elasticmachine · 2019-06-13T13:22:17Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

elasticmachine · 2019-06-13T14:34:06Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

Prevent exposing headers.authorization in KibanaRequest. Introduce a mechanism to associate authorization headers with an incoming request and retrieve its value to perform a request to elasticsearch cluster.

mshustov · 2019-06-14T10:13:04Z

src/core/server/http/http_server.ts

-      authenticate: adoptToHapiAuthFormat(fn, this.authState.set),
+      authenticate: adoptToHapiAuthFormat(fn, (req, { state, headers }) => {
+        this.authState.set(req, state);
+        this.authHeaders.set(req, headers);


I used 2 different storages because they have different use cases

the state may be exposed to any downstream plugin

headers are meant to be used only by core when call elasticsearch cluster

mshustov · 2019-06-14T10:18:06Z

src/core/server/elasticsearch/cluster_client.ts

+  private getHeaders(
+    request?: KibanaRequest | Request | FakeRequest
+  ): Record<string, string | string[] | undefined> {
+    if (!isRealRequest(request)) {


when migrate from Legacy to New platform, we don't need to support Request as a valid input and can switch to

if(isKibanaRequest(request)){ const authHeaders = this.getAuthHeaders(request); const headers = toRawRequest(request).headers; return { ...headers, ...authHeaders }; } else { return request.headers; }

src/core/server/elasticsearch/cluster_client.ts

elasticmachine · 2019-06-14T10:29:11Z

Pinging @elastic/kibana-platform

elasticmachine · 2019-06-14T10:29:14Z

Pinging @elastic/kibana-security

elasticmachine · 2019-06-14T11:32:30Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

elasticmachine · 2019-06-14T12:54:58Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

src/core/server/http/cookie_sesson_storage.test.ts

src/core/server/elasticsearch/cluster_client.ts

joshdover · 2019-06-14T22:27:19Z

src/core/server/elasticsearch/cluster_client.ts

   */
-  public asScoped(req: { headers?: Headers } = {}) {
+  public asScoped(request?: KibanaRequest | Request | FakeRequest) {


should we use Legacy.Request instead of Request to emphasise it is a deprecated api?

That makes sense to me 👍

request is optional request? for BWC with the current code. what is the case when
cluster.asScoped(undefined) and ``cluster.asScoped({headers: undefined})` are valid?

Not sure I understand this issue. Both of those cases seem like they should have the same behavior. Am I missing something?

src/core/server/elasticsearch/cluster_client.ts

joshdover · 2019-06-14T22:35:33Z

src/core/server/elasticsearch/elasticsearch_service.ts

@@ -103,7 +109,7 @@ export class ElasticsearchService implements CoreService<ElasticsearchServiceSet
      dataClient$: clients$.pipe(map(clients => clients.dataClient)),

      createClient: (type: string, clientConfig: ElasticsearchClientConfig) => {
-        return this.createClusterClient(type, clientConfig);
+        return this.createClusterClient(type, clientConfig, deps.http.auth.getAuthHeaders);


The only example I can contrive where we may want to call asScoped on a ClusterClient with a KibanaRequest and not get the auth from the internal HTTP server is the proxy plugin which has it's own HTTPServer. That said, that plugin is not planning to use header/credential forwarding, but it may make the createNewServer API behave strangely to 3rd party consumers.

joshdover · 2019-06-14T22:36:56Z

src/core/server/http/router/request.ts

@@ -37,6 +37,7 @@ export interface KibanaRequestRoute {
  options: Required<RouteConfigOptions>;
 }

+const securedHeaders = ['authorization'];


How about secretHeaders

joshdover · 2019-06-14T22:38:15Z

src/core/server/http/router/request.ts

    this.url = request.url;

-    this[requestSymbol] = request;
+    // prevent Symbol exposure via Object.getOwnPropertySymbols()
+    Object.defineProperty(this, requestSymbol, {


That sounds like a smart move to me, can be done in future PR.

elasticmachine · 2019-06-17T15:27:57Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

elasticmachine · 2019-06-17T16:43:42Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

@joshdover

* Prevent exposing Hapi.Request to registerAuth. Prevent exposing headers.authorization in KibanaRequest. Introduce a mechanism to associate authorization headers with an incoming request and retrieve its value to perform a request to elasticsearch cluster. * fix tests * address @joshdover comments

@joshdover

* Prevent exposing Hapi.Request to registerAuth. Prevent exposing headers.authorization in KibanaRequest. Introduce a mechanism to associate authorization headers with an incoming request and retrieve its value to perform a request to elasticsearch cluster. * fix tests * address @joshdover comments

rayafratkina · 2019-07-12T01:08:51Z

@restrry can you please add a Dev Doc section to this PR for API changes document?

mshustov · 2019-07-15T13:37:00Z

@rayafratkina done

elasticmachine · 2019-07-15T14:39:47Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

mshustov force-pushed the issue-33775-restrict-access-to-hapi-request branch 3 times, most recently from e32deae to b44d3db Compare June 12, 2019 14:26

elastic deleted a comment from elasticmachine Jun 12, 2019

mshustov force-pushed the issue-33775-restrict-access-to-hapi-request branch 2 times, most recently from a2f41d8 to d1849b2 Compare June 13, 2019 08:03

elastic deleted a comment from elasticmachine Jun 13, 2019

mshustov commented Jun 13, 2019

View reviewed changes

elastic deleted a comment from elasticmachine Jun 13, 2019

mshustov commented Jun 13, 2019

View reviewed changes

Prevent exposing Hapi.Request to registerAuth.

090fdde

Prevent exposing headers.authorization in KibanaRequest. Introduce a mechanism to associate authorization headers with an incoming request and retrieve its value to perform a request to elasticsearch cluster.

mshustov force-pushed the issue-33775-restrict-access-to-hapi-request branch from ddbdafe to 090fdde Compare June 14, 2019 10:12

mshustov commented Jun 14, 2019

View reviewed changes

src/core/server/elasticsearch/cluster_client.ts Outdated Show resolved Hide resolved

mshustov added the v7.3.0 label Jun 14, 2019

mshustov marked this pull request as ready for review June 14, 2019 10:29

mshustov requested a review from a team as a code owner June 14, 2019 10:29

Merge branch 'master' into issue-33775-restrict-access-to-hapi-request

7cf45df

fix tests

4b30212

mshustov mentioned this pull request Jun 14, 2019

[New platform] Security plugin integration #33775

Closed

34 tasks

mshustov requested a review from azasypkin June 14, 2019 16:31

joshdover reviewed Jun 14, 2019

View reviewed changes

mshustov added 2 commits June 17, 2019 15:16

Merge branch 'master' into issue-33775-restrict-access-to-hapi-request

104cd0b

address @joshdover comments

bef4266

mshustov requested a review from joshdover June 17, 2019 14:30

joshdover approved these changes Jun 17, 2019

View reviewed changes

mshustov merged commit 9e044c5 into elastic:master Jun 18, 2019

mshustov deleted the issue-33775-restrict-access-to-hapi-request branch June 18, 2019 08:21

mshustov mentioned this pull request Jun 18, 2019

[7.x] Restrict access to hapi Request in registerAuth (#38763) #39151

Merged

mshustov removed the review label Jun 25, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Restrict access to hapi Request in registerAuth #38763

Restrict access to hapi Request in registerAuth #38763

mshustov commented Jun 12, 2019 •

edited

Loading

elasticmachine commented Jun 12, 2019

mshustov Jun 13, 2019

mshustov Jun 13, 2019

azasypkin Jun 13, 2019

mshustov Jun 14, 2019

joshdover Jun 14, 2019

mshustov Jun 17, 2019

mshustov Jun 13, 2019 •

edited

Loading

joshdover Jun 14, 2019

mshustov Jun 13, 2019

mshustov Jun 13, 2019

joshdover Jun 14, 2019

mshustov Jun 17, 2019

mshustov Jun 13, 2019 •

edited

Loading

joshdover Jun 14, 2019 •

edited

Loading

mshustov Jun 17, 2019

joshdover Jun 17, 2019

elasticmachine commented Jun 13, 2019

elasticmachine commented Jun 13, 2019

mshustov Jun 14, 2019

mshustov Jun 14, 2019

elasticmachine commented Jun 14, 2019

elasticmachine commented Jun 14, 2019

elasticmachine commented Jun 14, 2019

elasticmachine commented Jun 14, 2019

joshdover Jun 14, 2019 •

edited

Loading

joshdover Jun 14, 2019

joshdover Jun 14, 2019

joshdover Jun 14, 2019

elasticmachine commented Jun 17, 2019

elasticmachine commented Jun 17, 2019

rayafratkina commented Jul 12, 2019

mshustov commented Jul 15, 2019

elasticmachine commented Jul 15, 2019

Restrict access to hapi Request in registerAuth #38763

Restrict access to hapi Request in registerAuth #38763

Conversation

mshustov commented Jun 12, 2019 • edited Loading

Summary

Checklist

For maintainers

Dev docs

elasticmachine commented Jun 12, 2019

💔 Build Failed

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

mshustov Jun 13, 2019 • edited Loading

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

mshustov Jun 13, 2019 • edited Loading

Choose a reason for hiding this comment

joshdover Jun 14, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elasticmachine commented Jun 13, 2019

💚 Build Succeeded

elasticmachine commented Jun 13, 2019

💚 Build Succeeded

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elasticmachine commented Jun 14, 2019

elasticmachine commented Jun 14, 2019

elasticmachine commented Jun 14, 2019

💔 Build Failed

elasticmachine commented Jun 14, 2019

💚 Build Succeeded

joshdover Jun 14, 2019 • edited Loading

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 Jun 17, 2019

💔 Build Failed

elasticmachine commented Jun 17, 2019

💚 Build Succeeded

rayafratkina commented Jul 12, 2019

mshustov commented Jul 15, 2019

elasticmachine commented Jul 15, 2019

💔 Build Failed

mshustov commented Jun 12, 2019 •

edited

Loading

mshustov Jun 13, 2019 •

edited

Loading

mshustov Jun 13, 2019 •

edited

Loading

joshdover Jun 14, 2019 •

edited

Loading

joshdover Jun 14, 2019 •

edited

Loading