Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
19 commits
Select commit Hold shift + click to select a range
8793b2a
Starting audit logger
jonathan-buttner Mar 23, 2021
21b20aa
Merge branch 'cases-rbac-poc' of github.com:elastic/kibana into cases…
jonathan-buttner Mar 25, 2021
25d2403
Finishing auth audit logger
jonathan-buttner Mar 25, 2021
400601b
Fixing tests and types
jonathan-buttner Mar 26, 2021
ffc81ec
Adding audit event creator
jonathan-buttner Mar 26, 2021
188186e
Merge branch 'cases-rbac-poc' of github.com:elastic/kibana into cases…
jonathan-buttner Mar 26, 2021
62e4d24
Renaming class to scope
jonathan-buttner Mar 26, 2021
2b51111
Merge branch 'cases-rbac-poc' of github.com:elastic/kibana into cases…
jonathan-buttner Apr 5, 2021
e8e03ec
Adding audit logger messages to create and find
jonathan-buttner Apr 5, 2021
1c57dcf
Merge branch 'master' of github.com:elastic/kibana into cases-rbac-au…
jonathan-buttner Apr 5, 2021
0b1db8d
Adding comments and fixing import issue
jonathan-buttner Apr 5, 2021
94a3a59
Merge branch 'cases-rbac-poc' of github.com:elastic/kibana into cases…
jonathan-buttner Apr 5, 2021
32242b2
Fixing type errors
jonathan-buttner Apr 5, 2021
4b4691b
Fixing tests and adding username to message
jonathan-buttner Apr 5, 2021
1a33a7e
Addressing PR feedback
jonathan-buttner Apr 6, 2021
1da64bc
Removing unneccessary log and generating id
jonathan-buttner Apr 6, 2021
f2da6d0
Merge branch 'cases-rbac-poc' of github.com:elastic/kibana into cases…
jonathan-buttner Apr 6, 2021
01c2edf
Fixing module issue and remove expect.anything
jonathan-buttner Apr 6, 2021
b1ab09f
Merge branch 'cases-rbac-poc' of github.com:elastic/kibana into cases…
jonathan-buttner Apr 6, 2021
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
131 changes: 131 additions & 0 deletions x-pack/plugins/cases/server/authorization/audit_logger.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,131 @@
/*
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
* or more contributor license agreements. Licensed under the Elastic License
* 2.0; you may not use this file except in compliance with the Elastic License
* 2.0.
*/

import { OperationDetails } from '.';
import { AuditLogger, EventCategory, EventOutcome } from '../../../security/server';

enum AuthorizationResult {
Unauthorized = 'Unauthorized',
Authorized = 'Authorized',
}

export class AuthorizationAuditLogger {
private readonly auditLogger?: AuditLogger;

constructor(logger: AuditLogger | undefined) {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Why it can be undefined?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

It's because we get the AuditLogger from the security plugin and the security plugin can be undefined (my guess is that'll happen when security is disabled).

this.auditLogger = logger;
}

private createMessage({
result,
owner,
operation,
}: {
result: AuthorizationResult;
owner?: string;
operation: OperationDetails;
}): string {
const ownerMsg = owner == null ? 'of any owner' : `with "${owner}" as the owner`;
/**
* This will take the form:
* `Unauthorized to create case with "securitySolution" as the owner`
* `Unauthorized to find cases of any owner`.
*/
return `${result} to ${operation.verbs.present} ${operation.docType} ${ownerMsg}`;
}

private logSuccessEvent({
message,
operation,
username,
}: {
message: string;
operation: OperationDetails;
username?: string;
}) {
this.auditLogger?.log({
message: `${username ?? 'unknown user'} ${message}`,
event: {
action: operation.action,
category: EventCategory.DATABASE,
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Do you think we should create EventCategory.AUTHORIZATION and use that?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

There's no official authorization field in the ecs spec currently: https://www.elastic.co/guide/en/ecs/current/ecs-allowed-values-event-category.html I believe we'd want whatever we use to be present there. We could bring it up to the ECS team though.

@legrego should we use DATABASE for our category in the Authorization audit logs?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Since I expect these events are recording CRUD operations against case saved objects, then yes, let's prefer EventCategory.DATABASE. This is consistent with the way we record events against "normal" saved objects within the security plugin:

category: EventCategory.DATABASE,

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Thanks Larry!

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I see! I wasn't aware of the ECS mapping. Thank you both!

type: operation.type,
outcome: EventOutcome.SUCCESS,
},
...(username != null && {
user: {
name: username,
},
}),
});
}

