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

Kibana reserved role app privs #32137

Merged

Conversation

kobelb
Copy link
Contributor

@kobelb kobelb commented Jul 17, 2018

Begins to implement #32091, some of the security integration tests are failing as a result. This still needs to be investigated.

@kobelb kobelb added the WIP label Jul 17, 2018
@kobelb kobelb requested a review from jaymode July 17, 2018 17:01
@colings86 colings86 added the :Security/Authorization Roles, Privileges, DLS/FLS, RBAC/ABAC label Jul 18, 2018
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-security

@kobelb
Copy link
Contributor Author

kobelb commented Jul 18, 2018

The KibanaUserRoleIntegTests.testSearchAndMSearch test is failing with the following stacktrace:

 > Throwable #1: java.lang.IllegalArgumentException: unknown application privilege [all]
   >    at org.elasticsearch.xpack.core.security.authz.privilege.ApplicationPrivilege.resolve(ApplicationPrivilege.java:196)
   >    at org.elasticsearch.xpack.core.security.authz.privilege.ApplicationPrivilege.get(ApplicationPrivilege.java:178)
   >    at org.elasticsearch.xpack.security.authz.store.CompositeRolesStore.lambda$buildRoleFromDescriptors$19(CompositeRolesStore.java:326)
   >    at java.base/java.util.HashMap.forEach(HashMap.java:1341)
   >    at org.elasticsearch.xpack.security.authz.store.CompositeRolesStore.lambda$buildRoleFromDescriptors$20(CompositeRolesStore.java:325)
   >    at org.elasticsearch.action.ActionListener$1.onResponse(ActionListener.java:60)
   >    at org.elasticsearch.xpack.security.authz.store.NativePrivilegeStore.lambda$getPrivileges$2(NativePrivilegeStore.java:94)
   >    at org.elasticsearch.action.ActionListener$1.onResponse(ActionListener.java:60)
   >    at org.elasticsearch.xpack.security.authz.store.NativePrivilegeStore$1.onResponse(NativePrivilegeStore.java:147)
   >    at org.elasticsearch.xpack.security.authz.store.NativePrivilegeStore$1.onResponse(NativePrivilegeStore.java:141)
   >    at org.elasticsearch.action.support.ContextPreservingActionListener.onResponse(ContextPreservingActionListener.java:43)
   >    at org.elasticsearch.action.support.TransportAction$1.onResponse(TransportAction.java:66)
   >    at org.elasticsearch.action.support.TransportAction$1.onResponse(TransportAction.java:62)
   >    at org.elasticsearch.action.support.ContextPreservingActionListener.onResponse(ContextPreservingActionListener.java:43)
   >    at org.elasticsearch.action.support.single.shard.TransportSingleShardAction$AsyncSingleAction$2.handleResponse(TransportSingleShardAction.java:260)
   >    at org.elasticsearch.action.support.single.shard.TransportSingleShardAction$AsyncSingleAction$2.handleResponse(TransportSingleShardAction.java:246)
   >    at org.elasticsearch.transport.TransportService$ContextRestoreResponseHandler.handleResponse(TransportService.java:1059)
   >    at org.elasticsearch.transport.TcpTransport$2.doRun(TcpTransport.java:1571)
   >    at org.elasticsearch.common.util.concurrent.AbstractRunnable.run(AbstractRunnable.java:37)
   >    at org.elasticsearch.common.util.concurrent.EsExecutors$1.execute(EsExecutors.java:135)
   >    at org.elasticsearch.transport.TcpTransport.handleResponse(TcpTransport.java:1563)
   >    at org.elasticsearch.transport.TcpTransport.messageReceived(TcpTransport.java:1519)
   >    at org.elasticsearch.transport.TcpTransport.consumeNetworkReads(TcpTransport.java:1328)
   >    at org.elasticsearch.transport.nio.TcpReadWriteHandler.consumeReads(TcpReadWriteHandler.java:42)
   >    at org.elasticsearch.nio.SocketChannelContext.handleReadBytes(SocketChannelContext.java:213)
   >    at org.elasticsearch.xpack.security.transport.nio.SSLChannelContext.read(SSLChannelContext.java:137)
   >    at org.elasticsearch.nio.EventHandler.handleRead(EventHandler.java:119)
   >    at org.elasticsearch.nio.NioSelector.handleRead(NioSelector.java:355)
   >    at org.elasticsearch.nio.NioSelector.processKey(NioSelector.java:216)
   >    at org.elasticsearch.nio.NioSelector.singleLoop(NioSelector.java:144)
   >    at org.elasticsearch.nio.NioSelector.runLoop(NioSelector.java:109)
   >    at java.base/java.lang.Thread.run(Thread.java:844)Throwable #2: java.lang.AssertionError: Shard [.security-6][0] is still locked after 5 sec waiting
   >    at org.elasticsearch.test.InternalTestCluster.assertAfterTest(InternalTestCluster.java:2052)
   >    at org.elasticsearch.test.ESIntegTestCase.afterInternal(ESIntegTestCase.java:592)
   >    at org.elasticsearch.test.ESIntegTestCase.cleanUpCluster(ESIntegTestCase.java:2185)
   >    at jdk.internal.reflect.GeneratedMethodAccessor42.invoke(Unknown Source)
   >    at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
   >    at java.base/java.lang.reflect.Method.invoke(Method.java:564)
   >    at java.base/java.lang.Thread.run(Thread.java:844)

