From f75d72b2dc4ecda476e81014af65be2a76e71bad Mon Sep 17 00:00:00 2001 From: Joe Portner <5295965+jportner@users.noreply.github.com> Date: Tue, 12 Nov 2019 11:34:06 -0500 Subject: [PATCH 1/5] Add extra validation checks for cookies Cookies are now checked for attributes that match the current Kibana configuration. --- .../core/server/kibana-plugin-server.md | 1 + ....sessioncookievalidationresult.issecure.md | 13 ++ ...r.sessioncookievalidationresult.isvalid.md | 13 ++ ...in-server.sessioncookievalidationresult.md | 22 ++++ ...rver.sessioncookievalidationresult.path.md | 13 ++ ...ssionstoragecookieoptions.encryptionkey.md | 2 +- ...ugin-server.sessionstoragecookieoptions.md | 4 +- ...er.sessionstoragecookieoptions.validate.md | 4 +- .../server/http/cookie_session_storage.ts | 49 +++++++- .../server/http/cookie_sesson_storage.test.ts | 68 +++++++++-- src/core/server/http/http_server.test.ts | 2 +- src/core/server/http/index.ts | 5 +- .../integration_tests/core_services.test.ts | 2 +- .../http/integration_tests/lifecycle.test.ts | 2 +- src/core/server/index.ts | 1 + src/core/server/server.api.md | 9 +- .../authentication/authenticator.test.ts | 113 +++++++++--------- .../server/authentication/authenticator.ts | 28 ++++- .../security/server/authentication/index.ts | 30 ++++- 19 files changed, 295 insertions(+), 86 deletions(-) create mode 100644 docs/development/core/server/kibana-plugin-server.sessioncookievalidationresult.issecure.md create mode 100644 docs/development/core/server/kibana-plugin-server.sessioncookievalidationresult.isvalid.md create mode 100644 docs/development/core/server/kibana-plugin-server.sessioncookievalidationresult.md create mode 100644 docs/development/core/server/kibana-plugin-server.sessioncookievalidationresult.path.md diff --git a/docs/development/core/server/kibana-plugin-server.md b/docs/development/core/server/kibana-plugin-server.md index 9907750b8742f0..8cfec989825490 100644 --- a/docs/development/core/server/kibana-plugin-server.md +++ b/docs/development/core/server/kibana-plugin-server.md @@ -114,6 +114,7 @@ The plugin integrates with the core system via lifecycle events: `setup` | [SavedObjectsResolveImportErrorsOptions](./kibana-plugin-server.savedobjectsresolveimporterrorsoptions.md) | Options to control the "resolve import" operation. | | [SavedObjectsUpdateOptions](./kibana-plugin-server.savedobjectsupdateoptions.md) | | | [SavedObjectsUpdateResponse](./kibana-plugin-server.savedobjectsupdateresponse.md) | | +| [SessionCookieValidationResult](./kibana-plugin-server.sessioncookievalidationresult.md) | Return type from a function to validate cookie contents. | | [SessionStorage](./kibana-plugin-server.sessionstorage.md) | Provides an interface to store and retrieve data across requests. | | [SessionStorageCookieOptions](./kibana-plugin-server.sessionstoragecookieoptions.md) | Configuration used to create HTTP session storage based on top of cookie mechanism. | | [SessionStorageFactory](./kibana-plugin-server.sessionstoragefactory.md) | SessionStorage factory to bind one to an incoming request | diff --git a/docs/development/core/server/kibana-plugin-server.sessioncookievalidationresult.issecure.md b/docs/development/core/server/kibana-plugin-server.sessioncookievalidationresult.issecure.md new file mode 100644 index 00000000000000..ae9d5c58113d1b --- /dev/null +++ b/docs/development/core/server/kibana-plugin-server.sessioncookievalidationresult.issecure.md @@ -0,0 +1,13 @@ + + +[Home](./index.md) > [kibana-plugin-server](./kibana-plugin-server.md) > [SessionCookieValidationResult](./kibana-plugin-server.sessioncookievalidationresult.md) > [isSecure](./kibana-plugin-server.sessioncookievalidationresult.issecure.md) + +## SessionCookieValidationResult.isSecure property + +The "Secure" attribute of the cookie; if the cookie is invalid, this is used to clear it. + +Signature: + +```typescript +isSecure?: boolean; +``` diff --git a/docs/development/core/server/kibana-plugin-server.sessioncookievalidationresult.isvalid.md b/docs/development/core/server/kibana-plugin-server.sessioncookievalidationresult.isvalid.md new file mode 100644 index 00000000000000..6e5f6acca2eb90 --- /dev/null +++ b/docs/development/core/server/kibana-plugin-server.sessioncookievalidationresult.isvalid.md @@ -0,0 +1,13 @@ + + +[Home](./index.md) > [kibana-plugin-server](./kibana-plugin-server.md) > [SessionCookieValidationResult](./kibana-plugin-server.sessioncookievalidationresult.md) > [isValid](./kibana-plugin-server.sessioncookievalidationresult.isvalid.md) + +## SessionCookieValidationResult.isValid property + +Whether the cookie is valid or not. + +Signature: + +```typescript +isValid: boolean; +``` diff --git a/docs/development/core/server/kibana-plugin-server.sessioncookievalidationresult.md b/docs/development/core/server/kibana-plugin-server.sessioncookievalidationresult.md new file mode 100644 index 00000000000000..748f8688110850 --- /dev/null +++ b/docs/development/core/server/kibana-plugin-server.sessioncookievalidationresult.md @@ -0,0 +1,22 @@ + + +[Home](./index.md) > [kibana-plugin-server](./kibana-plugin-server.md) > [SessionCookieValidationResult](./kibana-plugin-server.sessioncookievalidationresult.md) + +## SessionCookieValidationResult interface + +Return type from a function to validate cookie contents. + +Signature: + +```typescript +export interface SessionCookieValidationResult +``` + +## Properties + +| Property | Type | Description | +| --- | --- | --- | +| [isSecure](./kibana-plugin-server.sessioncookievalidationresult.issecure.md) | boolean | The "Secure" attribute of the cookie; if the cookie is invalid, this is used to clear it. | +| [isValid](./kibana-plugin-server.sessioncookievalidationresult.isvalid.md) | boolean | Whether the cookie is valid or not. | +| [path](./kibana-plugin-server.sessioncookievalidationresult.path.md) | string | The "Path" attribute of the cookie; if the cookie is invalid, this is used to clear it. | + diff --git a/docs/development/core/server/kibana-plugin-server.sessioncookievalidationresult.path.md b/docs/development/core/server/kibana-plugin-server.sessioncookievalidationresult.path.md new file mode 100644 index 00000000000000..8ca6d452213aac --- /dev/null +++ b/docs/development/core/server/kibana-plugin-server.sessioncookievalidationresult.path.md @@ -0,0 +1,13 @@ + + +[Home](./index.md) > [kibana-plugin-server](./kibana-plugin-server.md) > [SessionCookieValidationResult](./kibana-plugin-server.sessioncookievalidationresult.md) > [path](./kibana-plugin-server.sessioncookievalidationresult.path.md) + +## SessionCookieValidationResult.path property + +The "Path" attribute of the cookie; if the cookie is invalid, this is used to clear it. + +Signature: + +```typescript +path?: string; +``` diff --git a/docs/development/core/server/kibana-plugin-server.sessionstoragecookieoptions.encryptionkey.md b/docs/development/core/server/kibana-plugin-server.sessionstoragecookieoptions.encryptionkey.md index 167ab03d7567f5..ef65735e7bdbab 100644 --- a/docs/development/core/server/kibana-plugin-server.sessionstoragecookieoptions.encryptionkey.md +++ b/docs/development/core/server/kibana-plugin-server.sessionstoragecookieoptions.encryptionkey.md @@ -4,7 +4,7 @@ ## SessionStorageCookieOptions.encryptionKey property -A key used to encrypt a cookie value. Should be at least 32 characters long. +A key used to encrypt a cookie's value. Should be at least 32 characters long. Signature: diff --git a/docs/development/core/server/kibana-plugin-server.sessionstoragecookieoptions.md b/docs/development/core/server/kibana-plugin-server.sessionstoragecookieoptions.md index de412818142f25..62fe10248561ee 100644 --- a/docs/development/core/server/kibana-plugin-server.sessionstoragecookieoptions.md +++ b/docs/development/core/server/kibana-plugin-server.sessionstoragecookieoptions.md @@ -16,8 +16,8 @@ export interface SessionStorageCookieOptions | Property | Type | Description | | --- | --- | --- | -| [encryptionKey](./kibana-plugin-server.sessionstoragecookieoptions.encryptionkey.md) | string | A key used to encrypt a cookie value. Should be at least 32 characters long. | +| [encryptionKey](./kibana-plugin-server.sessionstoragecookieoptions.encryptionkey.md) | string | A key used to encrypt a cookie's value. Should be at least 32 characters long. | | [isSecure](./kibana-plugin-server.sessionstoragecookieoptions.issecure.md) | boolean | Flag indicating whether the cookie should be sent only via a secure connection. | | [name](./kibana-plugin-server.sessionstoragecookieoptions.name.md) | string | Name of the session cookie. | -| [validate](./kibana-plugin-server.sessionstoragecookieoptions.validate.md) | (sessionValue: T) => boolean | Promise<boolean> | Function called to validate a cookie content. | +| [validate](./kibana-plugin-server.sessionstoragecookieoptions.validate.md) | (sessionValue: T) => SessionCookieValidationResult | Function called to validate a cookie's decrypted value. | diff --git a/docs/development/core/server/kibana-plugin-server.sessionstoragecookieoptions.validate.md b/docs/development/core/server/kibana-plugin-server.sessionstoragecookieoptions.validate.md index f3cbfc0d84e18e..795f08e81cbef6 100644 --- a/docs/development/core/server/kibana-plugin-server.sessionstoragecookieoptions.validate.md +++ b/docs/development/core/server/kibana-plugin-server.sessionstoragecookieoptions.validate.md @@ -4,10 +4,10 @@ ## SessionStorageCookieOptions.validate property -Function called to validate a cookie content. +Function called to validate a cookie's decrypted value. Signature: ```typescript -validate: (sessionValue: T) => boolean | Promise; +validate: (sessionValue: T) => SessionCookieValidationResult; ``` diff --git a/src/core/server/http/cookie_session_storage.ts b/src/core/server/http/cookie_session_storage.ts index 8a1b56d87fb4c9..c8b00fe96ad092 100644 --- a/src/core/server/http/cookie_session_storage.ts +++ b/src/core/server/http/cookie_session_storage.ts @@ -34,19 +34,38 @@ export interface SessionStorageCookieOptions { */ name: string; /** - * A key used to encrypt a cookie value. Should be at least 32 characters long. + * A key used to encrypt a cookie's value. Should be at least 32 characters long. */ encryptionKey: string; /** - * Function called to validate a cookie content. + * Function called to validate a cookie's decrypted value. */ - validate: (sessionValue: T) => boolean | Promise; + validate: (sessionValue: T) => SessionCookieValidationResult; /** * Flag indicating whether the cookie should be sent only via a secure connection. */ isSecure: boolean; } +/** + * Return type from a function to validate cookie contents. + * @public + */ +export interface SessionCookieValidationResult { + /** + * Whether the cookie is valid or not. + */ + isValid: boolean; + /** + * The "Path" attribute of the cookie; if the cookie is invalid, this is used to clear it. + */ + path?: string; + /** + * The "Secure" attribute of the cookie; if the cookie is invalid, this is used to clear it. + */ + isSecure?: boolean; +} + class ScopedCookieSessionStorage> implements SessionStorage { constructor( private readonly log: Logger, @@ -98,15 +117,35 @@ export async function createCookieSessionStorageFactory( cookieOptions: SessionStorageCookieOptions, basePath?: string ): Promise> { + function clearInvalidCookie( + req: Request | undefined, + path: string = basePath || '/', + isSecure: boolean = cookieOptions.isSecure + ) { + // if the cookie did not include the 'path' or 'isSecure' attributes in the session value, it is a legacy cookie + // we will assume that the cookie was created with the current configuration + log.debug(`Clearing invalid session cookie`); + // need to use Hapi toolkit to clear cookie with defined options + if (req) { + (req.cookieAuth as any).h.unstate(cookieOptions.name, { path, isSecure }); + } + } + await server.register({ plugin: hapiAuthCookie }); server.auth.strategy('security-cookie', 'cookie', { cookie: cookieOptions.name, password: cookieOptions.encryptionKey, - validateFunc: async (req, session: T) => ({ valid: await cookieOptions.validate(session) }), + validateFunc: async (req, session: T) => { + const result = cookieOptions.validate(session); + if (!result.isValid) { + clearInvalidCookie(req, result.path, result.isSecure); + } + return { valid: result.isValid }; + }, isSecure: cookieOptions.isSecure, path: basePath, - clearInvalid: true, + clearInvalid: false, isHttpOnly: true, isSameSite: false, }); diff --git a/src/core/server/http/cookie_sesson_storage.test.ts b/src/core/server/http/cookie_sesson_storage.test.ts index 5cd2fbaa1ebe8a..4d1031d5d63cb5 100644 --- a/src/core/server/http/cookie_sesson_storage.test.ts +++ b/src/core/server/http/cookie_sesson_storage.test.ts @@ -80,6 +80,7 @@ interface User { interface Storage { value: User; expires: number; + path: string; } function retrieveSessionCookie(cookies: string) { @@ -92,13 +93,18 @@ function retrieveSessionCookie(cookies: string) { const userData = { id: '42' }; const sessionDurationMs = 1000; +const path = '/'; +const sessVal = () => ({ value: userData, expires: Date.now() + sessionDurationMs, path }); const delay = (ms: number) => new Promise(res => setTimeout(res, ms)); const cookieOptions = { name: 'sid', encryptionKey: 'something_at_least_32_characters', - validate: (session: Storage) => session.expires > Date.now(), + validate: (session: Storage) => { + const isValid = session.path === path && session.expires > Date.now(); + return { isValid, path: session.path, isSecure: false }; + }, isSecure: false, - path: '/', + path, }; describe('Cookie based SessionStorage', () => { @@ -107,9 +113,9 @@ describe('Cookie based SessionStorage', () => { const { server: innerServer, createRouter } = await server.setup(setupDeps); const router = createRouter(''); - router.get({ path: '/', validate: false }, (context, req, res) => { + router.get({ path, validate: false }, (context, req, res) => { const sessionStorage = factory.asScoped(req); - sessionStorage.set({ value: userData, expires: Date.now() + sessionDurationMs }); + sessionStorage.set(sessVal()); return res.ok({}); }); @@ -136,6 +142,7 @@ describe('Cookie based SessionStorage', () => { expect(sessionCookie.httpOnly).toBe(true); }); }); + describe('#get()', () => { it('reads from session storage', async () => { const { server: innerServer, createRouter } = await server.setup(setupDeps); @@ -145,7 +152,7 @@ describe('Cookie based SessionStorage', () => { const sessionStorage = factory.asScoped(req); const sessionValue = await sessionStorage.get(); if (!sessionValue) { - sessionStorage.set({ value: userData, expires: Date.now() + sessionDurationMs }); + sessionStorage.set(sessVal()); return res.ok(); } return res.ok({ body: { value: sessionValue.value } }); @@ -173,6 +180,7 @@ describe('Cookie based SessionStorage', () => { .set('Cookie', `${sessionCookie.key}=${sessionCookie.value}`) .expect(200, { value: userData }); }); + it('returns null for empty session', async () => { const { server: innerServer, createRouter } = await server.setup(setupDeps); @@ -198,7 +206,7 @@ describe('Cookie based SessionStorage', () => { expect(cookies).not.toBeDefined(); }); - it('returns null for invalid session & clean cookies', async () => { + it('returns null for invalid session (expired) & clean cookies', async () => { const { server: innerServer, createRouter } = await server.setup(setupDeps); const router = createRouter(''); @@ -208,7 +216,7 @@ describe('Cookie based SessionStorage', () => { const sessionStorage = factory.asScoped(req); if (!setOnce) { setOnce = true; - sessionStorage.set({ value: userData, expires: Date.now() + sessionDurationMs }); + sessionStorage.set(sessVal()); return res.ok({ body: { value: userData } }); } const sessionValue = await sessionStorage.get(); @@ -242,6 +250,50 @@ describe('Cookie based SessionStorage', () => { 'sid=; Max-Age=0; Expires=Thu, 01 Jan 1970 00:00:00 GMT; HttpOnly; Path=/', ]); }); + + it('returns null for invalid session (incorrect path) & clean cookies accurately', async () => { + const { server: innerServer, createRouter } = await server.setup(setupDeps); + + const router = createRouter(''); + + let setOnce = false; + router.get({ path: '/', validate: false }, async (context, req, res) => { + const sessionStorage = factory.asScoped(req); + if (!setOnce) { + setOnce = true; + sessionStorage.set({ ...sessVal(), path: '/foo' }); + return res.ok({ body: { value: userData } }); + } + const sessionValue = await sessionStorage.get(); + return res.ok({ body: { value: sessionValue } }); + }); + + const factory = await createCookieSessionStorageFactory( + logger.get(), + innerServer, + cookieOptions + ); + await server.start(); + + const response = await supertest(innerServer.listener) + .get('/') + .expect(200, { value: userData }); + + const cookies = response.get('set-cookie'); + expect(cookies).toBeDefined(); + + const sessionCookie = retrieveSessionCookie(cookies[0]); + const response2 = await supertest(innerServer.listener) + .get('/') + .set('Cookie', `${sessionCookie.key}=${sessionCookie.value}`) + .expect(200, { value: null }); + + const cookies2 = response2.get('set-cookie'); + expect(cookies2).toEqual([ + 'sid=; Max-Age=0; Expires=Thu, 01 Jan 1970 00:00:00 GMT; HttpOnly; Path=/foo', + ]); + }); + // use mocks to simplify test setup it('returns null if multiple session cookies are detected.', async () => { const mockServer = { @@ -342,7 +394,7 @@ describe('Cookie based SessionStorage', () => { sessionStorage.clear(); return res.ok({}); } - sessionStorage.set({ value: userData, expires: Date.now() + sessionDurationMs }); + sessionStorage.set(sessVal()); return res.ok({}); }); diff --git a/src/core/server/http/http_server.test.ts b/src/core/server/http/http_server.test.ts index acae9d8ff0e704..ceecfcfea1449c 100644 --- a/src/core/server/http/http_server.test.ts +++ b/src/core/server/http/http_server.test.ts @@ -34,7 +34,7 @@ import { HttpServer } from './http_server'; const cookieOptions = { name: 'sid', encryptionKey: 'something_at_least_32_characters', - validate: () => true, + validate: () => ({ isValid: true }), isSecure: false, }; diff --git a/src/core/server/http/index.ts b/src/core/server/http/index.ts index ff1ff3acfae3d0..895c899b008bb1 100644 --- a/src/core/server/http/index.ts +++ b/src/core/server/http/index.ts @@ -59,6 +59,9 @@ export { } from './lifecycle/auth'; export { OnPostAuthHandler, OnPostAuthToolkit } from './lifecycle/on_post_auth'; export { SessionStorageFactory, SessionStorage } from './session_storage'; -export { SessionStorageCookieOptions } from './cookie_session_storage'; +export { + SessionStorageCookieOptions, + SessionCookieValidationResult, +} from './cookie_session_storage'; export * from './types'; export { BasePath, IBasePath } from './base_path_service'; diff --git a/src/core/server/http/integration_tests/core_services.test.ts b/src/core/server/http/integration_tests/core_services.test.ts index 00629b811b28fc..954702870fc4ea 100644 --- a/src/core/server/http/integration_tests/core_services.test.ts +++ b/src/core/server/http/integration_tests/core_services.test.ts @@ -39,7 +39,7 @@ describe('http service', () => { const cookieOptions = { name: 'sid', encryptionKey: 'something_at_least_32_characters', - validate: (session: StorageData) => true, + validate: (session: StorageData) => ({ isValid: true }), isSecure: false, path: '/', }; diff --git a/src/core/server/http/integration_tests/lifecycle.test.ts b/src/core/server/http/integration_tests/lifecycle.test.ts index 4592a646b7f041..7c4a0097456ca8 100644 --- a/src/core/server/http/integration_tests/lifecycle.test.ts +++ b/src/core/server/http/integration_tests/lifecycle.test.ts @@ -408,7 +408,7 @@ describe('Auth', () => { const cookieOptions = { name: 'sid', encryptionKey: 'something_at_least_32_characters', - validate: () => true, + validate: () => ({ isValid: true }), isSecure: false, }; diff --git a/src/core/server/index.ts b/src/core/server/index.ts index 2a5631ad1c3801..cfd46295450adc 100644 --- a/src/core/server/index.ts +++ b/src/core/server/index.ts @@ -116,6 +116,7 @@ export { RouteConfigOptions, SessionStorage, SessionStorageCookieOptions, + SessionCookieValidationResult, SessionStorageFactory, } from './http'; export { Logger, LoggerFactory, LogMeta, LogRecord, LogLevel } from './logging'; diff --git a/src/core/server/server.api.md b/src/core/server/server.api.md index 97a04a4a4efaba..88aebf490a80c2 100644 --- a/src/core/server/server.api.md +++ b/src/core/server/server.api.md @@ -1561,6 +1561,13 @@ export class ScopedClusterClient implements IScopedClusterClient { callAsInternalUser(endpoint: string, clientParams?: Record, options?: CallAPIOptions): Promise; } +// @public +export interface SessionCookieValidationResult { + isSecure?: boolean; + isValid: boolean; + path?: string; +} + // @public export interface SessionStorage { clear(): void; @@ -1573,7 +1580,7 @@ export interface SessionStorageCookieOptions { encryptionKey: string; isSecure: boolean; name: string; - validate: (sessionValue: T) => boolean | Promise; + validate: (sessionValue: T) => SessionCookieValidationResult; } // @public diff --git a/x-pack/plugins/security/server/authentication/authenticator.test.ts b/x-pack/plugins/security/server/authentication/authenticator.test.ts index 78c6feac0fa297..2ef4f9e1f6223a 100644 --- a/x-pack/plugins/security/server/authentication/authenticator.test.ts +++ b/x-pack/plugins/security/server/authentication/authenticator.test.ts @@ -28,7 +28,12 @@ function getMockOptions(config: Partial = {}) { basePath: httpServiceMock.createSetupContract().basePath, loggers: loggingServiceMock.create(), isSystemAPIRequest: jest.fn(), - config: { sessionTimeout: null, authc: { providers: [], oidc: {}, saml: {} }, ...config }, + config: { + sessionTimeout: null, + authc: { providers: [], oidc: {}, saml: {} }, + secureCookies: false, + ...config, + }, sessionStorageFactory: sessionStorageMock.createFactory(), }; } @@ -72,10 +77,18 @@ describe('Authenticator', () => { let authenticator: Authenticator; let mockOptions: ReturnType; let mockSessionStorage: jest.Mocked>; + let mockSessVal: any; beforeEach(() => { mockOptions = getMockOptions({ authc: { providers: ['basic'], oidc: {}, saml: {} } }); mockSessionStorage = sessionStorageMock.create(); mockOptions.sessionStorageFactory.asScoped.mockReturnValue(mockSessionStorage); + mockSessVal = { + expires: null, + state: { authorization: 'Basic xxx' }, + provider: 'basic', + path: mockOptions.basePath.serverBasePath, + secure: mockOptions.config.secureCookies, + }; authenticator = new Authenticator(mockOptions); }); @@ -151,9 +164,8 @@ describe('Authenticator', () => { expect(mockSessionStorage.set).toHaveBeenCalledTimes(1); expect(mockSessionStorage.set).toHaveBeenCalledWith({ - expires: null, + ...mockSessVal, state: { authorization }, - provider: 'basic', }); }); @@ -167,13 +179,12 @@ describe('Authenticator', () => { }); it('clears session if it belongs to a different provider.', async () => { - const state = { authorization: 'Basic xxx' }; const user = mockAuthenticatedUser(); const credentials = { username: 'user', password: 'password' }; const request = httpServerMock.createKibanaRequest(); mockBasicAuthenticationProvider.login.mockResolvedValue(AuthenticationResult.succeeded(user)); - mockSessionStorage.get.mockResolvedValue({ expires: null, state, provider: 'token' }); + mockSessionStorage.get.mockResolvedValue({ ...mockSessVal, provider: 'token' }); const authenticationResult = await authenticator.login(request, { provider: 'basic', @@ -285,10 +296,18 @@ describe('Authenticator', () => { let authenticator: Authenticator; let mockOptions: ReturnType; let mockSessionStorage: jest.Mocked>; + let mockSessVal: any; beforeEach(() => { mockOptions = getMockOptions({ authc: { providers: ['basic'], oidc: {}, saml: {} } }); mockSessionStorage = sessionStorageMock.create(); mockOptions.sessionStorageFactory.asScoped.mockReturnValue(mockSessionStorage); + mockSessVal = { + expires: null, + state: { authorization: 'Basic xxx' }, + provider: 'basic', + path: mockOptions.basePath.serverBasePath, + secure: mockOptions.config.secureCookies, + }; authenticator = new Authenticator(mockOptions); }); @@ -344,9 +363,8 @@ describe('Authenticator', () => { expect(mockSessionStorage.set).toHaveBeenCalledTimes(1); expect(mockSessionStorage.set).toHaveBeenCalledWith({ - expires: null, + ...mockSessVal, state: { authorization }, - provider: 'basic', }); }); @@ -366,22 +384,20 @@ describe('Authenticator', () => { expect(mockSessionStorage.set).toHaveBeenCalledTimes(1); expect(mockSessionStorage.set).toHaveBeenCalledWith({ - expires: null, + ...mockSessVal, state: { authorization }, - provider: 'basic', }); }); it('does not extend session for system API calls.', async () => { const user = mockAuthenticatedUser(); - const state = { authorization: 'Basic xxx' }; const request = httpServerMock.createKibanaRequest(); mockOptions.isSystemAPIRequest.mockReturnValue(true); mockBasicAuthenticationProvider.authenticate.mockResolvedValue( AuthenticationResult.succeeded(user) ); - mockSessionStorage.get.mockResolvedValue({ expires: null, state, provider: 'basic' }); + mockSessionStorage.get.mockResolvedValue(mockSessVal); const authenticationResult = await authenticator.authenticate(request); expect(authenticationResult.succeeded()).toBe(true); @@ -393,31 +409,25 @@ describe('Authenticator', () => { it('extends session for non-system API calls.', async () => { const user = mockAuthenticatedUser(); - const state = { authorization: 'Basic xxx' }; const request = httpServerMock.createKibanaRequest(); mockOptions.isSystemAPIRequest.mockReturnValue(false); mockBasicAuthenticationProvider.authenticate.mockResolvedValue( AuthenticationResult.succeeded(user) ); - mockSessionStorage.get.mockResolvedValue({ expires: null, state, provider: 'basic' }); + mockSessionStorage.get.mockResolvedValue(mockSessVal); const authenticationResult = await authenticator.authenticate(request); expect(authenticationResult.succeeded()).toBe(true); expect(authenticationResult.user).toEqual(user); expect(mockSessionStorage.set).toHaveBeenCalledTimes(1); - expect(mockSessionStorage.set).toHaveBeenCalledWith({ - expires: null, - state, - provider: 'basic', - }); + expect(mockSessionStorage.set).toHaveBeenCalledWith(mockSessVal); expect(mockSessionStorage.clear).not.toHaveBeenCalled(); }); it('properly extends session timeout if it is defined.', async () => { const user = mockAuthenticatedUser(); - const state = { authorization: 'Basic xxx' }; const request = httpServerMock.createKibanaRequest(); const currentDate = new Date(Date.UTC(2019, 10, 10)).valueOf(); @@ -428,7 +438,7 @@ describe('Authenticator', () => { }); mockSessionStorage = sessionStorageMock.create(); - mockSessionStorage.get.mockResolvedValue({ expires: null, state, provider: 'basic' }); + mockSessionStorage.get.mockResolvedValue(mockSessVal); mockOptions.sessionStorageFactory.asScoped.mockReturnValue(mockSessionStorage); authenticator = new Authenticator(mockOptions); @@ -445,22 +455,20 @@ describe('Authenticator', () => { expect(mockSessionStorage.set).toHaveBeenCalledTimes(1); expect(mockSessionStorage.set).toHaveBeenCalledWith({ + ...mockSessVal, expires: currentDate + 3600 * 24, - state, - provider: 'basic', }); expect(mockSessionStorage.clear).not.toHaveBeenCalled(); }); it('does not touch session for system API calls if authentication fails with non-401 reason.', async () => { - const state = { authorization: 'Basic xxx' }; const request = httpServerMock.createKibanaRequest(); mockOptions.isSystemAPIRequest.mockReturnValue(true); mockBasicAuthenticationProvider.authenticate.mockResolvedValue( AuthenticationResult.failed(new Error('some error')) ); - mockSessionStorage.get.mockResolvedValue({ expires: null, state, provider: 'basic' }); + mockSessionStorage.get.mockResolvedValue(mockSessVal); const authenticationResult = await authenticator.authenticate(request); expect(authenticationResult.failed()).toBe(true); @@ -470,14 +478,13 @@ describe('Authenticator', () => { }); it('does not touch session for non-system API calls if authentication fails with non-401 reason.', async () => { - const state = { authorization: 'Basic xxx' }; const request = httpServerMock.createKibanaRequest(); mockOptions.isSystemAPIRequest.mockReturnValue(false); mockBasicAuthenticationProvider.authenticate.mockResolvedValue( AuthenticationResult.failed(new Error('some error')) ); - mockSessionStorage.get.mockResolvedValue({ expires: null, state, provider: 'basic' }); + mockSessionStorage.get.mockResolvedValue(mockSessVal); const authenticationResult = await authenticator.authenticate(request); expect(authenticationResult.failed()).toBe(true); @@ -488,7 +495,6 @@ describe('Authenticator', () => { it('replaces existing session with the one returned by authentication provider for system API requests', async () => { const user = mockAuthenticatedUser(); - const existingState = { authorization: 'Basic xxx' }; const newState = { authorization: 'Basic yyy' }; const request = httpServerMock.createKibanaRequest(); @@ -496,11 +502,7 @@ describe('Authenticator', () => { mockBasicAuthenticationProvider.authenticate.mockResolvedValue( AuthenticationResult.succeeded(user, { state: newState }) ); - mockSessionStorage.get.mockResolvedValue({ - expires: null, - state: existingState, - provider: 'basic', - }); + mockSessionStorage.get.mockResolvedValue(mockSessVal); const authenticationResult = await authenticator.authenticate(request); expect(authenticationResult.succeeded()).toBe(true); @@ -508,16 +510,14 @@ describe('Authenticator', () => { expect(mockSessionStorage.set).toHaveBeenCalledTimes(1); expect(mockSessionStorage.set).toHaveBeenCalledWith({ - expires: null, + ...mockSessVal, state: newState, - provider: 'basic', }); expect(mockSessionStorage.clear).not.toHaveBeenCalled(); }); it('replaces existing session with the one returned by authentication provider for non-system API requests', async () => { const user = mockAuthenticatedUser(); - const existingState = { authorization: 'Basic xxx' }; const newState = { authorization: 'Basic yyy' }; const request = httpServerMock.createKibanaRequest(); @@ -525,11 +525,7 @@ describe('Authenticator', () => { mockBasicAuthenticationProvider.authenticate.mockResolvedValue( AuthenticationResult.succeeded(user, { state: newState }) ); - mockSessionStorage.get.mockResolvedValue({ - expires: null, - state: existingState, - provider: 'basic', - }); + mockSessionStorage.get.mockResolvedValue(mockSessVal); const authenticationResult = await authenticator.authenticate(request); expect(authenticationResult.succeeded()).toBe(true); @@ -537,22 +533,20 @@ describe('Authenticator', () => { expect(mockSessionStorage.set).toHaveBeenCalledTimes(1); expect(mockSessionStorage.set).toHaveBeenCalledWith({ - expires: null, + ...mockSessVal, state: newState, - provider: 'basic', }); expect(mockSessionStorage.clear).not.toHaveBeenCalled(); }); it('clears session if provider failed to authenticate system API request with 401 with active session.', async () => { - const state = { authorization: 'Basic xxx' }; const request = httpServerMock.createKibanaRequest(); mockOptions.isSystemAPIRequest.mockReturnValue(true); mockBasicAuthenticationProvider.authenticate.mockResolvedValue( AuthenticationResult.failed(Boom.unauthorized()) ); - mockSessionStorage.get.mockResolvedValue({ expires: null, state, provider: 'basic' }); + mockSessionStorage.get.mockResolvedValue(mockSessVal); const authenticationResult = await authenticator.authenticate(request); expect(authenticationResult.failed()).toBe(true); @@ -562,14 +556,13 @@ describe('Authenticator', () => { }); it('clears session if provider failed to authenticate non-system API request with 401 with active session.', async () => { - const state = { authorization: 'Basic xxx' }; const request = httpServerMock.createKibanaRequest(); mockOptions.isSystemAPIRequest.mockReturnValue(false); mockBasicAuthenticationProvider.authenticate.mockResolvedValue( AuthenticationResult.failed(Boom.unauthorized()) ); - mockSessionStorage.get.mockResolvedValue({ expires: null, state, provider: 'basic' }); + mockSessionStorage.get.mockResolvedValue(mockSessVal); const authenticationResult = await authenticator.authenticate(request); expect(authenticationResult.failed()).toBe(true); @@ -579,13 +572,12 @@ describe('Authenticator', () => { }); it('clears session if provider requested it via setting state to `null`.', async () => { - const state = { authorization: 'Basic xxx' }; const request = httpServerMock.createKibanaRequest(); mockBasicAuthenticationProvider.authenticate.mockResolvedValue( AuthenticationResult.redirectTo('some-url', { state: null }) ); - mockSessionStorage.get.mockResolvedValue({ expires: null, state, provider: 'basic' }); + mockSessionStorage.get.mockResolvedValue(mockSessVal); const authenticationResult = await authenticator.authenticate(request); expect(authenticationResult.redirected()).toBe(true); @@ -595,14 +587,13 @@ describe('Authenticator', () => { }); it('does not clear session if provider can not handle system API request authentication with active session.', async () => { - const state = { authorization: 'Basic xxx' }; const request = httpServerMock.createKibanaRequest(); mockOptions.isSystemAPIRequest.mockReturnValue(true); mockBasicAuthenticationProvider.authenticate.mockResolvedValue( AuthenticationResult.notHandled() ); - mockSessionStorage.get.mockResolvedValue({ expires: null, state, provider: 'basic' }); + mockSessionStorage.get.mockResolvedValue(mockSessVal); const authenticationResult = await authenticator.authenticate(request); expect(authenticationResult.notHandled()).toBe(true); @@ -612,14 +603,13 @@ describe('Authenticator', () => { }); it('does not clear session if provider can not handle non-system API request authentication with active session.', async () => { - const state = { authorization: 'Basic xxx' }; const request = httpServerMock.createKibanaRequest(); mockOptions.isSystemAPIRequest.mockReturnValue(false); mockBasicAuthenticationProvider.authenticate.mockResolvedValue( AuthenticationResult.notHandled() ); - mockSessionStorage.get.mockResolvedValue({ expires: null, state, provider: 'basic' }); + mockSessionStorage.get.mockResolvedValue(mockSessVal); const authenticationResult = await authenticator.authenticate(request); expect(authenticationResult.notHandled()).toBe(true); @@ -629,14 +619,13 @@ describe('Authenticator', () => { }); it('clears session for system API request if it belongs to not configured provider.', async () => { - const state = { authorization: 'Basic xxx' }; const request = httpServerMock.createKibanaRequest(); mockOptions.isSystemAPIRequest.mockReturnValue(true); mockBasicAuthenticationProvider.authenticate.mockResolvedValue( AuthenticationResult.notHandled() ); - mockSessionStorage.get.mockResolvedValue({ expires: null, state, provider: 'token' }); + mockSessionStorage.get.mockResolvedValue({ ...mockSessVal, provider: 'token' }); const authenticationResult = await authenticator.authenticate(request); expect(authenticationResult.notHandled()).toBe(true); @@ -646,14 +635,13 @@ describe('Authenticator', () => { }); it('clears session for non-system API request if it belongs to not configured provider.', async () => { - const state = { authorization: 'Basic xxx' }; const request = httpServerMock.createKibanaRequest(); mockOptions.isSystemAPIRequest.mockReturnValue(false); mockBasicAuthenticationProvider.authenticate.mockResolvedValue( AuthenticationResult.notHandled() ); - mockSessionStorage.get.mockResolvedValue({ expires: null, state, provider: 'token' }); + mockSessionStorage.get.mockResolvedValue({ ...mockSessVal, provider: 'token' }); const authenticationResult = await authenticator.authenticate(request); expect(authenticationResult.notHandled()).toBe(true); @@ -667,10 +655,18 @@ describe('Authenticator', () => { let authenticator: Authenticator; let mockOptions: ReturnType; let mockSessionStorage: jest.Mocked>; + let mockSessVal: any; beforeEach(() => { mockOptions = getMockOptions({ authc: { providers: ['basic'], oidc: {}, saml: {} } }); mockSessionStorage = sessionStorageMock.create(); mockOptions.sessionStorageFactory.asScoped.mockReturnValue(mockSessionStorage); + mockSessVal = { + expires: null, + state: { authorization: 'Basic xxx' }, + provider: 'basic', + path: mockOptions.basePath.serverBasePath, + secure: mockOptions.config.secureCookies, + }; authenticator = new Authenticator(mockOptions); }); @@ -693,11 +689,10 @@ describe('Authenticator', () => { it('clears session and returns whatever authentication provider returns.', async () => { const request = httpServerMock.createKibanaRequest(); - const state = { authorization: 'Basic xxx' }; mockBasicAuthenticationProvider.logout.mockResolvedValue( DeauthenticationResult.redirectTo('some-url') ); - mockSessionStorage.get.mockResolvedValue({ expires: null, state, provider: 'basic' }); + mockSessionStorage.get.mockResolvedValue(mockSessVal); const deauthenticationResult = await authenticator.logout(request); @@ -710,7 +705,7 @@ describe('Authenticator', () => { it('only clears session if it belongs to not configured provider.', async () => { const request = httpServerMock.createKibanaRequest(); const state = { authorization: 'Bearer xxx' }; - mockSessionStorage.get.mockResolvedValue({ expires: null, state, provider: 'token' }); + mockSessionStorage.get.mockResolvedValue({ ...mockSessVal, state, provider: 'token' }); const deauthenticationResult = await authenticator.logout(request); diff --git a/x-pack/plugins/security/server/authentication/authenticator.ts b/x-pack/plugins/security/server/authentication/authenticator.ts index 18bdc9624b12b1..364bcf350f7115 100644 --- a/x-pack/plugins/security/server/authentication/authenticator.ts +++ b/x-pack/plugins/security/server/authentication/authenticator.ts @@ -52,6 +52,16 @@ export interface ProviderSession { * entirely determined by the authentication provider that owns the current session. */ state: unknown; + + /** + * Cookie "Secure" attribute that is validated against the current Kibana server configuration. + */ + secure: boolean; + + /** + * Cookie "Path" attribute that is validated against the current Kibana server configuration. + */ + path: string; } /** @@ -77,7 +87,7 @@ export interface ProviderLoginAttempt { } export interface AuthenticatorOptions { - config: Pick; + config: Pick; basePath: HttpServiceSetup['basePath']; loggers: LoggerFactory; clusterClient: IClusterClient; @@ -152,6 +162,16 @@ export class Authenticator { */ private readonly providers: Map; + /** + * Whether or not cookies will use the "Secure" attribute. + */ + private readonly secureCookies: boolean; + + /** + * Which base path the HTTP server is hosted on. + */ + private readonly serverBasePath: string; + /** * Session duration in ms. If `null` session will stay active until the browser is closed. */ @@ -201,6 +221,8 @@ export class Authenticator { ] as [string, BaseAuthenticationProvider]; }) ); + this.secureCookies = this.options.config.secureCookies; + this.serverBasePath = this.options.basePath.serverBasePath || '/'; this.ttl = this.options.config.sessionTimeout; } @@ -261,6 +283,8 @@ export class Authenticator { state: authenticationResult.state, provider: attempt.provider, expires: this.ttl && Date.now() + this.ttl, + secure: this.secureCookies, + path: this.serverBasePath, }); } @@ -416,6 +440,8 @@ export class Authenticator { : existingSession!.state, provider: providerType, expires: this.ttl && Date.now() + this.ttl, + secure: this.secureCookies, + path: this.serverBasePath, }); } } diff --git a/x-pack/plugins/security/server/authentication/index.ts b/x-pack/plugins/security/server/authentication/index.ts index df16dd375e858a..51fb8d14fa1713 100644 --- a/x-pack/plugins/security/server/authentication/index.ts +++ b/x-pack/plugins/security/server/authentication/index.ts @@ -65,18 +65,42 @@ export async function setupAuthentication({ .callAsCurrentUser('shield.authenticate')) as AuthenticatedUser; }; + const isValid = (sessionValue: ProviderSession) => { + // ensure that this cookie was created with the current Kibana configuration + if (sessionValue.path !== (http.basePath.serverBasePath || '/')) { + authLogger.debug(`Outdated session value with path "${sessionValue.path}"`); + return false; + } else if (sessionValue.secure !== config.secureCookies) { + authLogger.debug(`Outdated session value with secure "${sessionValue.secure}"`); + return false; + } + // ensure that this cookie is not expired + return !(sessionValue.expires && sessionValue.expires < Date.now()); + }; + const authenticator = new Authenticator({ clusterClient, basePath: http.basePath, - config: { sessionTimeout: config.sessionTimeout, authc: config.authc }, + config: { + sessionTimeout: config.sessionTimeout, + authc: config.authc, + secureCookies: config.secureCookies, + }, isSystemAPIRequest: (request: KibanaRequest) => getLegacyAPI().isSystemAPIRequest(request), loggers, sessionStorageFactory: await http.createCookieSessionStorageFactory({ encryptionKey: config.encryptionKey, isSecure: config.secureCookies, name: config.cookieName, - validate: (sessionValue: ProviderSession) => - !(sessionValue.expires && sessionValue.expires < Date.now()), + validate: (session: ProviderSession) => { + const array: ProviderSession[] = Array.isArray(session) ? session : [session]; + for (const sess of array) { + if (!isValid(sess)) { + return { isValid: false, path: sess.path, isSecure: sess.secure }; + } + } + return { isValid: true }; + }, }), }); From 4176916bf47f4c3d352a602eeeccebeb3b05d750 Mon Sep 17 00:00:00 2001 From: Joe Portner <5295965+jportner@users.noreply.github.com> Date: Thu, 14 Nov 2019 10:14:09 -0500 Subject: [PATCH 2/5] Remove `secure` attribute from session cookie value We don't need to check this attribute, as it doesn't influence the scope of the cookie. Only `path` infleunces the cookie's scope. --- ...rver.sessioncookievalidationresult.issecure.md | 13 ------------- ...plugin-server.sessioncookievalidationresult.md | 1 - src/core/server/http/cookie_session_storage.ts | 14 +++----------- .../server/http/cookie_sesson_storage.test.ts | 2 +- src/core/server/server.api.md | 1 - .../server/authentication/authenticator.test.ts | 3 --- .../server/authentication/authenticator.ts | 15 +-------------- .../security/server/authentication/index.ts | 11 ++--------- 8 files changed, 7 insertions(+), 53 deletions(-) delete mode 100644 docs/development/core/server/kibana-plugin-server.sessioncookievalidationresult.issecure.md diff --git a/docs/development/core/server/kibana-plugin-server.sessioncookievalidationresult.issecure.md b/docs/development/core/server/kibana-plugin-server.sessioncookievalidationresult.issecure.md deleted file mode 100644 index ae9d5c58113d1b..00000000000000 --- a/docs/development/core/server/kibana-plugin-server.sessioncookievalidationresult.issecure.md +++ /dev/null @@ -1,13 +0,0 @@ - - -[Home](./index.md) > [kibana-plugin-server](./kibana-plugin-server.md) > [SessionCookieValidationResult](./kibana-plugin-server.sessioncookievalidationresult.md) > [isSecure](./kibana-plugin-server.sessioncookievalidationresult.issecure.md) - -## SessionCookieValidationResult.isSecure property - -The "Secure" attribute of the cookie; if the cookie is invalid, this is used to clear it. - -Signature: - -```typescript -isSecure?: boolean; -``` diff --git a/docs/development/core/server/kibana-plugin-server.sessioncookievalidationresult.md b/docs/development/core/server/kibana-plugin-server.sessioncookievalidationresult.md index 748f8688110850..6d32c4cca3dd6a 100644 --- a/docs/development/core/server/kibana-plugin-server.sessioncookievalidationresult.md +++ b/docs/development/core/server/kibana-plugin-server.sessioncookievalidationresult.md @@ -16,7 +16,6 @@ export interface SessionCookieValidationResult | Property | Type | Description | | --- | --- | --- | -| [isSecure](./kibana-plugin-server.sessioncookievalidationresult.issecure.md) | boolean | The "Secure" attribute of the cookie; if the cookie is invalid, this is used to clear it. | | [isValid](./kibana-plugin-server.sessioncookievalidationresult.isvalid.md) | boolean | Whether the cookie is valid or not. | | [path](./kibana-plugin-server.sessioncookievalidationresult.path.md) | string | The "Path" attribute of the cookie; if the cookie is invalid, this is used to clear it. | diff --git a/src/core/server/http/cookie_session_storage.ts b/src/core/server/http/cookie_session_storage.ts index c8b00fe96ad092..9cce3dea1a160c 100644 --- a/src/core/server/http/cookie_session_storage.ts +++ b/src/core/server/http/cookie_session_storage.ts @@ -60,10 +60,6 @@ export interface SessionCookieValidationResult { * The "Path" attribute of the cookie; if the cookie is invalid, this is used to clear it. */ path?: string; - /** - * The "Secure" attribute of the cookie; if the cookie is invalid, this is used to clear it. - */ - isSecure?: boolean; } class ScopedCookieSessionStorage> implements SessionStorage { @@ -117,17 +113,13 @@ export async function createCookieSessionStorageFactory( cookieOptions: SessionStorageCookieOptions, basePath?: string ): Promise> { - function clearInvalidCookie( - req: Request | undefined, - path: string = basePath || '/', - isSecure: boolean = cookieOptions.isSecure - ) { + function clearInvalidCookie(req: Request | undefined, path: string = basePath || '/') { // if the cookie did not include the 'path' or 'isSecure' attributes in the session value, it is a legacy cookie // we will assume that the cookie was created with the current configuration log.debug(`Clearing invalid session cookie`); // need to use Hapi toolkit to clear cookie with defined options if (req) { - (req.cookieAuth as any).h.unstate(cookieOptions.name, { path, isSecure }); + (req.cookieAuth as any).h.unstate(cookieOptions.name, { path }); } } @@ -139,7 +131,7 @@ export async function createCookieSessionStorageFactory( validateFunc: async (req, session: T) => { const result = cookieOptions.validate(session); if (!result.isValid) { - clearInvalidCookie(req, result.path, result.isSecure); + clearInvalidCookie(req, result.path); } return { valid: result.isValid }; }, diff --git a/src/core/server/http/cookie_sesson_storage.test.ts b/src/core/server/http/cookie_sesson_storage.test.ts index 4d1031d5d63cb5..55cff25376d48e 100644 --- a/src/core/server/http/cookie_sesson_storage.test.ts +++ b/src/core/server/http/cookie_sesson_storage.test.ts @@ -101,7 +101,7 @@ const cookieOptions = { encryptionKey: 'something_at_least_32_characters', validate: (session: Storage) => { const isValid = session.path === path && session.expires > Date.now(); - return { isValid, path: session.path, isSecure: false }; + return { isValid, path: session.path }; }, isSecure: false, path, diff --git a/src/core/server/server.api.md b/src/core/server/server.api.md index 88aebf490a80c2..96ab7ab605bef9 100644 --- a/src/core/server/server.api.md +++ b/src/core/server/server.api.md @@ -1563,7 +1563,6 @@ export class ScopedClusterClient implements IScopedClusterClient { // @public export interface SessionCookieValidationResult { - isSecure?: boolean; isValid: boolean; path?: string; } diff --git a/x-pack/plugins/security/server/authentication/authenticator.test.ts b/x-pack/plugins/security/server/authentication/authenticator.test.ts index 2ef4f9e1f6223a..4d6c337f781fd3 100644 --- a/x-pack/plugins/security/server/authentication/authenticator.test.ts +++ b/x-pack/plugins/security/server/authentication/authenticator.test.ts @@ -87,7 +87,6 @@ describe('Authenticator', () => { state: { authorization: 'Basic xxx' }, provider: 'basic', path: mockOptions.basePath.serverBasePath, - secure: mockOptions.config.secureCookies, }; authenticator = new Authenticator(mockOptions); @@ -306,7 +305,6 @@ describe('Authenticator', () => { state: { authorization: 'Basic xxx' }, provider: 'basic', path: mockOptions.basePath.serverBasePath, - secure: mockOptions.config.secureCookies, }; authenticator = new Authenticator(mockOptions); @@ -665,7 +663,6 @@ describe('Authenticator', () => { state: { authorization: 'Basic xxx' }, provider: 'basic', path: mockOptions.basePath.serverBasePath, - secure: mockOptions.config.secureCookies, }; authenticator = new Authenticator(mockOptions); diff --git a/x-pack/plugins/security/server/authentication/authenticator.ts b/x-pack/plugins/security/server/authentication/authenticator.ts index 364bcf350f7115..d6e3a4e3ad09e7 100644 --- a/x-pack/plugins/security/server/authentication/authenticator.ts +++ b/x-pack/plugins/security/server/authentication/authenticator.ts @@ -53,11 +53,6 @@ export interface ProviderSession { */ state: unknown; - /** - * Cookie "Secure" attribute that is validated against the current Kibana server configuration. - */ - secure: boolean; - /** * Cookie "Path" attribute that is validated against the current Kibana server configuration. */ @@ -87,7 +82,7 @@ export interface ProviderLoginAttempt { } export interface AuthenticatorOptions { - config: Pick; + config: Pick; basePath: HttpServiceSetup['basePath']; loggers: LoggerFactory; clusterClient: IClusterClient; @@ -162,11 +157,6 @@ export class Authenticator { */ private readonly providers: Map; - /** - * Whether or not cookies will use the "Secure" attribute. - */ - private readonly secureCookies: boolean; - /** * Which base path the HTTP server is hosted on. */ @@ -221,7 +211,6 @@ export class Authenticator { ] as [string, BaseAuthenticationProvider]; }) ); - this.secureCookies = this.options.config.secureCookies; this.serverBasePath = this.options.basePath.serverBasePath || '/'; this.ttl = this.options.config.sessionTimeout; @@ -283,7 +272,6 @@ export class Authenticator { state: authenticationResult.state, provider: attempt.provider, expires: this.ttl && Date.now() + this.ttl, - secure: this.secureCookies, path: this.serverBasePath, }); } @@ -440,7 +428,6 @@ export class Authenticator { : existingSession!.state, provider: providerType, expires: this.ttl && Date.now() + this.ttl, - secure: this.secureCookies, path: this.serverBasePath, }); } diff --git a/x-pack/plugins/security/server/authentication/index.ts b/x-pack/plugins/security/server/authentication/index.ts index 51fb8d14fa1713..c3f0d053208995 100644 --- a/x-pack/plugins/security/server/authentication/index.ts +++ b/x-pack/plugins/security/server/authentication/index.ts @@ -70,9 +70,6 @@ export async function setupAuthentication({ if (sessionValue.path !== (http.basePath.serverBasePath || '/')) { authLogger.debug(`Outdated session value with path "${sessionValue.path}"`); return false; - } else if (sessionValue.secure !== config.secureCookies) { - authLogger.debug(`Outdated session value with secure "${sessionValue.secure}"`); - return false; } // ensure that this cookie is not expired return !(sessionValue.expires && sessionValue.expires < Date.now()); @@ -81,11 +78,7 @@ export async function setupAuthentication({ const authenticator = new Authenticator({ clusterClient, basePath: http.basePath, - config: { - sessionTimeout: config.sessionTimeout, - authc: config.authc, - secureCookies: config.secureCookies, - }, + config: { sessionTimeout: config.sessionTimeout, authc: config.authc }, isSystemAPIRequest: (request: KibanaRequest) => getLegacyAPI().isSystemAPIRequest(request), loggers, sessionStorageFactory: await http.createCookieSessionStorageFactory({ @@ -96,7 +89,7 @@ export async function setupAuthentication({ const array: ProviderSession[] = Array.isArray(session) ? session : [session]; for (const sess of array) { if (!isValid(sess)) { - return { isValid: false, path: sess.path, isSecure: sess.secure }; + return { isValid: false, path: sess.path }; } } return { isValid: true }; From 60d7789aaeced3aaff85b74700e6da55e8967576 Mon Sep 17 00:00:00 2001 From: Joe Portner <5295965+jportner@users.noreply.github.com> Date: Thu, 14 Nov 2019 10:33:53 -0500 Subject: [PATCH 3/5] Don't clear cookies with an undefined `path` in the session value --- x-pack/plugins/security/server/authentication/index.ts | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/x-pack/plugins/security/server/authentication/index.ts b/x-pack/plugins/security/server/authentication/index.ts index c3f0d053208995..0fe4e623d175cb 100644 --- a/x-pack/plugins/security/server/authentication/index.ts +++ b/x-pack/plugins/security/server/authentication/index.ts @@ -67,12 +67,13 @@ export async function setupAuthentication({ const isValid = (sessionValue: ProviderSession) => { // ensure that this cookie was created with the current Kibana configuration - if (sessionValue.path !== (http.basePath.serverBasePath || '/')) { + const { path, expires } = sessionValue; + if (path !== undefined && path !== (http.basePath.serverBasePath || '/')) { authLogger.debug(`Outdated session value with path "${sessionValue.path}"`); return false; } // ensure that this cookie is not expired - return !(sessionValue.expires && sessionValue.expires < Date.now()); + return !(expires && expires < Date.now()); }; const authenticator = new Authenticator({ From 129d6b4fd0552b5da842a3aea66bf4dedddbf986 Mon Sep 17 00:00:00 2001 From: Joe Portner <5295965+jportner@users.noreply.github.com> Date: Fri, 22 Nov 2019 09:17:27 -0500 Subject: [PATCH 4/5] Update parameter type on cookie validate callback The validate callback can expect to receive ProviderSession or an array of ProviderSessions. The types have been updated to reflect that. --- .../kibana-plugin-server.sessionstoragecookieoptions.md | 2 +- ...ana-plugin-server.sessionstoragecookieoptions.validate.md | 2 +- src/core/server/http/cookie_session_storage.ts | 4 ++-- src/core/server/http/cookie_sesson_storage.test.ts | 5 ++++- src/core/server/http/integration_tests/core_services.test.ts | 2 +- src/core/server/server.api.md | 2 +- x-pack/plugins/security/server/authentication/index.ts | 2 +- 7 files changed, 11 insertions(+), 8 deletions(-) diff --git a/docs/development/core/server/kibana-plugin-server.sessionstoragecookieoptions.md b/docs/development/core/server/kibana-plugin-server.sessionstoragecookieoptions.md index 62fe10248561ee..778dc27a190d91 100644 --- a/docs/development/core/server/kibana-plugin-server.sessionstoragecookieoptions.md +++ b/docs/development/core/server/kibana-plugin-server.sessionstoragecookieoptions.md @@ -19,5 +19,5 @@ export interface SessionStorageCookieOptions | [encryptionKey](./kibana-plugin-server.sessionstoragecookieoptions.encryptionkey.md) | string | A key used to encrypt a cookie's value. Should be at least 32 characters long. | | [isSecure](./kibana-plugin-server.sessionstoragecookieoptions.issecure.md) | boolean | Flag indicating whether the cookie should be sent only via a secure connection. | | [name](./kibana-plugin-server.sessionstoragecookieoptions.name.md) | string | Name of the session cookie. | -| [validate](./kibana-plugin-server.sessionstoragecookieoptions.validate.md) | (sessionValue: T) => SessionCookieValidationResult | Function called to validate a cookie's decrypted value. | +| [validate](./kibana-plugin-server.sessionstoragecookieoptions.validate.md) | (sessionValue: T | T[]) => SessionCookieValidationResult | Function called to validate a cookie's decrypted value. | diff --git a/docs/development/core/server/kibana-plugin-server.sessionstoragecookieoptions.validate.md b/docs/development/core/server/kibana-plugin-server.sessionstoragecookieoptions.validate.md index 795f08e81cbef6..effa4b6bbc077c 100644 --- a/docs/development/core/server/kibana-plugin-server.sessionstoragecookieoptions.validate.md +++ b/docs/development/core/server/kibana-plugin-server.sessionstoragecookieoptions.validate.md @@ -9,5 +9,5 @@ Function called to validate a cookie's decrypted value. Signature: ```typescript -validate: (sessionValue: T) => SessionCookieValidationResult; +validate: (sessionValue: T | T[]) => SessionCookieValidationResult; ``` diff --git a/src/core/server/http/cookie_session_storage.ts b/src/core/server/http/cookie_session_storage.ts index 9cce3dea1a160c..859c4a053e29c1 100644 --- a/src/core/server/http/cookie_session_storage.ts +++ b/src/core/server/http/cookie_session_storage.ts @@ -40,7 +40,7 @@ export interface SessionStorageCookieOptions { /** * Function called to validate a cookie's decrypted value. */ - validate: (sessionValue: T) => SessionCookieValidationResult; + validate: (sessionValue: T | T[]) => SessionCookieValidationResult; /** * Flag indicating whether the cookie should be sent only via a secure connection. */ @@ -128,7 +128,7 @@ export async function createCookieSessionStorageFactory( server.auth.strategy('security-cookie', 'cookie', { cookie: cookieOptions.name, password: cookieOptions.encryptionKey, - validateFunc: async (req, session: T) => { + validateFunc: async (req, session: T | T[]) => { const result = cookieOptions.validate(session); if (!result.isValid) { clearInvalidCookie(req, result.path); diff --git a/src/core/server/http/cookie_sesson_storage.test.ts b/src/core/server/http/cookie_sesson_storage.test.ts index 55cff25376d48e..bf0585ad280d50 100644 --- a/src/core/server/http/cookie_sesson_storage.test.ts +++ b/src/core/server/http/cookie_sesson_storage.test.ts @@ -99,7 +99,10 @@ const delay = (ms: number) => new Promise(res => setTimeout(res, ms)); const cookieOptions = { name: 'sid', encryptionKey: 'something_at_least_32_characters', - validate: (session: Storage) => { + validate: (session: Storage | Storage[]) => { + if (Array.isArray(session)) { + session = session[0]; + } const isValid = session.path === path && session.expires > Date.now(); return { isValid, path: session.path }; }, diff --git a/src/core/server/http/integration_tests/core_services.test.ts b/src/core/server/http/integration_tests/core_services.test.ts index 954702870fc4ea..f3867faa2ae75f 100644 --- a/src/core/server/http/integration_tests/core_services.test.ts +++ b/src/core/server/http/integration_tests/core_services.test.ts @@ -39,7 +39,7 @@ describe('http service', () => { const cookieOptions = { name: 'sid', encryptionKey: 'something_at_least_32_characters', - validate: (session: StorageData) => ({ isValid: true }), + validate: () => ({ isValid: true }), isSecure: false, path: '/', }; diff --git a/src/core/server/server.api.md b/src/core/server/server.api.md index 96ab7ab605bef9..0f7281e4900e7d 100644 --- a/src/core/server/server.api.md +++ b/src/core/server/server.api.md @@ -1579,7 +1579,7 @@ export interface SessionStorageCookieOptions { encryptionKey: string; isSecure: boolean; name: string; - validate: (sessionValue: T) => SessionCookieValidationResult; + validate: (sessionValue: T | T[]) => SessionCookieValidationResult; } // @public diff --git a/x-pack/plugins/security/server/authentication/index.ts b/x-pack/plugins/security/server/authentication/index.ts index 0fe4e623d175cb..7195a4e869e75f 100644 --- a/x-pack/plugins/security/server/authentication/index.ts +++ b/x-pack/plugins/security/server/authentication/index.ts @@ -86,7 +86,7 @@ export async function setupAuthentication({ encryptionKey: config.encryptionKey, isSecure: config.secureCookies, name: config.cookieName, - validate: (session: ProviderSession) => { + validate: (session: ProviderSession | ProviderSession[]) => { const array: ProviderSession[] = Array.isArray(session) ? session : [session]; for (const sess of array) { if (!isValid(sess)) { From c7f9e125994b96aff8266308cd0b0fdf198558f6 Mon Sep 17 00:00:00 2001 From: Joe Portner <5295965+jportner@users.noreply.github.com> Date: Fri, 22 Nov 2019 09:38:22 -0500 Subject: [PATCH 5/5] Address nits --- src/core/server/http/cookie_session_storage.ts | 2 +- .../security/server/authentication/authenticator.test.ts | 7 +------ 2 files changed, 2 insertions(+), 7 deletions(-) diff --git a/src/core/server/http/cookie_session_storage.ts b/src/core/server/http/cookie_session_storage.ts index 859c4a053e29c1..25b463140bfbc2 100644 --- a/src/core/server/http/cookie_session_storage.ts +++ b/src/core/server/http/cookie_session_storage.ts @@ -114,7 +114,7 @@ export async function createCookieSessionStorageFactory( basePath?: string ): Promise> { function clearInvalidCookie(req: Request | undefined, path: string = basePath || '/') { - // if the cookie did not include the 'path' or 'isSecure' attributes in the session value, it is a legacy cookie + // if the cookie did not include the 'path' attribute in the session value, it is a legacy cookie // we will assume that the cookie was created with the current configuration log.debug(`Clearing invalid session cookie`); // need to use Hapi toolkit to clear cookie with defined options diff --git a/x-pack/plugins/security/server/authentication/authenticator.test.ts b/x-pack/plugins/security/server/authentication/authenticator.test.ts index 4d6c337f781fd3..a58f768f0c7966 100644 --- a/x-pack/plugins/security/server/authentication/authenticator.test.ts +++ b/x-pack/plugins/security/server/authentication/authenticator.test.ts @@ -28,12 +28,7 @@ function getMockOptions(config: Partial = {}) { basePath: httpServiceMock.createSetupContract().basePath, loggers: loggingServiceMock.create(), isSystemAPIRequest: jest.fn(), - config: { - sessionTimeout: null, - authc: { providers: [], oidc: {}, saml: {} }, - secureCookies: false, - ...config, - }, + config: { sessionTimeout: null, authc: { providers: [], oidc: {}, saml: {} }, ...config }, sessionStorageFactory: sessionStorageMock.createFactory(), }; }