public failure({
username,
owner,
operation,
}: {
username?: string;
owner?: string;
operation: OperationDetails;
}): string {
const message = this.createMessage({
result: AuthorizationResult.Unauthorized,
owner,
operation,
});
this.auditLogger?.log({
message: `${username ?? 'unknown user'} ${message}`,
event: {
action: operation.action,
category: EventCategory.DATABASE,
type: operation.type,
outcome: EventOutcome.FAILURE,
},
// add the user information if we have it
...(username != null && {
user: {
name: username,
},
}),
});
return message;
}

public success({
username,
operation,
owner,
}: {
username: string;
owner: string;
operation: OperationDetails;
}): string {
const message = this.createMessage({
result: AuthorizationResult.Authorized,
owner,
operation,
});
this.logSuccessEvent({ message, operation, username });
return message;
}

public bulkSuccess({
username,
operation,
owners,
}: {
username?: string;
owners: string[];
operation: OperationDetails;
}): string {
const message = `${AuthorizationResult.Authorized} to ${operation.verbs.present} ${
operation.docType
} of owner: ${owners.join(', ')}`;
this.logSuccessEvent({ message, operation, username });
return message;
}
}
87 changes: 32 additions & 55 deletions x-pack/plugins/cases/server/authorization/authorization.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,11 +7,11 @@

import { KibanaRequest } from 'kibana/server';
import Boom from '@hapi/boom';
import { KueryNode } from '../../../../../src/plugins/data/server';
import { SecurityPluginStart } from '../../../security/server';
import { PluginStartContract as FeaturesPluginStart } from '../../../features/server';
import { GetSpaceFn, ReadOperations, WriteOperations } from './types';
import { AuthorizationFilter, GetSpaceFn } from './types';
import { getOwnersFilter } from './utils';
import { AuthorizationAuditLogger, OperationDetails, Operations } from '.';

/**
* This class handles ensuring that the user making a request has the correct permissions
Expand All @@ -21,25 +21,23 @@ export class Authorization {
private readonly request: KibanaRequest;
private readonly securityAuth: SecurityPluginStart['authz'] | undefined;
private readonly featureCaseOwners: Set<string>;
private readonly isAuthEnabled: boolean;
// TODO: create this
// private readonly auditLogger: AuthorizationAuditLogger;
private readonly auditLogger: AuthorizationAuditLogger;

private constructor({
request,
securityAuth,
caseOwners,
isAuthEnabled,
auditLogger,
}: {
request: KibanaRequest;
securityAuth?: SecurityPluginStart['authz'];
caseOwners: Set<string>;
isAuthEnabled: boolean;
auditLogger: AuthorizationAuditLogger;
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think this should be PublicMethodsOf<AuthorizationAuditLogger>

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Do we need to create a full mock for the AuthorizationAuditLogger? It's pretty easy to just initialize it with a fake logger since that's all it needs. We could just pass around the actual logger in our tests if we need to.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I see what you mean. I have a dilemma about that. From the perspective of a consumer of the AuthorizationAuditLogger only public methods should be available and PublicMethodsOf achieves that. On the other hand, declaring PublicMethodsOf all classes that are being passed around doesn't make much sense. Maybe when we export a class (like the cases client) it is ok to use the PublicMethodsOf and when we use the class internally it fine to declare them without it. I am fine as it is btw 😄

}) {
this.request = request;
this.securityAuth = securityAuth;
this.featureCaseOwners = caseOwners;
this.isAuthEnabled = isAuthEnabled;
this.auditLogger = auditLogger;
}

/**
Expand All @@ -50,13 +48,13 @@ export class Authorization {
securityAuth,
getSpace,
features,
isAuthEnabled,
auditLogger,
}: {
request: KibanaRequest;
securityAuth?: SecurityPluginStart['authz'];
getSpace: GetSpaceFn;
features: FeaturesPluginStart;
isAuthEnabled: boolean;
auditLogger: AuthorizationAuditLogger;
}): Promise<Authorization> {
// Since we need to do async operations, this static method handles that before creating the Auth class
let caseOwners: Set<string>;
Expand All @@ -74,102 +72,81 @@ export class Authorization {
caseOwners = new Set<string>();
}

return new Authorization({ request, securityAuth, caseOwners, isAuthEnabled });
return new Authorization({ request, securityAuth, caseOwners, auditLogger });
}

private shouldCheckAuthorization(): boolean {
return this.securityAuth?.mode?.useRbacForRequest(this.request) ?? false;
}

public async ensureAuthorized(owner: string, operation: ReadOperations | WriteOperations) {
// TODO: remove
if (!this.isAuthEnabled) {
return;
}

public async ensureAuthorized(owner: string, operation: OperationDetails) {
const { securityAuth } = this;
const isOwnerAvailable = this.featureCaseOwners.has(owner);

// TODO: throw if the request is not authorized
if (securityAuth && this.shouldCheckAuthorization()) {
// TODO: implement ensure logic
const requiredPrivileges: string[] = [securityAuth.actions.cases.get(owner, operation)];
const requiredPrivileges: string[] = [securityAuth.actions.cases.get(owner, operation.name)];

const checkPrivileges = securityAuth.checkPrivilegesDynamicallyWithRequest(this.request);
const { hasAllRequested, username, privileges } = await checkPrivileges({
const { hasAllRequested, username } = await checkPrivileges({
kibana: requiredPrivileges,
});

if (!isOwnerAvailable) {
// TODO: throw if any of the owner are not available
/**
* Under most circumstances this would have been caught by `checkPrivileges` as
* a user can't have Privileges to an unknown owner, but super users
* don't actually get "privilege checked" so the made up owner *will* return
* as Privileged.
* This check will ensure we don't accidentally let these through
*/
// TODO: audit log using `username`
throw Boom.forbidden('User does not have permissions for this owner');
throw Boom.forbidden(this.auditLogger.failure({ username, owner, operation }));
}