I believe this is occurring because we're creating reserved roles that reference application privileges that won't exist until Kibana starts up and inserts them, so we're explicitly throwing an error when we can't find the ApplicationPrivilegeDescriptor. @tvernum, do you have any recommendation for what we should be doing here?

@jaymode
Copy link
Member

jaymode commented Jul 18, 2018

seems like we might need special app privileges for all that can be resolved without the index

@kobelb
Copy link
Contributor Author

kobelb commented Jul 18, 2018

@jaymode for what it's worth, we're assigning the read privilege before it exists in the .security index as well.

Would it be possible to change the logic to not throw an exception when it doesn't exist in the .security index but just ignore it? Similar to the way that you could assign a user a role that doesn't exist, and it just ignores it?

@kobelb
Copy link
Contributor Author

kobelb commented Jul 18, 2018

I don't really know what I'm doing, but I gave this a shot at 937ec7c and it ostensibly seems to work, but now the tests are failing with:

FAILURE 6.18s | KibanaUserRoleIntegTests.testFieldMappings <<< FAILURES!
   > Throwable #1: java.lang.AssertionError: Shard [.security-6][0] is still locked after 5 sec waiting
   >    at __randomizedtesting.SeedInfo.seed([8C4F568F60C56F8:5C9A46FCFD12D78C]:0)
   >    at org.elasticsearch.test.InternalTestCluster.assertAfterTest(InternalTestCluster.java:2052)
   >    at org.elasticsearch.test.ESIntegTestCase.afterInternal(ESIntegTestCase.java:592)
   >    at org.elasticsearch.test.ESIntegTestCase.cleanUpCluster(ESIntegTestCase.java:2185)
   >    at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
   >    at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
   >    at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
   >    at java.base/java.lang.reflect.Method.invoke(Method.java:564)
   >    at java.base/java.lang.Thread.run(Thread.java:844)

so, I assume I did something horribly wrong.

@jaymode
Copy link
Member

jaymode commented Jul 18, 2018

I cannot see anything that was done horribly wrong. @tvernum can you take a look at the issue with roles that have missing application privileges?

kobelb added 3 commits July 19, 2018 06:23
This causes the test to properly close the security index in the @after
to ensure we aren't leaving the .security index open
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
Copy link
Contributor

@tvernum tvernum left a comment

Choose a reason for hiding this comment

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

Let me know if you'd like me to take some of this on.

