Allow disabling xsrf protection per an endpoint #58717

mshustov · 2020-02-27T12:12:13Z

Summary

Closes #53823
This PR:

allows disabling xsrf protection per an endpoint for NP plugins with xsrfRequired: false
deprecates server.xsrf.whitelist. it meant to be used for IdP endpoints only, which we can switch to xsrfRequired: false

Checklist

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

Route configuration allows to disable xsrf protection for destructive HTTP methods:

routet.get({ path: ..., validate: ..., options: { xsrfRequired: false } })

It meant to be used for IdP endpoints only, which we are going to refactor to disable xsrf requirement per a specific endpoint.

elasticmachine · 2020-02-27T12:12:15Z

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

elasticmachine · 2020-02-27T12:12:17Z

Pinging @elastic/kibana-security (Team:Security)

mshustov · 2020-02-27T12:17:24Z

src/core/server/http/lifecycle_handlers.ts

@@ -31,7 +31,11 @@ export const createXsrfPostAuthHandler = (config: HttpConfig): OnPostAuthHandler
  const { whitelist, disableProtection } = config.xsrf;

  return (request, response, toolkit) => {
-    if (disableProtection || whitelist.includes(request.route.path)) {
+    if (
+      disableProtection ||


It seems that we added server.xsrf.disableProtection in spalger#6 for testing purposes solely. I'm wondering if we want to collect all test-only API in the one document to have a better understanding of API surface in case of future refactoring. (in addition to --migrationgs.skip, /internal/saved_objects/_migrate etc)

mshustov · 2020-02-27T12:24:57Z

src/core/server/config/deprecation/core_deprecations.ts

+  if (has(settings, 'server.xsrf.whitelist')) {
+    log(
+      'It is not recommended to disable xsrf protections for API endpoints via [server.xsrf.whitelist].' +
+        'It will be removed in 8.0 release. Instead, supply the "kbn-xsrf" header.'


@joshdover what is the appropriate place to track v8 breaking changes after #40768 was closed?

I believe this doc: docs/migration/migrate_8_0.asciidoc

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

src/core/server/config/deprecation/core_deprecations.ts

pgayvallet · 2020-02-27T16:14:12Z

src/core/server/http/http_server.ts

+        const kibanaRouteState: KibanaRouteState = { xsrfRequired };
+
        this.server.route({
          handler: route.handler,
          method: route.method,
          path: route.path,
          options: {
            // Enforcing the comparison with true because plugins could overwrite the auth strategy by doing `options: { authRequired: authStrategy as any }`
            auth: authRequired === true ? undefined : false,
+            app: kibanaRouteState,


NIT: can't this be inlined to remove the local var?

We can, but I want to make sure that kibanaRouteState implements KibanaRouteState correctly.
This declaration doesn't show an error for unknown properties, for example:

app: { xsrfRequired } as KibanaRouteState

azasypkin · 2020-02-28T17:17:28Z

ACK: reviewing...

azasypkin

Looks good overall, but I found one issue while testing with a real route. Thanks for the quick turnaround on this one!

src/core/server/config/deprecation/core_deprecations.ts

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

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

azasypkin

LGTM, tested locally - worked perfectly!

Side note: maybe in the future we may not accept this option for "non-destructive" HTTP methods at compile-time so that xsrfRequired: true on GET wouldn't confuse API consumers, like you've done for body?: Method extends .....

mshustov · 2020-03-03T11:57:12Z

src/core/server/http/http_server.ts

@@ -147,16 +147,22 @@ export class HttpServer {
      for (const route of router.getRoutes()) {
        this.log.debug(`registering route handler for [${route.path}]`);
        // Hapi does not allow payload validation to be specified for 'head' or 'get' requests
-        const validate = ['head', 'get'].includes(route.method) ? undefined : { payload: true };
+        const validate = isSafeMethod(route.method) ? undefined : { payload: true };


hapi doesn't support handlers for the head method hapijs/hapi#795

* add xsrfRequired flag to a route definition interface * update tests * deprecate server.xsrf.whitelist It meant to be used for IdP endpoints only, which we are going to refactor to disable xsrf requirement per a specific endpoint. * update docs * do not fail on manual KibanaRequest creation * address comments * update tests * address comments * make xsrfRequired available only for destructive methods * update docs * another isSafeMethod usage

kibanamachine · 2020-03-03T16:42:09Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: a5c12a8

History

💚 Build #30339 succeeded a5c12a8
💚 Build #30298 succeeded e7829d9
💚 Build #30027 succeeded 016f664
💚 Build #29740 succeeded 5e54a83
💔 Build #29659 failed 5d4a98b

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

* add xsrfRequired flag to a route definition interface * update tests * deprecate server.xsrf.whitelist It meant to be used for IdP endpoints only, which we are going to refactor to disable xsrf requirement per a specific endpoint. * update docs * do not fail on manual KibanaRequest creation * address comments * update tests * address comments * make xsrfRequired available only for destructive methods * update docs * another isSafeMethod usage

mshustov added 4 commits February 27, 2020 11:23

add xsrfRequired flag to a route definition interface

40a6419

update tests

6b3b447

deprecate server.xsrf.whitelist

b466852

It meant to be used for IdP endpoints only, which we are going to refactor to disable xsrf requirement per a specific endpoint.

update docs

263689d

mshustov requested a review from a team as a code owner February 27, 2020 12:12

mshustov requested review from azasypkin and a team February 27, 2020 12:12

mshustov commented Feb 27, 2020

View reviewed changes

do not fail on manual KibanaRequest creation

a8b9f09

pgayvallet reviewed Feb 27, 2020

View reviewed changes

mshustov added 4 commits February 28, 2020 10:23

address comments

081527c

Merge branch 'master' into issue-53823-xsrf-per-router

5d4a98b

Merge branch 'master' into issue-53823-xsrf-per-router

3411fb1

update tests

5e54a83

azasypkin reviewed Feb 28, 2020

View reviewed changes

src/core/server/config/deprecation/core_deprecations.ts Outdated Show resolved Hide resolved

src/core/server/http/router/route.ts Outdated Show resolved Hide resolved

src/core/server/http/router/request.ts Outdated Show resolved Hide resolved

azasypkin mentioned this pull request Mar 2, 2020

Programmatically disable XSRF protection for certain SAML/OpenID Connect routes #59002

Closed

mshustov added 2 commits March 2, 2020 13:09

Merge branch 'master' into issue-53823-xsrf-per-router

7fcf2c8

address comments

016f664

azasypkin approved these changes Mar 2, 2020

View reviewed changes

Merge branch 'master' into issue-53823-xsrf-per-router

3868f71

mshustov requested review from pgayvallet and joshdover March 3, 2020 08:07

pgayvallet approved these changes Mar 3, 2020

View reviewed changes

mshustov added 2 commits March 3, 2020 10:08

make xsrfRequired available only for destructive methods

07d7248

update docs

e7829d9

mshustov commented Mar 3, 2020

View reviewed changes

another isSafeMethod usage

a5c12a8

mshustov merged commit 4226b6f into elastic:master Mar 3, 2020

mshustov deleted the issue-53823-xsrf-per-router branch March 3, 2020 14:46

mshustov mentioned this pull request Mar 3, 2020

[7.x] Allow disabling xsrf protection per an endpoint (#58717) #59151

Merged

mshustov mentioned this pull request Jun 26, 2020

Rename server.xsrf.whitelist to server.xsrf.allowlist #70006

Closed

mshustov mentioned this pull request Nov 3, 2020

[meta] Upcoming changes to the New Platform #40768

Closed

20 tasks

joshdover mentioned this pull request Nov 25, 2020

[meta] Core Team 8.0 Projects #84380

Closed

33 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow disabling xsrf protection per an endpoint #58717

Allow disabling xsrf protection per an endpoint #58717

mshustov commented Feb 27, 2020 •

edited

Loading

elasticmachine commented Feb 27, 2020

elasticmachine commented Feb 27, 2020

mshustov Feb 27, 2020

mshustov Feb 27, 2020

joshdover Mar 2, 2020

pgayvallet Feb 27, 2020

mshustov Feb 28, 2020

azasypkin commented Feb 28, 2020

azasypkin left a comment

azasypkin left a comment

mshustov Mar 3, 2020

kibanamachine commented Mar 3, 2020

Allow disabling xsrf protection per an endpoint #58717

Allow disabling xsrf protection per an endpoint #58717

Conversation

mshustov commented Feb 27, 2020 • edited Loading

Summary

Checklist

For maintainers

Dev Docs

elasticmachine commented Feb 27, 2020

elasticmachine commented Feb 27, 2020

mshustov Feb 27, 2020

Choose a reason for hiding this comment

mshustov Feb 27, 2020

Choose a reason for hiding this comment

joshdover Mar 2, 2020

Choose a reason for hiding this comment

pgayvallet Feb 27, 2020

Choose a reason for hiding this comment

mshustov Feb 28, 2020

Choose a reason for hiding this comment

azasypkin commented Feb 28, 2020

azasypkin left a comment

Choose a reason for hiding this comment

azasypkin left a comment

Choose a reason for hiding this comment

mshustov Mar 3, 2020

Choose a reason for hiding this comment

kibanamachine commented Mar 3, 2020

💚 Build Succeeded

History

mshustov commented Feb 27, 2020 •

edited

Loading