if (hasAllRequested) {
// TODO: user authorized. log success
this.auditLogger.success({ username, operation, owner });
} else {
const authorizedPrivileges = privileges.kibana.reduce<string[]>((acc, privilege) => {
if (privilege.authorized) {
return [...acc, privilege.privilege];
}
return acc;
}, []);

const unauthorizedPrivilages = requiredPrivileges.filter(
(privilege) => !authorizedPrivileges.includes(privilege)
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I looked through the alerting code and it seems like they're only using these to determine the consumer and scope. We don't have a concept of those and it didn't look like they were actually logging the either of the authorized or unauthorized privileges in the message so I don't think we need these 🤷‍♂️ let me know what you think @cnasikas

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Make sense to me!

);

// TODO: audit log
// TODO: User unauthorized. throw an error. authorizedPrivileges & unauthorizedPrivilages are needed for logging.
throw Boom.forbidden('Not authorized for this owner');
throw Boom.forbidden(this.auditLogger.failure({ owner, operation, username }));
}
} else if (!isOwnerAvailable) {
// TODO: throw an error
throw Boom.forbidden('Security is disabled but no owner was found');
throw Boom.forbidden(this.auditLogger.failure({ owner, operation }));
}

// else security is disabled so let the operation proceed
}

public async getFindAuthorizationFilter(
savedObjectType: string
): Promise<{
filter?: KueryNode;
ensureSavedObjectIsAuthorized: (owner: string) => void;
}> {
public async getFindAuthorizationFilter(savedObjectType: string): Promise<AuthorizationFilter> {
const { securityAuth } = this;
const operation = Operations.findCases;
if (securityAuth && this.shouldCheckAuthorization()) {
const { authorizedOwners } = await this.getAuthorizedOwners([ReadOperations.Find]);
const { username, authorizedOwners } = await this.getAuthorizedOwners([operation]);

if (!authorizedOwners.length) {
// TODO: Better error message, log error
throw Boom.forbidden('Not authorized for this owner');
throw Boom.forbidden(this.auditLogger.failure({ username, operation }));
}

return {
filter: getOwnersFilter(savedObjectType, authorizedOwners),
ensureSavedObjectIsAuthorized: (owner: string) => {
if (!authorizedOwners.includes(owner)) {
// TODO: log error
throw Boom.forbidden('Not authorized for this owner');
throw Boom.forbidden(this.auditLogger.failure({ username, operation, owner }));
}
},
logSuccessfulAuthorization: () => {
if (authorizedOwners.length) {
this.auditLogger.bulkSuccess({ username, owners: authorizedOwners, operation });
}
},
};
}

return { ensureSavedObjectIsAuthorized: (owner: string) => {} };
return {
ensureSavedObjectIsAuthorized: (owner: string) => {},
logSuccessfulAuthorization: () => {},
};
}

private async getAuthorizedOwners(
operations: Array<ReadOperations | WriteOperations>
operations: OperationDetails[]
): Promise<{
username?: string;
hasAllRequested: boolean;
Expand All @@ -182,7 +159,7 @@ export class Authorization {

for (const owner of featureCaseOwners) {
for (const operation of operations) {
requiredPrivileges.set(securityAuth.actions.cases.get(owner, operation), [owner]);
requiredPrivileges.set(securityAuth.actions.cases.get(owner, operation.name), [owner]);
}
}

Expand Down
Loading