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

Fix infinite redirect loop when multiple cookies are sent #50452

Merged
merged 7 commits into from
Nov 27, 2019
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions docs/development/core/server/kibana-plugin-server.md
Original file line number Diff line number Diff line change
Expand Up @@ -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 |
Expand Down
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; [SessionCookieValidationResult](./kibana-plugin-server.sessioncookievalidationresult.md) &gt; [isValid](./kibana-plugin-server.sessioncookievalidationresult.isvalid.md)

## SessionCookieValidationResult.isValid property

Whether the cookie is valid or not.

<b>Signature:</b>

```typescript
isValid: boolean;
```
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
<!-- Do not edit this file. It is automatically generated by API Documenter. -->

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

## SessionCookieValidationResult interface

Return type from a function to validate cookie contents.

<b>Signature:</b>

```typescript
export interface SessionCookieValidationResult
```

## Properties

| Property | Type | Description |
| --- | --- | --- |
| [isValid](./kibana-plugin-server.sessioncookievalidationresult.isvalid.md) | <code>boolean</code> | Whether the cookie is valid or not. |
| [path](./kibana-plugin-server.sessioncookievalidationresult.path.md) | <code>string</code> | The "Path" attribute of the cookie; if the cookie is invalid, this is used to clear it. |

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; [SessionCookieValidationResult](./kibana-plugin-server.sessioncookievalidationresult.md) &gt; [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.

<b>Signature:</b>

```typescript
path?: string;
```
Original file line number Diff line number Diff line change
Expand Up @@ -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.

<b>Signature:</b>

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,8 +16,8 @@ export interface SessionStorageCookieOptions<T>

| Property | Type | Description |
| --- | --- | --- |
| [encryptionKey](./kibana-plugin-server.sessionstoragecookieoptions.encryptionkey.md) | <code>string</code> | A key used to encrypt a cookie value. Should be at least 32 characters long. |
| [encryptionKey](./kibana-plugin-server.sessionstoragecookieoptions.encryptionkey.md) | <code>string</code> | A key used to encrypt a cookie's value. Should be at least 32 characters long. |
| [isSecure](./kibana-plugin-server.sessionstoragecookieoptions.issecure.md) | <code>boolean</code> | Flag indicating whether the cookie should be sent only via a secure connection. |
| [name](./kibana-plugin-server.sessionstoragecookieoptions.name.md) | <code>string</code> | Name of the session cookie. |
| [validate](./kibana-plugin-server.sessionstoragecookieoptions.validate.md) | <code>(sessionValue: T) =&gt; boolean &#124; Promise&lt;boolean&gt;</code> | Function called to validate a cookie content. |
| [validate](./kibana-plugin-server.sessionstoragecookieoptions.validate.md) | <code>(sessionValue: T) =&gt; SessionCookieValidationResult</code> | Function called to validate a cookie's decrypted value. |

Original file line number Diff line number Diff line change
Expand Up @@ -4,10 +4,10 @@

## SessionStorageCookieOptions.validate property

Function called to validate a cookie content.
Function called to validate a cookie's decrypted value.

<b>Signature:</b>

```typescript
validate: (sessionValue: T) => boolean | Promise<boolean>;
validate: (sessionValue: T) => SessionCookieValidationResult;
```
41 changes: 36 additions & 5 deletions src/core/server/http/cookie_session_storage.ts
Original file line number Diff line number Diff line change
Expand Up @@ -34,19 +34,34 @@ export interface SessionStorageCookieOptions<T> {
*/
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<boolean>;
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;
}

class ScopedCookieSessionStorage<T extends Record<string, any>> implements SessionStorage<T> {
constructor(
private readonly log: Logger,
Expand Down Expand Up @@ -98,15 +113,31 @@ export async function createCookieSessionStorageFactory<T>(
cookieOptions: SessionStorageCookieOptions<T>,
basePath?: string
): Promise<SessionStorageFactory<T>> {
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
jportner marked this conversation as resolved.
Show resolved Hide resolved
// 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 });
}
}

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);
}
return { valid: result.isValid };
},
Copy link
Contributor

@pgayvallet pgayvallet Nov 16, 2019

Choose a reason for hiding this comment

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

Is there a way to keep the functionality while calling clearInvalidCookie outside of validateFunc ? Adding behaviour in a validation method looks like introducing side effects to me, so I would prefer to avoid it if we can.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't love that it's there, but I couldn't think of a better place to put it.

I originally had it in the X-Pack authentication module in the Security plugin:

