Add an optional authentication mode for HTTP resources by mshustov · Pull Request #58589 · elastic/kibana

mshustov · 2020-02-26T12:08:55Z

Summary

closes #41959
The current PR adds optional authentication mode for HTTP resources that acts similarly to optional mode in hapi https://hapi.dev/api/?v=17.9.2#-routeoptionsauthmode

'optional' - authentication is optional - the request must include valid credentials or no credentials at all.

For this, I added the notHandled outcome for the auth interceptor. Getting it from auth hook, Kibana server behaves differently:

for optional authentication mode, allows an user to access a resource
for required authentication mode (default behavior), the Kibana server responds with the unauthorized response.

All other cases, such as handling redirection to 3rd party IdP is up to Security plugin internal logic. Should we create followup issues in the Security team backlog?
@elastic/kibana-security let me know if the current logic makes sense for you.

Checklist

Delete any items that are not applicable to this PR.

Unit or functional tests were updated or added to match the most common scenarios

For maintainers

This was checked for breaking API changes and was labeled appropriately

Dev docs

A route config accepts authRequired: 'optional'. A user can access a resource if has valid credentials or no credentials at all. Can be useful when we grant access to a resource but want to identify a user if possible.

router.get( { path: '/',  options: { authRequired: 'optional' } }, handler);

mshustov · 2020-02-26T13:15:06Z

src/core/server/http/http_server.ts

+  private getAuthOption(
+    authRequired: RouteConfigOptions<any>['authRequired'] = true
+  ): false | { mode: 'required' | 'optional' } {
+    if (this.authRegistered) {


It sets mode explicitly. I had to change the logic to inline it with mode: optional declaration since it cannot be used without auth strategy registration. the previous implementation relies on the internal hapi logic that true === undefined.

mshustov · 2020-02-26T13:17:50Z

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

    expect(response.header['www-authenticate']).toBe(authResponseHeader['www-authenticate']);
  });

