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

Encrypted saved-object attributes cause issues for import/export #82086

Closed
kobelb opened this issue Oct 29, 2020 · 22 comments
Closed

Encrypted saved-object attributes cause issues for import/export #82086

kobelb opened this issue Oct 29, 2020 · 22 comments
Labels
discuss NeededFor:Alerting Services Project:RemoveLegacyMultitenancy Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc Team:Security Team focused on: Auth, Users, Roles, Spaces, Audit Logging, and more!

Comments

@kobelb
Copy link
Contributor

kobelb commented Oct 29, 2020

Encrypted saved-object attributes were originally implemented to store end-user provided secrets that only needed to be consumed by the Kibana server. Since the secrets are only consumed by the Kibana server, we intentionally prevented them from being disclosed to end-users because they contain sensitive information. For example, Alerting's connectors store credentials for third-party services, and even though we allow them to be provided via Kibana's UI, we didn't want to disclose them to other end-users who should only be consuming the configured connector.

Since their original implementation, a change was made to allow the developer to opt-out of this protection and allow the secrets to be retrieved by end-users. In those situations, there aren't any complications with import/export. However, we still have quite a few situations where the attributes are being stripped from the responses.

When the encrypted saved-object attributes are excluded from the responses, this causes complications for import/export. When a user exports one of these saved-objects, the encrypted attributes are excluded from the response and this will cause complications on import.

Potential Solution

Import the saved-objects without these attributes

If we allow the saved-objects to be imported without these attributes, it can cause complications within the consuming code because the code must accommodate for the fact that these attributes are undefined. This can also convey the wrong message to our users, and make them think that the import was completed successfully and there's nothing else that needs to be done. Any functionality that is dependent on the existence of these attributes will continue to fail until the end-user re-provides the secrets. For example, Alerting's actions will fail to execute until the user specifies the credentials for the third-party services.

Prompt the user to provide the secrets on import

During the import process, we can detect when there are encrypted attributes that are missing and prompt the end-user to provide these credentials. However, the import process is already rather complicated, so adding another step potentially makes this process even more confusing.

Include the attributes in the export

We currently exclude the encrypted attributes from all saved-object operation responses. It's possible for us to only include these secrets from the export operation. We already have a "Saved-Object Management" feature that we could use to provide this elevated level of access. However, we'll want to make sure that we aren't violating the end-users expectations of secrecy by doing so.

Use-case specific import/export

It's also possible for us to not try to solve this at that platform level, and require a use-case specific import/export. This would allow the consumers of encrypted saved-object attributes to make this decision and implement one of the aforementioned solutions, or one we haven't outlined yet. This would create a rather fragmented experience, and make it difficult for end-users to import/export all of their saved-objects.

@kobelb kobelb added Project:RemoveLegacyMultitenancy Team:Security Team focused on: Auth, Users, Roles, Spaces, Audit Logging, and more! labels Oct 29, 2020
@elasticmachine
Copy link
Contributor

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

@kobelb kobelb added the Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc label Oct 29, 2020
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-platform (Team:Platform)

@kobelb
Copy link
Contributor Author

kobelb commented Oct 29, 2020

Pinging @elastic/kibana-alerting-services

@mikecote
Copy link
Contributor

mikecote commented Oct 30, 2020

From an alerting perspective, I have a few notes below (sorry if they talk outside encrypted saved objects specifically, though there are other issues alerting would have with import / export. See #60053 (comment)):

In regards to connectors, we are exploring the option of exposing encrypted attributes to the end user (#78704). If ever we go that approach, it would play nicely with the Include the attributes in the export option proposed.

More specifically to import / export, the @elastic/kibana-alerting-services team would need to confirm if ever these connector configurations do change when the end user is moving them between environments (ex: from staging to production). Is moving between environments the primary use case for import / export? Would the connectors all connect to the same endpoints? We could always alternatively add an enable / disable functionality and disable the connectors on import..

In regards to alerts, we encrypt the API key which will not exist when importing into a different environment. The option of Include the attributes in the export is most likely not going to work for this scenario. We could in theory create new API keys for each alert on import but this will change the expected privileges the alert would run as. Here as well, we could explore importing the alerts as disabled since the enable / disable feature exists already.

@mikecote
Copy link
Contributor

Use-case specific import/export

It's also possible for us to not try to solve this at that platform level, and require a use-case specific import/export. This would allow the consumers of encrypted saved-object attributes to make this decision and implement one of the aforementioned solutions, or one we haven't outlined yet. This would create a rather fragmented experience, and make it difficult for end-users to import/export all of their saved-objects.

This is sort of the path I've been thinking for a while and we've been tracking it here: #50266. I agree about the fragmented experience and hopefully we can come up with something better.

@legrego
Copy link
Member

legrego commented Nov 4, 2020

Include the attributes in the export

We currently exclude the encrypted attributes from all saved-object operation responses. It's possible for us to only include these secrets from the export operation. We already have a "Saved-Object Management" feature that we could use to provide this elevated level of access. However, we'll want to make sure that we aren't violating the end-users expectations of secrecy by doing so.

It's not possible today due to licensing restrictions, but it's technically possible to model this with sub-feature privileges. We could add a new sub-feature privilege to export encrypted secrets, and not grant this to the "primary" feature privileges by default. One downside with this approach is that it would require explicit action from an administrator to grant this privilege, which adds friction to the export process. That might be tolerable, but it's worth keeping in mind as we consider these options.

Use-case specific import/export

It's also possible for us to not try to solve this at that platform level, and require a use-case specific import/export. This would allow the consumers of encrypted saved-object attributes to make this decision and implement one of the aforementioned solutions, or one we haven't outlined yet. This would create a rather fragmented experience, and make it difficult for end-users to import/export all of their saved-objects.

I'm not opposed to this approach...it might be more palatable to think of this as "solution-specific import/export", as those are somewhat more contained than features or apps, and the solutions might be in a better position to make informed decisions about which objects/attributes to include in the export. There's admittedly a lot of open questions about exporting from the shared services such as Alerts, though.

@pgayvallet
Copy link
Contributor

Import the saved-objects without these attributes

Forces the consuming code to be aware of such limitation and to handle them accordingly. Extremely error prone imho.

Prompt the user to provide the secrets on import

Yea, as you said, this would be a pain to implement. Not doable at the moment at least, as there is no way for plugins to hook or add additional logic to the export/import process. I feel like this export 'enhancement' feature is going to be required to solve most of the issues we currently have with SO export/import, so long term, that might be an option? Still, allowing the client-side import workflow to be modified from plugins (like, to add this 'prompt secret' step) will be all but trivial.

Include the attributes in the export

It's not possible today due to licensing restrictions

@legrego Could you elaborate on that?

One downside with this approach is that it would require explicit action from an administrator to grant this privilege, which adds friction to the export process. That might be tolerable, but it's worth keeping in mind as we consider these options.

Also, that would still force to be able to support the first point (Import the saved-objects without these attributes), as a user exporting without this privilege would have the attributes removed.

Use-case specific import/export

Ideally at some point, the SO import/export APIs (note: not necessarily the SO management section, but at least the APIs) would be the single entry point to perform such operations (even if we are aware that in current state, it is not possible). But as a short term, I guess it would work. That's basically doing nothing on our side though, so not sure it should be considered an option.

@azasypkin
Copy link
Member

azasypkin commented Nov 4, 2020

Still, allowing the client-side import workflow to be modified from plugins (like, to add this 'prompt secret' step) will be all but trivial.

Just an idea, alternatively we could just proceed with import as usual and notify subscribers (e.g. Alerts subscribes to import events for the alert SO type) that these particular objects were imported so that plugin can analyze them and warn/guide users afterwards (or do anything it wishes with these objects). Not sure if it's technically feasible for the large imports though.

@legrego Could you elaborate on that?

I believe @legrego meant that sub-feature privileges is a gold+ Kibana feature right now.

@legrego
Copy link
Member

legrego commented Nov 4, 2020

@legrego Could you elaborate on that?

I believe @legrego meant that sub-feature privileges is a gold+ Kibana feature right now.

++ That's exactly right. We'd need to offer sub-feature privileges at the basic tier in order to take advantage of them here

@kobelb
Copy link
Contributor Author

kobelb commented Nov 4, 2020

With regard to the licensing of sub-feature privileges, this is only an issue because it would be a breaking change for Alerting's usage, right? Otherwise, if the users had a Basic license and they were granted all access to saved-object management their exports would always include the credentials. However, if the user had a Gold+ license, they could remove the sub-feature privilege from certain roles.

@pgayvallet
Copy link
Contributor

Just an idea, alternatively we could just proceed with import as usual and notify subscribers (e.g. Alerts subscribes to import events for the alert SO type) that these particular objects were imported so that plugin can analyze them and warn/guide users afterwards (or do anything it wishes with these objects). Not sure if it's technically feasible for the large imports though.

It may be easier. However performing this after the actual import would cause issues if the import workflow is interrupted before they are executed:

I.E:

  • I import a file
  • SOs are loaded into ES
  • this 'hooks' kicks in, and a specific plugin replies by asking the UI to show a dialog for the user to prompt the secret
  • user ignores it, or close the tab, or anything
  • SOs have been persisted, but in an invalid state

-> we are back to a state similar to option 1, Import the saved-objects without these attributes: it can cause complications within the consuming code because the code must accommodate for the fact that these attributes are undefined

@legrego
Copy link
Member

legrego commented Nov 5, 2020

With regard to the licensing of sub-feature privileges, this is only an issue because it would be a breaking change for Alerting's usage, right? Otherwise, if the users had a Basic license and they were granted all access to saved-object management their exports would always include the credentials. However, if the user had a Gold+ license, they could remove the sub-feature privilege from certain roles.

I don't follow the breaking change, can you elaborate? The only use of sub-features that I'm aware of are the "Create Short URL" privileges we shipped with the initial release of the feature. The primary concern for users with a Basic license comes down to how the sub-feature privilege would be granted:

If we say that the sub-feature privilege should not be granted by default (includeIn: 'none'), then it will not be available at all for users with a basic license. We could add yet another flag to indicate that this excluded privilege should be included in the event of an incompatible license, but I'm not in love with that idea.

If we say that the sub-feature privilege should be granted by default (includeIn: 'all'), then it will be available to all license levels, but it will be granted by default. This might surprise administrators who thought that their encrypted secrets would remain secret.

@azasypkin
Copy link
Member

-> we are back to a state similar to option 1, Import the saved-objects without these attributes: it can cause complications within the consuming code because the code must accommodate for the fact that these attributes are undefined

Correct, the idea was to make option 1 a bit more approachable, so that plugins are aware of possible non-functional SOs before user stumbles upon them. It'd be up to a plugin to either add an entry to a notification center or annoyingly prompt user to fix those or do something else right away or later.

Essentially we have just two high-level options:

  • Either we give solutions and plugins all the necessary tools to recover from the state when encrypted attributes are missing that'd be an additional burden on plugins. I guess plugins cannot avoid that work completely anyway. It's hard to scale that approach properly, but it isn't really blocked by anything right now.

  • Or we put that burden on Kibana administrators and let them decide who can access decrypted attributes during import, understand the consequences and make sure that sensitive content and AAD aren't leaked on the way (export to a password protected archive?). This option can make make life of power users easier, but not for our plugins: they'll still have to support cases when SOs are imported without encrypted attributes by a not power user.

@mshustov
Copy link
Contributor

mshustov commented Nov 5, 2020

Correct, the idea was to make option 1 a bit more approachable, so that plugins are aware of possible non-functional SOs before user stumbles upon them. It'd be up to a plugin to either add an entry to a notification center or annoyingly prompt user to fix those or do something else right away or later.

It seems @mikecote suggested a similar idea in #50266 (comment)

Or we put that burden on Kibana administrators and let them decide who can access decrypted attributes during import

Don't think this option is feasible. Considering the alerting plugin case where manual intervention might be still required after importing. from #50266 (comment)

When moving alert SOs to another cluster, the API keys will not exist within Elasticsearch
When importing alerts, task manager tasks will have to be created so these alerts run after importing
Is is possible that the user wants to provide different credentials in their connectors when moving to another cluster

@azasypkin
Copy link
Member

It seems @mikecote suggested a similar idea in #50266 (comment)

Yep, thanks, it seems the general idea is the same.

Or we put that burden on Kibana administrators and let them decide who can access decrypted attributes during import
Don't think this option is feasible. Considering the alerting plugin case where manual intervention might be still required after importing. from #50266 (comment)

++, the case when secrets may not compatible between source and destination is a tricky one and only specific plugins would know how to handle that.

@kobelb
Copy link
Contributor Author

kobelb commented Nov 9, 2020

@mikecote also brought up that since we don't allow IDs to be specified when creating encrypted saved-objects, this potentially causes issues on import.

@azasypkin
Copy link
Member

since we don't allow IDs to be specified when creating encrypted saved-objects

We do allow IDs if overwrite flag is set and SO version is defined. I naively thought that it's the case for import or it's not?

@pgayvallet
Copy link
Contributor

I naively thought that it's the case for import or it's not?

It seems to be. version is included in the export, and the encrypted client seems to support it also for bulk creates (which is the operation we are using for imports)

const canSpecifyID = options?.overwrite && object.version;
if (object.id && !canSpecifyID) {
throw new Error(
'Predefined IDs are not allowed for saved objects with encrypted attributes.'

@mikecote
Copy link
Contributor

Setting the SO version will fail the import if the document is missing. I tried this in DevTools and using the SO client.

Here's an example Screen Shot 2020-11-10 at 8 04 46 AM

There's a few combinations we'd have to support to import encrypted SO:
Screen Shot 2020-11-10 at 8 05 09 AM

@pgayvallet
Copy link
Contributor

Setting the SO version will fail the import if the document is missing

How is this currently working then the user selects automatically overwrite conflicts then? We are passing the version for all imported SOs, without checking for existence I believe (or do we)?

@mikecote
Copy link
Contributor

mikecote commented Nov 10, 2020

@pgayvallet I believe it's filtered out here:

// filter out the 'version' field of each object, if it exists
const objectsToCreate = filteredObjects.map(({ version, ...object }) => {

@kobelb
Copy link
Contributor Author

kobelb commented Dec 4, 2020

Thanks everyone for your thoughts and consideration on this issue. I'm going to close this issue as we've decided a path forward:

  1. Saved-object import warnings #84977
  2. Allow predefined ids for encrypted saved objects #83482

@kobelb kobelb closed this as completed Dec 4, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discuss NeededFor:Alerting Services Project:RemoveLegacyMultitenancy Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc Team:Security Team focused on: Auth, Users, Roles, Spaces, Audit Logging, and more!
Projects
None yet
Development

No branches or pull requests

7 participants