-
Notifications
You must be signed in to change notification settings - Fork 177
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
Changes from all commits
b327912
fd72d85
96e789b
5bb319a
e502fcf
e9138e1
fe93845
2b49375
e5da67c
49529b0
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should this be 4096? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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', | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,120 @@ | ||
/* | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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); | ||
}); | ||
}); |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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'; | ||
|
@@ -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; | ||
|
@@ -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, | ||
|
@@ -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 | ||
|
@@ -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; | ||
} | ||
|
@@ -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, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Gotcha, thank you for the clarification. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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! |
||
}, | ||
authType: this.type, | ||
expiryTime: Date.now() + this.config.session.ttl, | ||
}; | ||
} | ||
|
||
// TODO: Add token expiration check here | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Looks like this comment can be removed. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. |
||
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; | ||
} | ||
|
@@ -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; | ||
|
@@ -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; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. would this value not be null always since There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think |
||
if (authHeaderValue) { | ||
header.authorization = authHeaderValue; | ||
|
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See #1352 (comment)