Hello all,
Below are some answers/comments to the questions and feedback gathered
during the last round, along with some final ideas at the end of the email.
[Aaron]: There is almost always trade-off between security and ease-of-use.
The proposed implementation of JavaBeanAccessorBasedMethodAuthorizer makes
me feel uneasy because it would be super easy to create a method that
begins with "get" or "is" but is not actually a JavaBean accessor method.
However, I can also see how nice this would be in terms of ease-of-use.
[JUAN]: Agreed, it is actually one of the Risks / Unknowns / Disadvantages
[1] pointed out in the proposal.
[Aaron]: From a security standpoint I prefer
AnnotationBasedMethodAuthorizer because it's very explicit on which methods
may be executed. To remove the coupling between the configuration and
domain classes, you could separate out the mapping of which methods are
allowed into a different class and not use annotations.
[JUAN]: Sure, but that separation implies something even worse if you ask
me: we would be forcing our customers to modify their domain model in order
to use our product.
[Aaron]: How have other projects solved this problem? Can we add a "related
work" section to the proposal? If you've already looked and didn't really
find anything relevant you could also mention that in the proposal.
[JUAN]: I haven't found anything that really applies to our use case. The
"most similar solution" is Spring Method Security [2], which basically
implies annotating methods with explicit configuration about the roles
required to execute them. The `AnnotationBasedMethodAuthorizer` approach is
somewhat similar to that.
[Anthony]: I shouldn’t have to change my domain classes in order to run a
query.
[JUAN]: Fully agreed, and this is one of the reasons why the
`AnnotationBasedMethodAuthorizer` won't be provided out of the box. If
users would like to follow this approach, they can certainly provide an
implementation similar to one shown in the proposal and annotate the
methods on their domain model.
[Anthony]: I shouldn’t have to configure anything to run a “normal” query
that uses classes deployed into the cluster and stored in the region.
[JUAN]: I'd say I agree with this one, though I haven't found a feasible
way to implement it yet. The actual Region is not always available within
the query tree, specially when invoking chained methods. Going up through
the object hierarchy to find out whether the root object is part of the
Region would also be very poor in terms of performance, especially through
the usage of java reflection. Also, what about methods that actually mutate
the object?.
[Anthony]: By default the cluster is secure from malicious users trying to
execute untrusted code. If a user chooses to deploy code into the cluster
that does bad things that’s on them.
[JUAN]: Agreed but, again, what about methods that actually mutate the
object?.
[Dan]: Queries can be executed by users with read privileges. So if it
needs to be really clear to the operator whether the
MethodInvocationAuthorizer they are configuring is going to let users call
methods that modify the data or not.
[JUAN]: Agreed. I might need to investigate some more, but I don't see a
way of preventing users from modifying the objects if they invoke a mutator
method, anyway. Even if we restrict method invocation to getters
(`JavaBeanAccessorBasedMethodAuthorizer`), users can actually do whatever
they want within a get*/is* method, even modify the object itself...
executing a deep copy of the object and execute the method on that copy
instead of the actual reference might be doable, but do we really want to
go down this path?, it would be a nightmare performance wise.
[Dan]: OQL lets you invoke methods on objects that are passed as parameters
to the query. So that means that any class that exists on the server and is
serializable could potentially have methods invoked in it through a query
when someone opens up access with one of these MethodInvocationAuthorizers.
So the RegexBasedMethodAuthorizer needs to be used with care.
[JUAN]: Agreed and, adding to the your first point as well, this approach
both the most powerful and most dangerous one: users can accidentally allow
untrusted methods by making a mistake on the RegEx, but they can also be
sure to permit only safe methods as they are the only ones that actually
know what each method from the domain model is doing.
At this point I believe the major concern regarding the proposal is that
there's no out of the box implementation that allows users to get up and
running without needing to manually configure something extra (I'm also
reading that this implementation should be the one used by default when
security is enabled). I certainly understand that we want to make things
easy for our users and, in the end, it will always be their responsibility
to make that
