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

Require granted API Keys to have a name #71623

Merged
merged 2 commits into from
Jul 14, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
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
30 changes: 24 additions & 6 deletions x-pack/plugins/alerts/server/alerts_client.ts
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ import {
SavedObjectReference,
SavedObject,
} from 'src/core/server';
import _ from 'lodash';
import { ActionsClient } from '../../actions/server';
import {
Alert,
Expand Down Expand Up @@ -53,7 +54,7 @@ interface ConstructorOptions {
spaceId?: string;
namespace?: string;
getUserName: () => Promise<string | null>;
createAPIKey: () => Promise<CreateAPIKeyResult>;
createAPIKey: (name: string) => Promise<CreateAPIKeyResult>;
invalidateAPIKey: (params: InvalidateAPIKeyParams) => Promise<InvalidateAPIKeyResult>;
getActionsClient: () => Promise<ActionsClient>;
}
Expand Down Expand Up @@ -129,7 +130,7 @@ export class AlertsClient {
private readonly taskManager: TaskManagerStartContract;
private readonly savedObjectsClient: SavedObjectsClientContract;
private readonly alertTypeRegistry: AlertTypeRegistry;
private readonly createAPIKey: () => Promise<CreateAPIKeyResult>;
private readonly createAPIKey: (name: string) => Promise<CreateAPIKeyResult>;
private readonly invalidateAPIKey: (
params: InvalidateAPIKeyParams
) => Promise<InvalidateAPIKeyResult>;
Expand Down Expand Up @@ -167,7 +168,10 @@ export class AlertsClient {
const alertType = this.alertTypeRegistry.get(data.alertTypeId);
const validatedAlertTypeParams = validateAlertTypeParams(alertType, data.params);
const username = await this.getUserName();
const createdAPIKey = data.enabled ? await this.createAPIKey() : null;

const createdAPIKey = data.enabled
? await this.createAPIKey(this.generateAPIKeyName(alertType.id, data.name))
: null;

this.validateActions(alertType, data.actions);

Expand Down Expand Up @@ -334,7 +338,9 @@ export class AlertsClient {

const { actions, references } = await this.denormalizeActions(data.actions);
const username = await this.getUserName();
const createdAPIKey = attributes.enabled ? await this.createAPIKey() : null;
const createdAPIKey = attributes.enabled
? await this.createAPIKey(this.generateAPIKeyName(alertType.id, data.name))
: null;
const apiKeyAttributes = this.apiKeyAsAlertAttributes(createdAPIKey, username);

const updatedObject = await this.savedObjectsClient.update<RawAlert>(
Expand Down Expand Up @@ -406,7 +412,10 @@ export class AlertsClient {
id,
{
...attributes,
...this.apiKeyAsAlertAttributes(await this.createAPIKey(), username),
...this.apiKeyAsAlertAttributes(
await this.createAPIKey(this.generateAPIKeyName(attributes.alertTypeId, attributes.name)),
username
),
updatedBy: username,
},
{ version }
Expand Down Expand Up @@ -464,7 +473,12 @@ export class AlertsClient {
{
...attributes,
enabled: true,
...this.apiKeyAsAlertAttributes(await this.createAPIKey(), username),
...this.apiKeyAsAlertAttributes(
await this.createAPIKey(
this.generateAPIKeyName(attributes.alertTypeId, attributes.name)
),
username
),
updatedBy: username,
},
{ version }
Expand Down Expand Up @@ -697,4 +711,8 @@ export class AlertsClient {
references,
};
}

private generateAPIKeyName(alertTypeId: string, alertName: string) {
return _.truncate(`Alerting: ${alertTypeId}/${alertName}`, { length: 256 });
Copy link
Member

Choose a reason for hiding this comment

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

question: is this 256 limit documented anywhere? Just curious.

Copy link
Member Author

Choose a reason for hiding this comment

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

Nope, I found this by testing API keys with very long names. Also tested a bunch of "special" characters, and there doesn't seem to be a restriction on what you're allowed to put here

}
}
8 changes: 6 additions & 2 deletions x-pack/plugins/alerts/server/alerts_client_factory.ts
Original file line number Diff line number Diff line change
Expand Up @@ -70,15 +70,19 @@ export class AlertsClientFactory {
const user = await securityPluginSetup.authc.getCurrentUser(request);
return user ? user.username : null;
},
async createAPIKey() {
async createAPIKey(name: string) {
if (!securityPluginSetup) {
return { apiKeysEnabled: false };
}
// Create an API key using the new grant API - in this case the Kibana system user is creating the
// API key for the user, instead of having the user create it themselves, which requires api_key
// privileges
const createAPIKeyResult = await securityPluginSetup.authc.grantAPIKeyAsInternalUser(
request
request,
{
name,
role_descriptors: {},
}
);
if (!createAPIKeyResult) {
return { apiKeysEnabled: false };
Expand Down
38 changes: 34 additions & 4 deletions x-pack/plugins/security/server/authentication/api_keys.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -162,7 +162,10 @@ describe('API Keys', () => {
describe('grantAsInternalUser()', () => {
it('returns null when security feature is disabled', async () => {
mockLicense.isEnabled.mockReturnValue(false);
const result = await apiKeys.grantAsInternalUser(httpServerMock.createKibanaRequest());
const result = await apiKeys.grantAsInternalUser(httpServerMock.createKibanaRequest(), {
name: 'test_api_key',
role_descriptors: {},
});
expect(result).toBeNull();

expect(mockClusterClient.callAsInternalUser).not.toHaveBeenCalled();
Expand All @@ -174,21 +177,33 @@ describe('API Keys', () => {
id: '123',
name: 'key-name',
api_key: 'abc123',
expires: '1d',
});
const result = await apiKeys.grantAsInternalUser(
httpServerMock.createKibanaRequest({
headers: {
authorization: `Basic ${encodeToBase64('foo:bar')}`,
},
})
}),
{
name: 'test_api_key',
role_descriptors: { foo: true },
expiration: '1d',
}
);
expect(result).toEqual({
api_key: 'abc123',
id: '123',
name: 'key-name',
expires: '1d',
});
expect(mockClusterClient.callAsInternalUser).toHaveBeenCalledWith('shield.grantAPIKey', {
body: {
api_key: {
name: 'test_api_key',
role_descriptors: { foo: true },
expiration: '1d',
},
grant_type: 'password',
username: 'foo',
password: 'bar',
Expand All @@ -208,7 +223,12 @@ describe('API Keys', () => {
headers: {
authorization: `Bearer foo-access-token`,
},
})
}),
{
name: 'test_api_key',
role_descriptors: { foo: true },
expiration: '1d',
}
);
expect(result).toEqual({
api_key: 'abc123',
Expand All @@ -217,6 +237,11 @@ describe('API Keys', () => {
});
expect(mockClusterClient.callAsInternalUser).toHaveBeenCalledWith('shield.grantAPIKey', {
body: {
api_key: {
name: 'test_api_key',
role_descriptors: { foo: true },
expiration: '1d',
},
grant_type: 'access_token',
access_token: 'foo-access-token',
},
Expand All @@ -231,7 +256,12 @@ describe('API Keys', () => {
headers: {
authorization: `Digest username="foo"`,
},
})
}),
{
name: 'test_api_key',
role_descriptors: { foo: true },
expiration: '1d',
}
)
).rejects.toThrowErrorMatchingInlineSnapshot(
`"Unsupported scheme \\"Digest\\" for granting API Key"`
Expand Down
12 changes: 9 additions & 3 deletions x-pack/plugins/security/server/authentication/api_keys.ts
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ export interface CreateAPIKeyParams {
}

interface GrantAPIKeyParams {
api_key: CreateAPIKeyParams;
grant_type: 'password' | 'access_token';
username?: string;
password?: string;
Expand Down Expand Up @@ -188,7 +189,7 @@ export class APIKeys {
* Tries to grant an API key for the current user.
* @param request Request instance.
*/
async grantAsInternalUser(request: KibanaRequest) {
async grantAsInternalUser(request: KibanaRequest, createParams: CreateAPIKeyParams) {
Copy link
Member

Choose a reason for hiding this comment

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

optional nit: missing JSDoc for createParams (I admit these JSDocs are useless most of the time, so feel free to ignore to not wait for CI run once again).

Copy link
Member Author

Choose a reason for hiding this comment

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

I am going to merge w/o the JSDoc comment, just in the interest of time given our tight schedule today. I'll try to be more diligent about this going forward though!

if (!this.license.isEnabled()) {
return null;
}
Expand All @@ -200,7 +201,7 @@ export class APIKeys {
`Unable to grant an API Key, request does not contain an authorization header`
);
}
const params = this.getGrantParams(authorizationHeader);
const params = this.getGrantParams(createParams, authorizationHeader);

// User needs `manage_api_key` or `grant_api_key` privilege to use this API
let result: GrantAPIKeyResult;
Expand Down Expand Up @@ -281,9 +282,13 @@ export class APIKeys {
return disabledFeature === 'api_keys';
}

private getGrantParams(authorizationHeader: HTTPAuthorizationHeader): GrantAPIKeyParams {
private getGrantParams(
createParams: CreateAPIKeyParams,
authorizationHeader: HTTPAuthorizationHeader
): GrantAPIKeyParams {
if (authorizationHeader.scheme.toLowerCase() === 'bearer') {
return {
api_key: createParams,
grant_type: 'access_token',
access_token: authorizationHeader.credentials,
};
Expand All @@ -294,6 +299,7 @@ export class APIKeys {
authorizationHeader.credentials
);
return {
api_key: createParams,
grant_type: 'password',
username: basicCredentials.username,
password: basicCredentials.password,
Expand Down
12 changes: 9 additions & 3 deletions x-pack/plugins/security/server/authentication/index.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -374,7 +374,10 @@ describe('setupAuthentication()', () => {
});

describe('grantAPIKeyAsInternalUser()', () => {
let grantAPIKeyAsInternalUser: (request: KibanaRequest) => Promise<CreateAPIKeyResult | null>;
let grantAPIKeyAsInternalUser: (
request: KibanaRequest,
params: CreateAPIKeyParams
) => Promise<CreateAPIKeyResult | null>;
beforeEach(async () => {
grantAPIKeyAsInternalUser = (await setupAuthentication(mockSetupAuthenticationParams))
.grantAPIKeyAsInternalUser;
Expand All @@ -384,10 +387,13 @@ describe('setupAuthentication()', () => {
const request = httpServerMock.createKibanaRequest();
const apiKeysInstance = jest.requireMock('./api_keys').APIKeys.mock.instances[0];
apiKeysInstance.grantAsInternalUser.mockResolvedValueOnce({ api_key: 'foo' });
await expect(grantAPIKeyAsInternalUser(request)).resolves.toEqual({

const createParams = { name: 'test_key', role_descriptors: {} };

await expect(grantAPIKeyAsInternalUser(request, createParams)).resolves.toEqual({
api_key: 'foo',
});
expect(apiKeysInstance.grantAsInternalUser).toHaveBeenCalledWith(request);
expect(apiKeysInstance.grantAsInternalUser).toHaveBeenCalledWith(request, createParams);
});
});

Expand Down
3 changes: 2 additions & 1 deletion x-pack/plugins/security/server/authentication/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -187,7 +187,8 @@ export async function setupAuthentication({
areAPIKeysEnabled: () => apiKeys.areAPIKeysEnabled(),
createAPIKey: (request: KibanaRequest, params: CreateAPIKeyParams) =>
apiKeys.create(request, params),
grantAPIKeyAsInternalUser: (request: KibanaRequest) => apiKeys.grantAsInternalUser(request),
grantAPIKeyAsInternalUser: (request: KibanaRequest, params: CreateAPIKeyParams) =>
apiKeys.grantAsInternalUser(request, params),
invalidateAPIKey: (request: KibanaRequest, params: InvalidateAPIKeyParams) =>
apiKeys.invalidate(request, params),
invalidateAPIKeyAsInternalUser: (params: InvalidateAPIKeyParams) =>
Expand Down