introduce pre-/post-auth request hooks for HttpServer by mshustov · Pull Request #36690 · elastic/kibana

mshustov · 2019-05-20T13:57:01Z

Summary

I'm back to the problem. After talking with the Security team we figured out that several lifecycle stages are required.

a stage that is performed before auth - onPreAuth. Specificity: can perform request forwarding to another URL, before auth is performed and request hits a route handler. Analog in the LP - server.ext('onRequest',...). There is only one consumer as of now. But some extension points in LP can be moved here as well. one, two
auth stage. Specificity: can perform authc/authz logic and store user credentials in memory. State passed to t.authenticated in registerAuth

await registerAuth((req, sessionStorage, t) => t.authenticated(state), cookieOptions);

can be retrieved later as httpSetup.auth.get(request) => {state, status} in both New and Legacy platforms

a stage that is performed after auth - onPostAuth. Specificity: has access to auth status. Analog in the LP - server.ext('onPostAuth',...).
route can disable auth mechaism

const router = new Router('');
router.get({ path: '/', validate: false, authRequired: false }, handler);
httpSetup.registerRouter(router);

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

Kibana let define custom request interceptors for incoming requests:

http.registerAuth(handler, cookieOptions) => Promise<void>
To define custom authentication and/or authorization mechanism for incoming requests.
A handler should return a state to associate with the incoming request.
The state can be retrieved later via http.auth.get(..). Only one AuthenticationHandler can be registered.
http.registerOnPreAuth(handler) => void
To define custom logic to perform for incoming requests.
Runs the handler before Auth hook performs a check that user has access to requested resources, so it's the only place when you can forward a request to another URL right on the server.
Can register any number of registerOnPostAuth, which are called in sequence (from the first registered to the last).
http.registerOnPreAuth (handler) => void
To define custom logic to perform for incoming requests. Runs the handler after Auth hook did make sure a user has access to the requested resource.
The auth state is available at stage via http.auth.get(..)
Can register any number of registerOnPreAuth, which are called in sequence (from the first registered to the last).

mshustov · 2019-05-20T13:59:15Z

@legrego would you mind to have a look if it's sufficient to cover Security & Spaces use cases?

legrego · 2019-05-20T14:22:21Z

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

I'm not sure if it's out of scope for this PR or not, but the Spaces onPostAuth interceptor also needs to be able to render a hidden app via request takeover.

I'm not sure if the concept of "hidden applications" has been thought through for the new platform or not (@joshdover?), but we'll need equivalent functionality for both Security and Spaces

I'm not sure if the concept of "hidden applications" has been thought through for the new

No. Although, I think it's not a blocker. All Spaces / Security logic that is a blocker for NP plugin migration should be moved to NP asap. Remaining parts still have access to API in NP and we can postpone polishing for a bit. In this case that rendering logic can be expressed as :

const spaces = await newPlatform.plugins.spaces.client.asScoped(request).getAll(); if(spaces.length === 1) ... if(spaces.length > 0) ...

I think I agree with that, but we might need additional work to support dashboard-only-mode's interceptor) in the LP.

It's relying on "auth scopes" to determine when to render its hidden app, but auth scopes are identifiers attached to the request.auth.credentials object. Will NP's http service allow direct access to such properties in the LP once security moves to NP?

@elastic/kibana-security Do we have a desire to keep the “auth scopes” used for dashboard-only-mode? I’m working with Mikhail to make sure the new platform has the necessary functionality to replace onRequest and onPostAuth. We want to move the authentication logic to the NP ASAP to unblock other plugins from migrating, but part of the auth logic is the concept of scopes, which are used by dashboard only mode in the legacy platform. We don’t have a solution for rendering hidden apps yet (for dashboard mode), and we don’t want to block migrating authc on that if we don’t have to.

