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

Split up a value into multiple cookie payloads #1352

Merged
2 changes: 2 additions & 0 deletions common/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,8 @@ export const PRIVATE_TENANT_RENDERING_TEXT = 'Private';
export const globalTenantName = 'global_tenant';

export const MAX_INTEGER = 2147483647;
export const MAX_LENGTH_OF_COOKIE_BYTES = 4000;
Copy link
Member

Choose a reason for hiding this comment

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

why not 4096 here to equal 4kb?

Copy link

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be 4096?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@scrawfor99 I noticed that writing an empty cookie would take up around 30 bytes. Those bytes are probably not there when you add content to the cookie, but I figured a little margin for errors may be better.
Also, even if we only have one extra cookie, and the the payload is 4096 bytes, the current code will still try to write to the cookie - so we won't trip and fall on that 96 bytes difference.

Copy link

Choose a reason for hiding this comment

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

If you are researching for max cookie sizes, you get quite a few different figures, ranging between 5000 bytes, 4097, 4095, 4093 and 4000 bytes. Thus, we chose the most conservative figure to be safe in any case.

export const ESTIMATED_IRON_COOKIE_OVERHEAD = 1.5;

export enum AuthType {
BASIC = 'basicauth',
Expand Down
14 changes: 10 additions & 4 deletions server/auth/types/authentication_type.ts
Original file line number Diff line number Diff line change
Expand Up @@ -138,7 +138,7 @@ export abstract class AuthenticationType implements IAuthenticationType {
cookie = undefined;
}

if (!cookie || !(await this.isValidCookie(cookie))) {
if (!cookie || !(await this.isValidCookie(cookie, request))) {
// clear cookie
this.sessionStorageFactory.asScoped(request).clear();

Expand All @@ -160,7 +160,7 @@ export abstract class AuthenticationType implements IAuthenticationType {
}
// cookie is valid
// build auth header
const authHeadersFromCookie = this.buildAuthHeaderFromCookie(cookie!);
const authHeadersFromCookie = this.buildAuthHeaderFromCookie(cookie!, request);
Object.assign(authHeaders, authHeadersFromCookie);
const additonalAuthHeader = await this.getAdditionalAuthHeader(request);
Object.assign(authHeaders, additonalAuthHeader);
Expand Down Expand Up @@ -269,12 +269,18 @@ export abstract class AuthenticationType implements IAuthenticationType {
request: OpenSearchDashboardsRequest,
authInfo: any
): SecuritySessionCookie;
public abstract isValidCookie(cookie: SecuritySessionCookie): Promise<boolean>;
public abstract isValidCookie(
cookie: SecuritySessionCookie,
request: OpenSearchDashboardsRequest
): Promise<boolean>;
protected abstract handleUnauthedRequest(
request: OpenSearchDashboardsRequest,
response: LifecycleResponseFactory,
toolkit: AuthToolkit
): IOpenSearchDashboardsResponse | AuthResult;
public abstract buildAuthHeaderFromCookie(cookie: SecuritySessionCookie): any;
public abstract buildAuthHeaderFromCookie(
cookie: SecuritySessionCookie,
request: OpenSearchDashboardsRequest
): any;
public abstract init(): Promise<void>;
}
14 changes: 10 additions & 4 deletions server/auth/types/multiple/multi_auth.ts
Original file line number Diff line number Diff line change
Expand Up @@ -130,10 +130,13 @@ export class MultipleAuthentication extends AuthenticationType {
return {};
}

async isValidCookie(cookie: SecuritySessionCookie): Promise<boolean> {
async isValidCookie(
cookie: SecuritySessionCookie,
request: OpenSearchDashboardsRequest
): Promise<boolean> {
const reqAuthType = cookie?.authType?.toLowerCase();
if (reqAuthType && this.authHandlers.has(reqAuthType)) {
return this.authHandlers.get(reqAuthType)!.isValidCookie(cookie);
return this.authHandlers.get(reqAuthType)!.isValidCookie(cookie, request);
} else {
return false;
}
Expand Down Expand Up @@ -168,11 +171,14 @@ export class MultipleAuthentication extends AuthenticationType {
}
}

buildAuthHeaderFromCookie(cookie: SecuritySessionCookie): any {
buildAuthHeaderFromCookie(
cookie: SecuritySessionCookie,
request: OpenSearchDashboardsRequest
): any {
const reqAuthType = cookie?.authType?.toLowerCase();

if (reqAuthType && this.authHandlers.has(reqAuthType)) {
return this.authHandlers.get(reqAuthType)!.buildAuthHeaderFromCookie(cookie);
return this.authHandlers.get(reqAuthType)!.buildAuthHeaderFromCookie(cookie, request);
} else {
return {};
}
Expand Down
120 changes: 120 additions & 0 deletions server/auth/types/openid/openid_auth.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,120 @@
/*
Copy link
Contributor

Choose a reason for hiding this comment

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

As always, great test!

* Copyright OpenSearch Contributors
*
* Licensed under the Apache License, Version 2.0 (the "License").
* You may not use this file except in compliance with the License.
* A copy of the License is located at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* or in the "license" file accompanying this file. This file is distributed
* on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either
* express or implied. See the License for the specific language governing
* permissions and limitations under the License.
*/

import { httpServerMock } from '../../../../../../src/core/server/http/http_server.mocks';

import { OpenSearchDashboardsRequest } from '../../../../../../src/core/server/http/router';

import { OpenIdAuthentication } from './openid_auth';
import { SecurityPluginConfigType } from '../../../index';
import { SecuritySessionCookie } from '../../../session/security_cookie';
import { deflateValue } from '../../../utils/compression';
import {
IRouter,
CoreSetup,
ILegacyClusterClient,
Logger,
SessionStorageFactory,
} from '../../../../../../src/core/server';

describe('test OpenId authHeaderValue', () => {
let router: IRouter;
let core: CoreSetup;
let esClient: ILegacyClusterClient;
let sessionStorageFactory: SessionStorageFactory<SecuritySessionCookie>;
let logger: Logger;

// Consistent with auth_handler_factory.test.ts
beforeEach(() => {});

const config = ({
openid: {
header: 'authorization',
scope: [],
extra_storage: {
cookie_prefix: 'testcookie',
additional_cookies: 5,
},
},
} as unknown) as SecurityPluginConfigType;

test('make sure that cookies with authHeaderValue are still valid', async () => {
const openIdAuthentication = new OpenIdAuthentication(
config,
sessionStorageFactory,
router,
esClient,
core,
logger
);

const mockRequest = httpServerMock.createRawRequest();
const osRequest = OpenSearchDashboardsRequest.from(mockRequest);

const cookie: SecuritySessionCookie = {
credentials: {
authHeaderValue: 'Bearer eyToken',
},
};

const expectedHeaders = {
authorization: 'Bearer eyToken',
};

const headers = openIdAuthentication.buildAuthHeaderFromCookie(cookie, osRequest);

expect(headers).toEqual(expectedHeaders);
});

test('get authHeaderValue from split cookies', async () => {
const openIdAuthentication = new OpenIdAuthentication(
config,
sessionStorageFactory,
router,
esClient,
core,
logger
);

const testString = 'Bearer eyCombinedToken';
const testStringBuffer: Buffer = deflateValue(testString);
const cookieValue = testStringBuffer.toString('base64');
const cookiePrefix = config.openid!.extra_storage.cookie_prefix;
const splitValueAt = Math.ceil(
cookieValue.length / config.openid!.extra_storage.additional_cookies
);
const mockRequest = httpServerMock.createRawRequest({
state: {
[cookiePrefix + '1']: cookieValue.substring(0, splitValueAt),
[cookiePrefix + '2']: cookieValue.substring(splitValueAt),
},
});
const osRequest = OpenSearchDashboardsRequest.from(mockRequest);

const cookie: SecuritySessionCookie = {
credentials: {
authHeaderValueExtra: true,
},
};

const expectedHeaders = {
authorization: testString,
};

const headers = openIdAuthentication.buildAuthHeaderFromCookie(cookie, osRequest);

expect(headers).toEqual(expectedHeaders);
});
});
96 changes: 91 additions & 5 deletions server/auth/types/openid/openid_auth.ts
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ import {
import HTTP from 'http';
import HTTPS from 'https';
import { PeerCertificate } from 'tls';
import { Server, ServerStateCookieOptions } from '@hapi/hapi';
import { SecurityPluginConfigType } from '../../..';
import { SecuritySessionCookie } from '../../../session/security_cookie';
import { OpenIdAuthRoutes } from './routes';
Expand All @@ -37,6 +38,11 @@ import { callTokenEndpoint } from './helper';
import { composeNextUrlQueryParam } from '../../../utils/next_url';
import { getExpirationDate } from './helper';
import { AuthType, OPENID_AUTH_LOGIN } from '../../../../common';
import {
ExtraAuthStorageOptions,
getExtraAuthStorageValue,
setExtraAuthStorage,
} from '../../../session/cookie_splitter';

export interface OpenIdAuthConfig {
authorizationEndpoint?: string;
Expand Down Expand Up @@ -93,6 +99,8 @@ export class OpenIdAuthentication extends AuthenticationType {
this.openIdAuthConfig.tokenEndpoint = payload.token_endpoint;
this.openIdAuthConfig.endSessionEndpoint = payload.end_session_endpoint || undefined;

this.createExtraStorage();

const routes = new OpenIdAuthRoutes(
this.router,
this.config,
Expand All @@ -102,6 +110,7 @@ export class OpenIdAuthentication extends AuthenticationType {
this.coreSetup,
this.wreckClient
);

routes.setupRoutes();
} catch (error: any) {
this.logger.error(error); // TODO: log more info
Expand Down Expand Up @@ -135,6 +144,37 @@ export class OpenIdAuthentication extends AuthenticationType {
}
}

createExtraStorage() {
// @ts-ignore
const hapiServer: Server = this.sessionStorageFactory.asScoped({}).server;

const extraCookiePrefix = this.config.openid!.extra_storage.cookie_prefix;
const extraCookieSettings: ServerStateCookieOptions = {
isSecure: this.config.cookie.secure,
isSameSite: this.config.cookie.isSameSite,
password: this.config.cookie.password,
domain: this.config.cookie.domain,
path: this.coreSetup.http.basePath.serverBasePath || '/',
clearInvalid: false,
isHttpOnly: true,
ignoreErrors: true,
encoding: 'iron', // Same as hapi auth cookie
};

for (let i = 1; i <= this.config.openid!.extra_storage.additional_cookies; i++) {
hapiServer.states.add(extraCookiePrefix + i, extraCookieSettings);
}
}

private getExtraAuthStorageOptions(): ExtraAuthStorageOptions {
// If we're here, we will always have the openid configuration
return {
cookiePrefix: this.config.openid!.extra_storage.cookie_prefix,
additionalCookies: this.config.openid!.extra_storage.additional_cookies,
logger: this.logger,
};
}

requestIncludesAuthInfo(request: OpenSearchDashboardsRequest): boolean {
return request.headers.authorization ? true : false;
}
Expand All @@ -144,27 +184,37 @@ export class OpenIdAuthentication extends AuthenticationType {
}

getCookie(request: OpenSearchDashboardsRequest, authInfo: any): SecuritySessionCookie {
setExtraAuthStorage(
request,
request.headers.authorization as string,
this.getExtraAuthStorageOptions()
);

return {
username: authInfo.user_name,
credentials: {
authHeaderValue: request.headers.authorization,
authHeaderValueExtra: true,
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this now pass through? Why do we want this to be changed from the previous request.headers.authorization?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Originally I changed this so that existing valid user cookies would still be valid after an upgrade. When checking the cookie for a valid authorization header, both SAML and OpenId will look for authHeaderValue if no authHeaderValueExtra is found.

Copy link
Contributor

Choose a reason for hiding this comment

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

Gotcha, thank you for the clarification.

Copy link
Member

Choose a reason for hiding this comment

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

Can you add a brief comment on why hard-coding the boolean to true here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@DarshitChanpura Please see this answer for the reasoning behind it. Always open for suggestions though!
#1352 (comment)

},
authType: this.type,
expiryTime: Date.now() + this.config.session.ttl,
};
}

// TODO: Add token expiration check here
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like this comment can be removed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@scrawfor99 That comment is from another project, so I did not want to change anything.
There is actually a bug in the open id implementation caused by a missing expiration timestamp (not the check) a few lines above this comment. I will file an issue for that.

async isValidCookie(cookie: SecuritySessionCookie): Promise<boolean> {
async isValidCookie(
cookie: SecuritySessionCookie,
request: OpenSearchDashboardsRequest
): Promise<boolean> {
if (
cookie.authType !== this.type ||
!cookie.username ||
!cookie.expiryTime ||
!cookie.credentials?.authHeaderValue ||
(!cookie.credentials?.authHeaderValue && !this.getExtraAuthStorageValue(request, cookie)) ||
!cookie.credentials?.expires_at
) {
return false;
}

if (cookie.credentials?.expires_at > Date.now()) {
return true;
}
Expand All @@ -187,10 +237,17 @@ export class OpenIdAuthentication extends AuthenticationType {
// if no id_token from refresh token call, maybe the Idp doesn't allow refresh id_token
if (refreshTokenResponse.idToken) {
cookie.credentials = {
authHeaderValue: `Bearer ${refreshTokenResponse.idToken}`,
authHeaderValueExtra: true,
refresh_token: refreshTokenResponse.refreshToken,
expires_at: getExpirationDate(refreshTokenResponse), // expiresIn is in second
};

setExtraAuthStorage(
request,
`Bearer ${refreshTokenResponse.idToken}`,
this.getExtraAuthStorageOptions()
);

return true;
} else {
return false;
Expand Down Expand Up @@ -226,8 +283,37 @@ export class OpenIdAuthentication extends AuthenticationType {
}
}

buildAuthHeaderFromCookie(cookie: SecuritySessionCookie): any {
getExtraAuthStorageValue(request: OpenSearchDashboardsRequest, cookie: SecuritySessionCookie) {
let extraValue = '';
if (!cookie.credentials?.authHeaderValueExtra) {
return extraValue;
}

try {
extraValue = getExtraAuthStorageValue(request, this.getExtraAuthStorageOptions());
} catch (error) {
this.logger.info(error);
}

return extraValue;
}

buildAuthHeaderFromCookie(
cookie: SecuritySessionCookie,
request: OpenSearchDashboardsRequest
): any {
const header: any = {};
if (cookie.credentials.authHeaderValueExtra) {
try {
const extraAuthStorageValue = this.getExtraAuthStorageValue(request, cookie);
header.authorization = extraAuthStorageValue;
return header;
} catch (error) {
this.logger.error(error);
// TODO Re-throw?
// throw error;
}
}
const authHeaderValue = cookie.credentials?.authHeaderValue;
Copy link
Member

Choose a reason for hiding this comment

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

would this value not be null always since authHeaderValue is replaced by authHeaderValueExtra on L#150?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think authHeaderValueExtra acts as an indication whether this cookie uses the new splitting approach or is the older one. So, this part will support backward compatibility after a user upgrades to a version having this cookie split logic.

if (authHeaderValue) {
header.authorization = authHeaderValue;
Expand Down
Loading