Refactor SavedObjectsClient #134395

jportner · 2022-06-14T19:58:07Z

Overview

This PR removes the three Saved Objects Client (SOC) wrappers in favor of bespoke extensions that are consumed directly by the Saved Objects Repository (SOR).
This approach allows us to be more efficient and flexible for authorization in particular.

Notable changes

The three SOC wrappers for encryption, security, and spaces have been removed and replaced with extensions that are called directly from the SOR itself. Most of the behavior is functionally identical.
All the extensions can still be disabled when instantiating a scoped SOC by using the excludedWrappers option (see comment below).
Security: before, all SOC functions checked authorization before the SOR checked to see if parameters were valid. It wasn’t exactly wrong before, but it did lead to confusing integration tests where a "fully privileged" user would get different results from a "superuser". This would happen because a superuser would be authorized to access resources that aren’t registered with Elasticsearch (such as an invalid object type), passing through authorization in the Security wrapper and then getting a validation error in the Repository. On the other hand, a non-superuser Kibana administrator would get a 403 error instead. Now, each Repository function checks validation first and only checks authorization for object(s)/request(s) that are valid, and the integration tests have been updated to reflect that.
- As a result, each SOC function makes a single call to Elasticsearch to check for privileges instead of two calls 🎉
Unit tests for the new extensions -- these reside alongside the extensions themselves
Unit tests for the SOR with extensions enabled -- these reside in independent files alongside the existing repository unit tests. There are additional tests added to update_objects_spaces.test, internal_bulk_resolve.test, and collect_multi_namespace_references.test.

These are unrelated to the prior commit, but some ripple effect caused these integration tests to start failing. I had to fix them by removing a circular dependency and changing the test mocks. Leaving this in a separate commit because it's unrelated to the encryption extension.

jportner

Author's notes for reviewers.

jportner · 2022-06-29T22:03:47Z

src/core/public/saved_objects/saved_objects_client.ts

 type SavedObjectsFindOptions = Omit<
  SavedObjectFindOptionsServer,
-  'pit' | 'rootSearchFields' | 'searchAfter' | 'sortOrder' | 'typeToNamespacesMap'
+  'pit' | 'rootSearchFields' | 'searchAfter' | 'sortOrder'


This typeToNamespacesMap option existed for the server-side Security SOC wrapper to pass down authorization information to the SOR to use for building the ES query. We omitted this attribute from the options for the public SOC, as it was never meant for other consumers to use.

Now that we don't have SOC wrappers, we don't need this typeToNamespacesMap option at all anymore, I've completely removed it.

jportner · 2022-06-30T03:09:50Z

src/core/server/saved_objects/saved_objects_service.ts

+export const ENCRYPTION_EXTENSION_ID = 'encryptedSavedObjects' as const;
+export const SECURITY_EXTENSION_ID = 'security' as const;
+export const SPACES_EXTENSION_ID = 'spaces' as const;


These SO extension IDs are the same as the plugins that provide these extensions. This is purely for backwards compatibility for consumers that currently create a scoped SOC and disable one of the SOC wrappers.

jportner · 2022-06-30T03:13:49Z

src/core/server/saved_objects/service/lib/scoped_client_provider.ts

+  private getExtensions(
+    request: KibanaRequest,
+    excludedWrappers: string[]
+  ): SavedObjectsExtensions {
+    const isEncryptionExtensionIncluded =
+      !excludedWrappers.includes(ENCRYPTION_EXTENSION_ID) && !!this.encryptionExtensionFactory;
+    const encryptionExtension = isEncryptionExtensionIncluded
+      ? this.encryptionExtensionFactory?.({ typeRegistry: this._typeRegistry, request })
+      : undefined;
+    const isSecurityExtensionIncluded =
+      !excludedWrappers.includes(SECURITY_EXTENSION_ID) && !!this.securityExtensionFactory;
+    const securityExtension = isSecurityExtensionIncluded
+      ? this.securityExtensionFactory?.({ typeRegistry: this._typeRegistry, request })
+      : undefined;
+    const isSpacesExtensionIncluded =
+      !excludedWrappers.includes(SPACES_EXTENSION_ID) && !!this.spacesExtensionFactory;
+    const spacesExtension = isSpacesExtensionIncluded
+      ? this.spacesExtensionFactory?.({ typeRegistry: this._typeRegistry, request })
+      : undefined;
+
+    return {
+      encryptionExtension,
+      securityExtension,
+      spacesExtension,
+    };
+  }


This mimics the behavior of SOC wrappers in that any extension can be disabled if the extension ID is specified in the excludedWrappers option when instantiating a scoped SOC. I didn't want to rename that option as it would be a breaking change. This approach is fully backwards compatible with existing plugin behavior.

jportner · 2022-06-30T03:16:30Z

src/core/server/saved_objects/service/lib/collect_multi_namespace_references.test.ts

@@ -445,4 +446,6 @@ describe('collectMultiNamespaceReferences', () => {
      );
    });
  });
+
+  test.todo('with security enabled');


I added TODOs like this where unit tests need to be added to retain the code coverage that we had before this refactor.

jportner · 2022-06-30T03:29:33Z

src/core/server/saved_objects/service/lib/extensions/security.ts

+export interface CheckAuthorizationParams<A extends string> {
+  types: Set<string>;
+  spaces: Set<string>;
+  actions: A[];
+  options?: {
+    allowGlobalResource?: boolean;
+  };
+}


Sorry for the lack of TSdocs here, I was trying to throw this PR together as quickly as possible 😅

Now, the authorization check queries Elasticsearch for all permutations of specified types/spaces/actions.

When Kibana registers application privileges with Elasticsearch, each space ID corresponds to a specific resource. Instead of checking privileges for individual resources, you can choose to check privileges for "*", which is the global resource. When you create a role that grants access to "All spaces", under the hood it's granting the specified feature privileges for the global resource.

It's important to know that you can't check privileges for individual spaces and the global resource at the same time. Checking privileges for the global resource supersedes any individual space.

It's also important to know that if a saved object exists in "*" (all spaces), a user doesn't need to have access to the global resource to be authorized to access that saved object, they only need to be authorized to access one space.

For most SOR functions, we don't want to risk accidentally checking privileges for the global resource. For example, when you get an object in the Default space and it exists in namespaces: ['*'], the SOR will check privileges for spaces: ['default', '*']. By default, the security extension will omit '*' from that privilege check, because we are really only interested in knowing if the user has access to the Default space.

If the allowGlobalResource: true option is included, the security extension will not omit '*'. This is only used when the user is attempting to (1) share an object to all spaces, or (2) create an object that exists in all spaces -- in both of these cases, we actually want to ensure the user has access to the global resource.

jportner · 2022-06-30T03:30:48Z

src/core/server/saved_objects/service/lib/extensions/security.ts

+export enum AuditAction {
+  CREATE = 'saved_object_create',
+  GET = 'saved_object_get',
+  RESOLVE = 'saved_object_resolve',
+  UPDATE = 'saved_object_update',
+  DELETE = 'saved_object_delete',
+  FIND = 'saved_object_find',
+  REMOVE_REFERENCES = 'saved_object_remove_references',
+  OPEN_POINT_IN_TIME = 'saved_object_open_point_in_time',
+  CLOSE_POINT_IN_TIME = 'saved_object_close_point_in_time',
+  COLLECT_MULTINAMESPACE_REFERENCES = 'saved_object_collect_multinamespace_references', // this is separate from 'saved_object_get' because the user is only accessing an object's metadata
+  UPDATE_OBJECTS_SPACES = 'saved_object_update_objects_spaces', // this is separate from 'saved_object_update' because the user is only updating an object's metadata
+}
+
+export interface AddAuditEventParams {
+  action: AuditAction;
+  outcome?: EcsEventOutcome;
+  savedObject?: { type: string; id: string };
+  addToSpaces?: readonly string[];
+  deleteFromSpaces?: readonly string[];
+  error?: Error;
+}


SO audit event parameters and audit actions need to be defined in Core now, since the SOR is responsible for calling the security extension to add audit events.

jportner · 2022-06-30T03:33:43Z

src/core/server/saved_objects/service/lib/repository.ts

+/**
+ * @internal
+ */
+export interface SavedObjectsFindInternalOptions {
+  /** This is used for internal consumers that need to use a PIT finder but want to prevent extensions from functioning. */
+  disableExtensions?: boolean;
+}


We use the SOR's PointInTimeFinder internally when searching for aliases and shared origins for saved objects, but we need to disable the extensions for that to function correctly.

Before, when we had SOC wrappers, the SOR's PointInTimeFinder did not have any of the wrapper functionality applied. This disableExtensions internal option preserves that behavior.

jportner · 2022-06-30T03:40:14Z

src/core/server/saved_objects/service/lib/update_objects_spaces.test.ts

      ]);
    });
  });
+
+  test.todo('with security extension');


The security extension actually filters the object graph in addition to redacting space information. See the old Secure SOC wrapper unit tests for details. You should be able to largely copy/paste those tests here.

jportner · 2022-06-30T03:49:29Z

x-pack/plugins/security/server/spaces/secure_spaces_client_wrapper.ts

-            this.auditLogger.log(
-              savedObjectEvent({
-                action: isOnlySpace
-                  ? SavedObjectAction.DELETE
-                  : SavedObjectAction.UPDATE_OBJECTS_SPACES,
-                outcome: 'unknown',
-                savedObject: { type: savedObject.type, id: savedObject.id },
-                deleteFromSpaces: [id],
-              })
-            );
+            this.securityExtension!.addAuditEvent({
+              action: isOnlySpace
+                ? SavedObjectAction.DELETE
+                : SavedObjectAction.UPDATE_OBJECTS_SPACES,
+              outcome: 'unknown',
+              savedObject: { type: savedObject.type, id: savedObject.id },
+              ...(!isOnlySpace && { deleteFromSpaces: [id] }),
+            });


The old audit event included the deleteFromSpaces field whether the object was being updated or deleted.

Now, if the object is being deleted, we omit the deleteFromSpaces field.

jportner · 2022-06-30T03:54:31Z

x-pack/test/saved_object_api_integration/common/suites/resolve.ts

-  const expectSavedObjectForbidden = expectResponses.forbiddenTypes('get');
+  const expectSavedObjectForbidden = expectResponses.forbiddenTypes('bulk_get');


The Secure SOC wrapper used to check privileges differently for SOC.resolve and SOC.bulkResolve (it checked 'get' and 'bulk_get' privilege actions, respectively).

Now, the internalBulkResolve module is initiating authorization checks directly using the security extension, but it is used for the implementation of both the SOC.resolve and SOC.bulkResolve functions.

The cleanest change was to make internalBulkResolve always check for the 'bulk_get' privilege action, and change the resolve integration test to reflect that.

Forgot to do this in the spaces extension commit

src/core/server/saved_objects/service/lib/repository.ts

elasticmachine · 2022-10-03T17:16:55Z

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

elasticmachine · 2022-10-03T17:17:11Z

Pinging @elastic/kibana-core (Team:Core)

legrego · 2022-10-05T11:06:31Z

Buildkite, test this

mikecote

ResponseOps changes LGTM

elasticmachine · 2022-10-05T16:12:01Z

Pinging @elastic/fleet (Team:Fleet)

legrego

First pass through (including some manual testing), the tests look really great overall! Most of my comments are minor so far.

I didn't quite appreciate how much additional logic the repository would need in order to support calling these extensions 🙈

src/core/server/saved_objects/service/lib/extensions/encryption.ts

legrego · 2022-10-03T17:50:58Z

src/core/server/saved_objects/service/lib/collect_multi_namespace_references.ts

-  const objectOriginsToSearchFor = objectsWithContext
-    .filter(({ spaces }) => spaces.length !== 0)
-    .map(({ type, id, originId }) => ({ type, origin: originId || id }));
+  const objectOriginsToSearchFor = foundObjects.map(({ type, id, originId }) => ({


Nice, thanks for removing the redundant filter call!

legrego · 2022-10-05T18:12:48Z

src/core/server/saved_objects/service/lib/repository.security_extension.test.ts

+    });
+  });
+
+  describe('#bulkCreate', () => {


Similar question from create: Do you think it's worthwhile to include tests for multi-namespace types, since they influence what we pass into the security extension?

We do test the initial namespaces path. Do you mean adding a case where we mock an existing multi-space object with a different set of spaces than what is passed in for initial spaces? This would be applicable to create, but I think bulkCreate will just resort to using the namespace option/active space in that case.

Are there more cases outside of initial "namespaces"?

It looks like this code block isn't exercised by the current test suite, which is for existing multi-namespace types, both with and without initialNamespaces provided:

https://github.com/jportner/kibana/blob/d8110c4b9e858d2a1996a43e13fe99158e21b486/src/core/server/saved_objects/service/lib/repository.ts#L675-L677

legrego · 2022-10-05T18:14:55Z

src/core/server/saved_objects/service/lib/repository.ts

+        for (const [objType, entry] of preAuthorizationResult.typeMap) {
+          if (!entry.find) continue;
+          // This ensures that the query DSL can filter only for object types that the user is authorized to access for a given space
+          const { authorizedSpaces, isGloballyAuthorized } = entry.find;


I think we're missing a test for this case within repository.security_extension.test.ts. Do you think it's wortwhile to add one?

Good catch. I will add a test that executes find within 2 namespaces, and mock a checkAuthorization return simulating authz in only one of those spaces and validate the ES call is made with only the authorizes space. Is that what you had in mind?

Yeah, I think that'll do the trick!

src/core/server/saved_objects/service/lib/repository.ts

legrego · 2022-10-05T18:28:36Z

src/core/server/saved_objects/service/lib/repository.test.ts

-        return { message: () => `expected type and id to match without error`, pass: false };
-      }
-    },
-  });
  const expectSuccess = ({ type, id }: { type: string; id: string }) => {
    // @ts-expect-error TS is not aware of the extension
    return expect.toBeDocumentWithoutError(type, id);


optional: this appears to be the only place where toBeDocumentWithoutError is called. Do you think it's worth keeping this as a custom expect function within repository.test.common.ts, or should we inline the logic here instead?

src/core/server/saved_objects/service/lib/repository.ts

…e array test case.

juliaElastic

Fleet change LGTM

jeramysoucy · 2022-10-17T18:30:42Z

Closing this PR to replace with #142878 which manually merges these changes into the new packages structure.

Merges the changes of #134395 into the new packages structure. Resolves #133835 ### Description This PR represents a fully manual merge of the saved objects refactor of client wrapper system into repository extensions. These changes are being manually merged due to significant changes of the saved objects implementation in the main branch, specifically the migration to the new packages structure. ### Other changes - Bulk Delete: bulk delete was implemented in parallel to #134395 being completed and this PR will refactor that API to utilize the new extensions Co-authored-by: Jeramy Soucy <jeramy.soucy@elastic.co> Co-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com> Co-authored-by: Aleh Zasypkin <aleh.zasypkin@gmail.com>

jportner force-pushed the issue-133835-refactor-soc branch 5 times, most recently from b91196a to 04bf69f Compare June 21, 2022 15:25

jportner force-pushed the issue-133835-refactor-soc branch 3 times, most recently from fe1d817 to 2df8716 Compare June 27, 2022 12:53

jportner mentioned this pull request Jun 27, 2022

The removeReferencesTo function doesn't check authorization correctly #135259

Closed

jportner force-pushed the issue-133835-refactor-soc branch 8 times, most recently from 4f562f0 to 752f07f Compare June 29, 2022 19:37

Joe Portner added 5 commits June 29, 2022 16:50

Refactor encryption wrapper

fad0c01

Improve SOR function type-safety

5d5c44d

Refactor security wrapper

6a23e0d

Refactor spaces wrapper

c67e6ca

jportner force-pushed the issue-133835-refactor-soc branch from 752f07f to c67e6ca Compare June 29, 2022 20:51

jportner commented Jun 30, 2022

View reviewed changes

Unskip find integration tests

abfd2c5

Forgot to do this in the spaces extension commit

jportner requested a review from azasypkin June 30, 2022 04:07

azasypkin reviewed Jun 30, 2022

View reviewed changes

src/core/server/saved_objects/service/lib/repository.ts Outdated Show resolved Hide resolved

azasypkin self-requested a review June 30, 2022 10:46

Joe Portner added 2 commits June 30, 2022 08:03

Merge remote-tracking branch 'upstream/main' into pr/jportner/134395

fff9927

Fix preflight result index bug

2ba4f72

jeramysoucy mentioned this pull request Oct 3, 2022

Remove saved objects client wrapper system #142503

Closed

jeramysoucy marked this pull request as ready for review October 3, 2022 17:12

jeramysoucy requested review from a team as code owners October 3, 2022 17:12

kibanamachine added the 💝community label Oct 3, 2022

jeramysoucy added the Team:Security Team focused on: Auth, Users, Roles, Spaces, Audit Logging, and more! label Oct 3, 2022

jeramysoucy added the Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc label Oct 3, 2022

Updated error message checks in security extension tests.

d7f567e

This comment was marked as outdated.

Sign in to view

jeramysoucy added 2 commits October 5, 2022 08:38

Updated expected error messages in unut tests to reflect recent changes.

b3a6e5b

Complete removal of saved objects client wrapper system.

d8110c4

jeramysoucy requested review from a team as code owners October 5, 2022 15:12

jeramysoucy requested review from joeypoon and dasansol92 October 5, 2022 15:12

mikecote approved these changes Oct 5, 2022

View reviewed changes

botelastic bot added the Team:Fleet Team label for Observability Data Collection Fleet team label Oct 5, 2022

legrego reviewed Oct 5, 2022

View reviewed changes

thomheymann reviewed Oct 5, 2022

View reviewed changes

src/core/server/saved_objects/service/lib/repository.ts Show resolved Hide resolved

Cleanup of comments and unnecessary awaits. Added Find empty namespac…

de37a97

…e array test case.

TinaHeiligers mentioned this pull request Oct 7, 2022

Saved objects extensions refactor merge #142878

Merged

juliaElastic approved these changes Oct 17, 2022

View reviewed changes

jeramysoucy closed this Oct 17, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor SavedObjectsClient #134395

Refactor SavedObjectsClient #134395

jportner commented Jun 14, 2022 •

edited by jeramysoucy

Loading

jportner left a comment

jportner Jun 29, 2022

jportner Jun 30, 2022

jportner Jun 30, 2022

jportner Jun 30, 2022

jportner Jun 30, 2022

jportner Jun 30, 2022

jportner Jun 30, 2022

jportner Jun 30, 2022

jportner Jun 30, 2022

jportner Jun 30, 2022

elasticmachine commented Oct 3, 2022

elasticmachine commented Oct 3, 2022

legrego commented Oct 5, 2022

This comment was marked as outdated.

mikecote left a comment

elasticmachine commented Oct 5, 2022

legrego left a comment

legrego Oct 3, 2022

legrego Oct 5, 2022

jeramysoucy Oct 5, 2022

legrego Oct 6, 2022

legrego Oct 5, 2022

jeramysoucy Oct 5, 2022

legrego Oct 6, 2022

legrego Oct 5, 2022

juliaElastic left a comment

jeramysoucy commented Oct 17, 2022

		const expectSavedObjectForbidden = expectResponses.forbiddenTypes('get');
		const expectSavedObjectForbidden = expectResponses.forbiddenTypes('bulk_get');

Refactor SavedObjectsClient #134395

Refactor SavedObjectsClient #134395

Conversation

jportner commented Jun 14, 2022 • edited by jeramysoucy Loading

Overview

Notable changes

jportner left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elasticmachine commented Oct 3, 2022

elasticmachine commented Oct 3, 2022

legrego commented Oct 5, 2022

This comment was marked as outdated.

mikecote left a comment

Choose a reason for hiding this comment

elasticmachine commented Oct 5, 2022

legrego left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

juliaElastic left a comment

Choose a reason for hiding this comment

jeramysoucy commented Oct 17, 2022

jportner commented Jun 14, 2022 •

edited by jeramysoucy

Loading