Beginning to use the ES APIs to insert/check privileges

[kobelb]

No description provided.

[legrego]

I'm liking it! Just a couple of comments/questions for you.

[legrego]

Is options.type a required search property? If not, what happens within this._performAuthorizationCheck when a type isn't provided?

[kobelb]

I still need to track down all the usages, the basic features that I've tested provide the options.type, and if they don't it's not going to authorize them

[kobelb]

Have you come across any where we aren't providing this?

[legrego]

We might be able to remove attributes and options now. It doesn't look like we'll need them.

[kobelb]

Awesome, I'll remove it for the time being and we can add it back if we need to.

[legrego]

Do you have any thoughts yet on how we'd make this space aware? We don't need a solution for phase 1, but what do you think about parameterizing or abstracting this piece?

[kobelb]

I've been delaying that for the moment, and have 'default' hard-coded a few places. We can introduce a constant that we use for this at the moment, a bunch of stuff is going to likely change once we become space aware but we'll likely still need the constant around because we'll be treating the default space "special" when it comes to routing and behaving without spaces.

[legrego]

I'm working on building/running this now, but here's my feedback (mostly questions) so far

[legrego]

Is the DEFAULT_RESOURCE check only in place until Spaces are implemented?

[kobelb]

I was initially going to try to leave the non-default privilege assignments in place, and this is a remnant of that. I'll go ahead and remove it, I'm not fond of the approach any longer.

[legrego]

What was your rationale for making this a named parameter, rather than putting it within the options bag? I'm fine with either, just curious. It eventually makes its way into the Saved Object Client's options one way or another

[kobelb]

I was attempting to follow the convention that you established by passing it the server as a separate parameter. That way the options parameter is the unmodified options that would normally be passed to the SavedObjectClient and we're supplementing it with additional values all inside the secureSavedObjectsClientOptionsBuilder.

When implementing this, I began to question the benefit of the secureSavedObjectsClientOptionsBuilder function and at one point considered inlining the whole thing, but decided on holding off on doing so since you wrote the initial implementation and saw the merit of having it a separate function.

[legrego]

Well I can't fault you there then 😄

In a perfect world, we would not need the secureSavedObjectsClientOptionsBuilder, but we require this because the options that come out of these builders are passed to the base OSS SavedObjectsClient, and the security plugin needs to swap out the default callCluster implementation when RBAC is enabled (callWithInternalUser vs callWithRequest).

The SOC wrappers don't have the ability to modify the original set of options, which is where these builders come in to play.

[kobelb]

Gotcha, when I mentioned inlining it earlier, I meant something similar to the following in the security/index.js:

      savedObjectsClientProvider.addClientOptionBuilder(options => {
        const adminCluster = server.plugins.elasticsearch.getCluster('admin');
        const { callWithInternalUser } = adminCluster;

        return {
          ...options,
          callCluster: callWithInternalUser,
          requestHasPrivileges
        };
      });

[legrego]

Ah I'm with you now, sorry... I split that out because it seemed like the security plugin's index.js overall did a decent job of not having a lot of its own logic, so I was trying not to throw too much in there. I'm not married to that approach though, so if you think it's clearer to inline, go for it

[kobelb]

I don't feel too strongly either way at this point, I think it all depends on how we surface the hasPrivileges check to other consumers, which I've put off deciding at this moment.

[legrego]

It looks like ES supports role names up to 1024 characters. Is there a reason we don't mirror that on the Kibana side?

[kobelb]

Not a good one! I have an excuse but I won't bore you with it.

[legrego]

Does it make more sense to call this applicationPrivileges or applicationsPrivileges? If I understand correctly, this represents all of the custom privileges on the role, while $scope.applicationPrivileges below references the privileges for this specific instance of Kibana.

[legrego]

Since we use the rbacApplication to derive the role name, what do you think about validating that this also conforms to the role naming rules?

Role names must be at least 1 and no more than 1024 characters. 
They can contain alphanumeric characters (a-z, A-Z, 0-9), spaces, punctuation, and printable symbols in the Basic Latin (ASCII) block.
Leading or trailing whitespace is not allowed.

[kobelb]

Good call.

[kobelb]

I chose a more restrictive pattern because it'd look inconsistent to accept punctuation and spaces and then append _rbac_user and _rbac_dashboard_only_user to the end.

[legrego]

