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

Object Level Security, Phase 1 #82725

Closed
legrego opened this issue Nov 5, 2020 · 11 comments
Closed

Object Level Security, Phase 1 #82725

legrego opened this issue Nov 5, 2020 · 11 comments
Labels
blocked enhancement New value added to drive a business result Feature:Saved Objects Feature:Security/Object Level Security 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

@legrego
Copy link
Member

legrego commented Nov 5, 2020

Object Level Security, Phase 1

RFC: #93115
Blocked by: elastic/elasticsearch#65277


This issue describes the first phase of Object Level Security ("OLS") #39259.

The goal of Phase 1 is to deliver enough functionality to support user-specific settings and preferences. What follows is a summary of a conversation I had with @pgayvallet.

Scope

For the first phase of OLS will introduce support for Saved Object ownership. This will be described with metadata attached to the root level of each Saved Object consistent with the ACL specified in #39259:

{
    "acl": {
        "owner": "_unique user identifier_ (may not be a simple string)"
    },
    "id": "user-settings:foo",
    "attributes": {},
    "references": [],
    "namespaces": ["default"]
}

Explicitly out of scope:

  • Adding the additional ACL properties to control sharing
  • Adding/Updating user interfaces to account for "owned" saved objects
  • Facilities for managing object ownership (?)

Tasks

These are the high level tasks that we've identified so far.

  • Update root index mapping to support the new acl property
  • Update SO Client options for each operation to accept an optional acl property
  • Update SO Repository to return ACL information for each saved object
  • Update Security SO Client Wrapper:
    • Provide ownership information when creating objects
    • Enforce security controls based on the SO's declared ACL

High-level authorization flow

image

@legrego legrego added 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! enhancement New value added to drive a business result Feature:Saved Objects labels Nov 5, 2020
@elasticmachine
Copy link
Contributor

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

@elasticmachine
Copy link
Contributor

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

@pgayvallet
Copy link
Contributor

Update SO APIs to return allowed operations for each object (read, update, delete, etc) Owner: Kibana Core Team

I'm unsure about the owner for this one. I would ideally be performing that in core, but we currently have no way to compute this information from core atm, as we don't have the information of the current user's id. So I don't think it can be done on our side?

A possible solution would be to let core return true for every operation, and have the security client wrapper updates them with the 'correct' permissions? Or do you see another possible solution?

Update SO find operation to allow SO wrappers to augment the query, so that we may filter results based on object owner

We already discussed about it, but for the record, what do we exactly want here?

  • Would adapting getQueryParams to support this new find parameter be sufficient, or
  • Do we want a new extension point for client wrapper to mutate the return of getSearchDsl that is currently done here:

seq_no_primary_term: true,
...getSearchDsl(this._mappings, this._registry, {

For spaces, the decision was to keep the actual query logic into core. Are we in the same situation here or not. Note that solution 1 is probably way easier to implement (well, depending on the response of the next point)

"owner": "unique user identifier (may not be a simple string)"

As a side note, it would be amazing if this could actually be serialized to a string format. It would be a pain to have a mapping that depends on the authentication provider's principal format (not sure about the feasability tbh)

@legrego
Copy link
Member Author

legrego commented Nov 10, 2020

A possible solution would be to let core return true for every operation, and have the security client wrapper updates them with the 'correct' permissions? Or do you see another possible solution?

I was thinking we'd end up with something like this. My justification was that core could potentially leverage this to implement readonly kibana assets (#70461)

We already discussed about it, but for the record, what do we exactly want here?

  • Would adapting getQueryParams to support this new find parameter be sufficient, or
  • Do we want a new extension point for client wrapper to mutate the return of getSearchDsl that is currently done

I'm favoring the first option right now. I think that'll be somewhat easier to reason about.

As a side note, it would be amazing if this could actually be serialized to a string format. It would be a pain to have a mapping that depends on the authentication provider's principal format (not sure about the feasability tbh)

I think a string representation would be easy enough to do. I'm not sure yet if we'll need to query against the fields that make up the serialized id, so we may need to include both representations. Something like:

{
   "owner": {
      "id": serialize(realmType, realmName, username),
      "realmName": "saml1",
      "realmType": "saml",
      "username": "sally" 
   }
}

Querying against the single owner.id field is "easy", but we will need to eventually support more complex queries against this ACL. For example, we'd want to _find all SOs where any of the following are true:

  • Object is owned by the current user
  • Object has been shared to the current user directly (optionally filtering based on read or all access)
  • Object has been shared to one of the roles assigned to the current user (optionally filtering based on read or all access).

We don't need to support these complex queries initially, but we should keep them in mind when designing the interface here.

@kobelb
Copy link
Contributor

kobelb commented Nov 10, 2020

A possible solution would be to let core return true for every operation, and have the security client wrapper updates them with the 'correct' permissions? Or do you see another possible solution?

Will this require us to make changes so that the security plugin doesn't replace the base SavedObjectsClient with

savedObjects.setClientFactoryProvider(
(repositoryFactory) => ({ request, includedHiddenTypes }) => {
const kibanaRequest = getKibanaRequest(request);
return new SavedObjectsClient(
authz.mode.useRbacForRequest(kibanaRequest)
? repositoryFactory.createInternalRepository(includedHiddenTypes)
: repositoryFactory.createScopedRepository(kibanaRequest, includedHiddenTypes)
);
}
);
?

@legrego
Copy link
Member Author

legrego commented Nov 10, 2020

Will this require us to make changes so that the security plugin doesn't replace the base SavedObjectsClient

I don't believe so, but the fact that you're asking makes me think you have a scenario in mind that I haven't considered yet. The SOC and repository created by the security plugin are still owned by core. Security's wrapper is in a good position to replace the default flags set by core before returning them to the consumer.

@kobelb
Copy link
Contributor

kobelb commented Nov 10, 2020

I don't believe so, but the fact that you're asking makes me think you have a scenario in mind that I haven't considered yet. The SOC and repository created by the security plugin are still owned by core. Security's wrapper is in a good position to replace the default flags set by core before returning them to the consumer.

For some reason, I have it stuck in my head that because the security plugin is replacing the base SavedObjectsClient that we shouldn't be implementing too much logic there. I thought this was the reason for us having to implement so much of the saved-objects logic with in the repository itself, as opposed to the SavedObjectsClient. However, you're right that we're just constructing a new instance of the SavedObjectsClient which is owned by core, so this doesn't make sense... I'll keep trying to think of why this might be a valid concern, but until then, feel free to disregard my previous statement.

@pgayvallet
Copy link
Contributor

I'm not sure yet if we'll need to query against the fields that make up the serialized id

I don't see any usage yet, but as long as these additional fields do not depends on the user's realm and can be defined as a static mappings, it should be alright. Ideally we would limit the new OLS-related find parameter to a minimum though (I would like to avoid having like 10 additional options in the find param object)

Querying against the single owner.id field is "easy", but we will need to eventually support more complex queries against this ACL. For example, we'd want to _find all SOs where any of the following are true:

Object is owned by the current user
Object has been shared to the current user directly (optionally filtering based on read or all access)
Object has been shared to one of the roles assigned to the current user (optionally filtering based on read or all access).

Looking at the proposal in #39259, it should be fine. This will be some really fun queries to write (and to test), but nothing impossible here.

{
  "owner": 123456789,
  "read": {
    "users": [ { id: 123456789, can_share: true } ],
    "roles": [ { id: "role_one", can_share: false } ]
  },
  "write": {
    "users": [ { id: 123456789, can_share: true } ],
    "roles": [ { id: "role_one", can_share: false } ]
  }
}

@kobelb
Copy link
Contributor

kobelb commented Dec 4, 2020

@lukasolson @lizozom, will Background Sessions be taking advantage of OLS phase 1 for these background sessions to be user-specific? Have we ensured that there won't be any licensing conflicts here?

@exalate-issue-sync exalate-issue-sync bot added impact:low Addressing this issue will have a low level of impact on the quality/strength of our product. loe:small Small Level of Effort labels Aug 5, 2021
@legrego legrego removed EnableJiraSync loe:small Small Level of Effort impact:low Addressing this issue will have a low level of impact on the quality/strength of our product. labels Aug 18, 2022
@pgayvallet
Copy link
Contributor

So... Almost 4 years, uh? I wonder, at this point, should we just close this @legrego? Or is there still the slightest chance we ever get back to this?

@legrego
Copy link
Member Author

legrego commented Jul 8, 2024

@pgayvallet it's probably safe to close this. @azasypkin will soon be starting designs for Private Saved Objects, which I view as a successor to this.

@legrego legrego closed this as not planned Won't fix, can't repro, duplicate, stale Jul 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
blocked enhancement New value added to drive a business result Feature:Saved Objects Feature:Security/Object Level Security 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

Successfully merging a pull request may close this issue.

5 participants