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

Implement HTTP Authentication provider and allow ApiKey authentication by default. #58126

Merged
merged 4 commits into from
Feb 28, 2020
Merged
Show file tree
Hide file tree
Changes from 1 commit
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
41 changes: 15 additions & 26 deletions x-pack/plugins/case/server/routes/api/__fixtures__/authc_mock.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,33 +3,22 @@
* or more contributor license agreements. Licensed under the Elastic License;
* you may not use this file except in compliance with the Elastic License.
*/
import { Authentication } from '../../../../../security/server';
import { AuthenticatedUser } from '../../../../../security/server';
import { securityMock } from '../../../../../security/server/mocks';

const getCurrentUser = jest.fn().mockReturnValue({
username: 'awesome',
full_name: 'Awesome D00d',
});
const getCurrentUserThrow = jest.fn().mockImplementation(() => {
throw new Error('Bad User - the user is not authenticated');
});
function createAuthenticationMock({
currentUser,
}: { currentUser?: AuthenticatedUser | null } = {}) {
const { authc } = securityMock.createSetup();
authc.getCurrentUser.mockReturnValue(
currentUser !== undefined
? currentUser
: ({ username: 'awesome', full_name: 'Awesome D00d' } as AuthenticatedUser)
);
return authc;
}

export const authenticationMock = {
Copy link
Member Author

Choose a reason for hiding this comment

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

note: consumers are supposed to use mocks provided by the plugin itself and to not redefine it to be as much in sync with real types as possible.

create: (): jest.Mocked<Authentication> => ({
login: jest.fn(),
createAPIKey: jest.fn(),
getCurrentUser,
invalidateAPIKey: jest.fn(),
isAuthenticated: jest.fn(),
logout: jest.fn(),
getSessionInfo: jest.fn(),
}),
createInvalid: (): jest.Mocked<Authentication> => ({
login: jest.fn(),
createAPIKey: jest.fn(),
getCurrentUser: getCurrentUserThrow,
invalidateAPIKey: jest.fn(),
isAuthenticated: jest.fn(),
logout: jest.fn(),
getSessionInfo: jest.fn(),
}),
create: () => createAuthenticationMock(),
createInvalid: () => createAuthenticationMock({ currentUser: null }),
};
10 changes: 2 additions & 8 deletions x-pack/plugins/case/server/services/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -149,14 +149,8 @@ export class CaseService {
}
},
getUser: async ({ request, response }: GetUserArgs) => {
let user;
try {
this.log.debug(`Attempting to authenticate a user`);
user = await authentication!.getCurrentUser(request);
} catch (error) {
this.log.debug(`Error on GET user: ${error}`);
throw error;
}
this.log.debug(`Attempting to authenticate a user`);
const user = authentication!.getCurrentUser(request);
Copy link
Member Author

Choose a reason for hiding this comment

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

note: getCurrentUser is synchronous and doesn't throw errors anymore, but it can return null.

if (!user) {
this.log.debug(`Error on GET user: Bad User`);
throw new Error('Bad User - the user is not authenticated');
Expand Down
64 changes: 36 additions & 28 deletions x-pack/plugins/security/server/authentication/authenticator.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
*/

jest.mock('./providers/basic');
jest.mock('./providers/saml');

import Boom from 'boom';
import { duration, Duration } from 'moment';
Expand All @@ -23,15 +24,25 @@ import { Authenticator, AuthenticatorOptions, ProviderSession } from './authenti
import { DeauthenticationResult } from './deauthentication_result';
import { BasicAuthenticationProvider } from './providers';

function getMockOptions(config: Partial<AuthenticatorOptions['config']> = {}) {
function getMockOptions({
session,
providers,
}: {
session?: AuthenticatorOptions['config']['session'];
providers?: string[];
} = {}) {
return {
clusterClient: elasticsearchServiceMock.createClusterClient(),
basePath: httpServiceMock.createSetupContract().basePath,
loggers: loggingServiceMock.create(),
config: {
session: { idleTimeout: null, lifespan: null },
authc: { providers: [], oidc: {}, saml: {} },
...config,
session: { idleTimeout: null, lifespan: null, ...(session || {}) },
authc: {
providers: providers || [],
oidc: {},
saml: {},
http: { enabled: true, autoSchemesEnabled: true, schemes: [] },
},
},
sessionStorageFactory: sessionStorageMock.createFactory<ProviderSession>(),
};
Expand All @@ -55,20 +66,13 @@ describe('Authenticator', () => {

describe('initialization', () => {
it('fails if authentication providers are not configured.', () => {
const mockOptions = getMockOptions({
authc: { providers: [], oidc: {}, saml: {} },
});
expect(() => new Authenticator(mockOptions)).toThrowError(
expect(() => new Authenticator(getMockOptions())).toThrowError(
'No authentication provider is configured. Verify `xpack.security.authc.providers` config value.'
);
});

it('fails if configured authentication provider is not known.', () => {
const mockOptions = getMockOptions({
authc: { providers: ['super-basic'], oidc: {}, saml: {} },
});

expect(() => new Authenticator(mockOptions)).toThrowError(
expect(() => new Authenticator(getMockOptions({ providers: ['super-basic'] }))).toThrowError(
'Unsupported authentication provider name: super-basic.'
);
});
Expand All @@ -80,9 +84,7 @@ describe('Authenticator', () => {
let mockSessionStorage: jest.Mocked<SessionStorage<ProviderSession>>;
let mockSessVal: any;
beforeEach(() => {
mockOptions = getMockOptions({
authc: { providers: ['basic'], oidc: {}, saml: {} },
});
mockOptions = getMockOptions({ providers: ['basic'] });
mockSessionStorage = sessionStorageMock.create();
mockOptions.sessionStorageFactory.asScoped.mockReturnValue(mockSessionStorage);
mockSessVal = {
Expand Down Expand Up @@ -232,9 +234,7 @@ describe('Authenticator', () => {
let mockSessionStorage: jest.Mocked<SessionStorage<ProviderSession>>;
let mockSessVal: any;
beforeEach(() => {
mockOptions = getMockOptions({
authc: { providers: ['basic'], oidc: {}, saml: {} },
});
mockOptions = getMockOptions({ providers: ['basic'] });
mockSessionStorage = sessionStorageMock.create<ProviderSession>();
mockOptions.sessionStorageFactory.asScoped.mockReturnValue(mockSessionStorage);
mockSessVal = {
Expand Down Expand Up @@ -377,7 +377,7 @@ describe('Authenticator', () => {
idleTimeout: duration(3600 * 24),
lifespan: null,
},
authc: { providers: ['basic'], oidc: {}, saml: {} },
providers: ['basic'],
});

mockSessionStorage = sessionStorageMock.create();
Expand Down Expand Up @@ -416,7 +416,7 @@ describe('Authenticator', () => {
idleTimeout: duration(hr * 2),
lifespan: duration(hr * 8),
},
authc: { providers: ['basic'], oidc: {}, saml: {} },
providers: ['basic'],
});

mockSessionStorage = sessionStorageMock.create();
Expand Down Expand Up @@ -468,7 +468,7 @@ describe('Authenticator', () => {
idleTimeout: null,
lifespan,
},
authc: { providers: ['basic'], oidc: {}, saml: {} },
providers: ['basic'],
});

mockSessionStorage = sessionStorageMock.create();
Expand Down Expand Up @@ -718,9 +718,7 @@ describe('Authenticator', () => {
let mockSessionStorage: jest.Mocked<SessionStorage<ProviderSession>>;
let mockSessVal: any;
beforeEach(() => {
mockOptions = getMockOptions({
authc: { providers: ['basic'], oidc: {}, saml: {} },
});
mockOptions = getMockOptions({ providers: ['basic'] });
mockSessionStorage = sessionStorageMock.create();
mockOptions.sessionStorageFactory.asScoped.mockReturnValue(mockSessionStorage);
mockSessVal = {
Expand Down Expand Up @@ -809,9 +807,7 @@ describe('Authenticator', () => {
let mockOptions: ReturnType<typeof getMockOptions>;
let mockSessionStorage: jest.Mocked<SessionStorage<ProviderSession>>;
beforeEach(() => {
mockOptions = getMockOptions({
authc: { providers: ['basic'], oidc: {}, saml: {} },
});
mockOptions = getMockOptions({ providers: ['basic'] });
mockSessionStorage = sessionStorageMock.create();
mockOptions.sessionStorageFactory.asScoped.mockReturnValue(mockSessionStorage);

Expand Down Expand Up @@ -851,4 +847,16 @@ describe('Authenticator', () => {
expect(sessionInfo).toBe(null);
});
});

describe('`isProviderEnabled` method', () => {
it('returns `true` only if specified provider is enabled', () => {
let authenticator = new Authenticator(getMockOptions({ providers: ['basic'] }));
expect(authenticator.isProviderEnabled('basic')).toBe(true);
expect(authenticator.isProviderEnabled('saml')).toBe(false);

authenticator = new Authenticator(getMockOptions({ providers: ['basic', 'saml'] }));
expect(authenticator.isProviderEnabled('basic')).toBe(true);
expect(authenticator.isProviderEnabled('saml')).toBe(true);
});
});
});
13 changes: 13 additions & 0 deletions x-pack/plugins/security/server/authentication/authenticator.ts
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ import {
TokenAuthenticationProvider,
OIDCAuthenticationProvider,
PKIAuthenticationProvider,
HTTPAuthenticationProvider,
isSAMLRequestQuery,
} from './providers';
import { AuthenticationResult } from './authentication_result';
Expand Down Expand Up @@ -105,6 +106,7 @@ const providerMap = new Map<
[TokenAuthenticationProvider.type, TokenAuthenticationProvider],
[OIDCAuthenticationProvider.type, OIDCAuthenticationProvider],
[PKIAuthenticationProvider.type, PKIAuthenticationProvider],
[HTTPAuthenticationProvider.type, HTTPAuthenticationProvider],
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we get some benefit I'm not realizing from making the HTTPAuthenticationProvider part of the providerMap? If it wasn't part of the providerMap, could we get rid of the custom validate function? Separately, I was wondering whether we could pass all of the enabled providers into the HTTPAuthenticationProvider constructor, add a method to each provider to denote which auth header should be supported, and use this as the basis for isSchemeSupported...

Copy link
Member Author

Choose a reason for hiding this comment

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

Do we get some benefit I'm not realizing from making the HTTPAuthenticationProvider part of the providerMap?

Not really, was experimenting with different options and just stopped at some point.

If it wasn't part of the providerMap, could we get rid of the custom validate function?

Yep, that would simplify the config part.

Separately, I was wondering whether we could pass all of the enabled providers into the HTTPAuthenticationProvider constructor, add a method to each provider to denote which auth header should be supported, and use this as the basis for isSchemeSupported...

That's a great idea! But I'd propose slightly modified version - Authenticator will calculate proper supportedSchemes on its own and just give them to HTTPAuthenticationProvider so that it doesn't have to deal with other providers or even know that they exist. I'll make that change and you'll tell if it looks good to you or not :)

]);

function assertRequest(request: KibanaRequest) {
Expand Down Expand Up @@ -191,6 +193,7 @@ export class Authenticator {
client: this.options.clusterClient,
logger: this.options.loggers.get('tokens'),
}),
isProviderEnabled: this.isProviderEnabled.bind(this),
};

const authProviders = this.options.config.authc.providers;
Expand All @@ -206,6 +209,8 @@ export class Authenticator {
? (this.options.config.authc as Record<string, any>)[providerType]
: undefined;

this.logger.debug(`Enabling "${providerType}" authentication provider.`);

return [
providerType,
instantiateProvider(
Expand Down Expand Up @@ -385,6 +390,14 @@ export class Authenticator {
return null;
}

/**
* Checks whether specified provider type is currently enabled.
* @param providerType Type of the provider (`basic`, `saml`, `pki` etc.).
*/
isProviderEnabled(providerType: string) {
return this.providers.has(providerType);
}

/**
* Returns provider iterator where providers are sorted in the order of priority (based on the session ownership).
* @param sessionValue Current session value.
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,58 @@
/*
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
* or more contributor license agreements. Licensed under the Elastic License;
* you may not use this file except in compliance with the Elastic License.
*/

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

import { getHTTPAuthenticationScheme } from './get_http_authentication_scheme';

describe('getHTTPAuthenticationScheme', () => {
it('returns `null` if request does not have authorization header', () => {
expect(getHTTPAuthenticationScheme(httpServerMock.createKibanaRequest())).toBeNull();
});

it('returns `null` if authorization header value isn not a string', () => {
expect(
getHTTPAuthenticationScheme(
httpServerMock.createKibanaRequest({
headers: { authorization: ['Basic xxx', 'Bearer xxx'] as any },
})
)
).toBeNull();
});

it('returns `null` if authorization header value is an empty string', () => {
expect(
getHTTPAuthenticationScheme(
httpServerMock.createKibanaRequest({ headers: { authorization: '' } })
)
).toBeNull();
});

it('returns only scheme portion of the authorization header value in lower case', () => {
const headerValueAndSchemeMap = [
['Basic xxx', 'basic'],
['Basic xxx yyy', 'basic'],
['basic xxx', 'basic'],
['basic', 'basic'],
// We don't trim leading whitespaces in scheme.
Copy link
Member Author

Choose a reason for hiding this comment

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

note: we never trimmed so we're not changing anything here (not sure if Core HTTP service does this though).

[' Basic xxx', ''],
['Negotiate xxx', 'negotiate'],
['negotiate xxx', 'negotiate'],
['negotiate', 'negotiate'],
['ApiKey xxx', 'apikey'],
['apikey xxx', 'apikey'],
['Api Key xxx', 'api'],
];

for (const [authorization, scheme] of headerValueAndSchemeMap) {
expect(
getHTTPAuthenticationScheme(
httpServerMock.createKibanaRequest({ headers: { authorization } })
)
).toBe(scheme);
}
});
});
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
/*
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
* or more contributor license agreements. Licensed under the Elastic License;
* you may not use this file except in compliance with the Elastic License.
*/

import { KibanaRequest } from '../../../../../src/core/server';

/**
* Parses request's `Authorization` HTTP header if present and extracts authentication scheme.
* https://www.iana.org/assignments/http-authschemes/http-authschemes.xhtml#authschemes
* @param request Request instance to extract authentication scheme for.
*/
export function getHTTPAuthenticationScheme(request: KibanaRequest) {
const authorizationHeaderValue = request.headers.authorization;
if (!authorizationHeaderValue || typeof authorizationHeaderValue !== 'string') {
return null;
}

return authorizationHeaderValue.split(/\s+/)[0].toLowerCase();
}
3 changes: 2 additions & 1 deletion x-pack/plugins/security/server/authentication/index.mock.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,11 +9,12 @@ import { Authentication } from '.';
export const authenticationMock = {
create: (): jest.Mocked<Authentication> => ({
login: jest.fn(),
logout: jest.fn(),
isProviderEnabled: jest.fn(),
createAPIKey: jest.fn(),
getCurrentUser: jest.fn(),
invalidateAPIKey: jest.fn(),
isAuthenticated: jest.fn(),
logout: jest.fn(),
getSessionInfo: jest.fn(),
}),
};
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,7 @@ describe('setupAuthentication()', () => {
lifespan: null,
},
cookieName: 'my-sid-cookie',
authc: { providers: ['basic'] },
authc: { providers: ['basic'], http: { enabled: true } },
}),
true
);
Expand Down
1 change: 1 addition & 0 deletions x-pack/plugins/security/server/authentication/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -169,6 +169,7 @@ export async function setupAuthentication({
login: authenticator.login.bind(authenticator),
logout: authenticator.logout.bind(authenticator),
getSessionInfo: authenticator.getSessionInfo.bind(authenticator),
isProviderEnabled: authenticator.isProviderEnabled.bind(authenticator),
getCurrentUser,
createAPIKey: (request: KibanaRequest, params: CreateAPIKeyParams) =>
apiKeys.create(request, params),
Expand Down
Loading