Do we need to add exception handling here? If this call throws, then I think the error will get thrown out of the SavedObjectsClient uninspected, which condradicts its documentation

[kobelb]

Good catch, I'll make sure we're throwing the appropriate wrapped errors.

[kobelb]

Since this will be used outside of the context of the SavedObjectClient, I'm going to have the SavedObjectClient's usage wrap these errors instead of modifying it directly in the hasPrivileges service.

[legrego]

This could very well be YAGNI, but I'll ask anyway: Do we envision having service accounts in the future, for reporting or other background tasks? In other words, could we have accounts that have privileges within Kibana, but without the ability to login and have an interactive session?

[kobelb]

I haven't heard any discussion of service accounts in this form, and they'd all have to authenticate somehow (making the login action relevant). I'm having trouble envisioning a situation where we couldn't rely on the login privilege to be present.

[legrego]

I guess the corrective action would be "Stop doing that", but are there any steps we should document for users who find themselves in this situation? ("no" is a perfectly acceptable answer 😄)

[kobelb]

This is definitely something that we'll want to document. There isn't really a corrective action besides "don't do that" unfortunately.

[legrego]

Is this test named correctly?

[kobelb]

It's definitely not proper english.

[rhoboat]

issue: it's also returning success = true here

[legrego]

I'm not sure if this is within the scope of this PR or not, but I noticed a few authorization issues while testing:

Create Index Pattern

1. Enable RBAC:

xpack.security.rbac.enabled: true
xpack.security.rbac.application: "larry"

2. Add user larry with the newly created larry_rbac_user role
3. Login as larry
4. Attempt to create an Index Pattern
   Result:
   Page does not finish loading, because the search operation fails

View Reporting

[follow steps 1-3 above]
4. Attempt to view the Reporting management tab:

Access Monitoring App

1. Add the monitor cluster permission to the larry_rbac_user role
2. Login as larry, and attempt to access the Montioring app:

[kobelb]

With regard to those auth failures that you see happening, they should 'mirror' what is done without RBAC enabled, even though the user experience is less than ideal.

[legrego]

I retested with and without RBAC enabled, and I agree -- thanks for clarifying!

I found a separate issue while retesting: It doesn't seem possible to create a new role with RBAC disabled, because there are no app permission defined:

[kobelb]

I found a separate issue while retesting: It doesn't seem possible to create a new role with RBAC disabled, because there are no app permission defined:

I just pushed a fix for this, it was affecting the roles pages with RBAC enabled as well if you didn't assign any Kibana privileges (which is completely valid)

[kobelb]

If we ever get an error thrown when checking the hasPrivileges, I'm considering it a "general error" so it'll surface as a 500. We should've authenticated the user before this point, and we won't get a 403 (as this will just return true/false) and all other exceptions are something we don't want to surface to the user via customized HTTP access codes.

[rhoboat]

makes sense. do we have a test(s) for this?

[kobelb]

We're missing a lot of tests in the RBAC Phase 1 branch, I'm going to be working on adding better test coverage in an alternate PR (since I've already done a bit too much in this one).

[rhoboat]

nit: wording here is weird too. "false success" just reads weirdly. hmm. i can't suggest something better atm.

[kobelb]

Yeah, I struggled with this one...

success of false read weird also, '{ success: false }' and non-success seemed equally weird. I'm open to alternate suggestions :)

[rhoboat]

potential issue: Maybe it's just me, but this reads so strangely to me. I either want it to be create_RequestHasPrivileges to emphasize we're not creating a request with privileges, which would be weird wording too. Or I want it to be createRequestHasPrivilegesCheck to emphasize that we're creating a checking function, not a privilege, not a request. It's just unclear at first glance what this does.

[rhoboat]

what if this was
const hasPrivileges = checkRequestPrivileges(request);

[rhoboat]

what if
const checkRequestPrivileges = createRequestPrivilegesCheck(mockServer);
or
const checkRequestPrivileges = createCheckRequestPrivileges(mockServer);
?

[rhoboat]

I left some comments. Mostly about naming since that was tripping me up as I read. Overall I think it's worth doing.

[legrego]

Does dedent do anything to a single-line string?

[kobelb]

Nope. Originally it was a multi-line when copying the error message from Elasticsearch directly, but after simplifying the regex/description it's no longer needed. Thanks for catching this.

[legrego]

LGTM!