HLRest: model role and privileges #35128

albertzaharovits · 2018-10-31T12:35:01Z

EDITED:
~~This adds the security put role API.~~
The following "user privileges" entities have been created on the client side:

ApplicationResourcePrivileges
~~ClusterPrivilege~~ This is now a plain String
~~IndexPrivilege~~ This is now a plain String
IndicesPrivileges
~~ManageApplicationsPrivilege~~ Replaced by the general GlobalPrivileges

as well as the aggregating Role entity.

== Note: WIP ~~(no API call, I only have modeled the client side entities)~~
There will be no API call in this PR, but only client side objects.

Relates: #29827

elasticmachine · 2018-10-31T12:35:05Z

Pinging @elastic/es-core-infra

elasticmachine · 2018-10-31T12:35:07Z

Pinging @elastic/es-security

tvernum

I left a number of comments.
Many of them come down to questions about how we want to approach the HLRC, so it's probably worth having @hub-cap respond to the comments even if he doesn't have capacity to read the whole PR.

...in/java/org/elasticsearch/client/security/user/privileges/ApplicationResourcePrivileges.java

tvernum · 2018-11-01T03:01:34Z

...-level/src/main/java/org/elasticsearch/client/security/user/privileges/ClusterPrivilege.java

+            return builtin;
+        }
+        if (false == ALL_CLUSTER_PATTERN.matcher(name).matches()) {
+            throw new IllegalArgumentException("[" + name + "] is not a cluster action privilege.");


I wonder about this - what's our general view on being this strict in the HLRC?
It means if we add a new cluster privilege then it needs to be added to the HLRC (and, theoretically to every other language client if they have this same level of validation), and customers need to upgrade to a new client just to be allowed to use a new string.
If the developers of the COBOL client did this, and needed to be notified every time a new cluster privilege was added, so they could update the client, I'd think they were crazy.

From a separation of concerns point of view, why should the client know/care which strings are understood by the server, and duplicate that logic?
Isn't it OK for the user of the client to get a server-generated error instead of a client-generated error if they use an incorrect string?

The worst case for this will be if an older client cannot even retrieve the list of roles from the cluster because one of the roles contains a privilege string that the client rejects.

Interesting 🤔

It means if we add a new cluster privilege then it needs to be added to the HLRC

There is no need to update the client in this case, unless you need this new privilege that we added on the server. If you need this, then you have to change client code as well, and since you're there why not update the client library as well? (aside that COBOL developers might prefer the low level client instead)

From a separation of concerns point of view, why should the client know/care which strings are understood by the server, and duplicate that logic?

I believe it kinda should do. This is what high-level client means in my head. It enforces some types, but sometimes it enforces some values too.
Tying on a previous discussion, why should we validate for null values on the client side, at all? Because having a null username or password for example, would be invalidated on the server.

I think this particular example is vulnerable to this type of argument, because the type and value mingle. What if instead of the formatted strings, we would've had a naming scheme based on Enums? Would we be duplicating the Enums on the client or would we be using Strings?

The worst case for this will be if an older client cannot even retrieve the list of roles from the cluster because one of the roles contains a privilege string that the client rejects.

I agree, this is the worst case. But even if the client would be able to deserialize they would not be useful, because the client cannot possibly understand them. There is no solution to infinite forward compatibility in the high level client. You can always stick with the low level client.

The alternative we all have in our heads is using Strings. It's the alternative low level client uses.

If this is something that should be enforced like this, we should turn it into an enum instead.

I have made ClusterPrivilege and IndexPrivilege strings. The previous classes did not hold any other information besides the string value and there was no reason to validate the format of the privilege value on the client.

Being able to construct a privilege on the client side is a higher client's business.

For what it's worth, I think there's friendliness in defining constants for the Strings for that developers can use IndexPrivilege.READ, but I don't think the client needs to enforce that you only use the enumerated values.

Especially since the server doesn't even care about that...

PUT _xpack/security/role/test1 { "cluster": [ "foobar" ] } {"role":{"created":true}} GET _xpack/security/role/test1 {"test1":{ "cluster":["foobar"], "indices":[], "applications":[], "run_as":[], "metadata":{}, "transient_metadata":{"enabled":true} }}

wow, this is news to me. It will throw when trying to build the role out of the role descriptors (most def - looked at the code, but did not test it), but this is unexpected... TIL
Thank you for pointing this out, to me!

...main/java/org/elasticsearch/client/security/user/privileges/ManageApplicationsPrivilege.java

...nt/rest-high-level/src/main/java/org/elasticsearch/client/security/user/privileges/Role.java

...main/java/org/elasticsearch/client/security/user/privileges/ManageApplicationsPrivilege.java

...nt/rest-high-level/src/main/java/org/elasticsearch/client/security/user/privileges/Role.java

albertzaharovits · 2018-11-08T02:01:21Z

I've pitched this problem of future proofing the high-level client, wrt response changes, to the core-infra folk. To be clear, the problem is that a client which ignores fields that it does not understand, while parsing a response, would create an incomplete client object, that if used in a put request, will subtly update the object on the server side (setting the ignored fields from before to null).

There is no solution at the moment, but it is expected that API versioning will solve this problem since both the request as well as the response will be versioned.
Right now, we're happy with ignoring unknown fields because that's not usually a problem (think search results). However, if we talk about inadvertently changing privileges or roles, that can have pernicious consequences. Therefore, I have made the change to not ignore unknown fields for privilege and role objects, and the client will break if calling new API versions. 🙉

As the privileges are modeled right now, the client will not break for new Global Application privileges, that might be added by kibana. It will however break if we create new global privileges, outside of the application namespace (eg manage template privilege).

CC @tvernum @nik9000

tvernum

I like where this has gone. I left a few comments. It's probably at the point where we could merge it and then iterate of a couple of things later if you'd like.

tvernum · 2018-11-08T09:44:20Z

...-level/src/main/java/org/elasticsearch/client/security/user/privileges/GlobalPrivileges.java

+ * privileges over applications. {@code ApplicationResourcePrivileges} model
+ * application privileges over resources. This models user privileges over
+ * applications. Every client is responsible to manage the applications as well
+ * as the privileges for them.


This is kind of true for now, but not really the intent.

"global" privileges are just cluster privileges that
(a) Have some sort of "parameters" (that is, they aren't just a plain string). So my expectation is that if we werer to implement it "manage_watcher" is a cluster privilege but { "manage_watches_by_name": ["albert-*"] } is a global privilege.
(a) In order to accomodate the above, have a custom JSON format.

The only ones that exist "manage applications", so the Javadoc is true, but I don't think it explains it to someone trying to use the API.

Yes, I tried to keep the balance between abstract and pragmatic documentation.
I leaned towards documenting the pragmatic, present behavior. I know it discounts the technical marvel behind, but if I had used the scope, category or conditional jargon, it was my impression that javadocs will lose their benefit to the user. We can always update the javadoc when we stretch the abstraction later on. Yet, the counter argument is that we should aim for more stability in the docs as well, because these are client classes.

Noted. I'll take them to one more round of polish.

Yeah, my thought was that the Javadoc is probably there to explain to users the point of "GlobalPrivileges".
The short answer is, in this release they probably don't care (as long as they handle round-trip GET + PUT correct) because it's just "application privilege management", but in the future it will do more.

tvernum · 2018-11-08T09:53:53Z

...-level/src/main/java/org/elasticsearch/client/security/user/privileges/GlobalPrivileges.java

+    }
+
+    public Set<? extends GlobalScopedPrivilege> getPrivileges() {
+        return applicationPrivileges;


I'm not sure this is quite right, but it might be - I guess it depends where you think you'd take it next.
It's obviously tricky since we're not 100% sure where we will go with "global" privileges, but let's assume the next thing we do is restict the ability to modify some cluster settings.

To do that we might end up with JSON like:

"global" : { "application": { "manage": { "applications": [ "*" ] } }, "settings" : { "update" : { "names": [ "logger.*" ] } } }

In that case, what does getPrivileges return? Is it a set like:

[ ManageApplicationPrivilege("*") , UpdateClusterSettingsPrivilege("logger.*") ]

Or do we have separate getApplicationPrivileges() and getSettingsPrivileges() (probably not).

Or should this return a Map<String, Set<GlobalScopedPrivilege>> where the keys are "application" and "settings" ?

If we think we only need a single Set<...> getPrivileges() method, then maybe we only need 1 field too, and it should simply be privileges not applicationPrivileges.

I don't have a strong opinion (really!), but I'd like us to have an idea of how to evolve this when we add more global privileges.

Thank you for the comprehensive explanation!

Really my though was that there's no way we can future proof this, the object will have to evolve in lock step with the JSON response (the parser is static, we cannot ignore unknwon fields, etc..). So I didn't gave it much though about how this "evolution" would look like. It would be breaking anyway, so who cares, design it for today.

But! I got a small idea from your Map<String, Set<GlobalScopedPrivilege>>, stay tuned!

I've decided to use a Set. I think a map exposes internal implementation, the client should not be informed that we broken them down by category.

...l/src/main/java/org/elasticsearch/client/security/user/privileges/GlobalScopedPrivilege.java

tvernum · 2018-11-08T10:00:55Z

...l/src/main/java/org/elasticsearch/client/security/user/privileges/GlobalScopedPrivilege.java

+public class GlobalScopedPrivilege {
+
+    private final String scope;
+    private final Map<String, Object> privilege;


I wonder if it would help to include the "category" ("application", and in theory "settings") in this class too.
We can wait add it in the future if we need it, but I think we will, depending on how GlobalPrivileges ends up looking.

Hmmm, I cannot think of a reason why not to do that 👍
More so, since I am now leaning towards having a set in GlobalPrivileges because a map would be exposing unnecessary implementation details.

This was a very good idea. I know this is how they are modeled on the server, but I didn't give it much interest at that time.

...level/src/main/java/org/elasticsearch/client/security/user/privileges/IndicesPrivileges.java

tvernum · 2018-11-08T10:09:27Z

.../main/java/org/elasticsearch/client/security/user/privileges/ManageApplicationPrivilege.java

+
+    @SuppressWarnings("unchecked")
+    public Set<String> getApplications() {
+        return (Set<String>)getRaw().get(APPLICATIONS);


👍 I really like how you ended up handling this.
( I suspect it might need a little tweaking when we get to parsing the JSON )

( I suspect it might need a little tweaking when we get to parsing the JSON )

Yeah, we parse the base class, not the concrete ManageApplicationPrivilege.
I think we'll need a factory to actually instantiate a concrete class. I will mock one up.

I've changed my mind. I'm not really sure how we'll use this class. I'll defer it to the follow-up PR (put role).

...nt/rest-high-level/src/main/java/org/elasticsearch/client/security/user/privileges/Role.java

tvernum · 2018-11-08T10:13:37Z

...nt/rest-high-level/src/main/java/org/elasticsearch/client/security/user/privileges/Role.java

+    private final Set<IndicesPrivileges> indicesPrivileges;
+    private final Set<ApplicationResourcePrivileges> applicationResourcePrivileges;
+    private final Set<String> runAs;
+    private final Map<String, Object> metadata;


I think that's a problem. I wonder if we should just make the server return a name?
We'll need to do something - I think a role really needs to know its name,

albertzaharovits · 2018-11-08T20:24:09Z

@tvernum I think it's worthy now !? Can you please check it again and say the word and I'll push the merge button 🔴

tvernum

I left a couple of comments. I think the Javadoc is misleading, but you can fix that up easily enough.
The other is just a suggestion.

tvernum · 2018-11-09T06:04:09Z

...rc/main/java/org/elasticsearch/client/security/user/privileges/GlobalOperationPrivilege.java

- * outside of the Elasticsearch jurisdiction.
+ * Represents generic global cluster privileges that can be scoped by categories
+ * and then by operations. The privilege definition, as well as the operation
+ * identifier, are outside of the Elasticsearch jurisdiction. Categories are


I don't quite follow (or perhaps don't agree) with your point about the definition being "outside of the Elasticsearch jurisdiction". Elasticsearch definitely controls & evaluates these.

I was plainly wrong in my understanding.
This is now:

Represents generic global cluster privileges that can be scoped by categories and then further by operations. The privilege's syntactic and semantic meaning is specific to each category and operation; there is no general definition template. It is not permitted to define different privileges under the same category and operation.

tvernum · 2018-11-09T06:06:46Z

...-level/src/main/java/org/elasticsearch/client/security/user/privileges/GlobalPrivileges.java

- * as the privileges for them.
+ * generic cluster privileges. These privileges are organized into categories.
+ * Elasticsearch defines the set of categories. Under each category there are
+ * operations that are under the clients jurisdiction. The privilege is hence


This is not true.
The definition

"application": { "manage": { "applications": [ "kibana-*" ] } }

Instructs Elasticsearch that this role is allowed to manage application privileges under the application names "kibana-*".
It's not an instruction for Kibana, it's enforced by Elasticsearch.

I was wrong, I got carried astray by too much abstraction.
This is now:

Constructs privileges under a specific {@code category} and for some {@code operation}. The privilege definition is flexible, it is a {@code Map}, and the semantics is bound to the {@code category} and {@code operation}.

tvernum · 2018-11-09T06:09:26Z

...-level/src/main/java/org/elasticsearch/client/security/user/privileges/GlobalPrivileges.java

        }
    }

    @Override
    public XContentBuilder toXContent(XContentBuilder builder, Params params) throws IOException {
+        final Map<String, List<GlobalOperationPrivilege>> privilegesByCategoryMap =
+                this.privileges.stream().collect(Collectors.groupingBy(GlobalOperationPrivilege::getCategory));


Should we just keep this Map around?
We use it in the contructor, and in toXContent, couldn't it just be a field?

I went with your suggestion. At first, I have resisted duplicating information, but since it's immutable and private, I suppose it's OK.

albertzaharovits · 2018-11-09T16:27:49Z

run gradle build tests

albertzaharovits · 2018-11-10T00:04:30Z

run gradle build tests

albertzaharovits · 2018-11-11T10:22:16Z

run gradle build tests

The following "user privileges" objects have been created on the client side: * ApplicationResourcePrivileges * IndicesPrivileges * GlobalOperationPrivilege * GlobalPrivileges as well as the aggregating `Role` entity.

* master: Add docs for CCR stats API (elastic#35439) Fix the names of CCR stats endpoints in usage API (elastic#35438) Switch to default to no build qualifier (elastic#35415) Clarify S3 repository storage class parameter (elastic#35400) SQL: Fix query translation for scripted queries (elastic#35408) [TEST] Instead of ignoring the ccr downgrade to basic license qa test avoid the assertions that check the log files, because that does not work on Windows. The rest of the test is still useful and should work on Windows CI. Upgrade to Joda 2.10.1 (elastic#35410) HLRest: model role and privileges (elastic#35128)

* master: Address handling of OS pretty name on some OS (elastic#35451) HLRC support for getTask (elastic#35166) upgrade to lucene-8.0.0-snapshot-6d9c714052 (elastic#35428) Add docs for CCR stats API (elastic#35439) Fix the names of CCR stats endpoints in usage API (elastic#35438) Switch to default to no build qualifier (elastic#35415) Clarify S3 repository storage class parameter (elastic#35400) SQL: Fix query translation for scripted queries (elastic#35408) [TEST] Instead of ignoring the ccr downgrade to basic license qa test avoid the assertions that check the log files, because that does not work on Windows. The rest of the test is still useful and should work on Windows CI. Upgrade to Joda 2.10.1 (elastic#35410) HLRest: model role and privileges (elastic#35128)

albertzaharovits added 14 commits October 24, 2018 10:15

ClusterPrivilege

706b736

IndexPrivilege

21bc280

Index Privileges

9e174eb

ApplicationResourcePrivilege WIP

64a91e2

Merge branch 'master' into hlrc_put_role

d98232b

IndicesPrivileges is neat!

aabbc5f

Oh my!

9883726

Proper manage app priv

ba34f65

Bare RoleDescriptor

e03a05b

Slightly less bare RoleDescriptor

a7b176b

All Collections are Lists, well..

701f85e

WIP :((

523f108

Fields are Set

ee37904

Role entity

36f7be5

albertzaharovits added >enhancement WIP v7.0.0 :Core/Java High Level REST Client :Security/Authorization Roles, Privileges, DLS/FLS, RBAC/ABAC v6.6.0 labels Oct 31, 2018

albertzaharovits self-assigned this Oct 31, 2018

albertzaharovits requested review from hub-cap, tvernum, jaymode and jkakavas October 31, 2018 12:35

elasticmachine mentioned this pull request Oct 31, 2018

Java high-level REST client completeness for xpack APIs #29827

Closed

98 tasks

tvernum reviewed Nov 1, 2018

View reviewed changes

Merge branch 'master' into hlrc_put_role

395e29c

albertzaharovits added 4 commits November 7, 2018 17:01

ApplicationResourcePrivilegesTests

1d9f874

WIP

d4170f2

More tests

9869124

More docs

3ead50c

tvernum reviewed Nov 8, 2018

View reviewed changes

albertzaharovits added 3 commits November 8, 2018 18:36

WIP

b68a3cf

Merge branch 'master' into hlrc_put_role

769a0e1

Done!

24351b4

tvernum approved these changes Nov 9, 2018

View reviewed changes

albertzaharovits added 3 commits November 9, 2018 11:41

Merge branch 'master' into hlrc_put_role

36d3203

Javadocs and privilegesByCategoryMap as member

20f14f7

Merge branch 'master' into hlrc_put_role

59014c6

albertzaharovits added >non-issue and removed :Security/Authorization Roles, Privileges, DLS/FLS, RBAC/ABAC >enhancement WIP labels Nov 9, 2018

Merge branch 'master' into hlrc_put_role

7fd629a

albertzaharovits merged commit 6050deb into elastic:master Nov 11, 2018

colings86 added v7.0.0-beta1 and removed v7.0.0 labels Feb 7, 2019

HLRest: model role and privileges #35128

HLRest: model role and privileges #35128

Conversation

albertzaharovits commented Oct 31, 2018 • edited Loading

elasticmachine commented Oct 31, 2018

elasticmachine commented Oct 31, 2018

tvernum left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

albertzaharovits Nov 5, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

albertzaharovits commented Nov 8, 2018 • edited Loading

tvernum left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

albertzaharovits Nov 8, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

albertzaharovits commented Nov 8, 2018

tvernum left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

albertzaharovits Nov 9, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

albertzaharovits commented Nov 9, 2018

albertzaharovits commented Nov 10, 2018

albertzaharovits commented Nov 11, 2018

albertzaharovits commented Oct 31, 2018 •

edited

Loading

albertzaharovits Nov 5, 2018 •

edited

Loading

albertzaharovits commented Nov 8, 2018 •

edited

Loading

albertzaharovits Nov 8, 2018 •

edited

Loading

albertzaharovits Nov 9, 2018 •

edited

Loading