geode-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Jacob Barrett <jbarr...@pivotal.io>
Subject Re: [PROPOSAL]: Improve OQL Method Invocation Security
Date Mon, 01 Jul 2019 15:06:52 GMT


> On Jul 1, 2019, at 6:55 AM, Juan José Ramos <jramos@pivotal.io> wrote:
> 
>> Can we safely assume that some caching of authorization requests will
>> be performed? What will the scope and lifetime of this caching be? Are the
>> authentication rules and modules assumed to be immutable at runtime? All of
>> this will have significant implications on performance.
> My assumption right
> now is that there will be some caching performed and will certainly try to
> keep the authorizers immutable but, however, no PoC has been done and the
> implementations details shown in the proposal are just a draft, so I'm not
> in a position to accurately answer these questions. I also read that
> *"premature
> optimization is the root of all evil", *so I guess we can benchmark the
> solution with and without caching, and decide what's better based on some
> real data (I certainly see some gains using caching on the
> *RegexBasedMethodAuthorizer*, not sure about the
> *JavaBeanAccessorBasedMethodAuthorizer*).

Premature optimization can be bad but so can ignoring it. Most importantly here is to define
the scope of an authorization. This will need to be defined as part of the API/SPI. If I was
to implement one of these authorizers I would need to know under what conditions my authorize
method going to be called. Is it going to be for every object that is scanned by this method?
The API suggests this might be the case given that target is an Object and not a Class. We
can do some profiling in our head an know that if we invoke this method for every potential
object we scan then performance is going to suffer. The regex matcher will destroy the CPU.
What if my implementation was to use LDAP, ouch! If we define scope to be per query this is
orders of magnitude better. The API/SPI should probably change to reflect this. Maybe target
should be the Class? Should the query itself be included? Now if the target is a Class, and
classes are immutable, perhaps now the authorize method could be invoked once for the life
of the application only when a new Class is touched in our object scan. Such scope would require
that either the API/SPI define that rules are also immutable or that there is some invalidation
mechanism to tell the system the rules have been mutated. Again, this has API/SPI ramifications
that all need to be discussed in this proposal.

> The second issue is how does this differ, augment are compete with
> Java’s built in Security Manager / Policy system. It was designed for a lot
> of these same reasons, restricting application access to specific OS level
> operations that can be dangerous if executed by malicious code. Why is such
> a system not sufficient to handle our concerns in OQL? Beyond creating
> sockets, files, threads, forks, etc. what are we intending to prevent the
> OQL user executing?*
>> Thanks for pointing this out, I guess I should have included these examples
>> in the proposal (will certainly do it in the next iteration) as the
>> starting point so readers could see what we wanted to prevent when *GEODE-3247
>> [2]* was implemented.
>> *1. Reflection (This allows the user to do everything)*
>>  select * from /region r where
>> r.getClass().forName('java.lang.Runtime').getDeclaredMethods()[0].invoke()

All of the methods on Runtime use the SecurityManager to prevent invocations by unauthorized
contexts. If we executed queries with the correct context and a property configure SecurityManager
we are protected.

> *2. Doing anything with the cache (closing, accessing all regions, etc.)*
>  select * from /region.getCache().close()
> *3. Destroying, adding or invalidating the entire region or specific
> elements*
>  select * from /region.destroyRegion()
>  select * from /region.invalidate('xyz')
>  select * from /region.put('xyz','abc’)

We could add SecurityManager checks to our protected calls.

> *4. Modifying an object in place*
>  select r.setName('Zaraza') from /region r

How far do we go to prevent the user from shooting themselves in the foot? Here we could change
OQL to any support the bean interface and disallow all set operations.

> I honestly don't know much about the *Java Security Manager *and didn't
> consider its usage due to the fact that it was't considered in the first
> place when GEODE-3247 was implemented. I'll spend some time looking at it
> today, however, and see if it's applicable to our use case. If you (or
> anyone reading this) already has enough knowledge about the *Java Security
> Manager* and knows whether we can apply it to our scenario, please shout
> and let's have a talk :-).

I have spent plenty of time in the guts of SecurityManager in previous lives.


I want us to consider systems that exist before we spend time inventing our own. There is
cost in maintain our own solution in implementation time and bug squashing. There is also
the cost in training on our own solution. SecurityManger is discussed in numerous books and
blogs, our solution would not have that kind of coverage. Let’s think really critically
there. What are we trying to prevent? Is there a solution in the Java echo system that already
does what we really need to do? Can we use a preexisting solution and avoid the risk of implementing
our own?

I think, based on the intended use of this proposal, that Java SecurityManager more than meets
our needs.

-Jake




Mime
View raw message