@@ -193,7 +193,7 @@ private static ApplicationPrivilege resolve(String application, Set<String> name
if (descriptor != null) {
patterns.addAll(descriptor.getActions());
} else {
throw new IllegalArgumentException("unknown application privilege [" + name + "]");
patterns.add(name);
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should just log a warning and skip the name rather than adding it to the patterns.
It's not a pattern, and it isn't supposed to be used as one, but having it there might cause unexpected problems down the track.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

When I changed this to just ignore it, and I created a user with the kibana_user role (without inserting the privileges), I was seeing it match unexpected privilege names:

PUT {{es}}/_xpack/security/user/kb
Content-Type: application/json
Authorization: Basic elastic changeme

{
  "password" : "password",
  "roles" : [ "kibana_user" ],
  "full_name" : "Admin"
}
POST {{es}}/_xpack/security/user/_has_privileges
Content-Type: application/json
Authorization: Basic kb password

{
    "applications":[
        {
            "application":"kibana-.kibana",
            "resources":["*"],
            "privileges":[
              "read",
              "action:foo",
              "all",
              "i-dont-exist"
            ]
        }
    ]
}
HTTP/1.1 200 OK
content-type: application/json; charset=UTF-8
content-encoding: gzip
content-length: 144

{
  "username": "kb",
  "has_all_requested": false,
  "cluster": {},
  "index": {},
  "application": {
    "kibana-.kibana": {
      "*": {
        "all": true,
        "read": true,
        "action:foo": false,
        "i-dont-exist": true
      }
    }
  }
}

If you have any available bandwidth, any assistance would be much appreciated.

Copy link
Contributor

@tvernum tvernum Jul 20, 2018

Choose a reason for hiding this comment

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

I think we just need something like:

if (patterns.isEmpty()) {
  return NONE.apply(application);
} 

We didn't have it before because get has a names.isEmpty check, and it wasn't previously possible for non-empty names to produce non-empty patterns.

Or maybe it was, if the named privileges had no actions in them. That sounds like it was probably always a bug...

Copy link
Contributor

Choose a reason for hiding this comment

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

OK, it turns out that NONE doesn't work (in general) - I'll sort out a fix.

Copy link
Contributor

@tvernum tvernum Jul 20, 2018

Choose a reason for hiding this comment

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

I'm tired and my brain hurts, so I'm going to have to leave this for now.
The fundamental problem is that a Privilege with no actions, implies another privilege with no actions.
So if "all" doesn't exist, and "read" doesn't exist, and we treat them as simply not having actions, then they match each other.

That's logical, but not what we want. I think we want to say that a privilege with no actions matches nothing, except up to this point we've said that actions are optional, which clearly wasn't working, so I think we either need to

  1. say actions are required; or
  2. the privilege name is an implicit action all the time (so "abc" implies "abc", but not "xyz").

I'm not making a decision on that at 6pm on a Friday, but my inclination is to go with (1).

Copy link
Contributor

@tvernum tvernum Jul 23, 2018

Choose a reason for hiding this comment

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

I realised option (2) doesn't work. If the privilege name is always an implicit action, then "all" would not imply "read".

The only way something like that would work is we added the privilege name if it had no actions, but in that case we could just require that the person defining the privileges did that explicitly rather than implicitly.

So, I'm going to make actions required, and if some app doesn't need them, then they just give everything a single action of action:<privilege-name>.

Copy link
Contributor

Choose a reason for hiding this comment

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

I opened a PR for this - #32272

I believe the consequence for this PR is two-fold:

  1. You can stop adding the name to the patterns list if the privilege doesn't exist
  2. If the privileges have not been defined yet, then "read" will not imply "read" because both are meaningless references to non-existing privileges. So _has_privileges will return false for all Kibana privileges until such a time as they have been stored through the API.

I feel like that's the correct response. Treating privileges with the same name as special, even though they haven't been defined is messy. It means "all" implies "all", and "read" implies "read", but "all" does not imply "read". A blanket "these don't exist", so they can't be true is a safer security position.

I would be happy to go back to the previous state where calling _has_privileges with arguments that aren't defined is an error, but I don't want to introduce leniency around special matching of names that aren't defined.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is great, thanks so much @tvernum!

.build() }, null, MetadataUtils.DEFAULT_RESERVED_METADATA))
.put("kibana_user", new RoleDescriptor("kibana_user", null, null, new RoleDescriptor.ApplicationResourcePrivileges[] {
RoleDescriptor.ApplicationResourcePrivileges.builder()
.application("kibana-.kibana").resources("*").privileges("all").build() },
Copy link
Contributor

Choose a reason for hiding this comment

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

The old indices privilege granted .kibana* but this seems to only grant app privileges to the main/default kibana app, is that intentional?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yup, it's intentional, we don't want them to have "direct index access" any longer.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry, I mean old kibana_user allowed all kibana instances, but this only includes the main kibana instance.

@tvernum
Copy link
Contributor

tvernum commented Jul 23, 2018

@kobelb I'm curious about your recent change to continue to have the index-level privileges as well as the application privileges.
That's different to your previous plan - how do we plan for this to work now?

@tvernum
Copy link
Contributor

tvernum commented Jul 23, 2018

Also, I fixed a few checkstyle problems and merged the branch in to try and help get this over the line.

@tvernum
Copy link
Contributor

tvernum commented Jul 23, 2018

This looks pretty good other than

  • not adding the "name" to the actions
  • me not understanding the plan for removing direct index access.

@kobelb
Copy link
Contributor Author

kobelb commented Jul 23, 2018

not adding the "name" to the actions

Done!

me not understanding the plan for removing direct index access.

So, the current plan is to keep the direct index access for the kibana_user and kibana_dashboard_only_user until the 7.0 upgrade, when we are alright incurring some downtime. Kibana's server in 6.4+ will effective ignore the direct index privileges, and use the application privileges, but leaving them there will make the migration have no down-time.

When user's create their own custom roles that are meant to limit access to a specific Space, or when we implement granular application privileges, they shouldn't be assigning direct index access anymore, as this allows the user to either read or write everything in the .kibana index. Instead, we'll be documenting and recommending using the application privileges.

@kobelb kobelb added review and removed WIP labels Jul 23, 2018
@kobelb
Copy link
Contributor Author

kobelb commented Jul 23, 2018

All of the issues should be addressed, if you all could give this a final review @jaymode and @tvernum.

Copy link
Contributor

@tvernum tvernum left a comment

Choose a reason for hiding this comment

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

LGTM

@kobelb kobelb merged commit 1c1240d into elastic:security-app-privs Jul 24, 2018
@kobelb kobelb deleted the kibana-reserved-role-app-privs branch July 24, 2018 01:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:Security/Authorization Roles, Privileges, DLS/FLS, RBAC/ABAC
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants