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

Backport cookie splitter #1662

2 changes: 1 addition & 1 deletion .github/actions/install-dashboards/action.yml
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@ runs:
- id: branch-switch-if-possible
continue-on-error: true # Defaults onto main if the branch switch doesn't work
if: ${{ steps.osd-version.outputs.osd-version }}
run: git checkout ${{ steps.osd-version.outputs.osd-version }} || git checkout ${{ steps.osd-version.outputs.osd-x-version }}x
run: git checkout 1.3.14 || git checkout ${{ steps.osd-version.outputs.osd-version }} || git checkout ${{ steps.osd-version.outputs.osd-x-version }}x
Copy link
Member

Choose a reason for hiding this comment

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

why hardcode: git checkout 1.3.14?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure, and maybe that one should go. I cherry picked that commit from Craig's PR, but maybe that was more of a PoC? Without it, the tests would not run, although I can't remember what the error was.
@cwperks Do you know if this was a temporary fix, and what the alternative should be?

Copy link
Member

Choose a reason for hiding this comment

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

If I remember correctly, this was added temporarily because the 1.x branch was stale and #1688 was not merged yet. Since the branch was stale, it was trying to check out an older patch release of OSD core so I added this line in.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hi @DarshitChanpura is the comment you were awaiting addressed above please, just checking? Thank you.

Copy link
Member

Choose a reason for hiding this comment

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

This line should be removed then I think since the branch is not longer stale.

working-directory: ./OpenSearch-Dashboards
shell: bash

Expand Down
3 changes: 2 additions & 1 deletion common/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,8 @@ export const DEFAULT_TENANT = 'default';
export const GLOBAL_TENANT_RENDERING_TEXT = 'Global';
export const PRIVATE_TENANT_RENDERING_TEXT = 'Private';
export const globalTenantName = 'global_tenant';

export const MAX_LENGTH_OF_COOKIE_BYTES = 4000;
export const ESTIMATED_IRON_COOKIE_OVERHEAD = 1.5;
export enum AuthType {
BASIC = 'basicauth',
OPEN_ID = 'openid',
Expand Down
18 changes: 14 additions & 4 deletions server/auth/types/authentication_type.ts
Original file line number Diff line number Diff line change
Expand Up @@ -118,7 +118,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 @@ -140,7 +140,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 = this.getAdditionalAuthHeader(request);
Object.assign(authHeaders, additonalAuthHeader);
Expand Down Expand Up @@ -236,11 +236,21 @@ export abstract class AuthenticationType implements IAuthenticationType {
request: OpenSearchDashboardsRequest,
authInfo: any
): SecuritySessionCookie;
protected abstract async 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;
protected abstract buildAuthHeaderFromCookie(cookie: SecuritySessionCookie): any;

public abstract buildAuthHeaderFromCookie(
cookie: SecuritySessionCookie,
request: OpenSearchDashboardsRequest
): any;
public abstract init(): Promise<void>;
}
158 changes: 158 additions & 0 deletions server/auth/types/openid/openid_auth.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,158 @@
/*
* 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 esClient: ILegacyClusterClient;
let logger: Logger;

const router: Partial<IRouter> = {
get: jest.fn(),
post: jest.fn(),
};
const core = ({
http: {
basePath: {
serverBasePath: '/',
},
resources: {
register: jest.fn(),
},
},
} as unknown) as CoreSetup;

const sessionStorageFactory: SessionStorageFactory<SecuritySessionCookie> = {
asScoped: jest.fn().mockImplementation(() => {
return {
server: {
states: {
add: jest.fn(),
},
},
};
}),
};

// Consistent with auth_handler_factory.test.ts
beforeEach(() => {
// @ts-ignore
jest.spyOn(OpenIdAuthentication.prototype, 'init').mockImplementation(async () => {});
});

const config = ({
cookie: {
secure: false,
},
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 as IRouter,
esClient,
core,
logger
);

// The init method has a spyOn and is not executed, so we call createExtraStorage separately.
// This is not really needed for the test, but may help in spotting errors.
openIdAuthentication.createExtraStorage();

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 as IRouter,
esClient,
core,
logger
);

// The init method has a spyOn and is not executed, so we call createExtraStorage separately.
// This is not really needed for the test, but may help in spotting errors.
openIdAuthentication.createExtraStorage();

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);
});
});
95 changes: 90 additions & 5 deletions server/auth/types/openid/openid_auth.ts
Original file line number Diff line number Diff line change
Expand Up @@ -29,12 +29,18 @@ 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';
import { AuthenticationType } from '../authentication_type';
import { callTokenEndpoint } from './helper';
import { composeNextUrlQeuryParam } from '../../../utils/next_url';
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 Down Expand Up @@ -135,6 +143,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 +183,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,
},
authType: this.type,
expiryTime: Date.now() + this.config.session.ttl,
};
}

// TODO: Add token expiration check here
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 +236,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: Date.now() + refreshTokenResponse.expiresIn! * 1000, // expiresIn is in second
};

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

return true;
} else {
return false;
Expand Down Expand Up @@ -226,8 +282,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;
if (authHeaderValue) {
header.authorization = authHeaderValue;
Expand Down
Loading
Loading