const isValid = (sessionValue: ProviderSession) => {
// ensure that this cookie was created with the current Kibana configuration
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 !(expires && expires < Date.now());
};
const authenticator = new Authenticator({
clusterClient,
basePath: http.basePath,
config: { sessionTimeout: config.sessionTimeout, authc: config.authc },
isSystemAPIRequest: (request: KibanaRequest) => getLegacyAPI().isSystemAPIRequest(request),
loggers,
sessionStorageFactory: await http.createCookieSessionStorageFactory({
encryptionKey: config.encryptionKey,
isSecure: config.secureCookies,
name: config.cookieName,
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 };
}
}
return { isValid: true };
},
}),
});

However, I wanted to keep usage of the Hapi toolkit abstracted away from X-Pack, so I decided to move that code into Core in cookie_session_storage.ts.

If you have a better idea of where to put it, I'm all ears!

FWIW, Hapi's default behavior to clear the cookie (which I've overridden in this commit) already originates in its validation method. https://github.com/hapijs/cookie/blob/3a6e046e7adafafc382054000239c338a1a55d8c/lib/index.js#L197-L199

Copy link
Contributor

Choose a reason for hiding this comment

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

I guess it's we are doing nothing that hapi was already doing, this should be alright. Unless someone has a better idea on where we could move this (don't have much experience on hapi lifecycle), maybe @joshdover or @restrry?

Copy link
Contributor

Choose a reason for hiding this comment

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

What if the platform doesn't handle the removal, but the plugin controls it? Security plugin already has access to CookieStorage and can perform all setup/teardown logic.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I thought about that, but we are relying on the Hapi toolkit for the actual cookie removal -- right now we don't expose Hapi directly to plugins in the New Platform, we abstract it away. My impression was that we wanted to keep it that way.

Copy link
Contributor

Choose a reason for hiding this comment

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

Correct, but we can extend API and provide methods to remove the cookie. This method will be a wrapper around hapi

Copy link
Contributor

@mshustov mshustov Nov 25, 2019

Choose a reason for hiding this comment

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

Ok, nvm. The core defines the auth strategy and sets the cookie in hapi. Probably, it's better to keep your current implementation with explicit removal in the core. Ok for me

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay, thanks!

@pgayvallet any other concerns or nits?

isSecure: cookieOptions.isSecure,
path: basePath,
clearInvalid: true,
clearInvalid: false,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

As mentioned above, we no longer need Hapi to automatically clear invalid cookies.

isHttpOnly: true,
isSameSite: false,
});
Expand Down
68 changes: 60 additions & 8 deletions src/core/server/http/cookie_sesson_storage.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -80,6 +80,7 @@ interface User {
interface Storage {
value: User;
expires: number;
path: string;
}

function retrieveSessionCookie(cookies: string) {
Expand All @@ -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,
path: '/',
path,
};

describe('Cookie based SessionStorage', () => {
Expand All @@ -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({});
});

Expand All @@ -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);
Expand All @@ -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 } });
Expand Down Expand Up @@ -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);

Expand All @@ -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('');
Expand All @@ -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();
Expand Down Expand Up @@ -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 = {
Expand Down Expand Up @@ -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({});
});

Expand Down
2 changes: 1 addition & 1 deletion src/core/server/http/http_server.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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,
};

Expand Down
5 changes: 4 additions & 1 deletion src/core/server/http/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Original file line number Diff line number Diff line change
Expand Up @@ -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: '/',
};
Expand Down
2 changes: 1 addition & 1 deletion src/core/server/http/integration_tests/lifecycle.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -408,7 +408,7 @@ describe('Auth', () => {
const cookieOptions = {
name: 'sid',
encryptionKey: 'something_at_least_32_characters',
validate: () => true,
validate: () => ({ isValid: true }),
isSecure: false,
};

Expand Down
1 change: 1 addition & 0 deletions src/core/server/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -116,6 +116,7 @@ export {
RouteConfigOptions,
SessionStorage,
SessionStorageCookieOptions,
SessionCookieValidationResult,
SessionStorageFactory,
} from './http';
export { Logger, LoggerFactory, LogMeta, LogRecord, LogLevel } from './logging';
Expand Down
8 changes: 7 additions & 1 deletion src/core/server/server.api.md
Original file line number Diff line number Diff line change
Expand Up @@ -1561,6 +1561,12 @@ export class ScopedClusterClient implements IScopedClusterClient {
callAsInternalUser(endpoint: string, clientParams?: Record<string, any>, options?: CallAPIOptions): Promise<any>;
}

// @public
export interface SessionCookieValidationResult {
isValid: boolean;
path?: string;
}

// @public
export interface SessionStorage<T> {
clear(): void;
Expand All @@ -1573,7 +1579,7 @@ export interface SessionStorageCookieOptions<T> {
encryptionKey: string;
isSecure: boolean;
name: string;
validate: (sessionValue: T) => boolean | Promise<boolean>;
validate: (sessionValue: T) => SessionCookieValidationResult;
}

// @public
Expand Down
Loading