From 621bf0efec3e96884b86d94462b1698171933f87 Mon Sep 17 00:00:00 2001 From: Court Ewing Date: Thu, 1 Sep 2016 21:44:24 -0400 Subject: [PATCH] Separate xsrf handling and version checking Traditionally we've relied on the kbn-version header for csrf protection, but that is also used for the client-side Kibana version check that alerts users when their client is out of date and needs to be refreshed, so it must match the version of Kibana exactly. This poses a problem for any programmatic access that would only get set up once but may run repeatedly throughout the future (e.g. watcher), so there's now the additional option of specifying the kbn-xsrf header instead. The value of the header does not matter, but its very presence is all that is necessary to avoid xsrf attacks. The xsrf protection just needs either one of these headers to exist. --- src/server/http/__tests__/version_check.js | 76 ++++++++++++++++++++++ src/server/http/__tests__/xsrf.js | 40 ++++++------ src/server/http/index.js | 4 ++ src/server/http/version_check.js | 19 ++++++ src/server/http/xsrf.js | 22 +++---- 5 files changed, 131 insertions(+), 30 deletions(-) create mode 100644 src/server/http/__tests__/version_check.js create mode 100644 src/server/http/version_check.js diff --git a/src/server/http/__tests__/version_check.js b/src/server/http/__tests__/version_check.js new file mode 100644 index 0000000000000..389d07cd4bf6b --- /dev/null +++ b/src/server/http/__tests__/version_check.js @@ -0,0 +1,76 @@ +import expect from 'expect.js'; +import { fromNode } from 'bluebird'; +import { resolve } from 'path'; +import * as kbnTestServer from '../../../../test/utils/kbn_server'; + +const src = resolve.bind(null, __dirname, '../../../../src'); + +const versionHeader = 'kbn-version'; +const version = require(src('../package.json')).version; + +describe('version_check request filter', function () { + function makeRequest(kbnServer, opts) { + return fromNode(cb => { + kbnTestServer.makeRequest(kbnServer, opts, (resp) => { + cb(null, resp); + }); + }); + } + + async function makeServer() { + const kbnServer = kbnTestServer.createServer(); + + await kbnServer.ready(); + + kbnServer.server.route({ + path: '/version_check/test/route', + method: 'GET', + handler: function (req, reply) { + reply(null, 'ok'); + } + }); + + return kbnServer; + }; + + let kbnServer; + beforeEach(async () => kbnServer = await makeServer()); + afterEach(async () => await kbnServer.close()); + + it('accepts requests with the correct version passed in the version header', async function () { + const resp = await makeRequest(kbnServer, { + url: '/version_check/test/route', + method: 'GET', + headers: { + [versionHeader]: version, + }, + }); + + expect(resp.statusCode).to.be(200); + expect(resp.payload).to.be('ok'); + }); + + it('rejects requests with an incorrect version passed in the version header', async function () { + const resp = await makeRequest(kbnServer, { + url: '/version_check/test/route', + method: 'GET', + headers: { + [versionHeader]: `invalid:${version}`, + }, + }); + + expect(resp.statusCode).to.be(400); + expect(resp.headers).to.have.property(versionHeader, version); + expect(resp.payload).to.match(/"Browser client is out of date/); + }); + + it('accepts requests that do not include a version header', async function () { + const resp = await makeRequest(kbnServer, { + url: '/version_check/test/route', + method: 'GET' + }); + + expect(resp.statusCode).to.be(200); + expect(resp.payload).to.be('ok'); + }); +}); diff --git a/src/server/http/__tests__/xsrf.js b/src/server/http/__tests__/xsrf.js index 81d600ea70b55..65c02b1e15778 100644 --- a/src/server/http/__tests__/xsrf.js +++ b/src/server/http/__tests__/xsrf.js @@ -7,8 +7,9 @@ const nonDestructiveMethods = ['GET', 'HEAD']; const destructiveMethods = ['POST', 'PUT', 'DELETE']; const src = resolve.bind(null, __dirname, '../../../../src'); -const xsrfHeader = 'kbn-version'; -const version = require(src('../package.json')).version; +const xsrfHeader = 'kbn-xsrf'; +const versionHeader = 'kbn-version'; +const actualVersion = require(src('../package.json')).version; describe('xsrf request filter', function () { function inject(kbnServer, opts) { @@ -57,31 +58,30 @@ describe('xsrf request filter', function () { else expect(resp.payload).to.be('ok'); }); - it('failes on invalid tokens', async function () { + it('accepts requests with the xsrf header', async function () { const resp = await inject(kbnServer, { url: '/xsrf/test/route', method: method, headers: { - [xsrfHeader]: `invalid:${version}`, + [xsrfHeader]: 'anything', }, }); - expect(resp.statusCode).to.be(400); - expect(resp.headers).to.have.property(xsrfHeader, version); + expect(resp.statusCode).to.be(200); if (method === 'HEAD') expect(resp.payload).to.be.empty(); - else expect(resp.payload).to.match(/"Browser client is out of date/); + else expect(resp.payload).to.be('ok'); }); }); } for (const method of destructiveMethods) { context(`destructiveMethod: ${method}`, function () { // eslint-disable-line no-loop-func - it('accepts requests with the correct token', async function () { + it('accepts requests with the xsrf header', async function () { const resp = await inject(kbnServer, { url: '/xsrf/test/route', method: method, headers: { - [xsrfHeader]: version, + [xsrfHeader]: 'anything', }, }); @@ -89,27 +89,29 @@ describe('xsrf request filter', function () { expect(resp.payload).to.be('ok'); }); - it('rejects requests without a token', async function () { + // this is still valid for existing csrf protection support + // it does not actually do any validation on the version value itself + it('accepts requests with the version header', async function () { const resp = await inject(kbnServer, { url: '/xsrf/test/route', - method: method + method: method, + headers: { + [versionHeader]: actualVersion, + }, }); - expect(resp.statusCode).to.be(400); - expect(resp.payload).to.match(/"Missing kbn-version header/); + expect(resp.statusCode).to.be(200); + expect(resp.payload).to.be('ok'); }); - it('rejects requests with an invalid token', async function () { + it('rejects requests without either an xsrf or version header', async function () { const resp = await inject(kbnServer, { url: '/xsrf/test/route', - method: method, - headers: { - [xsrfHeader]: `invalid:${version}`, - }, + method: method }); expect(resp.statusCode).to.be(400); - expect(resp.payload).to.match(/"Browser client is out of date/); + expect(resp.payload).to.match(/"Request must contain an kbn-xsrf header/); }); }); } diff --git a/src/server/http/index.js b/src/server/http/index.js index b6440bec66044..46e19e07aa4d6 100644 --- a/src/server/http/index.js +++ b/src/server/http/index.js @@ -5,6 +5,8 @@ import fs from 'fs'; import Boom from 'boom'; import Hapi from 'hapi'; import getDefaultRoute from './get_default_route'; +import versionCheckMixin from './version_check'; + module.exports = async function (kbnServer, server, config) { @@ -132,5 +134,7 @@ module.exports = async function (kbnServer, server, config) { } }); + kbnServer.mixin(versionCheckMixin); + return kbnServer.mixin(require('./xsrf')); }; diff --git a/src/server/http/version_check.js b/src/server/http/version_check.js new file mode 100644 index 0000000000000..e0897ec41303c --- /dev/null +++ b/src/server/http/version_check.js @@ -0,0 +1,19 @@ +import { badRequest } from 'boom'; + +export default function (kbnServer, server, config) { + const versionHeader = 'kbn-version'; + const actualVersion = config.get('pkg.version'); + + server.ext('onPostAuth', function (req, reply) { + const versionRequested = req.headers[versionHeader]; + + if (versionRequested && versionRequested !== actualVersion) { + return reply(badRequest('Browser client is out of date, please refresh the page', { + expected: actualVersion, + got: versionRequested + })); + } + + return reply.continue(); + }); +} diff --git a/src/server/http/xsrf.js b/src/server/http/xsrf.js index 1cfdb89068ffb..63790672abd28 100644 --- a/src/server/http/xsrf.js +++ b/src/server/http/xsrf.js @@ -1,21 +1,21 @@ import { badRequest } from 'boom'; export default function (kbnServer, server, config) { - const version = config.get('pkg.version'); const disabled = config.get('server.xsrf.disableProtection'); - const header = 'kbn-version'; + const versionHeader = 'kbn-version'; + const xsrfHeader = 'kbn-xsrf'; server.ext('onPostAuth', function (req, reply) { - const noHeaderGet = (req.method === 'get' || req.method === 'head') && !req.headers[header]; - if (disabled || noHeaderGet) return reply.continue(); + if (disabled) { + return reply.continue(); + } + + const isSafeMethod = req.method === 'get' || req.method === 'head'; + const hasVersionHeader = versionHeader in req.headers; + const hasXsrfHeader = xsrfHeader in req.headers; - const submission = req.headers[header]; - if (!submission) return reply(badRequest(`Missing ${header} header`)); - if (submission !== version) { - return reply(badRequest('Browser client is out of date, please refresh the page', { - expected: version, - got: submission - })); + if (!isSafeMethod && !hasVersionHeader && !hasXsrfHeader) { + return reply(badRequest(`Request must contain an ${xsrfHeader} header`)); } return reply.continue();