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 to Roles #30164

Merged
merged 39 commits into from Jun 7, 2018

Conversation

Projects
None yet
6 participants
@tvernum
Copy link
Contributor

commented Apr 26, 2018

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

Closes: #29820

tvernum added some commits Apr 26, 2018

Support application (custom) privileges
Adds
 - CRUD actions for defining application level privileges
 - application privileges in role definitions
 - application privileges in the has_privileges API
@elasticmachine

This comment has been minimized.

Copy link
Collaborator

commented Apr 26, 2018

@tvernum

This comment has been minimized.

Copy link
Contributor Author

commented Apr 26, 2018

@kobelb

This comment has been minimized.

Copy link
Contributor

commented Apr 26, 2018

This is awesome, thanks so much @tvernum. I just started integration it with our stuff, and I'll let you know how it progresses.

I noticed that users with the superuser role don't intrinsically get all application privileges, would it be possible to change this so they do?

@tvernum

This comment has been minimized.

Copy link
Contributor Author

commented Apr 26, 2018

I noticed that users with the superuser role don't intrinsically get all application privileges, would it be possible to change this so they do?

Thanks for the reminder, I had forgotten about that.

@kobelb

This comment has been minimized.

Copy link
Contributor

commented Apr 27, 2018

Also, in the _has_privileges call, it would seem that we'd want to be using "actions" the key in the application array, as opposed to "privileges", as we're checking to see if they have the specified actions.

@kobelb

This comment has been minimized.

Copy link
Contributor

commented Apr 27, 2018

You might be planning to explain this in the docs, but would you mind explaining the role that the : plays when defining the actions for the custom privileges? I was able to create the following privilege/actions:

{
  "kibana": {
    "read": {
      "application": "kibana",
      "name": "read",
      "actions": [
        "version:7.0.0-alpha1",
        "saved-objects/dashboard/get",
      ],
      "metadata": {}
    }
  }
}

but then when I went to check the _has_privileges with

{
    "applications":[
        {
            "application":"kibana",
            "resources":["*"],
            "privileges":["saved-objects/dashboard/get"]
        }
    ]
}

it was returning false.

I noticed that I probably should be using the action: prefix so that I can always be doing the version check properly, and then everything started to work, but I feel like I'm missing something behind how all these components work together.

@kobelb

This comment has been minimized.

Copy link
Contributor

commented Apr 27, 2018

Apologies for all the questions, does Elasticsearch do some type of caching with the application privileges? I've noticed a few situations where things were behaving weirdly and not authorizing users as I expected where if I restarted Elasticsearch and ran the steps again, it's begin working again. It might just be a "user error" as well.

@tvernum

This comment has been minimized.

Copy link
Contributor Author

commented Apr 30, 2018

it would seem that we'd want to be using "actions" the key in the application array, as opposed to "privileges", as we're checking to see if they have the specified actions.

We could, but I've tried to mimic the cluster and index fields in has_privileges which use privileges.
The "privileges" field accepts either actions or high-level-privilege names. For your purposes it only makes sense to include action names, but the field allows a mix of both.

would you mind explaining the role that the : plays when defining the actions for the custom privileges

There isn't supposed to be anything to explain, and I don't understand why you saw the behaviour you did.
The only rule is that we need some way to distinguish between privilege names and action names, and we do that with / or : (or *, but that's actually playing the part of a wildcard).
You definitely shouldn't see any difference between / and :, except that security generally treats a leading / as indicating that the pattern is a regex, not a simple wildcard.

does Elasticsearch do some type of caching with the application privileges?

There shouldn't be any. There's caching on roles, but I pulled out what I think we the last piece of caching on privielges because it was broken.

@tvernum

This comment has been minimized.

Copy link
Contributor Author

commented Apr 30, 2018

I don't understand why you saw the behaviour you did.

I haven't been able to reproduce this. I suspect it's related to whatever caching issue you saw (whether real or user-error), but if you can still reproduce it let me know.

My test case is:
Privileges

POST /_xpack/security/privilege
{"kibana": {
   "login":{"application":"kibana","name":"login","actions":["action:login","version:6.4.*"]},
   "read":{"application":"kibana","name":"read","actions":["action:login","version:6.4.*","data:read/*","saved-objects/dashboard/get"]}
}}

Role

POST /_xpack/security/role/test
{"applications":[{"application":"kibana","privileges":["read"],"resources":["*"]}]}

User

POST /_xpack/security/user/test
{ "password" : "magic123", "roles" : [ "test" ] }

Check

GET /_xpack/security/user/_has_privileges
{
  "index": [],  
  "applications": [ { "application": "kibana", "resources":["test"], "privileges":["saved-objects/dashboard/get"] } ]
}
# ...
{
  "username" : "test",
  "has_all_requested" : true,
  "cluster" : { },
  "index" : { },
  "application" : {
    "kibana" : {
      "test" : {
        "saved-objects/dashboard/get" : true
      }
    }
  }
}
@jaymode
Copy link
Member

left a comment

I left some initial feedback. I haven't made it through the PR yet but wanted to provide what I have so far.

}

