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

[saved objects][pit finder] Add support for multiple namespaces. #99044

Closed
lukeelmers opened this issue May 3, 2021 · 8 comments · Fixed by #109062
Closed

[saved objects][pit finder] Add support for multiple namespaces. #99044

lukeelmers opened this issue May 3, 2021 · 8 comments · Fixed by #109062
Labels
enhancement New value added to drive a business result Feature:Saved Objects Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc

Comments

@lukeelmers
Copy link
Member

lukeelmers commented May 3, 2021

When we originally added support for using savedObjectsClient.find with an Elasticsearch point-in-time (PIT), we decided to defer adding support for multiple namespaces until we had a concrete need for it.

With some of the recent refactoring around visualization usage collectors, this need has come up again.

To do this, we'd need to:

  • Update soRepository.openPointInTimeForType to handle namespaces and typeToNamespacesMap in the same way as find (including the runtime checks for making sure you don't try to provide both)
  • Update the secure SOC wrapper implementation of openPointInTimeForType to perform a privilege check and build the namespaces map to provide to the base repository
  • Remove the restriction on combining pit with namespaces from secure SOC wrapper find
  • soClient.createPointInTimeFinder will need to ensure the typeToNamespacesMap is handed off when calling openPointInTimeForType
  • Add tests to ensure searching across namespaces works as expected when using createPointInTimeFinder

Blocks #99031, #99023, #98914, #98903, #91615

@lukeelmers lukeelmers added Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc triage_needed enhancement New value added to drive a business result Feature:Saved Objects labels May 3, 2021
@elasticmachine
Copy link
Contributor

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

@pgayvallet
Copy link
Contributor

So we would need to handle a situation where, for example, someone tried to use find with a pit ID and a list of types which had different backing indices than the indices already associated with that PIT.

You lost me. How is that specific to the multi-namespaces enhancement?

@lukeelmers
Copy link
Member Author

How is that specific to the multi-namespaces enhancement?

It's not just you. I typed this issue up very quickly earlier as I was moving on to something else, and now I'm re-reading and the wording doesn't make sense to me either. 😂

The problem I had in mind was around any authz issues that could arise from the relationship between types & namespaces (typeToNamespacesMap). Specifically what would happen if a user called openPointInTimeForType with a specific set of types which may only be authorized in certain namespaces. Since ES will associate that pit with the index for that type, are there any issues that would come up if find was then called later with that pit but had different types or namespaces specified? e.g. Would we want to allow PITs to be opened against any type regardless of namespace, since the namespace will still be filtered out in the query generated by find?

These are questions that could be resolved in a PR w/ the security team's input, just wanted to mention them here as I haven't yet thought through all of the possible implications. I'll update the issue description to be a bit clearer.

@afharo
Copy link
Member

afharo commented May 5, 2021

I see the concerns with querying different types in the same pit. But how namespaces is a blocker? AFAIK, we only link alternative indices to types, but not namespaces.

Maybe that's changed since the last time I checked?

@lukeelmers
Copy link
Member Author

I see the concerns with querying different types in the same pit. But how namespaces is a blocker?

namespaces is not a blocker pre se, just a consideration that would need to be discussed in the PR. The case I'm thinking about is when the DSL query is written in find, and typeToNamespacesMap is used... in that case, AFAICT a type can be dropped entirely if it isn't authorized in any namespace. (I am by no means an expert on how we are securing spaces, just was looking at this test case):

it('supersedes `type` and `namespaces` parameters', () => {
const result = getQueryParams({
registry,
type: ['pending', 'saved', 'shared', 'global'],
namespaces: ['foo', 'bar', 'default'],
typeToNamespacesMap: new Map([
['pending', ['foo']], // 'pending' is only authorized in the 'foo' namespace
// 'saved' is not authorized in any namespaces
['shared', ['bar', 'default']], // 'shared' is only authorized in the 'bar' and 'default' namespaces
['global', ['foo', 'bar', 'default']], // 'global' is authorized in all namespaces (which are ignored anyway)
]),
});
expectResult(
result,
createTypeClause('pending', ['foo']),
createTypeClause('shared', ['bar', 'default']),
createTypeClause('global')
);
});

So the question I had in mind was, would it be possible for the type or namespace to differ between the calls to openPointInTimeForType and find as a result of the settings in the typeToNamespacesMap?

I think as long as someone is using the PIT finder, the answer to this is "no" as it requires the same configuration to be passed which is used in both opening the PIT and calling find.

But if someone isn't using the PIT finder and is directly using openPointInTimeForType with find, do we need to worry about an edge case where privileges are changed between both of those calls, causing the find query to require fields which may no longer be available?

The possible answers I see to this are:

  1. Don't worry about it and implement typeToNamespaceMap in openPointInTimeForType as well, as that scenario feels highly unlikely. In this case we would just assume that privileges don't change between opening an PIT and using find, and if it does, you may not get the expected results.
  2. Don't implement typeToNamespaceMap in openPointInTimeForType, and simply open the PIT against the indices for all allowed types in the request. If permissions change later, the correct namespace filters should still be applied in find. The tradeoff is that you are allowing the PIT to be open against some types which could potentially not be allowed in any namespaces, however as you aren't able to query those with find, this may be fine.

Option 2 feels simplest to me, but there may be a gaping hole in my understanding here, so I'd defer to @elastic/kibana-security for input and clarification 🙂

@legrego
Copy link
Member

legrego commented May 5, 2021

From a security standpoint, find will always filter out results that you're unable to see, no matter how the PIT was configured. You can get unexpected results if your privileges change between the time the PIT was created and the time you finish calling find, but unexpected results are expected behavior -- we don't ever cache privileges, and "find with PIT" is by no means atomic.

To me the real question comes down to: do we want to open a PIT against types the user isn't authorized to query? Ideally no, as I expect there is overhead involved in doing so on the ES side. If I'm thinking about this correctly, all we'd have to do to support Option 1 above is to open the PIT against only those types which appear in the typeToNamespaceMap, instead of all requested types. I wouldn't expect that to be terribly involved, but I might be overlooking some complexity there

@lukeelmers
Copy link
Member Author

lukeelmers commented May 5, 2021

all we'd have to do to support Option 1 above is to open the PIT against only those types which appear in the typeToNamespaceMap

@legrego That makes sense, so in secure SOC wrapper we'd need to build the typeToNamespacesMap when opening PIT (as we do in find), and in the repository we'd have openPointInTimeForType mimic the base find behavior of pulling the types out of the map when they're provided. Then we can lift the restriction on providing pit with namespaces in find, because we know the indices for the correct types should have already been applied to the PIT.

I think that's pretty straightforward then, as namespace support would really just happen as a side effect.

EDIT: Issue description updated to reflect this approach. Folks feel free to chime in if anything seems amiss.

@pgayvallet
Copy link
Contributor

This is also a blocker for the multi-NS export (#91615), as exportByType relies on a PIT search under the hood

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New value added to drive a business result Feature:Saved Objects Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants