Skip to content

Commit

Permalink
Allow disabling xsrf protection per an endpoint (#58717)
Browse files Browse the repository at this point in the history
* 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
  • Loading branch information
mshustov authored Mar 3, 2020
1 parent 1995a05 commit 4226b6f
Show file tree
Hide file tree
Showing 20 changed files with 185 additions and 14 deletions.
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
<!-- Do not edit this file. It is automatically generated by API Documenter. -->

[Home](./index.md) &gt; [kibana-plugin-server](./kibana-plugin-server.md) &gt; [DestructiveRouteMethod](./kibana-plugin-server.destructiveroutemethod.md)

## DestructiveRouteMethod type

Set of HTTP methods changing the state of the server.

<b>Signature:</b>

```typescript
export declare type DestructiveRouteMethod = 'post' | 'put' | 'delete' | 'patch';
```
2 changes: 2 additions & 0 deletions docs/development/core/server/kibana-plugin-server.md
Original file line number Diff line number Diff line change
Expand Up @@ -188,6 +188,7 @@ The plugin integrates with the core system via lifecycle events: `setup`<!-- -->
| [ConfigDeprecationLogger](./kibana-plugin-server.configdeprecationlogger.md) | Logger interface used when invoking a [ConfigDeprecation](./kibana-plugin-server.configdeprecation.md) |
| [ConfigDeprecationProvider](./kibana-plugin-server.configdeprecationprovider.md) | A provider that should returns a list of [ConfigDeprecation](./kibana-plugin-server.configdeprecation.md)<!-- -->.<!-- -->See [ConfigDeprecationFactory](./kibana-plugin-server.configdeprecationfactory.md) for more usage examples. |
| [ConfigPath](./kibana-plugin-server.configpath.md) | |
| [DestructiveRouteMethod](./kibana-plugin-server.destructiveroutemethod.md) | Set of HTTP methods changing the state of the server. |
| [ElasticsearchClientConfig](./kibana-plugin-server.elasticsearchclientconfig.md) | |
| [GetAuthHeaders](./kibana-plugin-server.getauthheaders.md) | Get headers to authenticate a user against Elasticsearch. |
| [GetAuthState](./kibana-plugin-server.getauthstate.md) | Gets authentication state for a request. Returned by <code>auth</code> interceptor. |
Expand Down Expand Up @@ -232,6 +233,7 @@ The plugin integrates with the core system via lifecycle events: `setup`<!-- -->
| [RouteValidationFunction](./kibana-plugin-server.routevalidationfunction.md) | The custom validation function if @<!-- -->kbn/config-schema is not a valid solution for your specific plugin requirements. |
| [RouteValidationSpec](./kibana-plugin-server.routevalidationspec.md) | Allowed property validation options: either @<!-- -->kbn/config-schema validations or custom validation functions<!-- -->See [RouteValidationFunction](./kibana-plugin-server.routevalidationfunction.md) for custom validation. |
| [RouteValidatorFullConfig](./kibana-plugin-server.routevalidatorfullconfig.md) | Route validations config and options merged into one object |
| [SafeRouteMethod](./kibana-plugin-server.saferoutemethod.md) | Set of HTTP methods not changing the state of the server. |
| [SavedObjectAttribute](./kibana-plugin-server.savedobjectattribute.md) | Type definition for a Saved Object attribute value |
| [SavedObjectAttributeSingle](./kibana-plugin-server.savedobjectattributesingle.md) | Don't use this type, it's simply a helper type for [SavedObjectAttribute](./kibana-plugin-server.savedobjectattribute.md) |
| [SavedObjectMigrationFn](./kibana-plugin-server.savedobjectmigrationfn.md) | A migration function for a [saved object type](./kibana-plugin-server.savedobjectstype.md) used to migrate it to a given version |
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,4 +19,5 @@ export interface RouteConfigOptions<Method extends RouteMethod>
| [authRequired](./kibana-plugin-server.routeconfigoptions.authrequired.md) | <code>boolean</code> | A flag shows that authentication for a route: <code>enabled</code> when true <code>disabled</code> when false<!-- -->Enabled by default. |
| [body](./kibana-plugin-server.routeconfigoptions.body.md) | <code>Method extends 'get' &#124; 'options' ? undefined : RouteConfigOptionsBody</code> | Additional body options [RouteConfigOptionsBody](./kibana-plugin-server.routeconfigoptionsbody.md)<!-- -->. |
| [tags](./kibana-plugin-server.routeconfigoptions.tags.md) | <code>readonly string[]</code> | Additional metadata tag strings to attach to the route. |
| [xsrfRequired](./kibana-plugin-server.routeconfigoptions.xsrfrequired.md) | <code>Method extends 'get' ? never : boolean</code> | Defines xsrf protection requirements for a route: - true. Requires an incoming POST/PUT/DELETE request to contain <code>kbn-xsrf</code> header. - false. Disables xsrf protection.<!-- -->Set to true by default |

Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
<!-- Do not edit this file. It is automatically generated by API Documenter. -->

[Home](./index.md) &gt; [kibana-plugin-server](./kibana-plugin-server.md) &gt; [RouteConfigOptions](./kibana-plugin-server.routeconfigoptions.md) &gt; [xsrfRequired](./kibana-plugin-server.routeconfigoptions.xsrfrequired.md)

## RouteConfigOptions.xsrfRequired property

Defines xsrf protection requirements for a route: - true. Requires an incoming POST/PUT/DELETE request to contain `kbn-xsrf` header. - false. Disables xsrf protection.

Set to true by default

<b>Signature:</b>

```typescript
xsrfRequired?: Method extends 'get' ? never : boolean;
```
Original file line number Diff line number Diff line change
Expand Up @@ -9,5 +9,5 @@ The set of common HTTP methods supported by Kibana routing.
<b>Signature:</b>

```typescript
export declare type RouteMethod = 'get' | 'post' | 'put' | 'delete' | 'patch' | 'options';
export declare type RouteMethod = SafeRouteMethod | DestructiveRouteMethod;
```
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
<!-- Do not edit this file. It is automatically generated by API Documenter. -->

[Home](./index.md) &gt; [kibana-plugin-server](./kibana-plugin-server.md) &gt; [SafeRouteMethod](./kibana-plugin-server.saferoutemethod.md)

## SafeRouteMethod type

Set of HTTP methods not changing the state of the server.

<b>Signature:</b>

```typescript
export declare type SafeRouteMethod = 'get' | 'options';
```
13 changes: 13 additions & 0 deletions src/core/server/config/deprecation/core_deprecations.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,19 @@ describe('core deprecations', () => {
});
});

