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

tvernum
Copy link
Contributor

@tvernum tvernum 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

Adds
 - CRUD actions for defining application level privileges
 - application privileges in role definitions
 - application privileges in the has_privileges API
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-security

@tvernum
Copy link
Contributor Author

tvernum commented Apr 26, 2018

@kobelb This is ready for you guys to test out and offer feedback.
I'm afraid there's no docs yet, but I'll give you some pointers in case you want to play with it before the docs are ready.

For REST Endpoints see

To see the usage of those APIs check

@kobelb
Copy link
Contributor

kobelb 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
Copy link
Contributor Author

tvernum 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
Copy link
Contributor

kobelb 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
Copy link
Contributor

kobelb 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
Copy link
Contributor

kobelb 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
Copy link
Contributor Author

tvernum 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
Copy link
Contributor Author

tvernum 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
      }
    }
  }
}

Copy link
Member

@jaymode jaymode left a comment

Choose a reason for hiding this comment

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

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);
Copy link
Member

Choose a reason for hiding this comment

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

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) {
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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);
Copy link
Member

Choose a reason for hiding this comment

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

lets wrap in unmodifiable set here

}

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

Choose a reason for hiding this comment

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

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);
Copy link
Member

Choose a reason for hiding this comment

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

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());
Copy link
Member

Choose a reason for hiding this comment

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

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);
Copy link
Member

Choose a reason for hiding this comment

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

this feels like a trace level log to me

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

public boolean grants(ApplicationPrivilege other, String resource) {
Copy link
Member

Choose a reason for hiding this comment

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

Can you add javadocs?

}

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

Choose a reason for hiding this comment

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

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,
Copy link
Member

Choose a reason for hiding this comment

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

do we need this constructor?

@kobelb
Copy link
Contributor

kobelb 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
Copy link
Contributor Author

tvernum 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
Copy link
Contributor Author

tvernum 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
Copy link

jinmu03 commented May 4, 2018

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

@kobelb
Copy link
Contributor

kobelb 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 tvernum changed the base branch from master to security-app-privs May 28, 2018 03:48
@tvernum
Copy link
Contributor Author

tvernum 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
Copy link
Contributor

kobelb 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.

Copy link
Member

@jaymode jaymode left a comment

Choose a reason for hiding this comment

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

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}
Copy link
Member

Choose a reason for hiding this comment

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

s/list/collection

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

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

Choose a reason for hiding this comment

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

lets put this into ESTestCase with javadocs and remove the duplication

@tvernum

This comment has been minimized.

@tvernum
Copy link
Contributor Author

tvernum 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.

@tvernum tvernum merged commit 03e5e72 into elastic:security-app-privs Jun 7, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
>feature :Security/Authorization Roles, Privileges, DLS/FLS, RBAC/ABAC
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants