Fix ESO migration decryption for converted saved object types #106897

jportner · 2021-07-27T18:40:56Z

A bug in the algorithm neglected to differentiate when a converted object may have had its ID changed (this happens when an object exists in a non-default space and then it is converted).
This PR fixes the bug and adds several more unit tests and integration tests to exercise different permutations of migrations.

Note that this is a bug fix but I am not labeling it as such, and I am not backporting it to earlier versions of Kibana, because no consumers are using the aforementioned conversion functionality yet.

A bug in the algorithm neglected to differentiate when a converted object may have had its ID changed (this happens when an object exists in a non-default space and then it is converted). This commit fixes the bug and adds several more unit tests and integration tests to exercise different permutations of migrations.

jportner

Author's notes for reviewers

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

ESO migrations should never encrypt non-single-namespace saved object types with a namespace descriptor. This commit updates the migration context to expose an additional field that will help the ESO service to better decide whether to use a namespace in the encryption descriptor.

watson

Besides my comments, there's nothing that stands out to me, but as usual SSO is a complicated beast, so I don't feel 100% comfortable giving this a green light. But fingeres crossed I haven't missed anything 😅

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

* Address PR review feedback * Rename "convertToMultiNamespaceType" boolean to "isTypeBeingConverted" * Update createMigration() function to use consistent encryption descriptors too * Add post-conversion migration unit test cases for createMigration() * Update decryptAttributesSync unit test cases for ESO service

jportner · 2021-07-28T16:55:15Z

Besides my comments, there's nothing that stands out to me, but as usual SSO is a complicated beast, so I don't feel 100% comfortable giving this a green light. But fingeres crossed I haven't missed anything 😅

Thanks a ton for the review! Actually when I went to change "Semver" to "semver", I realized I was missing a pretty important unit test case for when the call to "semver.lte()" returns false (post-conversion, e.g., test with migrationVersion >= 8.0.1). I added that and some other missing stuff in 1f70513.

Since this is so complex, perhaps @legrego or @azasypkin can also take a second pass?

src/core/server/saved_objects/migrations/types.ts

TinaHeiligers

Changes to the SO migrations look fine to me, I'm glad we discovered the issue early 😄 .
I just have one question around retaining support for single-namespace SO types but that' more out of curiosity.
LGTM once CI goes green

These broke with the most recent merge upstream.

azasypkin · 2021-07-29T10:25:26Z

Since this is so complex, perhaps @legrego or @azasypkin can also take a second pass?

ACK: taking a look...

azasypkin

LGTM

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

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

kibanamachine · 2021-07-29T16:02:22Z

💚 Build Succeeded

Metrics [docs]

Unknown metric groups

API count

id	before	after	diff
`actions`	-	117	+117
`advancedSettings`	-	23	+23
`alerting`	-	242	+242
`apm`	-	39	+39
`apmOss`	-	4	+4
`banners`	-	9	+9
`bfetch`	-	73	+73
`canvas`	-	6	+6
`cases`	-	445	+445
`charts`	-	190	+190
`cloud`	-	21	+21
`core`	-	2363	+2363
`dashboard`	-	160	+160
`dashboardEnhanced`	-	51	+51
`dashboardMode`	-	11	+11
`data`	-	3743	+3743
`dataEnhanced`	-	16	+16
`dataVisualizer`	-	104	+104
`devTools`	-	10	+10
`discover`	-	81	+81
`discoverEnhanced`	-	39	+39
`embeddable`	-	456	+456
`embeddableEnhanced`	-	14	+14
`encryptedSavedObjects`	-	28	+28
`enterpriseSearch`	-	2	+2
`esUiShared`	-	92	+92
`eventLog`	-	70	+70
`expressionError`	-	12	+12
`expressionImage`	-	4	+4
`expressionMetric`	-	4	+4
`expressionRepeatImage`	-	28	+28
`expressionRevealImage`	-	4	+4
`expressions`	-	2003	+2003
`expressionShape`	-	90	+90
`features`	-	215	+215
`fileUpload`	-	128	+128
`fleet`	-	1140	+1140
`globalSearch`	-	68	+68
`home`	-	94	+94
`indexLifecycleManagement`	-	4	+4
`indexManagement`	-	162	+162
`indexPatternFieldEditor`	-	41	+41
`infra`	-	25	+25
`inspector`	-	101	+101
`kibanaLegacy`	-	66	+66
`kibanaReact`	-	260	+260
`kibanaUtils`	-	551	+551
`lens`	-	207	+207
`licenseApiGuard`	-	8	+8
`licenseManagement`	-	3	+3
`licensing`	-	117	+117
`lists`	-	150	+150
`management`	-	40	+40
`maps`	-	204	+204
`mapsEms`	-	75	+75
`metricsEntities`	-	9	+9
`ml`	-	278	+278
`monitoring`	-	10	+10
`navigation`	-	31	+31
`newsfeed`	-	17	+17
`observability`	-	219	+219
`osquery`	-	11	+11
`presentationUtil`	-	176	+176
`remoteClusters`	-	4	+4
`reporting`	-	133	+133
`rollup`	-	20	+20
`ruleRegistry`	-	60	+60
`runtimeFields`	-	24	+24
`savedObjects`	-	213	+213
`savedObjectsManagement`	-	96	+96
`savedObjectsTagging`	-	54	+54
`savedObjectsTaggingOss`	-	89	+89
`screenshotMode`	-	22	+22
`security`	-	112	+112
`securityOss`	-	12	+12
`securitySolution`	-	1298	+1298
`share`	-	123	+123
`snapshotRestore`	-	23	+23
`spaces`	-	106	+106
`spacesOss`	-	72	+72
`stackAlerts`	-	4	+4
`taskManager`	-	52	+52
`telemetry`	-	42	+42
`telemetryCollectionManager`	-	29	+29
`telemetryCollectionXpack`	-	1	+1
`telemetryManagementSection`	-	14	+14
`timelines`	-	887	+887
`triggersActionsUi`	-	237	+237
`uiActions`	-	127	+127
`uiActionsEnhanced`	-	205	+205
`urlForwarding`	-	15	+15
`usageCollection`	-	57	+57
`visTypeTimeseries`	-	10	+10
`visualizations`	-	247	+247
total			+19352

API count missing comments

