mirror of
https://github.com/elastic/kibana.git
synced 2025-06-27 18:51:07 -04:00
[Sessions] Check and allow session cookies if they are all the same (#220430)
## Summary Closes https://github.com/elastic/kibana/issues/220637 Closes https://github.com/elastic/kibana/issues/220755 Testing steps: #### Steps to reproduce the bug, in firefox and on main: - Download/update to latest firefox version - Run ES and kibana locally - Login to Kibana - Set up the sample data (E-commerce for example) - Force cache disabling (open dev tools -> Network tab -> Ensure Disable cache is not checked and keep dev tools open) - Navigate to Stack Management -> Data views (under kibana in left nav) - Click on first data view - Triggers a logout ### Verify the fix - Run the same steps above on this PR ## Release notes Rework cookie and session storage to prevent unexpected logouts for certain users with certain use cases. --------- Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
This commit is contained in:
parent
6ad900d8b4
commit
7a3bd993ce
6 changed files with 216 additions and 11 deletions
|
@ -305,6 +305,7 @@ enabled:
|
|||
- x-pack/test/security_api_integration/saml_cloud.config.ts
|
||||
- x-pack/test/security_api_integration/chips.config.ts
|
||||
- x-pack/test/security_api_integration/features.config.ts
|
||||
- x-pack/test/security_api_integration/session_cookie.config.ts
|
||||
- x-pack/test/security_api_integration/session_idle.config.ts
|
||||
- x-pack/test/security_api_integration/session_shard_missing.config.ts
|
||||
- x-pack/test/security_api_integration/session_invalidate.config.ts
|
||||
|
|
|
@ -18,6 +18,8 @@ import type {
|
|||
SessionStorageCookieOptions,
|
||||
} from '@kbn/core-http-server';
|
||||
|
||||
import { isDeepStrictEqual } from 'util';
|
||||
|
||||
import { ensureRawRequest } from '@kbn/core-http-router-server-internal';
|
||||
|
||||
class ScopedCookieSessionStorage<T extends object> implements SessionStorage<T> {
|
||||
|
@ -30,21 +32,41 @@ class ScopedCookieSessionStorage<T extends object> implements SessionStorage<T>
|
|||
public async get(): Promise<T | null> {
|
||||
try {
|
||||
const session = await this.server.auth.test('security-cookie', this.request);
|
||||
|
||||
// A browser can send several cookies, if it's not an array, just return the session value
|
||||
if (!Array.isArray(session)) {
|
||||
if (!Array.isArray(session.credentials)) {
|
||||
return session.credentials as T;
|
||||
}
|
||||
|
||||
// If we have an array with one value, we're good also
|
||||
if (session.length === 1) {
|
||||
return session[0] as T;
|
||||
if (session.credentials.length === 1) {
|
||||
return session.credentials[0] as T;
|
||||
}
|
||||
|
||||
// Otherwise, we have more than one and won't be authing the user because we don't
|
||||
// know which session identifies the actual user. There's potential to change this behavior
|
||||
// to ensure all valid sessions identify the same user, or choose one valid one, but this
|
||||
// is the safest option.
|
||||
this.log.warn(`Found ${session.length} auth sessions when we were only expecting 1.`);
|
||||
// If we have more than one session, return the first one if they are all the same
|
||||
if (session.credentials.length > 1) {
|
||||
this.log.warn(
|
||||
`Found multiple auth sessions. Found:[${session.credentials.length}] sessions. Checking equality...`
|
||||
);
|
||||
const [firstSession, ...rest] = session.credentials;
|
||||
const allEqual = rest.every((s) => {
|
||||
return isDeepStrictEqual(s, firstSession);
|
||||
});
|
||||
if (allEqual) {
|
||||
this.log.error(
|
||||
`Found multiple auth sessions. Found:[${session.credentials.length}] equal sessions`
|
||||
);
|
||||
return firstSession as T;
|
||||
}
|
||||
}
|
||||
|
||||
// Otherwise, we have more than one session that are not the same as each other
|
||||
// and won't be authing the user because we don't know which session identifies
|
||||
// the actual user. There's potential to change this behavior to ensure all valid sessions
|
||||
// identify the same user, or choose one valid one, but this is the safest option.
|
||||
this.log.warn(
|
||||
`Found multiple auth sessions. Found:[${session.credentials.length}] unequal sessions`
|
||||
);
|
||||
return null;
|
||||
} catch (error) {
|
||||
this.log.debug(String(error));
|
||||
|
|
|
@ -326,7 +326,9 @@ describe('Cookie based SessionStorage', () => {
|
|||
register: jest.fn(),
|
||||
auth: {
|
||||
strategy: jest.fn(),
|
||||
test: jest.fn(() => ['foo', 'bar']),
|
||||
test: jest.fn(() => ({
|
||||
credentials: ['foo', 'bar'],
|
||||
})),
|
||||
},
|
||||
};
|
||||
|
||||
|
@ -352,7 +354,48 @@ describe('Cookie based SessionStorage', () => {
|
|||
);
|
||||
|
||||
expect(loggingSystemMock.collect(logger).warn).toEqual([
|
||||
['Found 2 auth sessions when we were only expecting 1.'],
|
||||
['Found multiple auth sessions. Found:[2] sessions. Checking equality...'],
|
||||
['Found multiple auth sessions. Found:[2] unequal sessions'],
|
||||
]);
|
||||
});
|
||||
|
||||
it('returns sessions if multiple session cookies are detected and are equal.', async () => {
|
||||
const mockServer = {
|
||||
register: jest.fn(),
|
||||
auth: {
|
||||
strategy: jest.fn(),
|
||||
test: jest.fn(() => ({
|
||||
credentials: ['foo', 'foo'],
|
||||
})),
|
||||
},
|
||||
};
|
||||
|
||||
const mockRequest = httpServerMock.createKibanaRequest();
|
||||
|
||||
const factory = await createCookieSessionStorageFactory(
|
||||
logger.get(),
|
||||
mockServer as any,
|
||||
cookieOptions,
|
||||
true
|
||||
);
|
||||
|
||||
expect(mockServer.register).toBeCalledTimes(1);
|
||||
expect(mockServer.auth.strategy).toBeCalledTimes(1);
|
||||
|
||||
const session = await factory.asScoped(mockRequest).get();
|
||||
expect(session).toBe('foo');
|
||||
|
||||
expect(mockServer.auth.test).toBeCalledTimes(1);
|
||||
expect(mockServer.auth.test).toHaveBeenCalledWith(
|
||||
'security-cookie',
|
||||
ensureRawRequest(mockRequest)
|
||||
);
|
||||
|
||||
expect(loggingSystemMock.collect(logger).warn).toEqual([
|
||||
['Found multiple auth sessions. Found:[2] sessions. Checking equality...'],
|
||||
]);
|
||||
expect(loggingSystemMock.collect(logger).error).toEqual([
|
||||
['Found multiple auth sessions. Found:[2] equal sessions'],
|
||||
]);
|
||||
});
|
||||
|
||||
|
@ -361,7 +404,9 @@ describe('Cookie based SessionStorage', () => {
|
|||
register: jest.fn(),
|
||||
auth: {
|
||||
strategy: jest.fn(),
|
||||
test: jest.fn(() => ['foo']),
|
||||
test: jest.fn(() => ({
|
||||
credentials: ['foo'],
|
||||
})),
|
||||
},
|
||||
};
|
||||
|
||||
|
|
|
@ -0,0 +1,37 @@
|
|||
/*
|
||||
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
|
||||
* or more contributor license agreements. Licensed under the Elastic License
|
||||
* 2.0; you may not use this file except in compliance with the Elastic License
|
||||
* 2.0.
|
||||
*/
|
||||
|
||||
import { resolve } from 'path';
|
||||
|
||||
import type { FtrConfigProviderContext } from '@kbn/test';
|
||||
|
||||
import { services } from './services';
|
||||
|
||||
// the default export of config files must be a config provider
|
||||
// that returns an object with the projects config values
|
||||
export default async function ({ readConfigFile }: FtrConfigProviderContext) {
|
||||
const xPackAPITestsConfig = await readConfigFile(require.resolve('../api_integration/config.ts'));
|
||||
|
||||
return {
|
||||
testFiles: [resolve(__dirname, './tests/session_cookie')],
|
||||
services,
|
||||
servers: xPackAPITestsConfig.get('servers'),
|
||||
esTestCluster: {
|
||||
...xPackAPITestsConfig.get('esTestCluster'),
|
||||
serverArgs: [...xPackAPITestsConfig.get('esTestCluster.serverArgs')],
|
||||
},
|
||||
|
||||
kbnTestServer: {
|
||||
...xPackAPITestsConfig.get('kbnTestServer'),
|
||||
serverArgs: [...xPackAPITestsConfig.get('kbnTestServer.serverArgs')],
|
||||
},
|
||||
|
||||
junit: {
|
||||
reportName: 'X-Pack Security API Integration Tests (Session Cookies)',
|
||||
},
|
||||
};
|
||||
}
|
|
@ -0,0 +1,14 @@
|
|||
/*
|
||||
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
|
||||
* or more contributor license agreements. Licensed under the Elastic License
|
||||
* 2.0; you may not use this file except in compliance with the Elastic License
|
||||
* 2.0.
|
||||
*/
|
||||
|
||||
import type { FtrProviderContext } from '../../ftr_provider_context';
|
||||
|
||||
export default function ({ loadTestFile }: FtrProviderContext) {
|
||||
describe('security APIs - Session Cookie', function () {
|
||||
loadTestFile(require.resolve('./session_cookie'));
|
||||
});
|
||||
}
|
|
@ -0,0 +1,86 @@
|
|||
/*
|
||||
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
|
||||
* or more contributor license agreements. Licensed under the Elastic License
|
||||
* 2.0; you may not use this file except in compliance with the Elastic License
|
||||
* 2.0.
|
||||
*/
|
||||
|
||||
import { parse as parseCookie } from 'tough-cookie';
|
||||
|
||||
import expect from '@kbn/expect';
|
||||
import { adminTestUser, kibanaTestUser } from '@kbn/test';
|
||||
|
||||
import type { FtrProviderContext } from '../../ftr_provider_context';
|
||||
|
||||
export default function ({ getService }: FtrProviderContext) {
|
||||
const supertest = getService('supertestWithoutAuth');
|
||||
|
||||
async function adminSessionCookie() {
|
||||
const response = await supertest
|
||||
.post('/internal/security/login')
|
||||
.set('kbn-xsrf', 'xxx')
|
||||
.send({
|
||||
providerType: 'basic',
|
||||
providerName: 'basic',
|
||||
currentURL: '/',
|
||||
params: { username: adminTestUser.username, password: adminTestUser.password },
|
||||
})
|
||||
.expect(200);
|
||||
|
||||
const cookies = response.headers['set-cookie'];
|
||||
expect(cookies).to.have.length(1);
|
||||
|
||||
const cookie = parseCookie(cookies[0])!;
|
||||
return cookie;
|
||||
}
|
||||
|
||||
async function basicSessionCookie() {
|
||||
const response = await supertest
|
||||
.post('/internal/security/login')
|
||||
.set('kbn-xsrf', 'xxx')
|
||||
.send({
|
||||
providerType: 'basic',
|
||||
providerName: 'basic',
|
||||
currentURL: '/',
|
||||
params: { username: kibanaTestUser.username, password: kibanaTestUser.password },
|
||||
})
|
||||
.expect(200);
|
||||
|
||||
const cookies = response.headers['set-cookie'];
|
||||
expect(cookies).to.have.length(1);
|
||||
|
||||
const cookie = parseCookie(cookies[0])!;
|
||||
return cookie;
|
||||
}
|
||||
|
||||
describe('Session Cookie', function () {
|
||||
it('should allow a single valid cookie', async () => {
|
||||
const cookie = await adminSessionCookie();
|
||||
await supertest
|
||||
.get('/internal/security/me')
|
||||
.set('kbn-xsrf', 'xxx')
|
||||
.set('Cookie', cookie.cookieString())
|
||||
.expect(200);
|
||||
});
|
||||
|
||||
it('should allow a multiple cookies that are the same', async () => {
|
||||
const cookie = await adminSessionCookie();
|
||||
|
||||
await supertest
|
||||
.get('/internal/security/me')
|
||||
.set('kbn-xsrf', 'xxx')
|
||||
.set('Cookie', [cookie.cookieString(), cookie.cookieString()])
|
||||
.expect(200);
|
||||
});
|
||||
|
||||
it('should not allow multiple different cookies', async () => {
|
||||
const cookie = await adminSessionCookie();
|
||||
const basicUserCookie = await basicSessionCookie();
|
||||
await supertest
|
||||
.get('/internal/security/me')
|
||||
.set('kbn-xsrf', 'xxx')
|
||||
.set('Cookie', [cookie.cookieString(), basicUserCookie.cookieString()])
|
||||
.expect(401);
|
||||
});
|
||||
});
|
||||
}
|
Loading…
Add table
Add a link
Reference in a new issue