Mikhail suggested adding a service in the NP that would allow plugins to associate arbitrary data with a particular request, which would also extend to the LP. I'm thinking we could use that to store the list of roles post-auth in NP, which dashboard-only-mode could use in the LP to determine if it should render its hidden app or not.

It's effectively the same logic, just without the concept of auth scopes. Dasbhoard-only-mode is the only consumer of the auth scopes service, and AFAIK, we don't have plans to use it elsewhere in the near future. It would be one less service to migrate to the NP, too. One of the goals in the NP is to restrict access to auth data server-side. Currently, any plugin can inspect the request headers and auth details, but Mikhail is working on abstracting that out so that plugins don't require direct access anymore.

Removing "auth scopes" seems entirely reasonable to me, as long as we can emulate the existing behavior of dashboard-only-mode using the mechanism that you've described. I'm not a huge fan of the way that Hapi does it's auth rules, so I'd much prefer we use our own abstractions.

We can extend core API:

registerAuth already receive credentials which we can store internally

const authenticate: AuthenticationHandler = async (req, sessionStorage, t) => { if (req.headers.authorization) { sessionStorage.set({ value: user, expires: Date.now() + sessionDurationMs }); return t.authenticated({ credentials: user }); } else { return t.rejected(Boom.unauthorized()); } };

add a method that returns passed credentials and auth status

interface Auth { status: 'authenticated' | 'unauthenticated' | 'unknown'; credentials: unknown; } HttpCoreSetup { ... getAuth: (request: KibanaRequest | Request, basePath: string) => Auth }

Oleg's comment regarding statuses #34631 (comment)

@kobelb what do you think about this api? or you have another abstraction in mind?

Looks good to me!

elasticmachine · 2019-05-20T15:12:39Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

mshustov · 2019-05-22T14:21:30Z

@elastic/kibana-security I also just realized that when we move Security to Ne platform we need to find a solution to set a cookie from Legacy.
Now if a request is not handled on NP, it is proxied to LP. Therefore, the response doesn't contain any headers (including set-cookie) set on NP.

legrego · 2019-05-23T12:28:45Z

I also just realized that when we move Security to Ne platform we need to find a solution to set a cookie from Legacy.
Now if a request is not handled on NP, it is proxied to LP. Therefore, the response doesn't contain any headers (including set-cookie) set on NP.

I'll keep that in mind when I start the authc migration. I might be able to use the getAuth function you proposed here to implement this in the LP.

mshustov · 2019-05-23T12:36:13Z

@legrego signed cookie is set via sessionStorage.set(). so you don't have direct access to it.
this problem is not a blocker for Spaces. is it? we can elaborate on this part later

legrego · 2019-05-23T12:42:21Z

signed cookie is set via sessionStorage.set(). so you don't have direct access to it. this problem is not a blocker for Spaces. is it? we can elaborate on this part later

I was thinking the security plugin could maintain a parallel implementation for LP -- essentially keeping what it has today for session management in the case of requests proxied to LP.

I honestly haven't given this a ton of thought yet, so I might be wildly off base. Either way, this isn't a blocker for the first phase of spaces migration.

elasticmachine · 2019-05-23T13:13:57Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

elasticmachine · 2019-05-23T13:45:04Z

Pinging @elastic/kibana-platform

elasticmachine · 2019-05-23T15:07:33Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

mshustov · 2019-05-24T09:23:49Z

src/core/server/http/http_server.ts

