[New platform] HTTP & Security integration #34631

mshustov · 2019-04-05T14:21:24Z

Summary

Parent issue: #33775
Short recap of the issue:

Auth interceptor

Provide Authenticate hook for Security plugin to register incoming requests intercepter.
Request intercepter is meant to be registered only once.
Security plugin performs user authentication and authorization checks for incoming requests.
Authenticate may finish Auth operation with one of next outcomes:

authenticated() - to allow request to pass through
redirected() - to interrupt request handling and redirect to configured url
rejected() - to fail the request with specified error.

Third party plugins should have an access to (optional) Security plugin to register their access to authorization information.

Request interceptor

Provide onRequest hook for plugins to register custom logic for incoming requests. Request interceptor doesn't have access to hapi Request object, but KibanaRequest
onRequest may finish Auth operation with one of next outcomes:

next() - to pass request to the next handler
redirected() - to interrupt request handling and redirect to configured url
rejected() - to fail the request with specified error.

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

elasticmachine · 2019-04-05T15:04:16Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

mshustov · 2019-04-05T14:25:29Z

src/core/server/http/http_server.ts

+    this.server.ext('onRequest', adoptToHapiOnRequestFormat(fn));
+  };
+
+  private registerAuth = async <T>(fn: Authenticate<T>, cookieOptions: CookieOptions) => {


NOTE: right now security config defines CookieOptions values but doesn't specify session storage implementation as cookie-storage. Seems to be okay if we don't want to add premature abstraction, later we can change API

interface CookieSessionStorage { type: 'cookie' options: CookieOptions } interface RedisSessionStorage { type: 'redis' options: ... } type SessionStorageParams = CookieSessionStorage | RedisSessionStorage | ... registerAuth (fn: Authenticate<T>, sessionStorageParams: SessionStorageParams){}

are we going to support this extension point compatibility for external users?

mshustov · 2019-04-05T14:37:03Z

src/core/server/http/lifecycle/auth.ts

+    h: ResponseToolkit
+  ): Promise<Lifecycle.ReturnValue> {
+    try {
+      const result = await fn(req, sessionStorage.asScoped(req), toolkit);


note it does not support returning an error nor throwing an error from authenticate function

mshustov · 2019-04-05T14:59:12Z

src/core/server/http/integration_tests/http_service.test.ts

+          res.ok({ content: 'ok' })
+        );
+        // TODO fix me when registerRouter is available before HTTP server is run
+        (root as any).server.http.registerRouter(router);


note right now we don't have a separation between setup and start phase, consequently, we don't provide any way to add route handler in userland. That seems to be a blocker because even Security cannot add their route handlers to perform login. we have to address that in following PRs

With the work going on in #18843, I think we'll be able to get the legacy code on the frontend more compatible with the lifecycle events in the new platform sooner than later. This would unblock adding back the start event.

I believe it's fine to introduce start only on the server side for the time being. That can be implemented without leaking lifecycle semantic to plugins:

the server runs setup for core services and plugins.

the server runs start for core services only (until [browser] Application service #18843 is ready).

the server runs legacy server.

mshustov · 2019-04-05T15:04:39Z

src/core/server/http/http_server.ts

@@ -127,4 +137,31 @@ export class HttpServer {
    const routePathStartIndex = routerPath.endsWith('/') && routePath.startsWith('/') ? 1 : 0;
    return `${routerPath}${routePath.slice(routePathStartIndex)}`;
  }
+
+  private registerOnRequest = (fn: OnRequest) => {


not directly related to Security integration directly, but interesting to check consistency in registerOnRequest and registerAuth API

Any reason this is defined as an arrow function instead of a class method?

this function is exposed as a part of setup contract. Otherwise, we either have to bind it in setup method or expose an unbound version.

mshustov · 2019-04-05T15:06:36Z

src/core/server/http/integration_tests/__fixtures__/plugins/dummy_security/server/plugin.ts

+        sessionStorage.set({ value: user, expires: Date.now() + 1000 });
+        return t.authenticated({ credentials: user });
+      } else {
+        return t.rejected(Boom.unauthorized());


are we going to restrict Boom usage? there is an issue, but don't see any discussions #12947

mshustov · 2019-04-05T15:12:51Z

src/test_utils/kbn_server.ts

-export function createRootWithSettings(...settings: Array<Record<string, any>>) {
+export function createRootWithSettings(
+  settings: Record<string, any>,
+  cliArgs: Partial<CliArgs> = {}


I had to hack createRoot to be able to specify env vars because additional plugins can be used in dev mode only.
even more interesting that we run integration_tests in production mode, but functional tests in development. what is the difference? I'd expect dev/prod/testing/staging parity https://12factor.net/dev-prod-parity

Yeah, agree, we should unify environment these tests run in at some point.

mshustov · 2019-04-05T15:28:46Z

@epixa @elastic/kibana-security @elastic/kibana-platform may I ask you about preliminary code review?

elasticmachine · 2019-04-05T15:29:22Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

elasticmachine · 2019-04-08T12:11:08Z

Pinging @elastic/kibana-platform

mshustov · 2019-04-08T16:32:45Z

src/core/server/http/http_server.ts

+    this.server.ext('onRequest', adoptToHapiOnRequestFormat(fn));
+  };
+
+  private registerAuth = async <T>(fn: Authenticate<T>, cookieOptions: CookieOptions<T>) => {


Right now we provide an access to the raw Hapi request object. Will be restricted specific data and methods in following PRs

legrego

Thanks for the PR @restrry! I have a couple comments, but I don't think there's anything that'll prevent you from moving forward. I didn't do an in-depth review, as you only asked for a preliminary one :)

I know that registering routes is still a WIP, but something we're relying on for Feature Controls is the ability for routes to add tags. These tags inform the security plugin that specific authorization is required above and beyond the initial authentication. We don't necessarily need/want tags specifically going forward, but the ability for a request interceptor to inspect the current route config, and determine if any additional work needs to be done. See api_authorization.ts for an example of this.

I still encourage @azasypkin and/or @kobelb to take a look too, in case I'm overlooking anything.

src/core/server/http/http_server.ts

src/core/server/http/session_storage.ts

src/core/server/http/http_server.ts

legrego · 2019-04-09T11:54:30Z

src/core/server/http/http_server.ts

+
+    const sessionStorage = await createCookieSessionStorageFor<T>(this.server, cookieOptions);
+
+    this.server.auth.scheme('login', () => ({


I think hapi will complain if the same scheme is registered more than once. If hapi ends up throwing an error here, then this will be leaking hapi internals to the consumer. Might want to prevent this from being called more than once, and/or wrap the underlying errors.

yes, I'm going to add a check that it is called only once 👍

mshustov · 2019-04-09T14:27:30Z

I know that registering routes is still a WIP, but something we're relying on for Feature Controls is the ability for routes to add tags. These tags inform the security plugin that specific authorization is required above and beyond the initial authentication. We don't necessarily need/want tags specifically going forward, but the ability for a request interceptor to inspect the current route config, and determine if any additional work needs to be done

that's why the whole hapi request is provided to authenticate function. Later we may restrict scope to only some fields

mshustov · 2019-04-16T07:59:09Z

Will create separate issues to do in follow-ups:

introduce public api to register a route [New platform] HTTP & Security integration #34631 (comment)
restrict access to Hapi Request object in AuthenticationHandler. Discuss with Security team what extension points they need to implement authorization logic (access to authorization header, a resource tags, etc.)
[New platform] HTTP & Security integration #34631 (comment)
try to unify onRequest toolkit and ResponseFactory interfaces [New platform] HTTP & Security integration #34631 (comment)

possible improvements to discuss:

Boom usage in NP [New platform] HTTP & Security integration #34631 (comment)
decide if same site cookie option is configurable [New platform] HTTP & Security integration #34631 (comment)
add an extended error message for several registerAuth calls [New platform] HTTP & Security integration #34631 (comment)
unify testing environment [New platform] HTTP & Security integration #34631 (comment)

elasticmachine · 2019-04-16T08:49:43Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

joshdover

LGTM w/ planned follow up PRs

joshdover · 2019-04-16T14:58:10Z

src/core/server/http/lifecycle/auth.ts

+  request: Request,
+  sessionStorage: SessionStorage<T>,
+  t: AuthToolkit
+) => Promise<AuthResult>;


I'm going to introduce another mechanism to control this header in following PR.

SGTM, I didn't see the prior comment. We could probably construct KibanaRequest with a filtered set of headers for regular route handlers and only expose the Authenication header to AuthenticationHandlers.

comments are addressed, Eli is on sick leave to re-review

elasticmachine · 2019-04-16T17:25:34Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

mshustov · 2019-04-16T17:47:29Z

retest

elasticmachine · 2019-04-16T18:46:43Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

@eli

* Add Auth session * add lifecycles * add types for hapi-auth-cookie * expose interceptors from http service * add integration tests * update tests * session storage cleanup * get SessionStorage type safe * add redirect, clear cookie security integration tests * add tests for onRequest * add tests for onAuth * register Auth interceptor only once * refactor redirect tests * fix typings, change error message, test suit naming * add integration test for session validation * add tests for cookie session storage * update docs * add integration tests for onRequest * update docs * cleanup onRequest integration tests * Generate docs for AuthToolkit & OnRequestToolkit * add test for an exception in interceptor * add test OnRequest interceptors dont share request object * cleanup * address comments from @eli * improve typings for onRequest * improve plugin typings * re-generate docs * only server defines cookie path * cookieOptions.password --> cookieOptions.encryptionKey * CookieOption --> SessionStorageCookieOptions * address comments @joshdover * resolve conflict leftovers * update @types/hapi-auth-cookie deps * update docs

@eli

* Add Auth session * add lifecycles * add types for hapi-auth-cookie * expose interceptors from http service * add integration tests * update tests * session storage cleanup * get SessionStorage type safe * add redirect, clear cookie security integration tests * add tests for onRequest * add tests for onAuth * register Auth interceptor only once * refactor redirect tests * fix typings, change error message, test suit naming * add integration test for session validation * add tests for cookie session storage * update docs * add integration tests for onRequest * update docs * cleanup onRequest integration tests * Generate docs for AuthToolkit & OnRequestToolkit * add test for an exception in interceptor * add test OnRequest interceptors dont share request object * cleanup * address comments from @eli * improve typings for onRequest * improve plugin typings * re-generate docs * only server defines cookie path * cookieOptions.password --> cookieOptions.encryptionKey * CookieOption --> SessionStorageCookieOptions * address comments @joshdover * resolve conflict leftovers * update @types/hapi-auth-cookie deps * update docs

With the introduction of elastic#34631 OSS Kibana now has a dependency on hapi-auth-cookie, but it's missing from the package.json

With the introduction of #34631 OSS Kibana now has a dependency on hapi-auth-cookie, but it's missing from the package.json

With the introduction of elastic#34631 OSS Kibana now has a dependency on hapi-auth-cookie, but it's missing from the package.json

With the introduction of #34631 OSS Kibana now has a dependency on hapi-auth-cookie, but it's missing from the package.json

mshustov added 8 commits April 3, 2019 18:08

Add Auth session

d827bfb

add lifecycles

8b15ce1

add types for hapi-auth-cookie

7d908de

expose interceptors from http service

218db72

add integration tests

4fbc19c

update tests

ef83bc1

session storage cleanup

cb2a5e3

get SessionStorage type safe

f417624

mshustov force-pushed the NP-security-integration branch from f129cf2 to f417624 Compare April 5, 2019 14:48

mshustov commented Apr 5, 2019

View reviewed changes

mshustov added Feature:New Platform Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc Team:Security Team focused on: Auth, Users, Roles, Spaces, Audit Logging, and more! v7.2.0 labels Apr 8, 2019

mshustov commented Apr 9, 2019

View reviewed changes

mshustov added 2 commits April 9, 2019 09:14

Merge branch 'master' into NP-security-integration

e76bd83

add redirect, clear cookie security integration tests

2f1dd74

legrego reviewed Apr 9, 2019

View reviewed changes

mshustov added 2 commits April 9, 2019 15:51

add tests for onRequest

fdc2ff9

add tests for onAuth

52a45bd

register Auth interceptor only once

92c08e5

elastic deleted a comment from elasticmachine Apr 9, 2019

mshustov added 3 commits April 10, 2019 08:23

refactor redirect tests

aa5e08c

fix typings, change error message, test suit naming

c70df0f

add integration test for session validation

01e6426

address comments @joshdover

e9e8da0

joshdover approved these changes Apr 16, 2019

View reviewed changes

Merge branch 'master' into NP-security-integration

74a8749

mshustov added 2 commits April 16, 2019 18:34

resolve conflict leftovers

805b05f

update @types/hapi-auth-cookie deps

9e2d026

mshustov mentioned this pull request Apr 16, 2019

[New platform] Security plugin integration #33775

Closed

34 tasks

update docs

4a2656b

mshustov merged commit a202232 into elastic:master Apr 16, 2019

mshustov deleted the NP-security-integration branch April 16, 2019 18:47

mshustov mentioned this pull request Apr 16, 2019

[7.x] [New platform] HTTP & Security integration (#34631) #35181

Merged

kobelb added a commit to kobelb/kibana that referenced this pull request Apr 22, 2019

Adding hapi-auth-cookie to Kibana's root package.json

0d8c6a5

With the introduction of elastic#34631 OSS Kibana now has a dependency on hapi-auth-cookie, but it's missing from the package.json

kobelb mentioned this pull request Apr 22, 2019

Adding hapi-auth-cookie to Kibana's root package.json #35420

Merged

kobelb added a commit that referenced this pull request Apr 22, 2019

Adding hapi-auth-cookie to Kibana's root package.json (#35420)

77b6a9a

With the introduction of #34631 OSS Kibana now has a dependency on hapi-auth-cookie, but it's missing from the package.json

kobelb added a commit that referenced this pull request Apr 22, 2019

Adding hapi-auth-cookie to Kibana's root package.json (#35420) (#35424)

161c2f5

With the introduction of #34631 OSS Kibana now has a dependency on hapi-auth-cookie, but it's missing from the package.json

mshustov mentioned this pull request May 22, 2019

introduce pre-/post-auth request hooks for HttpServer #36690

Merged

7 tasks

epixa removed the release_note:enhancement label Jun 5, 2019

LeeDr removed the release_note:enhancement label Jun 5, 2019

epixa added the release_note:plugin_api_changes Contains a Plugin API changes section for the breaking plugin API changes section. label Jun 5, 2019

mshustov mentioned this pull request Jun 5, 2019

Session storage refactoring #37992

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[New platform] HTTP & Security integration #34631

[New platform] HTTP & Security integration #34631

mshustov commented Apr 5, 2019 •

edited

Loading

elasticmachine commented Apr 5, 2019

mshustov Apr 5, 2019 •

edited

Loading

mshustov Apr 5, 2019

mshustov Apr 5, 2019 •

edited

Loading

joshdover Apr 8, 2019

mshustov Apr 9, 2019

mshustov Apr 5, 2019

eliperelman Apr 11, 2019

mshustov Apr 12, 2019

mshustov Apr 5, 2019 •

edited

Loading

mshustov Apr 5, 2019 •

edited

Loading

azasypkin Apr 12, 2019

mshustov commented Apr 5, 2019 •

edited

Loading

elasticmachine commented Apr 5, 2019

elasticmachine commented Apr 8, 2019

mshustov Apr 8, 2019

legrego left a comment

legrego Apr 9, 2019

mshustov Apr 9, 2019

mshustov commented Apr 9, 2019

mshustov commented Apr 16, 2019 •

edited

Loading

elasticmachine commented Apr 16, 2019

joshdover left a comment

joshdover Apr 16, 2019

elasticmachine commented Apr 16, 2019

mshustov commented Apr 16, 2019

elasticmachine commented Apr 16, 2019


		const sessionStorage = await createCookieSessionStorageFor<T>(this.server, cookieOptions);

		this.server.auth.scheme('login', () => ({

[New platform] HTTP & Security integration #34631

[New platform] HTTP & Security integration #34631

Conversation

mshustov commented Apr 5, 2019 • edited Loading

Summary

Auth interceptor

Request interceptor

Checklist

For maintainers

elasticmachine commented Apr 5, 2019

💚 Build Succeeded

mshustov Apr 5, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mshustov Apr 5, 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 Apr 5, 2019 • edited Loading

Choose a reason for hiding this comment

mshustov Apr 5, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mshustov commented Apr 5, 2019 • edited Loading

elasticmachine commented Apr 5, 2019

💚 Build Succeeded

elasticmachine commented Apr 8, 2019

Choose a reason for hiding this comment

legrego left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mshustov commented Apr 9, 2019

mshustov commented Apr 16, 2019 • edited Loading

elasticmachine commented Apr 16, 2019

💔 Build Failed

joshdover left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elasticmachine commented Apr 16, 2019

💔 Build Failed

mshustov commented Apr 16, 2019

elasticmachine commented Apr 16, 2019

💚 Build Succeeded

mshustov commented Apr 5, 2019 •

edited

Loading

mshustov Apr 5, 2019 •

edited

Loading

mshustov Apr 5, 2019 •

edited

Loading

mshustov Apr 5, 2019 •

edited

Loading

mshustov Apr 5, 2019 •

edited

Loading

mshustov commented Apr 5, 2019 •

edited

Loading

mshustov commented Apr 16, 2019 •

edited

Loading