Hello Jake, I've been doing some reading about the *Java Security Manager* and, even when it might work for our use case, I don't think is a good fit due to the following reasons: 1). We already have chosen *Shiro* for authentication and authorization, adding yet another security framework (and mapping between roles and permissions between the two of them) for OQL method invocation security seems overkilling to me. I'd rather spend some time improving the *ResourcePermissionBasedMethodAuthorizer [1] *and/or adding *Permissions*/*Roles* to our current security framework than introducing a new security framework altogether. 2). There can only be one *Security Manager* per JVM at a given time. 3). Customers already using a custom *Security Manager* on their own will be in trouble... we can certainly wrote our own implementation as a composite and parse multiple policy files, but this will probably break the backward compatibility for these customers (and requires yet more configuration and things to keep in mind when upgrading). 4). The current set of default *Permissions* (*PropertyPermission*, *FilePermission*, etc.) don't apply to our use case, so we'll need to create our own implementations and do some plumbing to map to what we currently have in terms of principals and roles (more boilerplate). 5). In order to check a permission we basically need to check whether a Security Manager is installed (*System.getSecurityManager() != null*) and execute the check afterwards (*securityManager**.checkPermission(perm)*); the *Security Manager* then looks at the classes of all methods currently within the call stack (roughly speaking) and returns or throws an exception... a lookup through a simple Map is probably faster, and easier to maintain and read. 6). Overall, it feels like killing mosquitoes with a bazooka to me: it's certainly a powerful framework and we might certainly use it to achieve our goal, but it just seems too complicated for what we are trying to do.
My expertise with the *Java Security Manager* has came from what I've recently read, anyway, so chances are high that I might be missing something :-). That said, if we all agree in that *we can't prevent users from shooting themselves in the foot* (thanks for that phrase!!) then things become somewhat trivial: we known which internal methods can compromise the integrity of the system and we also know which general methods are dangerous, so we just need to have an immutable structure within our default authorizer and check if the method belongs to it or not to decide whether it should be allowed/denied. We can certainly provide the other 2 or 3 implementations out of the box and allow customers to provide their own in case they want to restrict things further but, as far as this proposal goes, I think we're good to incorporate the comments/suggestions gathered and go for another feedback round (it's what I plan to do now). Cheers. [1]: https://cwiki.apache.org/confluence/display/GEODE/OQL+Method+Invocation+Security#OQLMethodInvocationSecurity-ResourcePermissionBasedMethodAuthorizer On Mon, Jul 1, 2019 at 12:57 PM Juan José Ramos <jra...@pivotal.io> wrote: > Hello Jake, > > Thanks for your reply. Some comments below, inline. > > *>> 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.* > [Juan]: Agreed, I will keep this in mind when modifying the proposal. > > *>> 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.* > [Juan]: I'm not sure about this one... how far do we go?, I guess it's a > decision the community needs to make. I certainly believe that we can't > prevent everything, but I also think that we can't force our uses to design > their model in terms of our one-direction decisions. As an example, why > should I call my accessor only method get*/is* to be able to invoke it on a > query?, what if the company I work for wants to use the local native tongue > and the equivalent for *get*/is** instead of using english all together?, > etc. > > > *>> 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?* > [Juan]: Agreed!. > > > *>> I have spent plenty of time in the guts of SecurityManager in previous > lives. I think, based on the intended use of this proposal, that Java > SecurityManager more than meets our needs.*[Juan]: Fair enough, I'll go > ahead and do some reading about the *Security Manager *on my own, will > probably ping you once I have enough context to discuss options (or even > ask you some questions :-)). > > One concern I have, however, is regarding *How far do we go to prevent > the user from shooting themselves in the foot?*. I think this needs to be > decided by the community, do we have any formal *poll* mechanism or > something like that?. As a matter of fact, if we decide to prevent OQL > method invocation only on our own internal/protected classes (and some > known dangerous things like getClass), the implementation would be waaaay > easier... > Cheers. > > On Mon, Jul 1, 2019 at 12:07 PM Jacob Barrett <jbarr...@pivotal.io> wrote: > >> >> >> > On Jul 1, 2019, at 6:55 AM, Juan José Ramos <jra...@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 >> >> >> >> > > -- > Juan José Ramos Cassella > Senior Technical Support Engineer > Email: jra...@pivotal.io > Office#: +353 21 4238611 > Mobile#: +353 87 2074066 > After Hours Contact#: +1 877 477 2269 > Office Hours: Mon - Thu 08:30 - 17:00 GMT. Fri 08:30 - 16:00 GMT > How to upload artifacts: > https://support.pivotal.io/hc/en-us/articles/204369073 > How to escalate a ticket: > https://support.pivotal.io/hc/en-us/articles/203809556 > > [image: support] <https://support.pivotal.io/> [image: twitter] > <https://twitter.com/pivotal> [image: linkedin] > <https://www.linkedin.com/company/3048967> [image: facebook] > <https://www.facebook.com/pivotalsoftware> [image: google plus] > <https://plus.google.com/+Pivotal> [image: youtube] > <https://www.youtube.com/playlist?list=PLAdzTan_eSPScpj2J50ErtzR9ANSzv3kl> > -- Juan José Ramos Cassella Senior Technical Support Engineer Email: jra...@pivotal.io Office#: +353 21 4238611 Mobile#: +353 87 2074066 After Hours Contact#: +1 877 477 2269 Office Hours: Mon - Thu 08:30 - 17:00 GMT. Fri 08:30 - 16:00 GMT How to upload artifacts: https://support.pivotal.io/hc/en-us/articles/204369073 How to escalate a ticket: https://support.pivotal.io/hc/en-us/articles/203809556 [image: support] <https://support.pivotal.io/> [image: twitter] <https://twitter.com/pivotal> [image: linkedin] <https://www.linkedin.com/company/3048967> [image: facebook] <https://www.facebook.com/pivotalsoftware> [image: google plus] <https://plus.google.com/+Pivotal> [image: youtube] <https://www.youtube.com/playlist?list=PLAdzTan_eSPScpj2J50ErtzR9ANSzv3kl>