id	before	after	diff
`actions`	-	117	+117
`advancedSettings`	-	22	+22
`alerting`	-	234	+234
`apm`	-	39	+39
`apmOss`	-	4	+4
`banners`	-	9	+9
`bfetch`	-	62	+62
`canvas`	-	5	+5
`cases`	-	407	+407
`charts`	-	159	+159
`cloud`	-	21	+21
`core`	-	1081	+1081
`dashboard`	-	137	+137
`dashboardEnhanced`	-	50	+50
`dashboardMode`	-	11	+11
`data`	-	3190	+3190
`dataEnhanced`	-	16	+16
`dataVisualizer`	-	104	+104
`devTools`	-	8	+8
`discover`	-	55	+55
`discoverEnhanced`	-	37	+37
`embeddable`	-	384	+384
`embeddableEnhanced`	-	14	+14
`encryptedSavedObjects`	-	26	+26
`enterpriseSearch`	-	2	+2
`esUiShared`	-	90	+90
`eventLog`	-	70	+70
`expressionError`	-	12	+12
`expressionImage`	-	4	+4
`expressionMetric`	-	4	+4
`expressionRepeatImage`	-	28	+28
`expressionRevealImage`	-	4	+4
`expressions`	-	1569	+1569
`expressionShape`	-	90	+90
`features`	-	97	+97
`fileUpload`	-	128	+128
`fleet`	-	1045	+1045
`globalSearch`	-	14	+14
`home`	-	70	+70
`indexLifecycleManagement`	-	4	+4
`indexManagement`	-	157	+157
`indexPatternFieldEditor`	-	36	+36
`infra`	-	22	+22
`inspector`	-	78	+78
`kibanaLegacy`	-	62	+62
`kibanaReact`	-	230	+230
`kibanaUtils`	-	359	+359
`lens`	-	191	+191
`licenseApiGuard`	-	8	+8
`licenseManagement`	-	3	+3
`licensing`	-	42	+42
`lists`	-	143	+143
`management`	-	40	+40
`maps`	-	203	+203
`mapsEms`	-	75	+75
`metricsEntities`	-	6	+6
`ml`	-	274	+274
`monitoring`	-	10	+10
`navigation`	-	31	+31
`newsfeed`	-	17	+17
`observability`	-	219	+219
`osquery`	-	11	+11
`presentationUtil`	-	149	+149
`remoteClusters`	-	4	+4
`reporting`	-	132	+132
`rollup`	-	20	+20
`ruleRegistry`	-	60	+60
`runtimeFields`	-	19	+19
`savedObjects`	-	199	+199
`savedObjectsManagement`	-	85	+85
`savedObjectsTagging`	-	50	+50
`savedObjectsTaggingOss`	-	50	+50
`screenshotMode`	-	17	+17
`security`	-	51	+51
`securityOss`	-	9	+9
`securitySolution`	-	1247	+1247
`share`	-	83	+83
`snapshotRestore`	-	22	+22
`spacesOss`	-	5	+5
`stackAlerts`	-	4	+4
`taskManager`	-	25	+25
`telemetryCollectionManager`	-	29	+29
`telemetryCollectionXpack`	-	1	+1
`telemetryManagementSection`	-	13	+13
`timelines`	-	768	+768
`triggersActionsUi`	-	228	+228
`uiActions`	-	88	+88
`uiActionsEnhanced`	-	147	+147
`urlForwarding`	-	15	+15
`usageCollection`	-	16	+16
`visTypeTimeseries`	-	10	+10
`visualizations`	-	229	+229
total			+15415

API count with any type

id	before	after	diff
`bfetch`	-	1	+1
`charts`	-	2	+2
`core`	-	148	+148
`dashboard`	-	1	+1
`data`	-	99	+99
`dataVisualizer`	-	3	+3
`embeddable`	-	4	+4
`esUiShared`	-	4	+4
`expressions`	-	58	+58
`fileUpload`	-	4	+4
`fleet`	-	15	+15
`indexManagement`	-	12	+12
`indexPatternFieldEditor`	-	1	+1
`inspector`	-	6	+6
`kibanaLegacy`	-	3	+3
`kibanaReact`	-	5	+5
`kibanaUtils`	-	3	+3
`maps`	-	2	+2
`mapsEms`	-	1	+1
`ml`	-	10	+10
`presentationUtil`	-	3	+3
`reporting`	-	1	+1
`savedObjects`	-	3	+3
`savedObjectsTaggingOss`	-	3	+3
`securitySolution`	-	8	+8
`share`	-	1	+1
`snapshotRestore`	-	1	+1
`timelines`	-	6	+6
`triggersActionsUi`	-	1	+1
`uiActionsEnhanced`	-	2	+2
`visTypeTimeseries`	-	1	+1
`visualizations`	-	13	+13
total			+425

Non-exported public API item count

id	before	after	diff
`actions`	-	7	+7
`advancedSettings`	-	1	+1
`alerting`	-	16	+16
`apm`	-	30	+30
`bfetch`	-	2	+2
`canvas`	-	3	+3
`cases`	-	14	+14
`charts`	-	1	+1
`core`	-	31	+31
`dashboard`	-	9	+9
`data`	-	64	+64
`dataEnhanced`	-	2	+2
`devTools`	-	2	+2
`discover`	-	6	+6
`discoverEnhanced`	-	2	+2
`embeddable`	-	3	+3
`encryptedSavedObjects`	-	4	+4
`esUiShared`	-	1	+1
`eventLog`	-	4	+4
`expressionError`	-	2	+2
`expressionImage`	-	1	+1
`expressionMetric`	-	1	+1
`expressionRevealImage`	-	1	+1
`expressions`	-	5	+5
`features`	-	2	+2
`fileUpload`	-	1	+1
`fleet`	-	8	+8
`globalSearch`	-	5	+5
`home`	-	5	+5
`indexManagement`	-	3	+3
`indexPatternFieldEditor`	-	3	+3
`infra`	-	3	+3
`inspector`	-	4	+4
`kibanaLegacy`	-	1	+1
`kibanaReact`	-	4	+4
`kibanaUtils`	-	8	+8
`lens`	-	23	+23
`licensing`	-	8	+8
`lists`	-	38	+38
`management`	-	5	+5
`maps`	-	11	+11
`metricsEntities`	-	1	+1
`ml`	-	33	+33
`monitoring`	-	2	+2
`navigation`	-	2	+2
`observability`	-	10	+10
`presentationUtil`	-	5	+5
`reporting`	-	14	+14
`ruleRegistry`	-	9	+9
`runtimeFields`	-	2	+2
`savedObjects`	-	5	+5
`screenshotMode`	-	1	+1
`security`	-	6	+6
`securityOss`	-	3	+3
`securitySolution`	-	28	+28
`share`	-	8	+8
`snapshotRestore`	-	1	+1
`taskManager`	-	8	+8
`telemetryCollectionManager`	-	4	+4
`timelines`	-	25	+25
`triggersActionsUi`	-	19	+19
`uiActions`	-	11	+11
`uiActionsEnhanced`	-	10	+10
`usageCollection`	-	2	+2
`visTypeTimeseries`	-	3	+3
`visualizations`	-	12	+12
total			+568

References to deprecated APIs

id	before	after	diff
`actions`	-	8	+8
`alerting`	-	23	+23
`apm`	-	7	+7
`canvas`	-	53	+53
`cases`	-	151	+151
`crossClusterReplication`	-	2	+2
`dashboard`	-	145	+145
`dashboardEnhanced`	-	13	+13
`dataEnhanced`	-	53	+53
`dataVisualizer`	-	16	+16
`discover`	-	129	+129
`discoverEnhanced`	-	21	+21
`embeddable`	-	2	+2
`encryptedSavedObjects`	-	2	+2
`fleet`	-	89	+89
`globalSearch`	-	4	+4
`graph`	-	2	+2
`indexLifecycleManagement`	-	2	+2
`indexManagement`	-	12	+12
`infra`	-	292	+292
`lens`	-	166	+166
`lists`	-	9	+9
`maps`	-	608	+608
`ml`	-	140	+140
`observability`	-	42	+42
`presentationUtil`	-	2	+2
`savedObjects`	-	6	+6
`savedObjectsManagement`	-	18	+18
`savedObjectsTaggingOss`	-	5	+5
`security`	-	2	+2
`securitySolution`	-	828	+828
`stackAlerts`	-	104	+104
`timelines`	-	76	+76
`transform`	-	16	+16
`uptime`	-	11	+11
`urlDrilldown`	-	18	+18
`visTypeTimeseries`	-	12	+12
`visualizations`	-	34	+34
total			+3123

History

💚 Build #141470 succeeded b60a5eb
💔 Build #141434 failed db8770e
💔 Build #141416 failed 1f70513
💚 Build #141146 succeeded 647d3c4
💚 Build #141099 succeeded beda4b9

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

…c#106897)

kibanamachine · 2021-07-29T16:05:15Z

💚 Backport successful

Status	Branch	Result
✅	7.x

This backport PR will be merged automatically after passing CI.

…106897) (#107189) * Fix ESO migration decryption for converted saved object types (#106897) * Get rid of integration test changes The integration test changes can only be applied in 8.0 or later. We can safely remove that from the 7.x branch. Co-authored-by: Joe Portner <5295965+jportner@users.noreply.github.com>

…c#106897)

jportner added v8.0.0 release_note:skip Skip the PR/issue when compiling release notes v7.15.0 labels Jul 27, 2021

jportner commented Jul 27, 2021

View reviewed changes

x-pack/plugins/encrypted_saved_objects/server/create_migration.ts Outdated Show resolved Hide resolved

jportner marked this pull request as ready for review July 27, 2021 21:00

jportner requested review from a team as code owners July 27, 2021 21:00

chrisronline mentioned this pull request Jul 28, 2021

[8.0 only] Make rule and connector saved objects share-capable #100067

Closed

watson approved these changes Jul 28, 2021

View reviewed changes

x-pack/plugins/encrypted_saved_objects/server/create_migration.ts Outdated Show resolved Hide resolved

x-pack/plugins/encrypted_saved_objects/server/create_migration.ts Outdated Show resolved Hide resolved

jportner added 2 commits July 28, 2021 09:26

Merge branch 'master' into issue-106567-convert-eso-tests

e7f8100

Merge branch 'master' into issue-106567-convert-eso-tests

db8770e

TinaHeiligers reviewed Jul 28, 2021

View reviewed changes

src/core/server/saved_objects/migrations/types.ts Show resolved Hide resolved

TinaHeiligers approved these changes Jul 28, 2021

View reviewed changes

Fix unit tests

b60a5eb

These broke with the most recent merge upstream.

chrisronline mentioned this pull request Jul 28, 2021

[Actions] [7.x] Convert all savedObjectClient.get() calls to use savedObjectClient.resolve and handle the response appropriately #107069

Closed

azasypkin self-requested a review July 29, 2021 10:25

azasypkin approved these changes Jul 29, 2021

View reviewed changes

x-pack/plugins/encrypted_saved_objects/server/crypto/encrypted_saved_objects_service.ts Outdated Show resolved Hide resolved

Update x-pack/plugins/encrypted_saved_objects/server/crypto/encrypted…

30cd5a0

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

jportner added the auto-backport Deprecated - use backport:version if exact versions are needed label Jul 29, 2021

jportner enabled auto-merge (squash) July 29, 2021 13:39

jportner merged commit b0d21b5 into elastic:master Jul 29, 2021

kibanamachine pushed a commit to kibanamachine/kibana that referenced this pull request Jul 29, 2021

Fix ESO migration decryption for converted saved object types (elasti…

9742a52

…c#106897)

kibanamachine mentioned this pull request Jul 29, 2021

[7.x] Fix ESO migration decryption for converted saved object types (#106897) #107189

Merged

jportner deleted the issue-106567-convert-eso-tests branch July 29, 2021 17:30

streamich pushed a commit to vadimkibana/kibana that referenced this pull request Aug 8, 2021

Fix ESO migration decryption for converted saved object types (elasti…

1d1a560

…c#106897)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix ESO migration decryption for converted saved object types #106897

Fix ESO migration decryption for converted saved object types #106897

jportner commented Jul 27, 2021

jportner left a comment

watson left a comment

jportner commented Jul 28, 2021

TinaHeiligers left a comment

azasypkin commented Jul 29, 2021

azasypkin left a comment

kibanamachine commented Jul 29, 2021

API count

API count missing comments

API count with any type

Non-exported public API item count

References to deprecated APIs

kibanamachine commented Jul 29, 2021

Fix ESO migration decryption for converted saved object types #106897

Fix ESO migration decryption for converted saved object types #106897

Conversation

jportner commented Jul 27, 2021

jportner left a comment

Choose a reason for hiding this comment

watson left a comment

Choose a reason for hiding this comment

jportner commented Jul 28, 2021

TinaHeiligers left a comment

Choose a reason for hiding this comment

azasypkin commented Jul 29, 2021

azasypkin left a comment

Choose a reason for hiding this comment

kibanamachine commented Jul 29, 2021

💚 Build Succeeded

Metrics [docs]

API count

API count missing comments

API count with any type

Non-exported public API item count

References to deprecated APIs

History

kibanamachine commented Jul 29, 2021

💚 Backport successful