Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow disabling xsrf protection per an endpoint #58717

Merged
merged 15 commits into from
Mar 3, 2020
Merged
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 @@ -183,6 +183,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 @@ -227,6 +228,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.'
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

);
}
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 };
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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
6 changes: 5 additions & 1 deletion src/core/server/http/lifecycle_handlers.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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 ||
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

whitelist.includes(request.route.path) ||
request.route.options.xsrfRequired === false
) {
return toolkit.next();
}

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