Backwards incompatible Security API changes

[bolkedebruin]

Hi All,

@kokosing @martint

Maintainer of the Ranger plugin here. I just updated the plugin to support Presto 331 as many backwards incompatible changes happened between 317 and 331. This commit (#3030), part of 332, also introduces a change in the Security API that makes it backwards incompatible. I'm starting to feel a bit like chasing a moving target.

Ranger and probably others, like Open Policy Agent, follow a different release cycle than Presto. Given that Presto obviously also addresses security concerns in its releases it is pretty tough to align both, but nevertheless important. I see 3 options (or a combination of them):

1. Be much more conservative towards change the Security API
2. Provide [sane, configurable] defaults if changing the Security API
3. Integrate [Ranger, OPA] into the main tree of Presto

What are your thoughts?

[dain]

For the second point, there are 2 general approaches to security plugins:

1. extend AllowAllAccessControl and new checks will default to allow
2. extend ConnectorAccessControl and all new checks will be default to deny

Generally, I would choose the second one as I rather see failures than allow stuff through, but that requires keeping up with new SPI methods.

[electrum]

Note that we've been making many changes to security lately, but generally the SPI and security APIs should be stable. Until the recent changes, most of the changes were adding new access control checks.

[bolkedebruin]

@electrum That's correct, however every new access control check currently is an update to the API which defaults to deny. For example recently, checkCanExecuteFunction was added . This stopped every query in its tracks, when using a plugin that was not adjusted yet, that was using a function without being able to "just" update a policy. Now it required an update of the code, compile, test, deploy of the plugin and a policy update. checkCanExecuteProcedure was also recently added, likely to be followed by checkCanGrantExecuteProcedurePrivilege equivalent to checkCanGrantExecuteFunctionPrivilege but its not there yet. So can I expect to do another plugin update when 334 is released?

I dont like extending from AllowAllAccessControl for obvious reasons. I want the default to be deny, but I need to have a system operator to have the possibility to fix this by policy rather then to have a new release of its security component as a requirement. So I guess what I am arguing for (as one of the options) is to have a more generizable approach. Something along the lines of:

hasPermission(<resource>, <action>, <security_context>)

so it could be something like

hasPermission(function, AccessType.EXECUTE, context)

The "default" in the interface could then fallback to the method mentioned above instead of a straight out deny. This would allow a plugin [OpenPolicyAgent, Ranger] to override the method and for example implement a DSL without requiring a full redeploy of the plugin and keep the current API intact.

What do you think?

[findepi]

@bolkedebruin if I understand your proposal correctly, you are proposing to replace ConnectorAccessControl interface with 20-something methods with a (weakly-typed) single entry point into the security subsystem?

I am not convinced this is the right approach in general (it makes new implementations harder to understand and to implement), but if you want to try this route, you can give it a try. I would start with implementing an InvocationHandler and using j.l.r.Proxy to instantiate impl of ConnectorAccessControl and map the calls dynamically to your "backend" class.

[bolkedebruin]

@findepi No I am not exactly proposing that. I am proposing that for SystemAccessControl (why does ConnectorAccessControl pop up in this discussion? - Real question) the default fallback mechanism for the 20 something methods is to call the hasPermission with the generics instead of the direct deny. This can make plugins more future proof without sacrificing security by defaulting to allow (e.g. by extending AllowAllAccessControl). It also still allows you to override the specific method.

So not replacing, but rather complementary. Makes sense?

[findepi]

(why does ConnectorAccessControl pop up in this discussion? - Real question)

error on my part, but generally SystemAccessControl and ConnectorAccessControl are (and need to remain) similar interfaces, as far as query control is concerned,
so my comment applies the same.

default fallback mechanism for the 20 something methods is to call the hasPermission with the generics instead of the direct deny.

I am still not convinced such a general API is useful in a general case. I don't expect many implementations that would be DSL-backed and made "useful use" of such an API.
If the usages will be very limited, having the complexity within the project is not warranted (unless, maybe, it is also unavoidable).

In the case we determine we want to have such an API, we should validate this approach solves your problem first (POC).
And I still think it's doable with the Proxy and dynamic dispatch approach, within plugin-provided implementation.

[bolkedebruin]

@findepi let's see. I'm not sure if a proxy class and dynamic dispatch is cleaner and easier to understand tbh. But I am open for options.

[findepi]

I'm not sure if a proxy class and dynamic dispatch is cleaner and easier to understand tbh

I agree, I am sure it is not. I am just trying to be helpful, suggesting other options.