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 sure the cluster is secured, that the deployed code is "safe",
and that their internal users and operators are correctly trained to use
the product.
With that in mind, would it be correct to assume that the following (draft)
MethodAuthorizer addresses these concerns (the other two out of box
implementations will still be available to the users)?:
*[MeaningfulPrefix?]MethodAuthorizer*
Allow any method execution as long as the target object *does not belong to
Geode packages, or does belong but it's marked as safe*. Some known
dangerous methods (like getClass) will be rejected, no matter whether the
target object belongs to a Geode package or not. The implementation will
have an internal list of "safe" Geode methods, and use that to decide
whether a method invoked on a Geode class is allowed or denied.
The pseudocode:
- If the target object doesn't belong to a Geode package, allow its
execution.
- If the target object belongs to a Geode package:
. Is it marked as safe?, allow its execution.
. It is not marked as safe?, deny its execution.
*Advantages*
. Easy to implement.
. Easy to use, no extra configuration needed.
. Solves the general use case: deploy the domain model, start executing OQL
and invoking methods without further changes.
*Risks / Unknowns / Disadvantages*
. All methods will be allowed, including those that actually mutate objects.
. Methods on OQL bind parameters will be allowed as well, and that
basically can be anything.
Whatever we do and no matter which approach we follow, we'll end up
invoking alien methods anyway and we certainly can't prevent these methods
from modifying the objects in memory, at the end of the day it will the
user's responsibility to ensure operators are correctly trained and no
dangerous objects as used as part of the OQL bind parameters.
Sorry for the long email, I'll wait until Monday to add this new authorizer
as the default out of the box implementation to the proposal, give people
some time to analyze the changes, and will try to schedule a live session
afterwards.
Best regards.
[1]:
https://cwiki.apache.org/confluence/display/GEODE/OQL+Method+Invocation+Security#OQLMethodInvocationSecurity-Risks/Unknowns/Disadvantages.1
[2]:
https://docs.spring.io/spring-security/site/docs/5.1.5.RELEASE/reference/html/jc.html#jc-method
On Wed, Jun 26, 2019 at 2:46 PM Juan José Ramos <jra...@pivotal.io> wrote:
> Helo all,
>
> Thanks for all the feedback.
> I'll go over the comments and reply accordingly, hopefully between
> tomorrow and Friday.
> Best regards.
>
>
>
> On Wed, Jun 26, 2019 at 2:12 PM Dan Smith <dsm...@pivotal.io> wrote:
>
>> A couple of other things to keep in mind to make sure you aren't
>> introducing security holes
>>
>> 1) 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.
>> 2) 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.
>>
>> -Dan
>>
>> On Tue, Jun 25, 2019 at 11:53 AM Anthony Baker <aba...@pivotal.io> wrote:
>>
>> > Here are the things I think are important:
>> >
>> > 1) I shouldn’t have to change my domain classes in order to run a query.
>> > 2) I shouldn’t have to configure anything to run a “normal” query that
>> > uses classes deployed into the cluster and stored in the region.
>> > 3) By default the cluster is secure from malicious users trying to
>> execute
>> > untrusted code*.
>> >
>> >
>> > Anthony
>> >
>> > * if a user chooses to deploy code into the cluster that does bad things
>> > that’s on them
>> >
>> >
>> > > On Jun 25, 2019, at 11:07 AM, Aaron Lindsey <alind...@pivotal.io>
>> wrote:
>> > >
>> > > +1 to the proposal
>> > >
>> > > Some comments:
>> > >
>> > > - 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.
>> > > - 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.
>> > > - 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.
>> > >
>> > > - Aaron
>> > >
>> > >
>> > > On Mon, Jun 24, 2019 at 4:31 PM Jason Huynh <jhu...@pivotal.io>
>> wrote:
>> > >
>> > >> +1
>> > >>
>> > >> I have some concerns about all of the different ways we configure
>> geode
>> > to
>> > >> be secure, but that's a different issue ;-)
>> > >> Overall, very thorough proposal Juan!
>> > >>
>> > >>
>> > >>
>> > >> On Mon, Jun 24, 2019 at 4:22 PM Dan Smith <dsm...@pivotal.io> wrote:
>> > >>
>> > >>> +1
>> > >>>
>> > >>> This proposal looks good to me!
>> > >>>
>> > >>> On Mon, Jun 24, 2019 at 4:15 PM Udo Kohlmeyer <u...@apache.org>
>> wrote:
>> > >>>
>> > >>>> +1, Count me in
>> > >>>>
>> > >>>> On 6/24/19 13:06, Juan José Ramos wrote:
>> > >>>>> Hey Jake,
>> > >>>>>
>> > >>>>> Sure, I guess we could do a live session if there's enough
>> interest
>> > >>> after
>> > >>>>> people have reviewed the proposal.
>> > >>>>> Best regards.
>> > >>>>>
>> > >>>>> On Mon, Jun 24, 2019 at 4:17 PM Jacob Barrett <
>> jbarr...@pivotal.io>
>> > >>>> wrote:
>> > >>>>>
>> > >>>>>>
>> > >>>>>>> On Jun 24, 2019, at 11:49 AM, Juan José Ramos <
>> jra...@pivotal.io>
>> > >>>> wrote:
>> > >>>>>>>
>> > >>>>>>> I’d rather get feedback in any way and aggregate everything on
>> my
>> > >>> own
>> > >>>>>> than
>> > >>>>>>> maybe not getting anything because I'm explicitly limiting the
>> > >>> options
>> > >>>> to
>> > >>>>>>> provide it.
>> > >>>>>> Dealers choice so both it is!
>> > >>>>>>
>> > >>>>>> Could you also consider public live session on some medium, like
>> > >> Zoom,
>> > >>>>>> where you can walk through the proposal and take like feedback
>> and
>> > >>>>>> questions?
>> > >>>>>>
>> > >>>>>> Thanks,
>> > >>>>>> 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>