+  it('attach security header to not handled auth response', async () => {


duplicates tests in src/core/server/http/integration_tests/router.test.ts should I remove them?

One seems enough. Not sure in which file it makes more sense to keep it though. There seems to be overlapping 'coverage' between the two test files.

mshustov · 2020-02-26T13:21:56Z

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

+    const authOptions = request.route.settings.auth;
+    if (typeof authOptions === 'object') {
+      // 'try' is used in the legacy platform
+      if (authOptions.mode === 'optional' || authOptions.mode === 'try') {


It will throw en exception in case of try after the migration is done. I decided to add an exhaustive check to catch the options introduced in the new hapi versions if any.

elasticmachine · 2020-02-26T14:47:34Z

Pinging @elastic/kibana-platform (Team:Platform)

mshustov · 2020-02-26T17:04:31Z

@elasticmachine merge upstream

pgayvallet

Implementation looks good to me.

pgayvallet · 2020-02-27T10:08:46Z

src/core/public/application/capabilities/capabilities_service.tsx

pgayvallet · 2020-02-27T10:10:06Z

src/core/server/capabilities/routes/index.ts

NIT: why remove the prefix here?

pgayvallet · 2020-02-27T11:06:39Z

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

    expect(response.header['www-authenticate']).toBe(authResponseHeader['www-authenticate']);
  });

+  it('attach security header to not handled auth response', async () => {


One seems enough. Not sure in which file it makes more sense to keep it though. There seems to be overlapping 'coverage' between the two test files.

pgayvallet · 2020-02-27T11:11:54Z

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

Are the result && really needed? It seems result should not be false-ish with current typing?

I guess that's because we're receiving result from auth hook handler that can return whatever it wants? Then probably result?.type would be better since isNotHandled would always return boolean and not falsy value of result.

right. this API can be called from js code

azasypkin · 2020-03-03T14:43:16Z

ACK: will review tomorrow morning, sorry for the delay

x-pack/plugins/security/server/authorization/index.ts

joshdover · 2020-03-05T20:57:20Z

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

+      type: AuthResultType.notHandled,
+    };
+  },
+  redirected(headers: { location: string } & ResponseHeaders): AuthResult {


Has our convention been to use lowercase header names? Really just asking for my own education.

I don't remember we ever discussed it

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

joshdover · 2020-03-05T21:07:13Z

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

+describe('Options', () => {
+  describe('authRequired', () => {
+    describe('optional', () => {
+      it('User has access to a route if auth mechanism not registered', async () => {


Should we verify the value of context.auth.isAuthenticated in these tests?

we have separate tests for them, but it doesn't hurt since here we can verify:

a route was hit

auth API returns a correct result

src/core/server/index.ts

azasypkin · 2020-03-06T16:08:01Z

ACK: reviewing...

azasypkin

LGTM. Tested locally with login route - seems to work as expected, thanks!

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

* add authRequred: 'optional' * expose auth status via request context * update security plugin to use notHandled auth outcome * capabilities service uses optional auth * update tests * attach security headers only to unauthorised response * add isAuthenticated tests for 'optional' auth mode * security plugin relies on http.auth.isAuthenticated to calc capabilities * generate docs * reword test suit names * update tests * update test checking isAuth on optional auth path * address Oleg comments * add test for auth: try * fix * pass isAuthenticted as boolean via context * remove response header from notHandled * update docs * add redirected for auth interceptor * security plugin uses t.redirected to be compat with auth: optional * update docs * require location header in the interface * address comments #1 * declare isAuthenticated on KibanaRequest * remove auth.isAuthenticated from scope * update docs * remove unnecessary comment * do not fail on FakrRequest * small improvements

kibanamachine · 2020-03-07T12:34:51Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 86f4603

History

💚 Build #31513 succeeded 86f4603
💛 Build #31391 was flaky f177d69
💔 Build #31374 failed 01d995c
💚 Build #31033 succeeded fcb4994
💚 Build #30788 succeeded e96158b

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

* add authRequred: 'optional' * expose auth status via request context * update security plugin to use notHandled auth outcome * capabilities service uses optional auth * update tests * attach security headers only to unauthorised response * add isAuthenticated tests for 'optional' auth mode * security plugin relies on http.auth.isAuthenticated to calc capabilities * generate docs * reword test suit names * update tests * update test checking isAuth on optional auth path * address Oleg comments * add test for auth: try * fix * pass isAuthenticted as boolean via context * remove response header from notHandled * update docs * add redirected for auth interceptor * security plugin uses t.redirected to be compat with auth: optional * update docs * require location header in the interface * address comments #1 * declare isAuthenticated on KibanaRequest * remove auth.isAuthenticated from scope * update docs * remove unnecessary comment * do not fail on FakrRequest * small improvements

* master: (154 commits) Add an optional authentication mode for HTTP resources (elastic#58589) Implement embeddable drilldown menu options (elastic#59232) [Alerting] "Create alert" graph visualization design improvements (elastic#59399) Alerting update route throttle property is missing (elastic#59580) [SIEM] Adds 'Load prebuilt rules' Cypress test (elastic#59529) Show error if field is not found during filter rendering (elastic#59298) Navigate back to discover app during test, because the saved search from the preceding test has major performance problems when used with this test (elastic#59571) Check for alert dialog when doing a force logout (elastic#59329) ensure fs deletes are not cwd dependent (elastic#59570) Empty message for APM service map (elastic#59518) [Drilldowns] <ActionWizard/> Component (elastic#59032) [Reporting] Improve the page exit error messages (elastic#59351) Ensure logged out starting state for tests that need it (elastic#59322) Hide input value from kbn-config-schema error messages (elastic#58843) [ML] Transforms: Migrate client plugin to NP. (elastic#59443) [ML] Disable failing functional tests [SIEM] Update Timeline to use the latest euiFlyoutBody style (elastic#59524) Temporarily remove the project mappings for PR labels (elastic#59493) [Alerting] replace index threshold graph usage of watcher APIs with new API (elastic#59385) [ML] Show view series link in anomalies table for machine_learning_user role (elastic#59549) ...

mshustov added 6 commits February 26, 2020 13:04

add authRequred: 'optional'

89a1eff

expose auth status via request context

0f3a915

update security plugin to use notHandled auth outcome

05809a4

capabilities service uses optional auth

fc94baa

update tests

de22f7e

Merge branch 'master' into issue-41959-optional-auth

97ac4a3

mshustov commented Feb 26, 2020

View reviewed changes

mshustov added 3 commits February 26, 2020 14:47

attach security headers only to unauthorised response

006edef

add isAuthenticated tests for 'optional' auth mode

529eeee

security plugin relies on http.auth.isAuthenticated to calc capabilities

4c9f0bc

mshustov added Feature:New Platform release_note:plugin_api_changes Contains a Plugin API changes section for the breaking plugin API changes section. Team:Core Platform Core services: plugins, logging, config, saved objects, http, ES client, i18n, etc t// v7.7.0 v8.0.0 labels Feb 26, 2020

mshustov added 2 commits February 26, 2020 16:11

generate docs

a2f2546

reword test suit names

9bec57f

mshustov changed the title ~~Issue 41959 optional auth~~ Add an optional authentication mode for HTTP resources Feb 26, 2020

mshustov marked this pull request as ready for review February 26, 2020 17:05

mshustov requested review from a team as code owners February 26, 2020 17:05

pgayvallet approved these changes Feb 27, 2020

View reviewed changes

azasypkin self-requested a review March 3, 2020 14:43

mshustov added 2 commits March 4, 2020 09:08

Merge branch 'master' into issue-41959-optional-auth

6c4de35

update tests

82835fc

mshustov added 2 commits March 4, 2020 15:03

remove response header from notHandled

cc3d95c

update docs

e96158b

mshustov commented Mar 4, 2020

View reviewed changes

x-pack/plugins/security/server/authorization/index.ts Outdated Show resolved Hide resolved

mshustov added 4 commits March 5, 2020 11:12

add redirected for auth interceptor

9750ce1

security plugin uses t.redirected to be compat with auth: optional

5efb16a

update docs

c5cb1fd

require location header in the interface

fcb4994

joshdover reviewed Mar 5, 2020

View reviewed changes

mshustov added 6 commits March 6, 2020 08:53

address comments #1

f1ecba3

declare isAuthenticated on KibanaRequest

f6dad7f

remove auth.isAuthenticated from scope

d1901a8

update docs

0275e25

Merge branch 'master' into issue-41959-optional-auth

8b2a817

remove unnecessary comment

01d995c

mshustov requested review from azasypkin and joshdover March 6, 2020 10:29

do not fail on FakrRequest

f177d69

azasypkin approved these changes Mar 6, 2020

View reviewed changes

src/core/server/http/lifecycle/auth.ts Outdated Show resolved Hide resolved

small improvements

86f4603

mshustov merged commit 5c43653 into elastic:master Mar 7, 2020

mshustov deleted the issue-41959-optional-auth branch March 7, 2020 10:56

mshustov mentioned this pull request Mar 7, 2020

[7.x] Add an optional authentication mode for HTTP resources (#58589) #59614

Merged

legrego mentioned this pull request Mar 24, 2020

do not warn when switching capabilities for resources with optional auth #61043

Merged

2 tasks

legrego mentioned this pull request Feb 25, 2021

Migrate /bootstrap.js endpoint to core #92784

Merged

2 tasks

Conversation

mshustov commented Feb 26, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Checklist

For maintainers

Dev docs

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!

elasticmachine commented Feb 26, 2020

Uh oh!

mshustov commented Feb 26, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pgayvallet left a comment

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mshustov Mar 4, 2020 • 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!

azasypkin commented Mar 3, 2020

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

azasypkin commented Mar 6, 2020

Uh oh!

azasypkin left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

kibanamachine commented Mar 7, 2020

💚 Build Succeeded

History

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

mshustov commented Feb 26, 2020 •

edited

Loading

mshustov commented Feb 26, 2020 •

edited

Loading

mshustov Mar 4, 2020 •

edited

Loading