public DeletePrivilegesRequest(String application, String[] privileges) {
application(application);

This comment has been minimized.

Copy link
@jaymode

jaymode Apr 30, 2018

Member

can we use this.application = application and this.privileges = privileges? I find this much easier to follow than going to a method to see that this is all the method does.

this(client, DeletePrivilegesAction.INSTANCE);
}

public DeletePrivilegesRequestBuilder(ElasticsearchClient client, DeletePrivilegesAction action) {

This comment has been minimized.

Copy link
@jaymode

jaymode Apr 30, 2018

Member

I don't think we need this constructor and can just use the one above and call super with a instance of the request

This comment has been minimized.

Copy link
@tvernum

tvernum May 1, 2018

Author Contributor

Unfortunately need it in order to implement DeletePrivilegesAction.newRequestBuilder, but I've made it package protected.

}

public DeletePrivilegesResponse(Collection<String> found) {
this.found = new HashSet<>(found);

This comment has been minimized.

Copy link
@jaymode

jaymode Apr 30, 2018

Member

lets wrap in unmodifiable set here

}

public Set<String> found() {
return Collections.unmodifiableSet(this.found);

This comment has been minimized.

Copy link
@jaymode

jaymode Apr 30, 2018

Member

if we wrap in unmodifiable set in the constructor and serialization, this can just be return found;

public void readFrom(StreamInput in) throws IOException {
super.readFrom(in);
final int size = in.readVInt();
found = new HashSet<>(size);

This comment has been minimized.

Copy link
@jaymode

jaymode Apr 30, 2018

Member

Lets create a local value, add all of the values and then assign found to an unmodifiable set

return new PermissionEntry(k, Automatons.unionAndMinimize(Arrays.asList(existing.resources, patterns)));
}
}));
this.permissions = new ArrayList<>(permissionsByPrivilege.values());

This comment has been minimized.

Copy link
@jaymode

jaymode Apr 30, 2018

Member

make this an unmodifiable list?

public boolean grants(ApplicationPrivilege other, String resource) {
Automaton resourceAutomaton = Automatons.patterns(resource);
final boolean matched = permissions.stream().anyMatch(e -> e.grants(other, resourceAutomaton));
logger.debug("Permission [{}] {} grant [{} , {}]", this, matched ? "does" : "does not", other, resource);

This comment has been minimized.

Copy link
@jaymode

jaymode Apr 30, 2018

Member

this feels like a trace level log to me

this.permissions = new ArrayList<>(permissionsByPrivilege.values());
}

public boolean grants(ApplicationPrivilege other, String resource) {

This comment has been minimized.

Copy link
@jaymode

jaymode Apr 30, 2018

Member

Can you add javadocs?

}

public ApplicationPrivilege(String application, String privilegeName, String... patterns) {
this(application, Sets.newHashSet(privilegeName), patterns, Collections.emptyMap(), true);

This comment has been minimized.

Copy link
@jaymode

jaymode Apr 30, 2018

Member

use Collections.singleton here

this(application, Sets.newHashSet(privilegeName), patterns, Collections.emptyMap(), true);
}

