Support builtin privileges in get privileges API #42134

tvernum · 2019-05-14T04:43:12Z

Adds "_cluster" and "_index" endpoints to the get privileges API so
that builtin index and cluster privileges can be retrieved via the
Rest API

Resolves: #29771

Adds "_cluster" and "_index" endpoints to the get privileges API so that builtin index and cluster privileges can be retrieved via the Rest API Resolves: elastic#29771

elasticmachine · 2019-05-14T04:43:13Z

Pinging @elastic/es-security

tvernum · 2019-05-14T05:03:03Z

A few notes on this:

I'm raising this as-is, without doc or HLRC changes (other than fixing breakages), so we can discuss the direction I've taken. I will update the client & docs when we've reached agreement.
This re-uses the "Get Privileges API" to get cluster + index privileges. It was always my intention when I wrote that API to add cluster + index support to it (hence calling it "get privileges" not "get application privileges") but I didn't really think through the design well enough, so now it's a bit messy.
The existing API is:
- GET /_security/privilege/ to get all application privileges
- GET /_security/privilege/{application} to get app privileges for a named application
- GET /_security/privilege/{application}/{privileges} to get a subset of privileges for a named application
With this change (in v8.0), it would be:
- GET /_security/privilege/ to get all privileges (cluster, index, application)
- GET /_security/privilege/_cluster to get cluster privileges
- GET /_security/privilege/_index to get index privileges
- GET /_security/privilege/_application to get all application privileges
- GET /_security/privilege/{application} to get app privileges for a named application
- GET /_security/privilege/{application}/{privileges} to get a subset of privileges for a named application

