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

Expose ability to check if API Keys are enabled #63454

Merged
merged 13 commits into from
Apr 23, 2020
Merged
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,11 @@ export interface AuthenticationServiceSetup {
* Returns currently authenticated user and throws if current user isn't authenticated.
*/
getCurrentUser: () => Promise<AuthenticatedUser>;

/**
* Determines if API Keys are currently enabled.
*/
areAPIKeysEnabled: () => Promise<boolean>;
}

export class AuthenticationService {
Expand All @@ -37,11 +42,15 @@ export class AuthenticationService {
const getCurrentUser = async () =>
(await http.get('/internal/security/me', { asSystemRequest: true })) as AuthenticatedUser;

const areAPIKeysEnabled = async () =>
((await http.get('/internal/security/api_key/_enabled')) as { apiKeysEnabled: boolean })
.apiKeysEnabled;

loginApp.create({ application, config, getStartServices, http });
logoutApp.create({ application, http });
loggedOutApp.create({ application, getStartServices, http });
overwrittenSessionApp.create({ application, authc: { getCurrentUser }, getStartServices });

return { getCurrentUser };
return { getCurrentUser, areAPIKeysEnabled };
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -9,5 +9,6 @@ import { AuthenticationServiceSetup } from './authentication_service';
export const authenticationMock = {
createSetup: (): jest.Mocked<AuthenticationServiceSetup> => ({
getCurrentUser: jest.fn(),
areAPIKeysEnabled: jest.fn(),
}),
};
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ import { AuthenticationServiceSetup } from '../authentication_service';

interface CreateDeps {
application: ApplicationSetup;
authc: AuthenticationServiceSetup;
authc: Pick<AuthenticationServiceSetup, 'getCurrentUser'>;
getStartServices: StartServicesAccessor;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ import { AuthenticationStatePage } from '../components';

interface Props {
basePath: IBasePath;
authc: AuthenticationServiceSetup;
authc: Pick<AuthenticationServiceSetup, 'getCurrentUser'>;
}

export function OverwrittenSessionPage({ authc, basePath }: Props) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import { ApiKey, ApiKeyToInvalidate } from '../../../common/model';
interface CheckPrivilegesResponse {
areApiKeysEnabled: boolean;
isAdmin: boolean;
canManage: boolean;
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 previously, the API Keys app had a dual purpose for areApiKeysEnabled: It was true if and only if both of the following were true:

  • User was authorized to retrieve their own API Keys
  • API Keys were in fact enabled

This separates those two concerns into dedicated properties

}

interface InvalidateApiKeysResponse {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,6 @@ import { APIKeysGridPage } from './api_keys_grid_page';
import { coreMock } from '../../../../../../../src/core/public/mocks';
import { apiKeysAPIClientMock } from '../index.mock';

const mock403 = () => ({ body: { statusCode: 403 } });
const mock500 = () => ({ body: { error: 'Internal Server Error', message: '', statusCode: 500 } });

const waitForRender = async (
Expand Down Expand Up @@ -48,6 +47,7 @@ describe('APIKeysGridPage', () => {
apiClientMock.checkPrivileges.mockResolvedValue({
isAdmin: true,
areApiKeysEnabled: true,
canManage: true,
});
apiClientMock.getApiKeys.mockResolvedValue({
apiKeys: [
Expand Down Expand Up @@ -82,6 +82,7 @@ describe('APIKeysGridPage', () => {
it('renders a callout when API keys are not enabled', async () => {
apiClientMock.checkPrivileges.mockResolvedValue({
isAdmin: true,
canManage: true,
areApiKeysEnabled: false,
});

Expand All @@ -95,7 +96,11 @@ describe('APIKeysGridPage', () => {
});

it('renders permission denied if user does not have required permissions', async () => {
apiClientMock.checkPrivileges.mockRejectedValue(mock403());
apiClientMock.checkPrivileges.mockResolvedValue({
canManage: false,
isAdmin: false,
areApiKeysEnabled: true,
});

const wrapper = mountWithIntl(<APIKeysGridPage {...getViewProperties()} />);

Expand Down Expand Up @@ -152,6 +157,7 @@ describe('APIKeysGridPage', () => {
beforeEach(() => {
apiClientMock.checkPrivileges.mockResolvedValue({
isAdmin: false,
canManage: true,
areApiKeysEnabled: true,
});

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,6 @@ import {
import { i18n } from '@kbn/i18n';
import { FormattedMessage } from '@kbn/i18n/react';
import moment from 'moment-timezone';
import _ from 'lodash';
import { NotificationsStart } from 'src/core/public';
import { SectionLoading } from '../../../../../../../src/plugins/es_ui_shared/public';
import { ApiKey, ApiKeyToInvalidate } from '../../../../common/model';
Expand All @@ -47,10 +46,10 @@ interface State {
isLoadingApp: boolean;
isLoadingTable: boolean;
isAdmin: boolean;
canManage: boolean;
areApiKeysEnabled: boolean;
apiKeys: ApiKey[];
selectedItems: ApiKey[];
permissionDenied: boolean;
error: any;
}

Expand All @@ -63,9 +62,9 @@ export class APIKeysGridPage extends Component<Props, State> {
isLoadingApp: true,
isLoadingTable: false,
isAdmin: false,
canManage: false,
areApiKeysEnabled: false,
apiKeys: [],
permissionDenied: false,
selectedItems: [],
error: undefined,
};
Expand All @@ -77,19 +76,15 @@ export class APIKeysGridPage extends Component<Props, State> {

public render() {
const {
permissionDenied,
isLoadingApp,
isLoadingTable,
areApiKeysEnabled,
isAdmin,
canManage,
error,
apiKeys,
} = this.state;

if (permissionDenied) {
return <PermissionDenied />;
}

if (isLoadingApp) {
return (
<EuiPageContent>
Expand All @@ -103,6 +98,10 @@ export class APIKeysGridPage extends Component<Props, State> {
);
}

if (!canManage) {
return <PermissionDenied />;
}

if (error) {
const {
body: { error: errorTitle, message, statusCode },
Expand Down Expand Up @@ -495,26 +494,25 @@ export class APIKeysGridPage extends Component<Props, State> {

private async checkPrivileges() {
try {
const { isAdmin, areApiKeysEnabled } = await this.props.apiKeysAPIClient.checkPrivileges();
this.setState({ isAdmin, areApiKeysEnabled });
const {
isAdmin,
canManage,
areApiKeysEnabled,
} = await this.props.apiKeysAPIClient.checkPrivileges();
this.setState({ isAdmin, canManage, areApiKeysEnabled });

if (areApiKeysEnabled) {
this.initiallyLoadApiKeys();
} else {
// We're done loading and will just show the "Disabled" error.
if (!canManage || !areApiKeysEnabled) {
this.setState({ isLoadingApp: false });
}
} catch (e) {
if (_.get(e, 'body.statusCode') === 403) {
this.setState({ permissionDenied: true, isLoadingApp: false });
} else {
this.props.notifications.toasts.addDanger(
i18n.translate('xpack.security.management.apiKeys.table.fetchingApiKeysErrorMessage', {
defaultMessage: 'Error checking privileges: {message}',
values: { message: _.get(e, 'body.message', '') },
})
);
this.initiallyLoadApiKeys();
}
} catch (e) {
this.props.notifications.toasts.addDanger(
i18n.translate('xpack.security.management.apiKeys.table.fetchingApiKeysErrorMessage', {
defaultMessage: 'Error checking privileges: {message}',
values: { message: e.body?.message ?? '' },
})
);
}
}

Expand Down
4 changes: 2 additions & 2 deletions x-pack/plugins/security/public/plugin.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ describe('Security Plugin', () => {
)
).toEqual({
__legacyCompat: { logoutUrl: '/some-base-path/logout', tenant: '/some-base-path' },
authc: { getCurrentUser: expect.any(Function) },
authc: { getCurrentUser: expect.any(Function), areAPIKeysEnabled: expect.any(Function) },
license: {
isEnabled: expect.any(Function),
getFeatures: expect.any(Function),
Expand All @@ -63,7 +63,7 @@ describe('Security Plugin', () => {

expect(setupManagementServiceMock).toHaveBeenCalledTimes(1);
expect(setupManagementServiceMock).toHaveBeenCalledWith({
authc: { getCurrentUser: expect.any(Function) },
authc: { getCurrentUser: expect.any(Function), areAPIKeysEnabled: expect.any(Function) },
license: {
isEnabled: expect.any(Function),
getFeatures: expect.any(Function),
Expand Down
76 changes: 76 additions & 0 deletions x-pack/plugins/security/server/authentication/api_keys.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,82 @@ describe('API Keys', () => {
});
});

describe('areAPIKeysEnabled()', () => {
it('returns false when security feature is disabled', async () => {
mockLicense.isEnabled.mockReturnValue(false);

const result = await apiKeys.areAPIKeysEnabled();
expect(result).toEqual(false);
expect(mockScopedClusterClient.callAsCurrentUser).not.toHaveBeenCalled();
expect(mockScopedClusterClient.callAsInternalUser).not.toHaveBeenCalled();
expect(mockClusterClient.callAsInternalUser).not.toHaveBeenCalled();
});

it('returns false when the exception metadata indicates api keys are disabled', async () => {
mockLicense.isEnabled.mockReturnValue(true);
const error = new Error();
(error as any).body = {
error: { 'disabled.feature': 'api_keys' },
};
mockClusterClient.callAsInternalUser.mockRejectedValue(error);
const result = await apiKeys.areAPIKeysEnabled();
expect(mockClusterClient.callAsInternalUser).toHaveBeenCalledTimes(1);
expect(result).toEqual(false);
});

it('returns true when the operation completes without error', async () => {
mockLicense.isEnabled.mockReturnValue(true);
mockClusterClient.callAsInternalUser.mockResolvedValue({});
const result = await apiKeys.areAPIKeysEnabled();
expect(mockClusterClient.callAsInternalUser).toHaveBeenCalledTimes(1);
expect(result).toEqual(true);
});

it('throws the original error when exception metadata does not indicate that api keys are disabled', async () => {
mockLicense.isEnabled.mockReturnValue(true);
const error = new Error();
(error as any).body = {
error: { 'disabled.feature': 'something_else' },
};

mockClusterClient.callAsInternalUser.mockRejectedValue(error);
expect(apiKeys.areAPIKeysEnabled()).rejects.toThrowError(error);
expect(mockClusterClient.callAsInternalUser).toHaveBeenCalledTimes(1);
});

it('throws the original error when exception metadata does not contain `disabled.feature`', async () => {
mockLicense.isEnabled.mockReturnValue(true);
const error = new Error();
(error as any).body = {};

mockClusterClient.callAsInternalUser.mockRejectedValue(error);
expect(apiKeys.areAPIKeysEnabled()).rejects.toThrowError(error);
expect(mockClusterClient.callAsInternalUser).toHaveBeenCalledTimes(1);
});

it('throws the original error when exception contains no metadata', async () => {
mockLicense.isEnabled.mockReturnValue(true);
const error = new Error();

mockClusterClient.callAsInternalUser.mockRejectedValue(error);
expect(apiKeys.areAPIKeysEnabled()).rejects.toThrowError(error);
expect(mockClusterClient.callAsInternalUser).toHaveBeenCalledTimes(1);
});

it('calls callCluster with proper parameters', async () => {
mockLicense.isEnabled.mockReturnValue(true);
mockClusterClient.callAsInternalUser.mockResolvedValueOnce({});

const result = await apiKeys.areAPIKeysEnabled();
expect(result).toEqual(true);
expect(mockClusterClient.callAsInternalUser).toHaveBeenCalledWith('shield.invalidateAPIKey', {
body: {
id: 'kibana-api-key-service-test',
},
});
});
});

describe('create()', () => {
it('returns null when security feature is disabled', async () => {
mockLicense.isEnabled.mockReturnValue(false);
Expand Down
34 changes: 34 additions & 0 deletions x-pack/plugins/security/server/authentication/api_keys.ts
Original file line number Diff line number Diff line change
Expand Up @@ -125,6 +125,35 @@ export class APIKeys {
this.license = license;
}

/**
* Determines if API Keys are enabled in Elasticsearch.
*/
async areAPIKeysEnabled(): Promise<boolean> {
if (!this.license.isEnabled()) {
return false;
}

const id = `kibana-api-key-service-test`;

this.logger.debug(
`Testing if API Keys are enabled by attempting to invalidate a non-existant key: ${id}`
);

try {
await this.clusterClient.callAsInternalUser('shield.invalidateAPIKey', {
body: {
id,
},
});
return true;
Copy link
Member

Choose a reason for hiding this comment

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

question: what does it mean if ES replies with 200 to such request? Does Elasticsearch just ignores unknown API keys? It seems ES works with security tokens differently then elastic/elasticsearch#54532 (comment)

Copy link
Member Author

Choose a reason for hiding this comment

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

question: what does it mean if ES replies with 200 to such request? Does Elasticsearch just ignores unknown API keys? It seems ES works with security tokens differently then elastic/elasticsearch#54532 (comment)

Yeah, it returns a 200 response:
image

Copy link
Member

Choose a reason for hiding this comment

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

I see, thanks for confirming. Just out of curiosity, @jkakavas is there any valid reason to return different status codes for "unknown" api keys and access tokens during invalidation or we just didn't have chance to unify that yet?

Copy link
Member

Choose a reason for hiding this comment

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

It's the latter. I just opened elastic/elasticsearch#55671 to keep track of the work needed for the invalidate api keys API

} catch (e) {
if (this.doesErrorIndicateAPIKeysAreDisabled(e)) {
return false;
}
throw e;
}
}

/**
* Tries to create an API key for the current user.
* @param request Request instance.
Expand Down Expand Up @@ -247,6 +276,11 @@ export class APIKeys {
return result;
}

private doesErrorIndicateAPIKeysAreDisabled(e: Record<string, any>) {
const disabledFeature = e.body?.error?.['disabled.feature'];
return disabledFeature === 'api_keys';
}

private getGrantParams(authorizationHeader: HTTPAuthorizationHeader): GrantAPIKeyParams {
if (authorizationHeader.scheme.toLowerCase() === 'bearer') {
return {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ export const authenticationMock = {
login: jest.fn(),
logout: jest.fn(),
isProviderTypeEnabled: jest.fn(),
areAPIKeysEnabled: jest.fn(),
createAPIKey: jest.fn(),
getCurrentUser: jest.fn(),
grantAPIKeyAsInternalUser: jest.fn(),
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 @@ -172,6 +172,7 @@ export async function setupAuthentication({
getSessionInfo: authenticator.getSessionInfo.bind(authenticator),
isProviderTypeEnabled: authenticator.isProviderTypeEnabled.bind(authenticator),
getCurrentUser,
areAPIKeysEnabled: () => apiKeys.areAPIKeysEnabled(),
createAPIKey: (request: KibanaRequest, params: CreateAPIKeyParams) =>
apiKeys.create(request, params),
grantAPIKeyAsInternalUser: (request: KibanaRequest) => apiKeys.grantAsInternalUser(request),
Expand Down
1 change: 1 addition & 0 deletions x-pack/plugins/security/server/plugin.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,7 @@ describe('Security Plugin', () => {
"registerPrivilegesWithCluster": [Function],
},
"authc": Object {
"areAPIKeysEnabled": [Function],
"createAPIKey": [Function],
"getCurrentUser": [Function],
"getSessionInfo": [Function],
Expand Down
Loading