private ApplicationPrivilege(String application, Set<String> name, Collection<String> patterns, Map<String, Object> metadata,

This comment has been minimized.

Copy link
@jaymode

jaymode Apr 30, 2018

Member

do we need this constructor?

@kobelb

This comment has been minimized.

Copy link
Contributor

commented May 3, 2018

@tvernum you were right that it appears to be a caching issue. If I perform the following steps to create the privileges, and a role/user with those privileges, and then update the privileges, the _has_privileges API returns false until I clear the cache for that role:

POST _xpack/security/privilege
{
  "kibana": {
    "read":{
      "application":"kibana",
      "name":"read",
      "actions":[
        "version:7.0.0-alpha1",
        "action:saved-objects/config/get"
      ]
    }
  }
}
POST _xpack/security/role/kibana_rbac_user
{
  "cluster": [],
  "indices": [],
  "applications": [
    {
      "application": "kibana",
      "privileges": [ "read" ],
      "resources": [ "default" ]
    }
  ]
}
PUT _xpack/security/user/brandon
{
  "password" : "password",
  "roles" : [ "kibana_rbac_user" ],
  "full_name" : "Brandon",
  "email" : "someone@gmail.com"
}
POST _xpack/security/user/_has_privileges
{
    "applications":[
        {
            "application":"kibana",
            "resources":["default"],
            "privileges":["action:saved-objects/config/get"]
        }
    ]
}

^^ this returns has_all_requested: true

POST _xpack/security/privilege
{
  "kibana": {
    "read":{
      "application":"kibana",
      "name":"read",
      "actions":[
        "version:7.0.0-alpha1",
        "action:saved-objects/config/get",
        "action:saved-objects/config/search"
      ]
    }
  }
}
POST _xpack/security/user/_has_privileges
{
    "applications":[
        {
            "application":"kibana",
            "resources":["default"],
            "privileges":["action:saved-objects/config/search"]
        }
    ]
}

^^ this returns has_all_requested: false

Is this expected behavior? We're planning on occasionally updating the privileges map, and having to clear the cache of every role that has those privileges is rather cumbersome.

@tvernum

This comment has been minimized.

Copy link
Contributor Author

commented May 4, 2018

Is this expected behavior?

It wasn't something I predicted, but it's consistent with some other caches in security.
If you change role-mappings it doesn't affect users that already cached.

Updates to roles are live because the cached link between Users and Roles is simply by name, so if the role changes in the cache the User will see the new role on next request.

But if you change a role-mapping so that LDAP user "bob" is supposed to now have "kibana_user", that won't be effective until the cache for "bob" is cleared.

We're planning on occasionally updating the privileges map, and having to clear the cache of every role that has those privileges is rather cumbersome.

You can clear the cache for all roles in 1 API call, though obviously that's a pretty blunt instrument.

I'll look at options, but my gut feel is that I'd like to update the clear roles API to be able to accept an application name so you can clear all roles that have "kibana" privileges once you've finished adding/deleting/updating privileges.

@tvernum

This comment has been minimized.

Copy link
Contributor Author

commented May 4, 2018

We're planning on occasionally updating the privileges map,

@kobelb How often does this happen? We can change the clear-cache API to ben more granular, but if the updates are infrequent, I think it would be simpler just to clear the whole cache when you update.

@jinmu03

This comment has been minimized.

Copy link

commented May 4, 2018

@tvernum will a more granular control of the cache have any big impact on the performance of the API?

@kobelb

This comment has been minimized.

Copy link
Contributor

commented May 7, 2018

@tvernum the current plan is to execute the Privileges PUT every time that an instance of Kibana starts up for all the custom privileges to ensure that we have the information in Elasticsearch to perform the authorization using the current privileges.

In an ideal configuration, this would only happen once for every instance of Kibana for a long period of time; however, in an unideal situation or one where new instances were being provisioned frequently, it could happen somewhat frequently. Unfortunately, it depends...

tvernum added some commits May 25, 2018

@tvernum tvernum changed the base branch from master to security-app-privs May 28, 2018

@tvernum

This comment has been minimized.

Copy link
Contributor Author

commented May 28, 2018

@jaymode @albertzaharovits
This is ready for another review.

@albertzaharovits I addressed your comment on the JSON input, but I'm going to hold off on your two
performance related suggestions until after I split the ApplicationPrivilege class.
My plan is to do something like we do for Roles, where the Role class represents the runtime behaviour and the RoleDescriptor is the stored format.

@kobelb

This comment has been minimized.

Copy link
Contributor

commented Jun 1, 2018

@tvernum I just came across an interesting behavior, when I create a privilege with a capital letter in it, then the _has_privilege check always comes back false, but if change it to be all lower case letters then it works correctly.

@jaymode

jaymode approved these changes Jun 4, 2018

Copy link
Member

left a comment

I left a few minor comments, otherwise I am good with merging this to the feature branch and moving forward.

@@ -969,6 +970,16 @@ public void writeList(List<? extends Writeable> list) throws IOException {
}
}

/**
* Writes a list of generic objects via a {@link Writer}

This comment has been minimized.

Copy link
@jaymode

jaymode Jun 4, 2018

Member

s/list/collection

assertThat(copy.privileges(), Matchers.equalTo(original.privileges()));
}

private <T> T[] randomArray(int max, IntFunction<T[]> arrayConstructor, Supplier<T> valueConstructor) {

This comment has been minimized.

Copy link
@jaymode

jaymode Jun 4, 2018

Member

lets put this into ESTestCase with javadocs and remove the duplication

@tvernum

This comment was marked as resolved.

Copy link
Contributor Author

commented Jun 5, 2018

test this please

@tvernum

This comment has been minimized.

Copy link
Contributor Author

commented Jun 6, 2018

when I create a privilege with a capital letter in it, then the _has_privilege check always comes back false

I've tracked down the cause of this. I have a fix for it in a followup PR.

@kobelb kobelb referenced this pull request Jun 6, 2018

Closed

RBAC Phase 1 #19710

@tvernum tvernum merged commit 03e5e72 into elastic:security-app-privs Jun 7, 2018

3 checks passed

CLA Commit author is a member of Elasticsearch
Details
elasticsearch-ci Build finished.
Details
elasticsearch-ci/packaging-sample Build finished.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.