Migrate Encrypted Saved Objects plugin to the new platform. #49890

azasypkin · 2019-10-31T16:17:13Z

Migrate Encrypted Saved Objects plugin to the new platform.

NOTE: It'd be much easier to review PR commit by commit.

elasticmachine · 2019-10-31T16:17:17Z

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

azasypkin · 2019-10-31T16:25:56Z

x-pack/plugins/encrypted_saved_objects/server/crypto/encrypted_saved_objects_service.ts

   * @param audit Audit logger instance.
   */
  constructor(
    encryptionKey: string,
-    private readonly knownTypes: readonly string[],


note: this check didn't bring lots of value and it doesn't fit very well to NP since plugins will register their types during setup that would make order of these operations important. So since SO client will return error for not known type anyway I decided to ditch this check. What do you think?

That makes sense to me. Since we're relying on specific behavior in a dependency for this functionality, what do you think about adding a functional test here to ensure that the public encryption/decryption endpoints behave correctly when this happens?

Somehow missed that comment, sorry. Yeah, it's a great idea, will add this test in a follow-up.

elasticmachine · 2019-10-31T21:47:23Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: cfc031c

azasypkin · 2019-10-31T21:47:35Z

@legrego it's better to review PR commit by commit, I tried to separate plain moves from changes.

@mikecote I grouped all changes relevant to you in Changes in other plugins. commit

Thanks!

mikecote

🙏This is awesome, thank you for doing this. Everything in that commit LGTM!

legrego

This looks really good! Just a couple of questions for you while I test this locally

legrego · 2019-11-01T11:36:33Z

x-pack/plugins/encrypted_saved_objects/README.md

-});
+...
+public setup(core: CoreSetup, { encryptedSavedObjects }: PluginSetupDependencies) {
+  encrypted_saved_objects.registerType({


nit: the parameter name here is encryptedSavedObjects, but it's referenced on line 44 as `encrypted_saved_objects.

Ha, will fix, thanks!

legrego · 2019-11-01T11:51:58Z

x-pack/plugins/encrypted_saved_objects/server/config.ts

+
+export const ConfigSchema = schema.object({
+  enabled: schema.boolean({ defaultValue: true }),
+  encryptionKey: schema.conditional(


What do you think about renaming this from encryptionKey to encryption_key using the legacy deprecation system? Reading the full config path, xpack.encrypted_saved_objects.encryptionKey looks rather inconsistent with the mixing of snake_case and camelCase. I know you didn't technically change this as part of this PR, but maybe the NP migration is a good time to start to make these types of things consistent?

Yeah, this bothers me as well, Kibana also isn't consistent in plugin IDs: it's a mix of snake_case and camelCase, even for NP plugins 😢 How do you feel about keeping encryptionKey (to be consistent with security.encryptionKey and reporting.encryptionKey), but change plugin ID to encryptedSavedObjects?

How do you feel about keeping encryptionKey (to be consistent with security.encryptionKey and reporting.encryptionKey), but change plugin ID to encryptedSavedObjects?

That was actually going to be my original suggestion, but I changed my mind 😄
I'm struggling to find evidence of this, but I thought we were taking steps to move all settings (and therefore plugin ids) to snake_case in order to be more consistent with Elasticsearch.

camelCase is arguably easier for us on TS/JS though, because we have a linter rule that enforces camelCase variable names, so any time you want to use a plugin, you'd have to do that conversion.

So in summary.... 🤷‍♂. If you don't want to make this change now, then maybe we can leave it as-is and try to have a broader discussion around naming?

legrego · 2019-11-01T11:55:51Z

x-pack/plugins/encrypted_saved_objects/server/config.ts

+export const ConfigSchema = schema.object({
+  enabled: schema.boolean({ defaultValue: true }),
+  encryptionKey: schema.conditional(
+    schema.contextRef('dist'),


question I see that the security plugin also uses the dist context ref, but core's http_config is testing for dev. Are those the only two contexts that you're aware of? Does it matter which context you test for in different scenarios? I can surmise that dist is the built distribution, and dev is presumably dev mode, but I just wanted to make sure I'm not missing any nuances here.

Are those the only two contexts that you're aware of?

Core passes a bunch various context variables 1 and 2.

Does it matter which context you test for in different scenarios? I can surmise that dist is the built distribution, and dev is presumably dev mode, but I just wanted to make sure I'm not missing any nuances here.

Never thought about this to be honest, $dist that we always had in the past (e.g. in legacy security and reporting plugins) sounds more appropriate for things like this since it's something we distribute to others, but 🤷‍♂️

Ah, I see we have prod as a context variable. I wonder if that would be more appropriate? Or would that complicate our functional test setup? It seems like we would always want prod and dist to behave the same way in terms of configuration, but maybe not? I'm splitting hairs, I can leave this alone :)

Hmm, these are all valid questions, let's ask Operations team!

Hey @elastic/kibana-operations can you please explain the conceptual difference between prod and dist Kibana "context flags" and when we should use one vs another? I see we historically used dist for various cases (auto set encryption keys or disable advanced CORS settings if not "dist").

legrego · 2019-11-01T12:53:08Z

x-pack/plugins/encrypted_saved_objects/server/crypto/encrypted_saved_objects_service.ts

@@ -299,7 +295,7 @@ export class EncryptedSavedObjectsService {
    }

    if (Object.keys(attributesAAD).length) {


I know you didn't change this here, but is this check correct?

Suggested change

if (Object.keys(attributesAAD).length) {

if (!Object.keys(attributesAAD).length) {

Hmmm, good catch! It looks like 🤦‍♂️ on my side, let me check.

legrego

Let's be sure to update CODEOWNERS as well

elasticmachine · 2019-11-01T17:42:14Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: dccd3bd

azasypkin · 2019-11-01T18:03:26Z

@legrego PR is ready for the second pass, thanks!

legrego

LGTM! Thanks for the changes!

x-pack/plugins/encrypted_saved_objects/README.md

Co-Authored-By: Larry Gregory <[email protected]>

elasticmachine · 2019-11-01T21:29:09Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 345681d

azasypkin · 2019-11-02T06:59:38Z

7.x/7.6.0: 35615a3

azasypkin added Team:Security Team focused on: Auth, Users, Roles, Spaces, Audit Logging, and more! Feature:NP Migration v7.6.0 labels Oct 31, 2019

azasypkin requested a review from a team as a code owner October 31, 2019 16:17

azasypkin requested a review from a team October 31, 2019 16:17

azasypkin added the release_note:skip Skip the PR/issue when compiling release notes label Oct 31, 2019

azasypkin commented Oct 31, 2019

View reviewed changes

azasypkin force-pushed the issue-xxx-migrate-encrypted-so branch from 5380a52 to b051ef2 Compare October 31, 2019 16:30

legrego self-requested a review October 31, 2019 17:23

azasypkin force-pushed the issue-xxx-migrate-encrypted-so branch from b051ef2 to ef07b22 Compare October 31, 2019 17:40

This comment has been minimized.

Sign in to view

azasypkin added 3 commits October 31, 2019 21:11

Move files.

2087423

Changes in EncryptedSavedObjects plugin.

8c91d81

Changes in other plugins.

cfc031c

azasypkin force-pushed the issue-xxx-migrate-encrypted-so branch from ef07b22 to cfc031c Compare October 31, 2019 20:21

azasypkin requested a review from mikecote October 31, 2019 21:47

mikecote approved these changes Oct 31, 2019

View reviewed changes

legrego reviewed Nov 1, 2019

View reviewed changes

azasypkin added 4 commits November 1, 2019 16:03

Merge branch 'master' into issue-xxx-migrate-encrypted-so

1577e82

Review#1: Add code owner for new plugin.

7d1b8d6

Review#1: Chnage config prefix and plugin ID to encryptedSavedObjects.

bed212c

Review#1: Fix the wrong check for the absence of AAD attributes.

dccd3bd

legrego mentioned this pull request Nov 1, 2019

Adding NP spaces plugin to CODEOWNERS #49963

Merged

azasypkin requested a review from legrego November 1, 2019 18:02

legrego approved these changes Nov 1, 2019

View reviewed changes

x-pack/plugins/encrypted_saved_objects/README.md Outdated Show resolved Hide resolved

Update x-pack/plugins/encrypted_saved_objects/README.md

345681d

Co-Authored-By: Larry Gregory <[email protected]>

azasypkin merged commit 203ef55 into elastic:master Nov 1, 2019

azasypkin deleted the issue-xxx-migrate-encrypted-so branch November 1, 2019 21:32

azasypkin added the backport pending label Nov 1, 2019

azasypkin mentioned this pull request Nov 1, 2019

[7.x] Migrate Encrypted Saved Objects plugin to the new platform. (#49890) #49984

Merged

azasypkin added backported and removed backport pending labels Nov 2, 2019

legrego mentioned this pull request Jul 26, 2021

Encrypted saved objects plugin doesn't allow to register hidden types #42834

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Migrate Encrypted Saved Objects plugin to the new platform. #49890

Migrate Encrypted Saved Objects plugin to the new platform. #49890

azasypkin commented Oct 31, 2019

elasticmachine commented Oct 31, 2019

azasypkin Oct 31, 2019

legrego Nov 1, 2019

azasypkin Nov 2, 2019

This comment has been minimized.

This comment has been minimized.

elasticmachine commented Oct 31, 2019

azasypkin commented Oct 31, 2019

mikecote left a comment

legrego left a comment

legrego Nov 1, 2019

azasypkin Nov 1, 2019

legrego Nov 1, 2019

azasypkin Nov 1, 2019

legrego Nov 1, 2019

legrego Nov 1, 2019

azasypkin Nov 1, 2019

legrego Nov 1, 2019

azasypkin Nov 1, 2019

legrego Nov 1, 2019

azasypkin Nov 1, 2019

legrego left a comment

elasticmachine commented Nov 1, 2019

azasypkin commented Nov 1, 2019

legrego left a comment

elasticmachine commented Nov 1, 2019

azasypkin commented Nov 2, 2019

		@@ -299,7 +295,7 @@ export class EncryptedSavedObjectsService {
		}

		if (Object.keys(attributesAAD).length) {

	if (Object.keys(attributesAAD).length) {
	if (!Object.keys(attributesAAD).length) {

Migrate Encrypted Saved Objects plugin to the new platform. #49890

Migrate Encrypted Saved Objects plugin to the new platform. #49890

Conversation

azasypkin commented Oct 31, 2019

elasticmachine commented Oct 31, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment has been minimized.

This comment has been minimized.

elasticmachine commented Oct 31, 2019

💚 Build Succeeded

azasypkin commented Oct 31, 2019

mikecote left a comment

Choose a reason for hiding this comment

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

legrego left a comment

Choose a reason for hiding this comment

elasticmachine commented Nov 1, 2019

💚 Build Succeeded

azasypkin commented Nov 1, 2019

legrego left a comment

Choose a reason for hiding this comment

elasticmachine commented Nov 1, 2019

💚 Build Succeeded

azasypkin commented Nov 2, 2019