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

Introduce Application Privileges with support for Kibana RBAC #32309

Merged
merged 34 commits into from
Jul 24, 2018

Conversation

tvernum
Copy link
Contributor

@tvernum tvernum commented Jul 24, 2018

This commit introduces "Application Privileges" to the X-Pack security model.

Application Privileges are managed within Elasticsearch, and can be tested with the _has_privileges API, but do not grant access to any actions or resources within Elasticsearch.
Their purpose is to allow applications outside of Elasticsearch to represent and store their own privileges model within Elasticsearch roles.

Access to manage application privileges is handled in a new way that grants permission to specific application names only. This lays the foundation for more OLS on cluster privileges, which is implemented by allowing a cluster permission to inspect not just the action being executed, but also the request to which the action is applied.
To support this, a "conditional cluster privilege" is introduced, which is like the existing cluster privilege, except that it has a Predicate over the request as well as over the action name.

Specifically, this adds

  • GET/PUT/DELETE actions for defining application level privileges
  • application privileges in role definitions
  • application privileges in the has_privileges API
  • changes to the cluster permission class to support checking of request objects
  • a new "policy" element on role definition to provide cluster object level security (only for manage application privileges)
  • changes to kibana_user, kibana_dashboard_only_user and kibana_system roles to use and manage application privileges

tvernum and others added 30 commits June 7, 2018 14:56
This commit introduces "Application Privileges" (aka custom privileges) to the X-Pack security model.

Application Privileges are managed within Elasticsearch, and can be tested with the _has_privileges API, but do not grant access to any actions or resources within Elasticsearch.
Their purpose is to allow applications outside of Elasticsearch to represent and store their own privileges model within Elasticsearch roles.

Specifically, this adds
- GET/PUT/DELETE actions for defining application level privileges
- application privileges in role definitions
- application privileges in the has_privileges API
# Conflicts:
#	x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/action/user/TransportHasPrivilegesAction.java
#	x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/action/user/TransportHasPrivilegesActionTests.java
The ApplicationPrivilege class had a dual purpose of defining
application privileges as stored in the security index, and also being
the means by which those privileges were tested against roles.
This made the class difficult to work with - in particular validation
was dependent on which purpose it was being used for.

This commit splits the index storage part into a new
ApplicationPrivilegeDescriptor class
# Conflicts:
#	x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/action/user/TransportHasPrivilegesAction.java
The was a spurious toLowerCase in the privilege check that was left
over from a previous design approach
* Allowing the kibana system role to get/put privileges and roles

* Removing the ability to get/put roles

* Removing unnecessary white-space
# Conflicts:
#	x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authz/AuthorizationServiceTests.java
The serialization methods for `PutRoleRequest` did not handle the
applicationPrivileges array.
Fixes this and added tests
This extends the validation for application names to allow an optional
suffix of "-" (or "_") followed by any number of "filename safe
characters" (excluding '*')

The purpose of this is to support multiple kibana instances against a
single ES cluster where the name of each kibana application is
"kibana-${kibana-index}", assuming some reasonable limits on the
Kibana index name.

The change also retricts the wildcard handling of application names
to only support a trailing wildcard: e.g `*`, `kibana-*`, etc.
# Conflicts:
#	x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/authz/permission/Role.java
#	x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authz/store/CompositeRolesStore.java
This lays the foundation for OLS on cluster privileges as it means
that a cluster permission can be applied not just to actions, but also
the objects being acted upon by the request.
This commit adds a test for the most basic function of the `CompositeRolesStore` which
is to merge 2 simple role descriptors into a single role. There were a lot of tests around
FLS/DLS and custom roles providers, but nothing for the most simple case.
A conditional cluster privilege is like the existing cluster
privilege, except that it has a Predicate over the request as
well as over the action name.
It is the "role descriptor" level representation for the newly
introduced "ConditionalClusterPermission"