The response output will be the same format in all cases (but we don't output empty fields)

{
  "_cluster" : [ "all", "monitor", "etc" ],
  "_index" : [ "all", "read", "etc" ],
  "app01": {
        "priv01": {
             "application": "app01",
             "name": "priv01",
             "actions" : [] ,
             "metadata" : {}
        }
}

When backported to 7.x it would be changed to be backwards compatible so that GET /_security/privilege/ behaves like GET /_security/privilege/_application. Otherwise the "_cluster" and "_index" response fields would be breaking. In 7.x if you want cluster/index privileges, then you need to call those endpoints explicitly.
The _ (_cluster etc) is because an application name must start with a lowercase letter, so this naming scheme is safe.
The API is arguably wrong, in that application privileges ought to be under the _application endpoint and response field (or just appplication if we no longer needed to worry about name conflicts), but to fix that we need API Versioning.

I'm proposing this approach because I think it creates the correct API at a logical level (a single API for all privileges types, with the ability to filter the output to exactly what you want), even though it has some obvious warts. Then we can fix those warts (at some point in the future) with API versioning.

I'm open to creating distinct APIs for builtin privileges, but I think that we would prefer to have it all in a single API. If I had done the design work more thoroughly the first time, then I think the direction would be obvious.

jkakavas

I'm perfectly fine with the approach given the constraints and lack of API versioning Tim. I took a first look, will you update this PR with docs and HLRC (so we should wait for review) or merge this and tackle the rest in a followup? If the latter, I'll take one more pass tomorrow, but it already looks good to me

jkakavas · 2019-06-05T19:53:04Z

...c/main/java/org/elasticsearch/xpack/core/security/action/privilege/GetPrivilegesRequest.java

 public final class GetPrivilegesRequest extends ActionRequest implements ApplicationPrivilegesRequest {

    @Nullable
    private String application;


Can we change this to scope in 8.0 ?

jkakavas · 2019-06-05T19:55:42Z

...ain/java/org/elasticsearch/xpack/security/rest/action/privilege/RestGetPrivilegesAction.java

    @Override
    public RestChannelConsumer innerPrepareRequest(RestRequest request, NodeClient client) throws IOException {
-        final String application = request.param("application");
+        final String scope = request.param("application");


Can we change the parameter name to scope in 8.0.0 and add a small comment here to explain why we map application to a scope when backporting in 7.x ?

jkakavas · 2019-06-05T19:58:38Z

...ain/java/org/elasticsearch/xpack/security/rest/action/privilege/RestGetPrivilegesAction.java

+        // Application names cannot start with `_`, so we use this for built-in names
+        if ("_cluster".equals(scope)) {
+            requestBuilder.privilegeTypes(GetPrivilegesRequest.PrivilegeType.CLUSTER);
+        } else if ("_index".equals(scope)) {


suggestion: extract _cluster , _index , _application to class vars ?

tvernum · 2019-06-06T08:01:32Z

Thanks @jkakavas I'll wait for feedback from @albertzaharovits and then add add HLRC + docs if he's happy to move forward.

albertzaharovits · 2019-06-07T08:26:53Z

@elasticmachine update branch (I get Eclipse proj setup failures)

albertzaharovits · 2019-06-09T09:07:10Z

...c/main/java/org/elasticsearch/xpack/core/security/action/privilege/GetPrivilegesRequest.java

+    }
+
+    public Set<PrivilegeType> privilegeTypes() {
+       return Collections.unmodifiableSet(this.includedTypes);


nit: make the field immutable (do this wrapping in readFrom)

albertzaharovits · 2019-06-09T10:15:29Z

I am not very much a fan of this. In all such cases (names on the top level namespace) I would prefer to just deprecate the API and create a new one. I don't like it because the client has to account for "special" application names, with different parsing rules; just think of how you'd parse this in HLRC.

However, I think we have a better option here:

Given that the current API is consistent with PUT /_security/privilege/ and DELETE /_security/privilege/{application}/{privilege}, and that the proposed changes will return constants, which are not consistent with PUT and DELETE, I suggest we maintain the GET /_security/privilege/ variants as they are, but add a "new" API such as GET /_security/builtin_privileges or, less preferred, GET /_security/privilege/_builtin . What do you think?

I think these rather extensive format changes are not worth it just for returning some constants, however convincing your writeup is.

tvernum · 2019-06-18T05:28:32Z

OK, let's take a step back and ask what API we would want if we weren't constrained by the existing API.
Then we can decide if there's a path to that, or if there's a close-enough approximation that we can implement easily.

I think @albertzaharovits is correct in that, actually there's no real use case (or at least no currently identified use case) for retrieving all defined privileges (builtin & application). Kibana's Security Management UI wants to know about builtin privileges, and Applications will want to know about their own privileges, but I would question the use-case of anyone who actually wants to get all the builtin privileges and all the application privileges. There might be a reason for that, but I don't know what it is.
But there is definitely a use case for getting all builtin (cluster & index) privileges in 1 call (Security Management UI).

To my thinking, that means that we would want an endpoint design that:

makes it easy to get cluster & index privileges in 1 call
makes it easy to get application privileges for a named application
could be extended to do more if we decided we needed it.

So, that would be (roughly):

GET /_security/privilege/builtin
GET/POST /_security/privilege/application/{app}
GET/PUT/DELETE /_security/privilege/application/{app}/{privilege}

And then we could add

GET /_security/privilege/ down the track if we wanted an API to get everything (for some unknown reason).

But what we have is:

GET/POST /_security/privilege/
GET/POST /_security/privilege/{app}
GET/PUT/DELETE /_security/privilege/{app}/{privilege}

As a path forward, I'd prefer @albertzaharovits's alternative option of

GET /_security/privilege/_builtin

As it keeps everything under a single namespace and is a reasonable approximation of what I think we ought to have done from the start.

tvernum · 2019-06-18T12:55:48Z

@elasticmachine run elasticsearch-ci/bwc

tvernum · 2019-06-19T10:57:39Z

I've updated this PR based on my most recent proposal.
It is now a new endpoint & a new Action.

It also includes Docs & HLRC changes.

albertzaharovits

This is looking very polished! I have only one objection, which is that the privilege for the new API is to high at manage_security and I would suggest monitor (in the same basket with _ssl/certificates)
The changes entailed by this are small so I'll LGTM it now 👍

albertzaharovits · 2019-06-23T20:40:31Z

x-pack/docs/en/rest-api/security/get-builtin-privileges.asciidoc

+==== Authorization
+
+To use this API, you must have - the `manage_security` cluster privilege
+(or a greater privilege such as `all`)


I think the monitor privilege would be more suitable, what do you think?

albertzaharovits · 2019-06-23T20:43:37Z

...ava/org/elasticsearch/xpack/core/security/action/privilege/GetBuiltinPrivilegesResponse.java

+import java.util.Objects;
+
+/**
+ * Response containing one or more application privileges retrieved from the security index


s/one or more application privileges retrieved from the security index/built-in (cluster/index) privileges

albertzaharovits · 2019-06-23T20:47:23Z

...core/src/main/java/org/elasticsearch/xpack/core/security/authz/store/ReservedRolesStore.java

                        new String[] {
-                            "monitor", "manage_index_templates", MonitoringBulkAction.NAME, "manage_saml", "manage_token", "manage_oidc"
+                            "monitor", "manage_index_templates", MonitoringBulkAction.NAME, "manage_saml", "manage_token", "manage_oidc",
+                            GetBuiltinPrivilegesAction.NAME


If GetBuiltinPrivilegesAction collates under monitor, don't forget to update this.

albertzaharovits · 2019-06-23T20:56:38Z

...a/org/elasticsearch/xpack/security/action/privilege/TransportGetBuiltinPrivilegesAction.java

+import java.util.TreeSet;
+
+/**
+ * Transport action to retrieve one or more application privileges from the security index


built-in cluster/indices privileges

albertzaharovits · 2019-06-23T20:59:50Z

...a/org/elasticsearch/xpack/security/rest/action/privilege/RestGetBuiltinPrivilegesAction.java

+import static org.elasticsearch.rest.RestRequest.Method.GET;
+
+/**
+ * Rest action to retrieve an application privilege from the security index


built-in cluster/index privileges

tvernum · 2019-06-24T05:17:30Z

I have only one objection, which is that the privilege for the new API is to high at manage_security and I would suggest monitor

I had the same thought, but my plan was to raise a separate PR for a read-only security privilege instead.
Do you have a strong view on this being monitor rather than read_security ?

albertzaharovits · 2019-06-24T06:43:37Z

Do you have a strong view on this being monitor rather than read_security ?

@tvernum No preference.

tvernum · 2019-06-28T04:09:30Z

@jkakavas Do you want to review this? I'd like to get it into 7.3 if I can.

jkakavas · 2019-07-01T15:03:03Z

Apologies, I got lost in my backlog. I'm happy with this being merged with the explanation in #42134 (comment) and @albertzaharovits review

tvernum · 2019-07-03T02:50:45Z

@elasticmachine update branch

Adds a new "/_security/privilege/_builtin" endpoint so that builtin index and cluster privileges can be retrieved via the Rest API Backport of: elastic#42134

Adds a new "/_security/privilege/_builtin" endpoint so that builtin index and cluster privileges can be retrieved via the Rest API Backport of: #42134

Support builtin privileges in get privileges API

22287f3

Adds "_cluster" and "_index" endpoints to the get privileges API so that builtin index and cluster privileges can be retrieved via the Rest API Resolves: elastic#29771

tvernum added >enhancement >breaking :Security/Authorization Roles, Privileges, DLS/FLS, RBAC/ABAC v8.0.0 v7.2.0 labels May 14, 2019

tvernum requested review from albertzaharovits and jkakavas May 14, 2019 04:43

tvernum added v7.3.0 and removed v7.2.0 labels Jun 4, 2019

elastic deleted a comment from elasticmachine Jun 4, 2019

Merge branch 'master' into get-builtin-privileges

e4d1e20

jkakavas reviewed Jun 5, 2019

View reviewed changes

albertzaharovits reviewed Jun 9, 2019

View reviewed changes

tvernum added 3 commits June 18, 2019 15:34

Merge branch 'master' into get-builtin-privileges

7cdf466

Move get builtin privileges to new action

4a1bab3

Fix broken API specs

f4cadee

tvernum added 4 commits June 19, 2019 17:34

Add Get Builtin Privileges to HLRC

9cb68d8

Merge branch 'master' into get-builtin-privileges

84ec2be

Document get-builtin-privileges API

7127b36

Allow Kibana user access to builtin privileges

6d6943f

tvernum requested a review from albertzaharovits June 19, 2019 10:57

tvernum requested a review from jkakavas June 19, 2019 10:57

tvernum removed the >breaking label Jun 19, 2019

albertzaharovits approved these changes Jun 23, 2019

View reviewed changes

Merge branch 'master' into get-builtin-privileges

fcac2f8

jkakavas approved these changes Jul 1, 2019

View reviewed changes

elasticmachine and others added 3 commits July 2, 2019 19:50

Merge branch 'master' into get-builtin-privileges

fcffc0c

Fix ActionType from auto branch merge

a29fa38

Update tests to reflect new 'manage_api_key' privilege

44f1946

tvernum merged commit 539a120 into elastic:master Jul 3, 2019

tvernum added the backport pending label Jul 3, 2019

tvernum mentioned this pull request Jul 3, 2019

[Backport 7.x] Support builtin privileges in get privileges API #43901

Merged

tvernum added a commit that referenced this pull request Jul 3, 2019

Support builtin privileges in get privileges API (#43901)

2a8f30e

Adds a new "/_security/privilege/_builtin" endpoint so that builtin index and cluster privileges can be retrieved via the Rest API Backport of: #42134

legrego mentioned this pull request Jul 3, 2019

Roles Management - use privilege list from ES elastic/kibana#40247

Closed

tvernum removed the backport pending label Jul 3, 2019

jakelandis added v8.0.0-alpha1 and removed v8.0.0 labels Jul 26, 2021

Support builtin privileges in get privileges API #42134

Support builtin privileges in get privileges API #42134

Uh oh!

Conversation

tvernum commented May 14, 2019

Uh oh!

elasticmachine commented May 14, 2019

Uh oh!

tvernum commented May 14, 2019

Uh oh!

jkakavas left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tvernum commented Jun 6, 2019

Uh oh!

albertzaharovits commented Jun 7, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

albertzaharovits commented Jun 9, 2019

Uh oh!

tvernum commented Jun 18, 2019

Uh oh!

tvernum commented Jun 18, 2019

Uh oh!

tvernum commented Jun 19, 2019

Uh oh!

albertzaharovits left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tvernum commented Jun 24, 2019

Uh oh!

albertzaharovits commented Jun 24, 2019

Uh oh!

tvernum commented Jun 28, 2019

Uh oh!

jkakavas commented Jul 1, 2019

Uh oh!

tvernum commented Jul 3, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants