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

RFC: Object level security, Phase 1 #93115

Merged
merged 18 commits into from
Apr 12, 2021
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Binary file added rfcs/images/ols_phase_1_auth.png
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
260 changes: 260 additions & 0 deletions rfcs/text/0015_ols_phase_1.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,260 @@
- Start Date: 2020-03-01
- RFC PR: (leave this empty)
- Kibana Issue: (leave this empty)

---
- [1. Summary](#1-summary)
- [2. Motivation](#2-motivation)
- [3. Detailed design](#3-detailed-design)
- [4. Drawbacks](#4-drawbacks)
- [5. Alternatives](#5-alternatives)
- [6. Adoption strategy](#6-adoption-strategy)
- [7. How we teach this](#7-how-we-teach-this)
- [8. Unresolved questions](#8-unresolved-questions)

# 1. Summary

Object-level security ("OLS") authorizes Saved Object CRUD operations on a per-object basis.
This RFC focuses on the [phase 1](https://github.com/elastic/kibana/issues/82725), which introduces "private" saved object types. These private types
are owned by individual users, and are _generally_ only accessible by their owners.

This RFC does not address any [followup phases](https://github.com/elastic/kibana/issues/39259), which may support sharing, and ownership of "public" objects.

# 2. Motivation

OLS allows saved objects to be owned by individual users. This allows Kibana to store information that is specific
to each user, which enables further customization and collaboration throughout our solutions.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see the value in being able to designate a saved object as belonging to a specific user identity. But it's probably worth asking, do we also need security? Just like a user is able to completely mess up or delete another user's dashboard would it matter if there wasn't a hard control to prevent accessing a saved object belonging to another user, but this was simply enforced in the UI layer?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's a good question. I think there are some cases where recording identity would be enough, but there are other cases where we'd want to enforce security.

Take user preferences as an example: There isn't a ton of value in preventing Alice from reading Bob's preferences, but it would be really troubling if Alice could modify or delete Bob's preferences. Since all users would be authorized to update/delete these SOs, even a traditionally "readonly" user might be authorized to bulk delete the preferences of all other users. That's not something that an administrator could easily fix, unless they happened to have a snapshot of the Kibana index that they were willing to restore.


The most immediate feature this unlocks is [User settings and preferences (#17888)](https://github.com/elastic/kibana/issues/17888),
which is a very popular and long-standing request.

# 3. Detailed design

Phase 1 of OLS allows consumers to register "private" saved object types.
These saved objects are owned by individual end users, and are subject to additional security controls.

Public (non-private) saved object types are not impacted by this RFC. This proposal does not allow types to transition to/from `public`/`private`, and is considered out of scope for phase 1.

## 3.1 Saved Objects Service

### 3.1.1 Type registry
The [saved objects type registry](https://github.com/elastic/kibana/blob/701697cc4a34d07c0508c3bdf01dca6f9d40a636/src/core/server/saved_objects/saved_objects_type_registry.ts) will allow consumers to register "private" saved object types via a new `classification` property:
legrego marked this conversation as resolved.
Show resolved Hide resolved

```ts
/**
* The classification dictates the protection level of the saved object:
* * public (default): instances of this saved object type will be accessible to all users within the given namespace, who are authorized to act on objects of this type.
* * private: instances of this saved object type will belong to the user who created them, and will not be accessible by other users, except for administrators.
*/
export type SavedObjectsClassification = 'public' | 'private';

// Note: some existing properties have been omitted for brevity.
export interface SavedObjectsType {
name: string;
hidden: boolean;
namespaceType: SavedObjectsNamespaceType;
mappings: SavedObjectsTypeMappingDefinition;

/**
* The {@link SavedObjectsClassification | classification} for the type.
*/
classification?: SavedObjectsClassification;
}

// Example consumer
class MyPlugin {
setup(core: CoreSetup) {
core.savedObjects.registerType({
name: 'user-settings',
classification: 'private',
namespaceType: 'single',
hidden: false,
mappings,
})
}
}
```

### 3.1.2 Schema
Saved object ownership will be recorded as metadata within each `private` saved object. We do so by adding a top-level `acl` property ("access control list") with a singular `owner` property:
legrego marked this conversation as resolved.
Show resolved Hide resolved

```ts
/**
* The "Access Control List" describing which users should be authorized to access this SavedObject.
*
* @public
*/
export interface SavedObjectACL {
/** The owner of this SavedObject. */
owner: string;
}

// Note: some existing fields have been omitted for brevity
export interface SavedObject<T = unknown> {
id: string;
type: string;
attributes: T;
references: SavedObjectReference[];
namespaces?: string[];
/** The "Access Control List" describing which users should be authorized to access this SavedObject. */
acl?: SavedObjectACL;
}
```

### 3.1.3 Saved Objects Client: Security wrapper

The [security wrapper](https://github.com/elastic/kibana/blob/701697cc4a34d07c0508c3bdf01dca6f9d40a636/x-pack/plugins/security/server/saved_objects/secure_saved_objects_client_wrapper.ts) authorizes and audits operations against saved objects.

There are two primary changes to this wrapper:

#### Attaching ACLs

This wrapper will be responsible for attaching an ACL to all private objects before they are created.
It will also allow users to provide their own ACL in order to support the import/export use cases.
legrego marked this conversation as resolved.
Show resolved Hide resolved

Similar to the way we treat `namespaces`, it will not be possible to change an ACL via the `update`/`bulk_update` functions in this first phase. We may consider adding a dedicated function to update the ACL, similar to what we've done for sharing to spaces.

#### Authorization changes

This wrapper will be updated to ensure that access to private objects is only granted to authorized users. A user is authorized to operate on a private saved object if **all of the following** are true:
Step 1) The user is authorized to perform the operation on saved objects of the requested type, within the requested space. (Example: `update` a `user-settings` saved object in the `marketing` space)
Step 2) The user is authorized to access this specific instance of the saved object, as described by that object's ACL. For this first phase, the `acl.owner` is allowed to perform all operations. The only other users who are allowed to access this object are administrators (see [unresolved question 2](#82-authorization-for-private-objects))

Step 1 of this authorization check is the same check we perform today for all existing saved object types. Step 2 is a new authorization check, and **introduces additional overhead and complexity**. We explore the logic for this step in more detail later in this RFC.

![High-level authorization model for private objects](../images/ols_phase_1_auth.png)

## 3.2 Saved Objects API

OLS Phase 1 does not introduce any new APIs, but rather augments the existing Saved Object APIs.

APIs which return saved objects are augmented to include the top-level `acl` property when it exists. This includes the `export` API.

APIs that create saved objects are augmented to accept an `acl` property. This includes the `import` API.

### `get` / `bulk_get`

The security wrapper will ensure the user is authorized to access private objects before returning them to the consumer.

#### Performance considerations
None. The retrieved object contains all of the necessary information to authorize the current user, with no additional round trips to Elasticsearch.

### `create` / `bulk_create`

The security wrapper will ensure that an ACL is attached to all private objects.

If the caller has requested to overwrite existing `private` objects, then the security wrapper must ensure that the user is authorized to do so.

#### Performance considerations
When overwriting existing objects, the security wrapper must first retrieve all of the existing `private` objects to ensure that the user is authorized. This requires another round-trip to `get`/`bulk-get` all `private` objects so we can authorize the operation.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the SOR, we already do pre-flight requests for multi-namespace object types when creating, updating, or deleting objects, because we need to ensure that the object exists in the context of the current space.

Rather than adding an additional pre-flight request to the Security wrapper, perhaps we should consider exposing the existing pre-flight requests from the SOR to its wrappers? Maybe allow the wrapper to pass in a handler/callback for those requests. Maybe that's an optimization we can make in a later phase, but I wanted to put it forward for consideration.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for calling this out, I meant to explain my thinking here. I agree that an additional pre-flight request from the security wrapper is less than ideal, but I'm hesitant to over-engineer an optimal solution in this first phase, since I don't expect that we'll make heavy use of private multi-space objects in the near future.

Refresh my memory: why do the current set of pre-flight namespace checks live in the repository? I want to make sure I'm not overlooking a scenario where the repository should also be validating the accessControl.

Copy link
Contributor

@jportner jportner Mar 10, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When you take an action on an object, the repository has to retrieve it and make sure the object exists in the current space context. This was not needed when we had single-namespace objects (whose raw document IDs contained the namespace), but now that we have multi-namespace objects we need to do that preflight check.

At the moment the preflight check only occurs on multi-namespace object types, which is rare, but in the future most object types will be multi-namespace...

Edit: agree that we don't need to necessarily over-engineer something for Phase 1, but I also think we should plan ahead for the future when the performance hit will be more egregious.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry I should have been clearer: why does the repository have to perform the check, as opposed to say, the spaces wrapper?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because if the Spaces plugin is disabled, the repository should still behave this way (e.g., you should only be able to interact with objects that were in the default space before the Spaces plugin was disabled)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems like this auditing limitation might come up in a couple of other places. Do we have the same problem when:

  • updating a namespaceType: 'multiple' saved objects that exists, but not in the current namespace?
  • getting or updating a saved object that doesn't exist?
  • performing an operation which doesn't succeed because of a network error?

Yes, these situations are all possible today. We "mitigate" this by explaining that the audit log records an outcome of unknown for these scenarios. If you want a definitive answer, then you have to correlate Kibana audit logs with the corresponding Elasticsearch audit logs to be sure that the request actually completed. We can definitively tell you when the request has failed due to authorization checks in the wrapper, but beyond that the audit log will effectively tell the auditor that the user was authorized to perform this action, but we don't know if it completed successfully or not.

I'm quite nervous about the complexity of saved objects as a whole. My impression is that there's only a handful of people that somewhat grasp how it all fits together (and I find myself constantly learning some new behaviour like audit logging limitations). This implementation complexity also comes with a very complex set of tests where the amount of permutations keep steadily growing. So anything that could lower complexity feels worthwhile to explore.

No disagreement here on any of this!

We can always decide to adopt some technical debt / complexity in the short term, but I think a large part of the value of an RFC is starting to shape a more concrete long term technical vision so that if we adopt additional complexity we have some fleshed out ideas for how we can pay it down.

I can spend some time on this if that'd be helpful. Long-term, it seems like the structure of the saved objects service may change somewhat drastically (i.e. potentially deprecating the SOC and generic APIs). I know we've had some long-form discussions, but do we have anything more concise that explains what we're targeting? We probably do, I just can't find it 🙂.
If I know what we're targeting, then I can try to tailor the approach to match that vision.
If not, I can enumerate these other ideas with the assumption that the current architecture will remain largely in place, and then we can figure out what that looks like in the evolved architecture.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do we have anything more concise that explains what we're targeting

We need another RFC 🙈

I'm working on writing down a longer term strategy in 7.13. I think the most impactful change for auditing might be deprecating the saved objects client and generic API's in favour of plugins building custom HTTP API's. Previously auditing had to happen on the saved objects level, but if we're adding another layer into our architecture is that still the best place?

Maybe as a first step we can start by thinking through the ideal end state of audit logging and work back from that. This might be better suited to a sync discussion, but some questions we could explore:

  • If all data access happened through HTTP API's which either responded with 401, error or success could the URL and status code be an adequate replacement for generic saved_object_create/get/find/delete events?
  • there seems to be two classes of audit logs: generic saved_object_* events and events that are specific to the alerting domain, do we anticipate wanting to add more domain specific audit events?
  • from an auditing perspective what do we consider as interesting security events that needs to be logged. If a plugin has a hidden saved object exposed through an HTTP API and rejects a request because of some domain specific business rule, should this create an event? E.g. "only users with super-powers are allowed to break through steel doors"

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe as a first step we can start by thinking through the ideal end state of audit logging and work back from that. This might be better suited to a sync discussion, but some questions we could explore:

I forked this off into a shared doc to continue the discussion in a more sane medium than GH comment threads

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rudolf I started describing some alternate approaches in 682f6c8 (#93115)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Of the alternatives, I'm currently favoring 5.2.2 - Pluggable authorization, but not necessarily coupled to this first phase of OLS. I think this model could work within the context of our "Persistence layer"/"Saved objects are data" discussions.

I'm not opposed to reevaluating if our timelines end up converging, though


This overhead does not impact overwriting "public" objects. We only need to retrieve objects that are registered as `private`. As such, we do not expect any meaningful performance hit initially, but this will grow over time as the feature is used.

### `update` / `bulk_update`

The security wrapper will ensure that the user is authorized to update all existing `private` objects. It will also ensure that an ACL is not provided, as updates to the ACL are not permitted via `update`/`bulk_update`.

#### Performance considerations
Similar to the "create / override" scenario above, the security wrapper must first retrieve all of the existing `private` objects to ensure that the user is authorized. This requires another round-trip to `get`/`bulk-get` all `private` objects so we can authorize the operation.

This overhead does not impact updating "public" objects. We only need to retrieve objects that are registered as `private`. As such, we do not expect any meaningful performance hit initially, but this will grow over time as the feature is used.

### `delete`

The security wrapper will first retrieve the requested `private` object to ensure the user is authorized.

#### Performance considerations
The security wrapper must first retrieve the existing `private` object to ensure that the user is authorized. This requires another round-trip to `get` the `private` object so we can authorize the operation.

This overhead does not impact deleting "public" objects. We only need to retrieve objects that are registered as `private`. As such, we do not expect any meaningful performance hit initially, but this will grow over time as the feature is used.


### `find`
The security wrapper will supply or augment a [KQL `filter`](https://github.com/elastic/kibana/blob/701697cc4a34d07c0508c3bdf01dca6f9d40a636/src/core/server/saved_objects/types.ts#L118) which describes the objects the current user is authorized to see.

```ts
// Sample KQL filter
const filterClauses = typesToFind.reduce((acc, type) => {
if (this.typeRegistry.isPrivate(type)) {
return [
...acc,
// note: this relies on specific behavior of the SO service's `filter_utils`,
// which automatically wraps this in an `and` node to ensure the type is accounted for.
// we have added additional safeguards there, and functional tests will ensure that changes
// to this logic will not accidentally alter our authorization model.

// This is equivalent to writing the following, if this syntax was allowed by the SO `filter` option:
// esKuery.nodeTypes.function.buildNode('and', [
// esKuery.nodeTypes.function.buildNode('is', `acl.owner`, this.getOwner()),
// esKuery.nodeTypes.function.buildNode('is', `type`, type),
// ])
esKuery.nodeTypes.function.buildNode('is', `${type}.acl.owner`, this.getOwner()),
];
}
return acc;
}, []);

const privateObjectsFilter =
filterClauses.length > 0 ? esKuery.nodeTypes.function.buildNode('or', filterClauses) : null;
```

legrego marked this conversation as resolved.
Show resolved Hide resolved
#### Performance considerations
We are sending a more complex query to Elasticsearch for any find request which requests a `private` saved object. This has the potential to hurt query performance, but at this point it hasn't been quantified.

Since we are only requesting saved objects that the user is authorized to see, there is no additional overhead for Kibana once Elasticsearch has returned the results of the query.


## 3.3 Behavior with various plugin configurations
Kibana can run with and without security enabled. When security is disabled,
`private` saved objects will be accessible to all users.

| **Plugin Configuration** | Security | Security & Spaces | Spaces |
| ---- | ------ | ------ | --- |
|| ✅ Enforced | ✅ Enforced | 🚫 Not enforced: objects will be accessible to all

### Alternative
If this behavior is not desired, we can prevent `private` saved objects from being accessed whenever security is disabled.

See [unresolved question 3](#83-behavior-when-security-is-disabled)

# 4. Drawbacks

As outlined above, this approach introduces additional overhead to many of the saved object APIs. We minimize this by denoting which saved object types require this additional authorization.

This first phase also does not allow a public object to become private. Search sessions may migrate to OLS in the future, but this will likely be a coordinated effort with Elasticsearch, due to the differing ownership models between OLS and async searches.

# 5. Alternatives

OLS can be thought of as a Kibana-specific implementation of [Document level security](https://www.elastic.co/guide/en/elasticsearch/reference/current/document-level-security.html) ("DLS"). As such, we could consider enhancing the existing DLS feature to fit our needs. This would involve considerable work from the Elasticsearch security team before we could consider this, and may not scale to subsequent phases of OLS.
legrego marked this conversation as resolved.
Show resolved Hide resolved

# 6. Adoption strategy

Adoption for net-new features is hopefully straightforward. Like most saved object features, the saved objects service will transparently handle all authorization and auditing of these objects, so long as they are properly registered.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thinking through our security model, we would only be able to enforce security if Elasticsearch is behind a firewall preventing users from connecting directly and users don't have access to the developer console. Since the same is true of spaces is this something users are familiar with, I couldn't find anything in our documentation?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We expect that a vast majority of Kibana users will not have direct access to the .kibana index. We have a note in the breaking changes doc which implies this, but we don't do a great job of documenting this today 🙈.


Adoption for existing features (public saved object types) is not addressed in this first phase.

# 7. How we teach this

Updates to the saved object service's documentation to describe the different `classification`s would be required. Like other saved object security controls, we want to ensure that engineers understand that this only "works" when the security wrapper is applied. Creating a bespoke instance of the saved objects client, or using the raw repository will intentionally bypass these authorization checks.

# 8. Unresolved questions

## 8.1 `acl.owner`

The `acl.owner` property will uniquely identify the owner of each `private` saved object. We are still iterating with the Elasticsearch security team on what this value will ultimately look like. It is highly likely that this will not be a human-readable piece of text, but rather a GUID-style identifier.
legrego marked this conversation as resolved.
Show resolved Hide resolved

## 8.2 Authorization for private objects

The user identified by `acl.owner` will be authorized for all operations against that instance, provided they pass the existing type/space/action authorization checks.

In addition to the object owner, we also need to allow administrators to manage these saved objects. This is beneficial if they need to perform a bulk import/export of private objects, or if they wish to remove private objects from users that no longer exist. The open question is: **who counts as an administrator?**

We have historically used the `Saved Objects Management` feature for these administrative tasks. This feature grants access to all saved objects, even if you're not authorized to access the "owning" application. Do we consider this privilege sufficient to see and potentially manipulate private saved objects?
legrego marked this conversation as resolved.
Show resolved Hide resolved

## 8.3 Behavior when security is disabled
When security is disabled, should `private` saved objects still be accessible via the Saved Objects Client?
legrego marked this conversation as resolved.
Show resolved Hide resolved