This change adds the ConditionalClusterPrivilege interface, and
allows them to be attached to RoleDescriptors, but does not provide
any JSON/XContent support. This means that they cannot be used in the
Roles API, nor are they stored in the security index, but they can be
defined by custom Roles Providers and will be consulted as part of
authorization decisions on Roles.
This includes support in the Roles API and for storing in the security
index.

Traditional, action-name cluster privileges are still described by a cluster: []
element in the JSON, while conditional privileges are described in a policy: {}
element (the name is subject to change).

For the roles API, and builtin roles providers (native + file) the only supported
conditional privilege is ManageApplicationPrivileges represented in JSON as:

"application" : { "manage" : { "applications" : [ "my-app",  "app-*" ] } }

which restricts the use of the Get/Put/Delete Privileges actions to those that act upon the specified application names ("my-app", "app-*")
The kibana_system role can only manage privileges for applications
named "kibana-*".
The default kibana instance will have an application name of
"kibana-.kibana", and other instances will be named similarly but with
the ".kibana" replaced by the name of their kibana index.
@tvernum tvernum added >feature v7.0.0 :Security/Authorization Roles, Privileges, DLS/FLS, RBAC/ABAC v6.4.0 labels Jul 24, 2018
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-security

@tvernum

This comment has been minimized.

@tvernum tvernum changed the title Introduce Application Privileges Introduce Application Privileges with support for Kibana RBAC Jul 24, 2018
kobelb and others added 2 commits July 23, 2018 21:17
* Changed kibaan_user and kibana_dashboard_only_user to use the app privs

* Fixing ReservedRoleTests

* Fixing some style issues with the tests

* Adding the action to the patterns if there are no descriptors

* KibanaUserRoleIntegTests now inherits from NativeRealmIntegTestCase

This causes the test to properly close the security index in the @after
to ensure we aren't leaving the .security index open

* Switching the AuditTrailTests to use monitoring_user

This way the test doesn't transiently load the .security index because
it has application privileges and need to close the index when it's done

* kibana_user can no longer create indexes, or index documents

* Deleting unused imports

* Assigning both index and application privileges

* Fix line length

* Fix bad merge

* No longer adding privilege to actions when it has no actions

* Putting test back how it was
The javadoc and validation for ApplicationPrivileges supports the idea
that a privilege could have no actions. However, in that case every
privilege that had no actions grants every other action-less privilege
within the same action, including the NONE privilege.

This commit makes the following changes:
- It is not possible to PUT a privilege without any actions
- A permission with no actions, never grants another privilege even if
  that privilege is also a zero-action privilege (which, ideally would
  never exist, but can occur through missing privileges or index
  manipulation).
@tvernum
Copy link
Contributor Author

tvernum commented Jul 24, 2018

dpkg: error: dpkg status database is locked by another process

@elasticmachine run sample packaging tests

The rest tests require that any request that supports GET with body
must also support GET with source="..."

This changes the _has_privileges rest action to support the source
parameter as an alternative to reading a body
The "global" field stores cluster privileges that have a richer
privilege model than the traditional "cluster" privileges.

This commit renames the JSON field (in the API and security index)
from "policy" to "global"
@jaymode jaymode merged commit 387c3c7 into master Jul 24, 2018
jaymode pushed a commit that referenced this pull request Jul 24, 2018
This commit introduces "Application Privileges" to the X-Pack security
model.

Application Privileges are managed within Elasticsearch, and can be
tested with the _has_privileges API, but do not grant access to any
actions or resources within Elasticsearch. Their purpose is to allow
applications outside of Elasticsearch to represent and store their own
privileges model within Elasticsearch roles.

Access to manage application privileges is handled in a new way that
grants permission to specific application names only. This lays the
foundation for more OLS on cluster privileges, which is implemented by
allowing a cluster permission to inspect not just the action being
executed, but also the request to which the action is applied.
To support this, a "conditional cluster privilege" is introduced, which
is like the existing cluster privilege, except that it has a Predicate
over the request as well as over the action name.

Specifically, this adds
- GET/PUT/DELETE actions for defining application level privileges
- application privileges in role definitions
- application privileges in the has_privileges API
- changes to the cluster permission class to support checking of request
  objects
- a new "global" element on role definition to provide cluster object
  level security (only for manage application privileges)
- changes to `kibana_user`, `kibana_dashboard_only_user` and
  `kibana_system` roles to use and manage application privileges

Closes #29820
Closes #31559
dnhatn added a commit that referenced this pull request Jul 25, 2018
* 6.x:
  Security: revert to old way of merging automata (#32254)
  Fix a test bug in RangeQueryBuilderTests introduced in the field aliases backport.
  Introduce Application Privileges with support for Kibana RBAC (#32309)
  Undo a debugging change that snuck in during the field aliases merge.
  [test] port linux package packaging tests (#31943)
  Painless: Update More Methods to New Naming Scheme (#32305)
  Tribe: Add error with secure settings copied to tribe (#32298)
  Add V_6_3_3 version constant
  Add ERR to ranking evaluation documentation (#32314)
  [DOCS] Added link to 6.3.2 RNs
  [DOCS] Updates 6.3.2 release notes with PRs from ml-cpp repo (#32334)
  [Kerberos] Add Kerberos authentication support (#32263)
  [ML] Extract persistent task methods from MlMetadata (#32319)
  Backport - Add Snapshots Status API to High Level Rest Client (#32295)
  Make release notes ignore the `>test-failure` label. (#31309)
  [DOCS] Adds release highlights for search for 6.4 (#32095)
  Allow Integ Tests to run in a FIPS-140 JVM (#32316)
  Add support for field aliases to 6.x. (#32184)
  Register ERR metric with NamedXContentRegistry (#32320)
  fixes broken build for third-party-tests (#32315) Relates #31918 / Closes infra/issues/6085
  [DOCS] Rollup Caps API incorrectly mentions GET Jobs API (#32280)
  Rest HL client: Add put watch action (#32026) (#32191)
  Add WeightedAvg metric aggregation (#31037)
  Consistent encoder names (#29492)
  Switch monitoring to new style Requests (#32255)
  specify subdirs of lib, bin, modules in package (#32253)
  Rename ranking evaluation `quality_level` to `metric_score` (#32168)
  Add new permission for JDK11 to load JAAS libraries (#32132)
  Switch x-pack:core to new style Requests (#32252)
  Watcher: Store username on watch execution (#31873)
  Silence SSL reload test that fails on JDK 11
  Painless: Clean up add methods in PainlessLookup (#32258)
  CCE when re-throwing "shard not available" exception in TransportShardMultiGetAction (#32185)
  Fail shard if IndexShard#storeStats runs into an IOException (#32241)
  Fix `range` queries on `_type` field for singe type indices (#31756) (#32161)
  AwaitsFix RecoveryIT#testHistoryUUIDIsGenerated
  Add new fields to monitoring template for Beats state (#32085) (#32273)
  [TEST] improve REST high-level client naming conventions check (#32244)
  Check that client methods match API defined in the REST spec (#31825)
dnhatn added a commit that referenced this pull request Jul 25, 2018
* master:
  Security: revert to old way of merging automata (#32254)
  Networking: Fix test leaking buffer (#32296)
  Undo a debugging change that snuck in during the field aliases merge.
  Painless: Update More Methods to New Naming Scheme (#32305)
  [TEST] Fix assumeFalse -> assumeTrue in SSLReloadIntegTests
  Ingest: Support integer and long hex values in convert (#32213)
  Introduce fips_mode setting and associated checks (#32326)
  Add V_6_3_3 version constant
  [DOCS] Removed extraneous callout number.
  Rest HL client: Add put license action (#32214)
  Add ERR to ranking evaluation documentation (#32314)
  Introduce Application Privileges with support for Kibana RBAC (#32309)
  Build: Shadow x-pack:protocol into x-pack:plugin:core (#32240)
  [Kerberos] Add Kerberos authentication support (#32263)
  [ML] Extract persistent task methods from MlMetadata (#32319)
  Add Restore Snapshot High Level REST API
  Register ERR metric with NamedXContentRegistry (#32320)
  fixes broken build for third-party-tests (#32315)
  Allow Integ Tests to run in a FIPS-140 JVM (#31989)
  [DOCS] Rollup Caps API incorrectly mentions GET Jobs API (#32280)
  awaitsfix testRandomClusterStateUpdates
  [TEST] add version skip to weighted_avg tests
  Consistent encoder names (#29492)
  Add WeightedAvg metric aggregation (#31037)
  Switch monitoring to new style Requests (#32255)
  Rename ranking evaluation `quality_level` to `metric_score` (#32168)
  Fix a test bug around nested aggregations and field aliases. (#32287)
  Add new permission for JDK11 to load JAAS libraries (#32132)
  Silence SSL reload test that fails on JDK 11
  [test] package pre-install java check (#32259)
  specify subdirs of lib, bin, modules in package (#32253)
  Switch x-pack:core to new style Requests (#32252)
  awaitsfix SSLConfigurationReloaderTests
  Painless: Clean up add methods in PainlessLookup (#32258)
  Fail shard if IndexShard#storeStats runs into an IOException (#32241)
  AwaitsFix RecoveryIT#testHistoryUUIDIsGenerated
  Remove unnecessary warning supressions (#32250)
  CCE when re-throwing "shard not available" exception in TransportShardMultiGetAction (#32185)
  Add new fields to monitoring template for Beats state (#32085)
@Mpdreamz
Copy link
Member

This PR introduces:

  • xpack.security.put_privilege.json
  • xpack.security.put_privileges.json

Do we need both variants can we get by with just the plural accepting multiple privileges in one go? This would be the first API that has two different endpoints for the put operation.
If we do need both should we rename xpack.security.put_privileges.json to xpack.security.update_privileges.json in the same vein as indices.update_aliases.json? It would help to distinguish between the two API's.

xpack.security.put_privileges.json

Has the prefix put_* but defines:

    "methods": [ "POST"],

Which makes me feel a rename is in order even more.

xpack.security.put_privilege.json

defines the methods:

    "methods": [ "POST", "PUT" ],

I think this method should only accept PUT or document this as

    "methods": [ "PUT", "POST" ],

The only API documenting POST, PUT is the index API which has a quirk in that not all paths are valid for all methods something the spec currently does not catch.

@jaymode
Copy link
Member

jaymode commented Aug 15, 2018

@kobelb is Kibana using both APIs?

@Mpdreamz does the ordering of PUT/POST in the spec matter? Without knowledge of why the order matters it seems like a nit.

@tvernum when you're available can you chime in?

@jaymode jaymode deleted the security-app-privs branch August 15, 2018 14:42
@kobelb
Copy link
Contributor

kobelb commented Aug 15, 2018

@kobelb is Kibana using both APIs?

Kibana is using POST to create multiple privileges.

@jaymode
Copy link
Member

jaymode commented Aug 15, 2018

After thinking about this and confirming the current usage by Kibana, I raised #32879 to remove the put privilege API and only have the put privileges API that supports both PUT and POST. I labeled this issue for team discussion @elastic/es-security in case others oppose this approach.

@Mpdreamz
Copy link
Member

does the ordering of PUT/POST in the spec matter? Without knowledge of why the order matters it seems like a nit.

Total nit, it doesn't matter in my code generator but it could potentially in others. Just be a shame to break our consistency here. #32879 addresses this though 🎈 🍰

@codebrain
Copy link
Contributor

The documentation links in the REST JSON spec files are reading as TODO.

E.g.

Not a big deal, but with the documentation now available on the site, was there an intention to update the spec?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants