Security - Reorder Saved Objects Client wrappers #38444

legrego · 2019-06-07T19:16:08Z

Summary

Previously, the wrapped saved objects client worked in the following manner:

Security Plugin's wrapper authorized the saved objects operation, deriving the namespace to authorize against from the spacesService.getSpaceId() function exposed from the Spaces plugin.
Spaces Plugin's wrapper populated the namespace attribute which is used by the base Saved Objects Client/Repository for the actual operation.

With this change, the order of the wrappers has been switched, so now the wrapped saved objects client works in the following manner:

Spaces Plugin's wrapper populates the namespace attribute for use by downstream wrappers, as well as the base Saved Objects Client/Repository for the actual operation.
Security Plugin's wrapper authorizes the saved objects operation, using the namespace provided by the Spaces plugin's wrapper. This has a side benefit of removing the hard dependency between the security plugin's wrapper and the spaces plugin.

This also further abstracts the authorization logic from the client wrapper. Previously, the wrapper used checkPrivilegesDynamicallyWithRequest, which was responsible for using the correct authz method based on whether the Spaces plugin was enabled or not.

We've introduced a new checkSavedObjectsPrivileges function, which behaves in much the same way as checkPrivilegesDynamicallyWithRequest, but it's designed to work specifically with the namespace, converting to a space id when required.

…rappers

legrego · 2019-06-07T19:20:42Z

x-pack/plugins/spaces/server/lib/create_spaces_service.ts

@@ -26,6 +27,10 @@ export function createSpacesService(server: any): SpacesService {
    return spaceId;
  }

+  function setSpaceId(request: any, spaceId: string) {


I can remove this, as it's not used yet, but this is a potential solution for allowing Spaces to dictate which space id should be considered the "current" space id for features such as #37286

If we decide to keep in this PR, then I just need to write a few more tests before merging.

This has the benefit of not modifying the base path for the in-flight request, so request.getBasePath() / http.getBasePathFor(request) will not be impacted by calls to setSpaceId, which reduces the likelihood of side-effects as a result of changing the space id mid-request.

One tradeoff is that this necessitates having a cache here (or elsewhere) to store this information, which we had planned to remove in #35429

Hmm, I always figured this would just be done for "copy to space" by allowing "consumers" to specify the namespace in the options. Otherwise, we can't use the SOC instances for different spaces concurrently and we have do so serially.

Hmm, I always figured this would just be done for "copy to space" by allowing "consumers" to specify the namespace in the options. Otherwise, we can't use the SOC instances for different spaces concurrently and we have do so serially.

Yep, that's the other route we can take! I started down that path in this PR, but ended up reverting the required changes for now.

I'm happy to take either approach, I just wanted to present options. I'll revert this change in this PR, and re-introduce the ability for the spaces wrapper to accept a namespace from consumers.

elasticmachine · 2019-06-07T20:12:25Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

legrego · 2019-06-07T20:19:03Z

...plugins/security/server/lib/saved_objects_client/secure_saved_objects_client_wrapper.test.ts

+        type,
+      });
+      expect(mockErrors.decorateGeneralError).toHaveBeenCalledTimes(1);
+      expect(mockAuditLogger.savedObjectsAuthorizationFailure).not.toHaveBeenCalled();


I decided (for now) to keep the audit logging assertions as part of this test suite. We're technically testing an implementation detail of a dependency now, but I feel like it's important to verify that it works from the service level, and the Saved Object Client is as close to an API as we can get without mocking out even more services.

We have audit logging tests as part of https://github.com/elastic/kibana/blob/764322cc7f3a6f3b082a0044af151e1b2a1da21f/x-pack/plugins/security/server/lib/authorization/saved_objects/check_saved_objects_privileges.test.ts, so I don't have a strong opposition to removing the checks here if we want to simplify this test suite a bit.

legrego · 2019-06-07T20:20:05Z

...ck/plugins/security/server/lib/authorization/saved_objects/check_saved_objects_privileges.ts

+
+    try {
+      if (deps.spacesEnabled) {
+        const spaceId = namespaceToSpaceId(namespace);


🆕 no longer relies on the spaces service to get the current space id!

elasticmachine · 2019-06-07T21:26:34Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

…rappers

legrego · 2019-06-10T17:07:45Z

x-pack/plugins/security/server/lib/saved_objects_client/secure_saved_objects_client_wrapper.ts

+  private async ensureAuthorized(
+    typeOrTypes: string | string[] | undefined,
+    action: SavedObjectsOperation,
+    namespace: string | undefined,


The diff isn't very helpful since this was converted to TypeScript, but the only material difference in this file is the introduction of this namespace parameter, and the swapping of checkPrivilegesDynamicallyWithRequest with checkSavedObjectsPrivileges.

elasticmachine · 2019-06-10T18:06:47Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

elasticmachine · 2019-06-10T18:19:02Z

Pinging @elastic/kibana-security

kobelb · 2019-06-10T20:07:22Z

ACK: will review next

* Using SavedObjectsClientContract['errors'] instead of any for errors * Moving security's SOC wrapper to authorization/saved_objects * Renaming 'action' to operation * Renaming checkSavedObjectsPrivileges to ensureSavedObjectsPrivileges We use "check" elsewhere when returning a "check privileges result", and we use "ensure" elsewhere when an Error will be thrown if the user doesn't have any privileges. * Adding "general error" tests for ensure saved object privileges * Removing general error tests from wrapper * Change the SecureSavedObjectsClientWrapper tests to be strictly white-box

This reverts commit c98c1cd.

elasticmachine · 2019-06-12T11:21:47Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

rudolf · 2019-06-21T08:16:34Z

retest

elasticmachine · 2019-06-21T09:17:04Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

kobelb · 2019-06-21T13:52:40Z

Having to augment the SavedObjectNamespace definition which OSS sees for this solution feels wrong. Also, the types make it appear that you can provide a Symbol as the namespace, and then we're throwing an error if it's provided. At the risk of being pedantic, after seeing the final implementation of this solution, it just doesn't feel right.

rudolf · 2019-06-21T14:00:51Z

@legrego the only reason for putting the symbol type in the OSS SavedObjectNamespace was the weirdness of our documentation tool right?

It doesn't affect the resulting documentation so I'd be happy to live with it the way it was. I haven't had a chance to play around with it, but it might also be a bug intstead of just a limitation in api-extractor.

kobelb · 2019-06-21T14:12:57Z

If we could get away with not changing the OSS definition of the saved object namespace, that'd make me feel much better about this implementation. That'd allow us to change the "asserts" within the repository itself to ensure the namespace is either undefined or a string, without explicitly ensure that it's "not a symbol". Apologies if I'm being pedantic here.

legrego · 2019-06-24T11:19:46Z

the only reason for putting the symbol type in the OSS SavedObjectNamespace was the weirdness of our documentation tool right?

Yep, that was the only reason I made this change.

if we could get away with not changing the OSS definition of the saved object namespace, that'd make me feel much better about this implementation. That'd allow us to change the "asserts" within the repository itself to ensure the namespace is either undefined or a string, without explicitly ensure that it's "not a symbol". Apologies if I'm being pedantic here.

sure I'll revert.

This reverts commit 348ef98.

This reverts commit 8353f0f.

This reverts commit 739cd86.

…rappers

elasticmachine · 2019-06-24T12:11:57Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

elasticmachine · 2019-06-24T13:25:25Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

^ unrelated CI failure

…rappers

elasticmachine · 2019-06-24T19:26:04Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

…rappers

elasticmachine · 2019-06-25T14:38:54Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

…rappers

elasticmachine · 2019-06-26T14:50:13Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

mshustov · 2019-06-28T12:37:47Z

x-pack/legacy/plugins/spaces/server/lib/saved_objects_client/get_namespace.ts

+  currentSpaceId: string
+): string | undefined {
+  if (operationOptions.namespace) {
+    if (operationOptions.namespace === DEFAULT_SPACE_NAMESPACE) {


Who writes DEFAULT_SPACE_NAMESPACE in namespace field? If it serializable a symbol cannot be used. Can we use a special string instead? What is the benefit of using a separate type?
DEFAULT_SPACE_ID exists here only for backward compatibility?

Who writes DEFAULT_SPACE_NAMESPACE in namespace field? If it serializable a symbol cannot be used. Can we use a special string instead? What is the benefit of using a separate type?

Consumers of the Saved Objects Client can specify this for any operation they wish to perform explicitly against the default space. You bring up a good point about serializability that I hadn't considered -- @mikecote, do you foresee this being problematic for alerting? How are you planning/hoping to record the space/namespace which the alert should run in? Are you storing the space id, and translating to namespace when required, storing the namespace directly, or storing the base path and translating?

DEFAULT_SPACE_ID exists here only for backward compatibility?

DEFAULT_SPACE_ID is mostly for backwards compatibility, but it also serves as a way to allow the spaces plugin to be completely disabled/re-enabled at any time, without breaking Kibana.

How are you planning/hoping to record the space/namespace which the alert should run in? Are you storing the space id, and translating to namespace when required, storing the namespace directly, or storing the base path and translating?

This will be a conversation I'll have to have with you when we do the security phase. Right now we store the basePath and create clients with that. Though there may be a few places that forget to store the space and assumes the default space.

This will be a conversation I'll have to have with you when we do the security phase. Right now we store the basePath and create clients with that. Though there may be a few places that forget to store the space and assumes the default space.

Ok, then this shouldn't be an issue for alerting then.

In general, specifying a custom namespace when using the saved objects client should be a very rare event. I consider the namespace field an implementation detail of the space plugin, even though it is an OSS concept. If the spaces plugin exposes first-class methods for translating a space id to a namespace (and vice-versa), then I don't foresee Symbols being problematic here.

Can we use a special string instead? What is the benefit of using a separate type?

A special string would have to include some character that is disallowed as part of a space id, in order to guarantee that we never have a collision. It's certainly doable, so long as we don't change the set of allowed characters in the future.

A symbol makes it painfully clear that we're dealing with a special case, as opposed to a mis-typed space id or similar.

I consider the namespace field an implementation detail of the space plugin, even though it is an OSS concept.

ok, agree. In this case symbol is internal detail and not a part of public contract. why it leaks to sibling plugins? https://github.com/elastic/kibana/pull/38444/files#diff-9b008eb2578ead2253f9faf64b262f0eR79

If the spaces plugin exposes first-class methods for translating a space id to a namespace (and vice-versa), then I don't foresee Symbols being problematic here.

with this API can we have separate 2 types. don't we?
type SavedObjectsNamespace = string | undefined; in OSS
type SavedObjectsNamespaceInternal = string | undefined | symbol; in x-pack/spaces.

why it leaks to sibling plugins?

The type definition is declared in x-pack/types, which is shared by all x-pack plugins:

kibana/x-pack/typings/core.d.ts

Lines 8 to 13 in e2796ff

declare module 'src/core/server/saved_objects' {

type SavedObjectsNamespace = string | undefined | symbol;

interface SavedObjectsBaseOptions {

namespace?: SavedObjectsNamespace;

}

}

with this API can we have separate 2 types. don't we?
type SavedObjectsNamespace = string | undefined; in OSS
type SavedObjectsNamespaceInternal = string | undefined | symbol; in x-pack/spaces.

I don't know if it's really internal though. The wrapped saved objects client has a public api that expects either string | undefined | symbol, and this is consumable by both first and third-party plugins.

Plus, having a different name for this vs the OSS version makes overriding the type very difficult, or maybe even impossible (I haven't tried yet). The current override (above) is rather straightforward.

…rappers

elasticmachine · 2019-07-01T20:46:08Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

legrego · 2019-07-16T12:12:25Z

Abandoned in favor of #40275

legrego added 7 commits June 6, 2019 14:03

convert SecureSavedObjectsClientWrapper to TypeScript

c7acf55

change spaces soc wrapper order. allow space id to be set on the request

7a410c7

change security soc wrapper order. Extract privilege checking

e23adc0

Merge branch 'master' of github.com:elastic/kibana into reorder-soc-w…

4b97e2c

…rappers

add namespace tests for SOC wrapper

30d0641

testing checkSavedObjectsPrivileges

6f06c67

Merge branch 'master' of github.com:elastic/kibana into reorder-soc-w…

764322c

…rappers

legrego commented Jun 7, 2019

View reviewed changes

revert changes to spaces saved objects client wrapper

c98c1cd

legrego mentioned this pull request Jun 7, 2019

Copy Saved Objects to Spaces API #38014

Merged

legrego added 2 commits June 10, 2019 13:05

cleanup

2c95f6c

Merge branch 'master' of github.com:elastic/kibana into reorder-soc-w…

0b4a769

…rappers

legrego commented Jun 10, 2019

View reviewed changes

legrego added the Team:Security Team focused on: Auth, Users, Roles, Spaces, Audit Logging, and more! label Jun 10, 2019

legrego added the release_note:skip Skip the PR/issue when compiling release notes label Jun 10, 2019

legrego marked this pull request as ready for review June 10, 2019 18:19

legrego requested a review from a team as a code owner June 10, 2019 18:19

legrego added the review label Jun 10, 2019

legrego requested a review from kobelb June 10, 2019 20:01

kobelb and others added 3 commits June 12, 2019 06:13

revert changes to spaces service

dfff4ab

Revert "revert changes to spaces saved objects client wrapper"

b2c5884

This reverts commit c98c1cd.

peteharverson mentioned this pull request Jun 21, 2019

[ML] Machine Learning Detector help links 404 in 6.8 #39342

Closed

legrego added 6 commits June 24, 2019 07:20

Revert "use relative imports for SavedObjectsNamespace"

e2796ff

This reverts commit 348ef98.

Revert "assert that the repository cannot accept Symbol namespaces"

7c6bbfd

This reverts commit 8353f0f.

Revert "updating api docs"

cada7ea

This reverts commit 739cd86.

Merge branch 'master' of github.com:elastic/kibana into reorder-soc-w…

016479d

…rappers

assert that the repository cannot accept Symbol namespaces

0530155

update namespace assertions

4bfeb3b

fix typings

d44ab61

Merge branch 'master' of github.com:elastic/kibana into reorder-soc-w…

ba8d21f

…rappers

Merge branch 'master' of github.com:elastic/kibana into reorder-soc-w…

fee5532

…rappers

Merge branch 'master' of github.com:elastic/kibana into reorder-soc-w…

49956c2

…rappers

mshustov reviewed Jun 28, 2019

View reviewed changes

Merge branch 'master' of github.com:elastic/kibana into reorder-soc-w…

b5ffdac

…rappers

rudolf mentioned this pull request Jul 5, 2019

[Core][WIP] SavedObjects v2 #40380

Closed

kobelb mentioned this pull request Jul 8, 2019

Allow SOC wrappers to be excluded #40275

Merged

legrego closed this Jul 16, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Security - Reorder Saved Objects Client wrappers #38444

Security - Reorder Saved Objects Client wrappers #38444

legrego commented Jun 7, 2019 •

edited

Loading

legrego Jun 7, 2019 •

edited

Loading

kobelb Jun 11, 2019

legrego Jun 11, 2019

elasticmachine commented Jun 7, 2019

legrego Jun 7, 2019

legrego Jun 7, 2019

elasticmachine commented Jun 7, 2019

legrego Jun 10, 2019

elasticmachine commented Jun 10, 2019

elasticmachine commented Jun 10, 2019

kobelb commented Jun 10, 2019

elasticmachine commented Jun 12, 2019

rudolf commented Jun 21, 2019

elasticmachine commented Jun 21, 2019

kobelb commented Jun 21, 2019

rudolf commented Jun 21, 2019

kobelb commented Jun 21, 2019

legrego commented Jun 24, 2019

elasticmachine commented Jun 24, 2019

elasticmachine commented Jun 24, 2019 •

edited by legrego

Loading

elasticmachine commented Jun 24, 2019

elasticmachine commented Jun 25, 2019

elasticmachine commented Jun 26, 2019

mshustov Jun 28, 2019

legrego Jun 28, 2019

mikecote Jun 28, 2019

legrego Jun 28, 2019

mshustov Jul 1, 2019

legrego Jul 1, 2019

elasticmachine commented Jul 1, 2019

legrego commented Jul 16, 2019

	declare module 'src/core/server/saved_objects' {
	type SavedObjectsNamespace = string \| undefined \| symbol;
	interface SavedObjectsBaseOptions {
	namespace?: SavedObjectsNamespace;
	}
	}

Security - Reorder Saved Objects Client wrappers #38444

Security - Reorder Saved Objects Client wrappers #38444

Conversation

legrego commented Jun 7, 2019 • edited Loading

Summary

legrego Jun 7, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elasticmachine commented Jun 7, 2019

💚 Build Succeeded

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elasticmachine commented Jun 7, 2019

💚 Build Succeeded

Choose a reason for hiding this comment

elasticmachine commented Jun 10, 2019

💚 Build Succeeded

elasticmachine commented Jun 10, 2019

kobelb commented Jun 10, 2019

elasticmachine commented Jun 12, 2019

💚 Build Succeeded

rudolf commented Jun 21, 2019

elasticmachine commented Jun 21, 2019

💚 Build Succeeded

kobelb commented Jun 21, 2019

rudolf commented Jun 21, 2019

kobelb commented Jun 21, 2019

legrego commented Jun 24, 2019

elasticmachine commented Jun 24, 2019

💔 Build Failed

elasticmachine commented Jun 24, 2019 • edited by legrego Loading

💔 Build Failed

elasticmachine commented Jun 24, 2019

💔 Build Failed

elasticmachine commented Jun 25, 2019

💔 Build Failed

elasticmachine commented Jun 26, 2019

💚 Build Succeeded

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 Jul 1, 2019

💚 Build Succeeded

legrego commented Jul 16, 2019

legrego commented Jun 7, 2019 •

edited

Loading

legrego Jun 7, 2019 •

edited

Loading

elasticmachine commented Jun 24, 2019 •

edited by legrego

Loading