-
    for (const router of this.registeredRouters) {
      for (const route of router.getRoutes()) {
+        const isAuthRequired = Boolean(this.authRegistered && route.authRequired);


are we fine to introduce authRequired flag as unprotected route definition?

I think that's reasonable, as long as the default value is true

mshustov · 2019-05-27T13:40:12Z

docs/development/core/server/kibana-plugin-server.coresetup.http.md

+        registerOnPreAuth: HttpServiceSetup['registerOnPreAuth'];
        registerAuth: HttpServiceSetup['registerAuth'];
-        registerOnRequest: HttpServiceSetup['registerOnRequest'];
+        registerOnPostAuth: HttpServiceSetup['registerOnPostAuth'];


@eliperelman what do you think if we allocate basePath functionality under basePath namespace in httpSetup? If you agree on this interface, I can implement for server side as a part of #35816

basePath: { get (key) {} set (key, value) {} prepend(url) {} remove (url) {} }

I think this makes sense. I like it.

mshustov · 2019-05-27T14:41:42Z

src/core/server/http/http_server.ts

      ) => this.registerAuth(fn, cookieOptions, config.basePath),
      getBasePathFor: this.getBasePathFor.bind(this, config),
      setBasePathFor: this.setBasePathFor.bind(this),
+      auth: {


note: HttpServer in the current state is mix of infrastructure layer & application layer logic, we can restructure it later, as soon as finish with #36804 to avoid conflicts

elasticmachine · 2019-05-27T15:02:10Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

elasticmachine · 2019-05-27T16:29:34Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

legrego · 2019-05-28T12:23:05Z

meantime we can add onResponse hook to New Platform that allows us to redirect to an LP route, where page can be served from. I outlined PR here restrry/kibana@issue-33775-introduce-pre-post-auth...restrry:pre-response-handler

I think I'd prefer the second option, where NP delegates to LP for rendering in this case. I'm totally fine making that a different PR

joshdover

Design looks good to me, only really have nitpicky comments for ya.

src/core/server/http/http_server.ts

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

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

joshdover · 2019-05-28T17:44:51Z

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

  fn: AuthenticationHandler<T>,
-  sessionStorage: SessionStorageFactory<T>
+  sessionStorage: SessionStorageFactory<T>,
+  onSuccess: (req: Request, state: unknown) => void = noop


Any practical reason to widen the type of state from object to unknown here? I'd expect them to match.

they aren't. state contain authorization information which shouldn't be cached in cookies.
that was discussed with @legrego in DM

Right now, the cookie only stores authentication information, and we delegate all authorization to Elasticsearch at “runtime”. Storing this info in a cookie would mean caching authorization details, which we don’t want to do.

we can add another generic type to specify onSuccess allowed type.
but this require some hassle to pass type declaration to https://github.com/restrry/kibana/blob/b529a1ae1c6198d0c90125bcb01309e658ae34bf/src/core/server/http/http_server.ts#L131

so I decided to handle it as a block box for core

elasticmachine · 2019-05-29T10:20:55Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

* introduce pre-,post-auth stages * cleanup integration_tests. now contracts available in tests * auth per route is configurable * Unify lifecycle results structure * expose api to store auth state and status via http service * update tests * update docs * use full name, auth should not mutate request * move basePath functionality under namespace * regenerate docs * Revert "move basePath functionality under namespace" This reverts commit 9599d32. * Revert "regenerate docs" This reverts commit 1799d3b. * regenerate docs * updated yarn.lock no idea why * extract AuthStateStorage to a separate entity * get rid of nested ifs * describe what is the difference between hooks * re-wording

…38265) * introduce pre-/post-auth request hooks for HttpServer (#36690) * introduce pre-,post-auth stages * cleanup integration_tests. now contracts available in tests * auth per route is configurable * Unify lifecycle results structure * expose api to store auth state and status via http service * update tests * update docs * use full name, auth should not mutate request * move basePath functionality under namespace * regenerate docs * Revert "move basePath functionality under namespace" This reverts commit 9599d32. * Revert "regenerate docs" This reverts commit 1799d3b. * regenerate docs * updated yarn.lock no idea why * extract AuthStateStorage to a separate entity * get rid of nested ifs * describe what is the difference between hooks * re-wording * fix typings

legrego reviewed May 20, 2019

View reviewed changes

mshustov added 2 commits May 22, 2019 10:37

introduce pre-,post-auth stages

ddd5fe5

cleanup integration_tests. now contracts available in tests

112f30e

mshustov added 4 commits May 23, 2019 12:34

auth per route is configurable

4ce4967

Unify lifecycle results structure

013d815

expose api to store auth state and status via http service

84c5c1d

update tests

e0a607b

mshustov force-pushed the issue-33775-introduce-pre-post-auth branch from caef93a to e0a607b Compare May 23, 2019 11:46

update docs

4f75b71

mshustov marked this pull request as ready for review May 23, 2019 14:59

mshustov requested a review from a team as a code owner May 23, 2019 14:59

mshustov added the review label May 23, 2019

legrego mentioned this pull request May 23, 2019

Security - remove auth scope provider #36998

Merged

Merge branch 'master' into issue-33775-introduce-pre-post-auth

68ec950

mshustov mentioned this pull request May 24, 2019

[New platform] Security plugin integration #33775

Closed

34 tasks

mshustov commented May 24, 2019

View reviewed changes

mshustov commented May 27, 2019

View reviewed changes

regenerate docs

14667d3

mshustov requested review from kobelb and legrego and removed request for kobelb May 27, 2019 13:51

mshustov added 2 commits May 27, 2019 15:52

updated yarn.lock no idea why

39feb77

extract AuthStateStorage to a separate entity

9735891

mshustov commented May 27, 2019

View reviewed changes

Merge branch 'master' into issue-33775-introduce-pre-post-auth

b529a1a

mshustov mentioned this pull request May 27, 2019

[feat] create additional http servers #36804

Merged

7 tasks

joshdover approved these changes May 28, 2019

View reviewed changes

mshustov added 3 commits May 29, 2019 11:15

get rid of nested ifs

aa72f91

describe what is the difference between hooks

ea1edee

re-wording

b8d6041

mshustov added release_note:plugin_api_changes Contains a Plugin API changes section for the breaking plugin API changes section. and removed release_note:skip Skip the PR/issue when compiling release notes labels May 29, 2019

mshustov merged commit b0c0165 into elastic:master May 29, 2019

This was referenced Jun 6, 2019

Unify base path in HttpService #38237

Merged

[7.x] introduce pre-/post-auth request hooks for HttpServer (#36690) #38265

Merged

legrego mentioned this pull request Jun 14, 2019

[New platform] Spaces plugin integration #39011

Closed

25 tasks

mshustov mentioned this pull request Jun 17, 2019

New Platform and Legacy platform servers integration #39047

Merged

3 tasks

kobelb mentioned this pull request Jul 1, 2020

[POC] Fleet agent concurrency #70495

Closed

Conversation

mshustov commented May 20, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Checklist

For maintainers

Dev Docs

Uh oh!

mshustov commented May 20, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mshustov May 20, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mshustov May 22, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

elasticmachine commented May 20, 2019

💔 Build Failed

Uh oh!

mshustov commented May 22, 2019

Uh oh!

legrego commented May 23, 2019

Uh oh!

mshustov commented May 23, 2019

Uh oh!

legrego commented May 23, 2019

Uh oh!

elasticmachine commented May 23, 2019

💔 Build Failed

Uh oh!

elasticmachine commented May 23, 2019

Uh oh!

elasticmachine commented May 23, 2019

💔 Build Failed

Uh oh!

mshustov May 24, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mshustov May 27, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

elasticmachine commented May 27, 2019

💔 Build Failed

Uh oh!

elasticmachine commented May 27, 2019

💚 Build Succeeded

Uh oh!

legrego commented May 28, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

joshdover left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mshustov May 29, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

mshustov commented May 20, 2019 •

edited

Loading

mshustov May 20, 2019 •

edited

Loading

mshustov May 22, 2019 •

edited

Loading

mshustov May 24, 2019 •

edited

Loading

mshustov May 27, 2019 •

edited

Loading

legrego commented May 28, 2019 •

edited

Loading

mshustov May 29, 2019 •

edited

Loading