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

Removing circular dependency between spaces and security #81891

Merged
merged 15 commits into from
Nov 19, 2020

Conversation

legrego
Copy link
Member

@legrego legrego commented Oct 28, 2020

Summary

We've discussed a number of ways to break the circular dependency between security and spaces. A few of those options are outlined in #80496 (comment).

While I think we all agree it isn't the best solution, this PR implements "option 2" as described in the above comment:

Allow the SpacesClient to be augmented via a security wrapper (like we do for saved objects) which can then authorize access to spaces.

I view this as a mid-term solution until we can explore something more holistic that isn't so tightly coupled to Spaces.

In order to make the SpacesClient "wrappable", this PR introduces a new SpacesClientService, which exposes functions to influence the creation of client instances, as well as a factory function to create the scoped spaces clients.

The Security plugin registers a SpacesClient Wrapper during setup, which is responsible for performing authorization checks and audit logging.

Resolves #80496

@legrego legrego force-pushed the spaces/remove-circular-dependency branch 9 times, most recently from ca60798 to bc12f7f Compare November 9, 2020 18:22
@legrego legrego force-pushed the spaces/remove-circular-dependency branch from bc12f7f to 255f0ba Compare November 9, 2020 19:13
allowedAtSpace = true;
}

if (spaces && attemptSpaceRetrieval) {
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 implicit run order of the various Capability Switchers guaranteed that Enterprise Search wouldn't overwrite the decision made by the Spaces plugin to disable these features.

The run order of these switchers has changed now that we've changed the dependency between security and spaces, and so Enterprise Search now needs to check for itself whether or not its feature is disabled within the current space.

features,
getCurrentUser: this.authc.getCurrentUser,
});