describe('xsrfDeprecation', () => {
it('logs a warning if server.xsrf.whitelist is set', () => {
const { messages } = applyCoreDeprecations({
server: { xsrf: { whitelist: ['/path'] } },
});
expect(messages).toMatchInlineSnapshot(`
Array [
"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.",
]
`);
});
});

describe('rewriteBasePath', () => {
it('logs a warning is server.basePath is set and server.rewriteBasePath is not', () => {
const { messages } = applyCoreDeprecations({
Expand Down
14 changes: 14 additions & 0 deletions src/core/server/config/deprecation/core_deprecations.ts
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,19 @@ const dataPathDeprecation: ConfigDeprecation = (settings, fromPath, log) => {
return settings;
};

const xsrfDeprecation: ConfigDeprecation = (settings, fromPath, log) => {
if (
has(settings, 'server.xsrf.whitelist') &&
get<unknown[]>(settings, 'server.xsrf.whitelist').length > 0
) {
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.'
);
}
return settings;
};

const rewriteBasePathDeprecation: ConfigDeprecation = (settings, fromPath, log) => {
if (has(settings, 'server.basePath') && !has(settings, 'server.rewriteBasePath')) {
log(
Expand Down Expand Up @@ -177,4 +190,5 @@ export const coreDeprecationProvider: ConfigDeprecationProvider = ({
rewriteBasePathDeprecation,
cspRulesDeprecation,
mapManifestServiceUrlDeprecation,
xsrfDeprecation,
];
6 changes: 5 additions & 1 deletion src/core/server/http/http_server.mocks.ts
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ import {
RouteMethod,
KibanaResponseFactory,
RouteValidationSpec,
KibanaRouteState,
} from './router';
import { OnPreResponseToolkit } from './lifecycle/on_pre_response';
import { OnPostAuthToolkit } from './lifecycle/on_post_auth';
Expand All @@ -43,6 +44,7 @@ interface RequestFixtureOptions<P = any, Q = any, B = any> {
method?: RouteMethod;
socket?: Socket;
routeTags?: string[];
kibanaRouteState?: KibanaRouteState;
routeAuthRequired?: false;
validation?: {
params?: RouteValidationSpec<P>;
Expand All @@ -62,6 +64,7 @@ function createKibanaRequestMock<P = any, Q = any, B = any>({
routeTags,
routeAuthRequired,
validation = {},
kibanaRouteState = { xsrfRequired: true },
}: RequestFixtureOptions<P, Q, B> = {}) {
const queryString = stringify(query, { sort: false });

Expand All @@ -80,7 +83,7 @@ function createKibanaRequestMock<P = any, Q = any, B = any>({
search: queryString ? `?${queryString}` : queryString,
},
route: {
settings: { tags: routeTags, auth: routeAuthRequired },
settings: { tags: routeTags, auth: routeAuthRequired, app: kibanaRouteState },
},
raw: {
req: { socket },
Expand Down Expand Up @@ -109,6 +112,7 @@ function createRawRequestMock(customization: DeepPartial<Request> = {}) {
return merge(
{},
{
app: { xsrfRequired: true } as any,
headers: {},
path: '/',
route: { settings: {} },
Expand Down
2 changes: 2 additions & 0 deletions src/core/server/http/http_server.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -811,6 +811,7 @@ test('exposes route details of incoming request to a route handler', async () =>
path: '/',
options: {
authRequired: true,
xsrfRequired: false,
tags: [],
},
});
Expand Down Expand Up @@ -923,6 +924,7 @@ test('exposes route details of incoming request to a route handler (POST + paylo
path: '/',
options: {
authRequired: true,
xsrfRequired: true,
tags: [],
body: {
parse: true, // hapi populates the default
Expand Down
10 changes: 8 additions & 2 deletions src/core/server/http/http_server.ts
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ import { adoptToHapiOnPostAuthFormat, OnPostAuthHandler } from './lifecycle/on_p
import { adoptToHapiOnPreAuthFormat, OnPreAuthHandler } from './lifecycle/on_pre_auth';
import { adoptToHapiOnPreResponseFormat, OnPreResponseHandler } from './lifecycle/on_pre_response';

import { IRouter } from './router';
import { IRouter, KibanaRouteState, isSafeMethod } from './router';
import {
SessionStorageCookieOptions,
createCookieSessionStorageFactory,
Expand Down Expand Up @@ -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 };
const { authRequired = true, tags, body = {} } = route.options;
const { accepts: allow, maxBytes, output, parse } = body;

const kibanaRouteState: KibanaRouteState = {
xsrfRequired: route.options.xsrfRequired ?? !isSafeMethod(route.method),
};

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,
tags: tags ? Array.from(tags) : undefined,
// TODO: This 'validate' section can be removed once the legacy platform is completely removed.
// We are telling Hapi that NP routes can accept any payload, so that it can bypass the default
Expand Down
2 changes: 2 additions & 0 deletions src/core/server/http/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,8 @@ export {
RouteValidationError,
RouteValidatorFullConfig,
RouteValidationResultFactory,
DestructiveRouteMethod,
SafeRouteMethod,
} from './router';
export { BasePathProxyServer } from './base_path_proxy_server';
export { OnPreAuthHandler, OnPreAuthToolkit } from './lifecycle/on_pre_auth';
Expand Down
11 changes: 11 additions & 0 deletions src/core/server/http/integration_tests/lifecycle_handlers.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@ const versionHeader = 'kbn-version';
const xsrfHeader = 'kbn-xsrf';
const nameHeader = 'kbn-name';
const whitelistedTestPath = '/xsrf/test/route/whitelisted';
const xsrfDisabledTestPath = '/xsrf/test/route/disabled';
const kibanaName = 'my-kibana-name';
const setupDeps = {
context: contextServiceMock.createSetupContract(),
Expand Down Expand Up @@ -188,6 +189,12 @@ describe('core lifecycle handlers', () => {
return res.ok({ body: 'ok' });
}
);
((router as any)[method.toLowerCase()] as RouteRegistrar<any>)<any, any, any>(
{ path: xsrfDisabledTestPath, validate: false, options: { xsrfRequired: false } },
(context, req, res) => {
return res.ok({ body: 'ok' });
}
);
});

await server.start();
Expand Down Expand Up @@ -235,6 +242,10 @@ describe('core lifecycle handlers', () => {
it('accepts whitelisted requests without either an xsrf or version header', async () => {
await getSupertest(method.toLowerCase(), whitelistedTestPath).expect(200, 'ok');
});

it('accepts requests on a route with disabled xsrf protection', async () => {
await getSupertest(method.toLowerCase(), xsrfDisabledTestPath).expect(200, 'ok');
});
});
});
});
Expand Down
29 changes: 27 additions & 2 deletions src/core/server/http/lifecycle_handlers.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -24,20 +24,22 @@ import {
} from './lifecycle_handlers';
import { httpServerMock } from './http_server.mocks';
import { HttpConfig } from './http_config';
import { KibanaRequest, RouteMethod } from './router';
import { KibanaRequest, RouteMethod, KibanaRouteState } from './router';

const createConfig = (partial: Partial<HttpConfig>): HttpConfig => partial as HttpConfig;

const forgeRequest = ({
headers = {},
path = '/',
method = 'get',
kibanaRouteState,
}: Partial<{
headers: Record<string, string>;
path: string;
method: RouteMethod;
kibanaRouteState: KibanaRouteState;
}>): KibanaRequest => {
return httpServerMock.createKibanaRequest({ headers, path, method });
return httpServerMock.createKibanaRequest({ headers, path, method, kibanaRouteState });
};

describe('xsrf post-auth handler', () => {
Expand Down Expand Up @@ -142,6 +144,29 @@ describe('xsrf post-auth handler', () => {
expect(toolkit.next).toHaveBeenCalledTimes(1);
expect(result).toEqual('next');
});

it('accepts requests if xsrf protection on a route is disabled', () => {
const config = createConfig({
xsrf: { whitelist: [], disableProtection: false },
});
const handler = createXsrfPostAuthHandler(config);
const request = forgeRequest({
method: 'post',
headers: {},
path: '/some-path',
kibanaRouteState: {
xsrfRequired: false,
},
});

toolkit.next.mockReturnValue('next' as any);

const result = handler(request, responseFactory, toolkit);

expect(responseFactory.badRequest).not.toHaveBeenCalled();
expect(toolkit.next).toHaveBeenCalledTimes(1);
expect(result).toEqual('next');
});
});
});

Expand Down
10 changes: 7 additions & 3 deletions src/core/server/http/lifecycle_handlers.ts
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
import { OnPostAuthHandler } from './lifecycle/on_post_auth';
import { OnPreResponseHandler } from './lifecycle/on_pre_response';
import { HttpConfig } from './http_config';
import { isSafeMethod } from './router';
import { Env } from '../config';
import { LifecycleRegistrar } from './http_server';

Expand All @@ -31,15 +32,18 @@ export const createXsrfPostAuthHandler = (config: HttpConfig): OnPostAuthHandler
const { whitelist, disableProtection } = config.xsrf;

return (request, response, toolkit) => {
if (disableProtection || whitelist.includes(request.route.path)) {
if (
disableProtection ||
whitelist.includes(request.route.path) ||
request.route.options.xsrfRequired === false
) {
return toolkit.next();
}

const isSafeMethod = request.route.method === 'get' || request.route.method === 'head';
const hasVersionHeader = VERSION_HEADER in request.headers;
const hasXsrfHeader = XSRF_HEADER in request.headers;

if (!isSafeMethod && !hasVersionHeader && !hasXsrfHeader) {
if (!isSafeMethod(request.route.method) && !hasVersionHeader && !hasXsrfHeader) {
return response.badRequest({ body: `Request must contain a ${XSRF_HEADER} header.` });
}

Expand Down
4 changes: 4 additions & 0 deletions src/core/server/http/router/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -24,16 +24,20 @@ export {
KibanaRequestEvents,
KibanaRequestRoute,
KibanaRequestRouteOptions,
KibanaRouteState,
isRealRequest,
LegacyRequest,
ensureRawRequest,
} from './request';
export {
DestructiveRouteMethod,
isSafeMethod,
RouteMethod,
RouteConfig,
RouteConfigOptions,
RouteContentType,
RouteConfigOptionsBody,
SafeRouteMethod,
validBodyOutput,
} from './route';
export { HapiResponseAdapter } from './response_adapter';
Expand Down
14 changes: 11 additions & 3 deletions src/core/server/http/router/request.ts
Original file line number Diff line number Diff line change
Expand Up @@ -18,18 +18,24 @@
*/

import { Url } from 'url';
import { Request } from 'hapi';
import { Request, ApplicationState } from 'hapi';
import { Observable, fromEvent, merge } from 'rxjs';
import { shareReplay, first, takeUntil } from 'rxjs/operators';

import { deepFreeze, RecursiveReadonly } from '../../../utils';
import { Headers } from './headers';
import { RouteMethod, RouteConfigOptions, validBodyOutput } from './route';
import { RouteMethod, RouteConfigOptions, validBodyOutput, isSafeMethod } from './route';
import { KibanaSocket, IKibanaSocket } from './socket';
import { RouteValidator, RouteValidatorFullConfig } from './validator';

const requestSymbol = Symbol('request');

/**
* @internal
*/
export interface KibanaRouteState extends ApplicationState {
xsrfRequired: boolean;
}
/**
* Route options: If 'GET' or 'OPTIONS' method, body options won't be returned.
* @public
Expand Down Expand Up @@ -184,8 +190,10 @@ export class KibanaRequest<

const options = ({
authRequired: request.route.settings.auth !== false,
// some places in LP call KibanaRequest.from(request) manually. remove fallback to true before v8
xsrfRequired: (request.route.settings.app as KibanaRouteState)?.xsrfRequired ?? true,
tags: request.route.settings.tags || [],
body: ['get', 'options'].includes(method)
body: isSafeMethod(method)
? undefined
: {
parse,
Expand Down
Loading

0 comments on commit 4226b6f

Please sign in to comment.