if (spaces) {
Copy link
Member Author

@legrego legrego Nov 9, 2020

Choose a reason for hiding this comment

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

note: need to add tests for this yet.

/**
* @deprecated will be removed in 8.0
*/
export class LegacySpacesAuditLogger {
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: Renamed in anticipation of introducing an ECS audit logger for spaces

@@ -0,0 +1,207 @@
/*
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: It's a net-new file, but the logic was extracted from the existing SpacesClient, and so was mostly copy/paste. A review here is still highly beneficial though!

@@ -37,31 +37,6 @@ export function initShareToSpacesApi(deps: ExternalRouteDeps) {
object: schema.object({ type: schema.string(), id: schema.string() }),
});

externalRouter.get(
{
path: '/internal/spaces/_share_saved_object_permissions',
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: route has been moved to the security plugin under /internal/security/_share_saved_object_permissions

@legrego legrego added release_note:skip Skip the PR/issue when compiling release notes Team:Security Team focused on: Auth, Users, Roles, Spaces, Audit Logging, and more! v7.11 v8.0.0 labels Nov 9, 2020
@legrego legrego marked this pull request as ready for review November 9, 2020 20:51
@legrego legrego requested a review from a team November 9, 2020 20:51
@legrego legrego requested a review from a team as a code owner November 9, 2020 20:51
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-security (Team:Security)

@azasypkin
Copy link
Member

ACK: will review today or tomorrow morning at the latest.

Copy link
Contributor

@cee-chen cee-chen left a comment

Choose a reason for hiding this comment

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

Just a few comments (mostly around the tests file) from the Enterprise Search side of things, thank you so much for doing this for us Larry!!

Copy link
Member

@azasypkin azasypkin left a comment

Choose a reason for hiding this comment

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

The approach looks good to me! Haven't tested manually yet, but will do at the next review pass. Left mostly nits and a couple of questions.

@@ -58,30 +62,33 @@ export class Plugin {

private defaultSpaceService?: DefaultSpaceService;

private spacesService?: SpacesService;
Copy link
Member

Choose a reason for hiding this comment

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

nit: it seems we can just create this service in constructor?

Copy link
Member Author

Choose a reason for hiding this comment

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

good idea


type RequestFacade = KibanaRequest | Legacy.Request;

export interface SpacesServiceSetup {
scopedClient(request: RequestFacade): Promise<SpacesClient>;
scopedClient(request: RequestFacade): ISpacesClient;
Copy link
Member

Choose a reason for hiding this comment

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

question: any reason we don't want consumers to perform conversion to a KibanaRequest on their own in case they need to? I'd honestly prefer us having a clean public API and keep workarounds with RequestFacade within a context that makes it clear why workaround is needed.

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'll investigate -- we needed the RequestFacade earlier in the NP migration, but that might have been to interop with plugins that still lived in the LP. Now that we're fully migrated, we might be able to clean this up.

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 think I have this working. It involved changing the way that Alerts and Actions construct their fake requests, but pending review from the alerting team, this should be good!

auditLogger,
}: SpacesServiceDeps): Promise<SpacesServiceSetup> {
public setup({ http, config$ }: SpacesServiceDeps): SpacesServiceSetup {
this.spacesClientService = new SpacesClientService(
Copy link
Member

Choose a reason for hiding this comment

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

nit: it seems we could also create this service in constructor and pass config$ in its setup method (to access this.spacesClientService methods without !)?

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: I made the SpacesClientService a start dependency of the SpacesService in a recent refactoring, so the SpacesService is no longer responsible for constructing this instance.

};

return {
scopedClient: getScopedClient,
Copy link
Member

Choose a reason for hiding this comment

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

question: do we expose scopedClient in setup just temporarily to make migration easier? It seems it's not really functional until start and we expose it through start as well (createSpacesClient).

If we still want to expose this for the aforementioned reasons, can we either unify method names between setup and start or somehow rename method we expose in setup so that it's clear that no one should use it if possible?

Copy link
Member Author

Choose a reason for hiding this comment

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

Great question. I was going to refactor this service to stop exposing so much out of setup, but that ended up causing a lot of downstream changes that I wasn't comfortable keeping in this PR, so I opted against it.

If we still want to expose this for the aforementioned reasons, can we either unify method names between setup and start or somehow rename method we expose in setup so that it's clear that no one should use it if possible?

++

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 was going to refactor this service to stop exposing so much out of setup, but that ended up causing a lot of downstream changes that I wasn't comfortable keeping in this PR, so I opted against it.

Welp, I went and removed a bunch of functions from setup. It added a bit of scope to this PR, but I'm generally happier with it. If you're not comfortable with the scope creep, let me know and I'll split it out into a different PR.

Copy link
Member

Choose a reason for hiding this comment

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

Nah, let's keep it here, I believe this PR is an ideal place for a refactoring like this.

};

describe('SpacesService', () => {
describe('#getSpaceId', () => {
it('returns the default space id when no identifier is present', async () => {
const spacesServiceSetup = await createService();
const { spacesServiceSetup } = await createService();
Copy link
Member

Choose a reason for hiding this comment

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

nit: looks like we can make createService sync now and remove all these await in tests?

public async getAll({
purpose = 'any',
includeAuthorizedPurposes,
}: GetAllSpacesOptions = {}): Promise<Space[]> {
Copy link
Member

Choose a reason for hiding this comment

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

question: do we intentionally cast GetSpaceResult[] to Space[]?

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah good catch - I think this should be GetSpaceResult[] instead.

legrego and others added 2 commits November 12, 2020 07:47
Co-authored-by: Constance <constancecchen@users.noreply.github.com>
Co-authored-by: Aleh Zasypkin <aleh.zasypkin@gmail.com>
- Reorganize top level describes into 3 space-based blocks into based on spaces:
  - space disabled
  - spaces plugin unavailable
  - space enabled (most previous tests go under this new block) with new beforeEach

- wrote new tests for uncovered lines 58, 66-69
@spalger spalger added v7.11.0 and removed v7.11 labels Nov 12, 2020
@legrego legrego requested a review from a team as a code owner November 16, 2020 14:23
Copy link
Contributor

@gmmorris gmmorris left a comment

Choose a reason for hiding this comment

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

Actions, Alerts & EventLog LGTM 👍
In fact.. better than before ;)

@legrego
Copy link
Member Author

legrego commented Nov 17, 2020

Hey @gmmorris, would you mind re-reviewing this? The initial set of changes didn't consider the user's current space when alerts were running in the background, which caused this set of test failures.

As a result, I changed the way that Actions and Alerts constructed their fake requests to (IMO) better align with their intent. They were being incorrectly cast as a KibanaRequest before, which is how they slipped through my first pass.

I'm not sure if my approach is entirely correct or not though, so I'd appreciate a re-review to make sure that I didn't break anything related to Actions, Alerts, or the Event Log.

@legrego legrego requested a review from gmmorris November 17, 2020 00:46
Comment on lines 95 to 96
const path = spaceId ? `/s/${spaceId}` : '/';

const fakeRequest = KibanaRequest.from(({
headers: requestHeaders,
getBasePath: () => this.context.getBasePath(spaceId),
Copy link
Contributor

Choose a reason for hiding this comment

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

This change looks wrong to me... but I'm not actually sure.
I thought context.getBasePath was implemented by the Spaces plugin and so line 95 feels like Spaces domain... 🤔
In plugin.ts we do this:

  private getBasePath = (spaceId?: string): string => {
    return this.spaces && spaceId ? this.spaces.getBasePath(spaceId) : this.serverBasePath!;
  };

Perhaps @mikecote can help here?

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 thought context.getBasePath was implemented by the Spaces plugin and so line 95 feels like Spaces domain...

Yeah we were using getBasePath from the Spaces plugin in order to simulate a request coming from the Legacy platform. This PR removed support for the LP requests, and instead forces consumers to construct KibanaRequest instances. Alerting & Actions were creating a LP fake request, but casting it as a KibanaRequest.

One of the reasons to fake a request was so that the Spaces plugin could properly scope instances of the Saved Objects client. Since we aren't honoring LP requests anymore, there isn't a need to provide the getBasePath function -- rather, consumers need to inform core about the request's base path by calling core.http.basePath.set(fakeRequest, spaceAwarePath) instead.

^^^^^
At least, this is the intent behind my most recent set of changes.

Copy link
Member Author

Choose a reason for hiding this comment

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

@mikecote what are your thoughts?

Copy link
Contributor

Choose a reason for hiding this comment

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

consumers need to inform core about the request's base path by calling core.http.basePath.set(fakeRequest, spaceAwarePath) instead.

If that is their recommendation for now, I'm ok with this change. Alerting is hacking fake requests in Kibana until scope-able elasticsearch clients is around so we'll have some odd code to deal with until then either way.

Copy link
Contributor

Choose a reason for hiding this comment

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

In that case my review still stands, but on this specific question I'm relying on your understanding of this 😆

@legrego
Copy link
Member Author

legrego commented Nov 18, 2020

@azasypkin ready for another review round. I made a lot of changes since your last review...sorry 😬

@legrego legrego requested a review from azasypkin November 18, 2020 15:59
@azasypkin
Copy link
Member

ACK: reviewing...

Copy link
Member

@azasypkin azasypkin left a comment

Choose a reason for hiding this comment

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

Looks great! Tested the most common scenarios locally and everything seems to be in order.

@@ -87,11 +88,12 @@ export class TaskRunnerFactory {
requestHeaders.authorization = `ApiKey ${apiKey}`;
}

const path = spaceId ? `/s/${spaceId}` : '/';
Copy link
Member

Choose a reason for hiding this comment

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

question: any reason why we cannot use already exported addSpaceIdToPath here (to leave /s/ logic to Spaces plugin)?

Copy link
Member Author

Choose a reason for hiding this comment

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

Good call, I don't see why we can't use that

@@ -91,9 +92,10 @@ export class TaskRunner {
requestHeaders.authorization = `ApiKey ${apiKey}`;
}

return ({
const path = spaceId ? `/s/${spaceId}` : '/';
Copy link
Member

Choose a reason for hiding this comment

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

question: same question about addSpaceIdToPath here.

@@ -44,7 +44,10 @@ export function getMlSystemProvider(
return await getGuards(request, savedObjectsClient)
.isMinimumLicense()
.ok(async ({ mlClient }) => {
const { isMlEnabledInSpace } = spacesUtilsProvider(spaces, request);
const { isMlEnabledInSpace } =
getSpaces !== undefined
Copy link
Member

Choose a reason for hiding this comment

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

question: do we really need this check here? It seems spacesUtilsProvider already does the right thing internally if getSpaces isn't defined.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good catch, this was leftover from a previous (failed) implementation

const baseClient = new SpacesClient(
this.debugLogger,
this.config,
this.repositoryFactory!(request, coreStart.savedObjects)
Copy link
Member

Choose a reason for hiding this comment

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

note: no need to change anything here, just observation: if we move if (!this.repositoryFactory) { check inside of this method we will be able to get rid of ! in this.repositoryFactory! or even do something like this:

const repository = this.repositoryFactory
  ? this.repositoryFactory(request, coreStart.savedObjects)
  : coreStart.savedObjects.createScopedRepository(request, ['space']);
const baseClient = new SpacesClient(this.debugLogger, this.config, repository);

createSpacesClient: SpacesClientServiceStart['createSpacesClient'];

/**
* Retrieves the space id associated with the provided request.
Copy link
Member

Choose a reason for hiding this comment

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

Thanks for adding documentation!

auditLogger: SpacesAuditLogger;
interface SpacesServiceStartDeps {
basePath: IBasePath;
spacesClientService: SpacesClientServiceStart;
}

export class SpacesService {
private configSubscription$?: Subscription;
Copy link
Member

Choose a reason for hiding this comment

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

nit: looks like we don't need this in this service anymore, and we may want to move this to SpacesClientService instead?

@@ -0,0 +1,14 @@
/*
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 thought this was going to be tracked as a move/rename. I moved the spaces_client directory out of the lib folder (as we discussed a long time ago during NP conversion). I didn't expect it to add too much noise to the PR...sorry about that!

Copy link
Member

Choose a reason for hiding this comment

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

I moved the spaces_client directory out of the lib folder (as we discussed a long time ago during NP conversion). I didn't expect it to add too much noise to the PR...sorry about that!

Haha, no worries, and thanks for reducing the amount of stuff we have in a lib folder!

spaces?: SpacesPluginSetup;
}

export const setupSpacesClient = ({ audit, authz, spaces }: Deps) => {
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: This logic was part of server/plugin.ts during your initial review. I pulled it into its own file to better organize the spaces-related logic, and to facilitate unit testing.

}

export const setupSpacesClient = ({ audit, authz, spaces }: Deps) => {
if (!spaces) {
Copy link
Member Author

Choose a reason for hiding this comment

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

No real reason -- plugin.ts is already pretty large, so I felt like "hiding" the conditional logic here would make scanning the plugin's setup function a little easier. I don't have a strong opinion here one way or another.

@@ -1,588 +0,0 @@
/*
Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for asking, I meant to leave a comment here. My original plan was to port these to become jest integration tests, but that ended up more complex than I anticipated. I'll restore these tests for now so that they aren't lost to time.

@@ -87,11 +88,12 @@ export class TaskRunnerFactory {
requestHeaders.authorization = `ApiKey ${apiKey}`;
}

const path = spaceId ? `/s/${spaceId}` : '/';
Copy link
Member Author

Choose a reason for hiding this comment

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

Good call, I don't see why we can't use that

@@ -44,7 +44,10 @@ export function getMlSystemProvider(
return await getGuards(request, savedObjectsClient)
.isMinimumLicense()
.ok(async ({ mlClient }) => {
const { isMlEnabledInSpace } = spacesUtilsProvider(spaces, request);
const { isMlEnabledInSpace } =
getSpaces !== undefined
Copy link
Member Author

Choose a reason for hiding this comment

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

Good catch, this was leftover from a previous (failed) implementation

@@ -23,13 +23,13 @@ describe('Spaces Plugin', () => {
const spacesSetup = await plugin.setup(core, { features, licensing });
expect(spacesSetup).toMatchInlineSnapshot(`
Object {
"spacesClient": Object {
Copy link
Member Author

Choose a reason for hiding this comment

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

Yup, great idea! I'm also removing async from the setup phase too -- we don't need it anymore.

public async delete(id: string) {
const existingSavedObject = await this.repository.get('space', id);
if (isReservedSpace(this.transformSavedObjectToSpace(existingSavedObject))) {
throw Boom.badRequest('This Space cannot be deleted because it is reserved.');
Copy link
Member Author

Choose a reason for hiding this comment

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

It can't hurt!

Copy link
Member

@jgowdyelastic jgowdyelastic left a comment

Choose a reason for hiding this comment

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

ML changes LGTM

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

Distributable file count

id before after diff
default 42888 42894 +6

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id before after diff
spaces 290.5KB 290.9KB +321.0B

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

@legrego legrego merged commit 7f962e5 into elastic:master Nov 19, 2020
@legrego legrego deleted the spaces/remove-circular-dependency branch November 19, 2020 18:41
legrego added a commit to legrego/kibana that referenced this pull request Nov 19, 2020
* Removing circular dependency between spaces and security

* Apply suggestions from code review

Co-authored-by: Constance <constancecchen@users.noreply.github.com>
Co-authored-by: Aleh Zasypkin <aleh.zasypkin@gmail.com>

* Tests refactor

- Reorganize top level describes into 3 space-based blocks into based on spaces:
  - space disabled
  - spaces plugin unavailable
  - space enabled (most previous tests go under this new block) with new beforeEach

- wrote new tests for uncovered lines 58, 66-69

* Review1: address PR feedback

* changing fake requests for alerts/actions

* Fixing tests

* fixing more tests

* Additional testing and refactoring

* Apply suggestions from code review

Co-authored-by: Aleh Zasypkin <aleh.zasypkin@gmail.com>

* Review 2: Address feedback

* Make ESLint happy again

Co-authored-by: Constance <constancecchen@users.noreply.github.com>
Co-authored-by: Aleh Zasypkin <aleh.zasypkin@gmail.com>
Co-authored-by: Constance Chen <constance.chen.3@gmail.com>
legrego added a commit that referenced this pull request Nov 19, 2020
… (#83841)

* Removing circular dependency between spaces and security

* Apply suggestions from code review

Co-authored-by: Constance <constancecchen@users.noreply.github.com>
Co-authored-by: Aleh Zasypkin <aleh.zasypkin@gmail.com>

* Tests refactor

- Reorganize top level describes into 3 space-based blocks into based on spaces:
  - space disabled
  - spaces plugin unavailable
  - space enabled (most previous tests go under this new block) with new beforeEach

- wrote new tests for uncovered lines 58, 66-69

* Review1: address PR feedback

* changing fake requests for alerts/actions

* Fixing tests

* fixing more tests

* Additional testing and refactoring

* Apply suggestions from code review

Co-authored-by: Aleh Zasypkin <aleh.zasypkin@gmail.com>

* Review 2: Address feedback

* Make ESLint happy again

Co-authored-by: Constance <constancecchen@users.noreply.github.com>
Co-authored-by: Aleh Zasypkin <aleh.zasypkin@gmail.com>
Co-authored-by: Constance Chen <constance.chen.3@gmail.com>

Co-authored-by: Constance <constancecchen@users.noreply.github.com>
Co-authored-by: Aleh Zasypkin <aleh.zasypkin@gmail.com>
Co-authored-by: Constance Chen <constance.chen.3@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release_note:skip Skip the PR/issue when compiling release notes Team:Security Team focused on: Auth, Users, Roles, Spaces, Audit Logging, and more! v7.11.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Get rid of implicit circular dependency between Security and Spaces plugins.
9 participants