geode-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Bruce Schuchardt <bschucha...@pivotal.io>
Subject Fwd: Re: [PROPOSAL] use default value for validate-serializable-objects in dunit
Date Fri, 21 Dec 2018 23:26:29 GMT
I agree with running tests with the default settings but I do not agree 
with this change.

I think we need to enable this serialization validation by default.  
Otherwise servers and clients are exposed to serialization exploits.  We 
did not enable validation by default when the serialization filter was 
implemented, but I think that was a mistake.  Who wants to be exposed to 
someone running arbitrary code in their servers or clients due to 
exploitable flaws in java serialization?  You should have to opt in to that.

With validation turned off by default in our tests we have no assurance 
that someone hasn't introduced a java-serializable class in Geode that 
isn't white-listed by the filter.  If that happens a user enabling 
validation would then hit errors when trying to use Geode because the 
filter would reject instances of that class. Imagine one server sending 
a Function to another server that replied with an Enum value that wasn't 
whitelisted.  The message would be rejected and the thread issuing the 
function would hang.



On 12/21/18 2:42 PM, Kirk Lund wrote:
> <bump>
>
> I filed GEODE-6202: DUnit should not enable VALIDATE_SERIALIZABLE_OBJECTS
> by default
> https://urldefense.proofpoint.com/v2/url?u=https-3A__issues.apache.org_jira_browse_GEODE-2D6202&d=DwIBaQ&c=lnl9vOaLMzsy2niBC8-h_K-7QJuNJEsFrzdndhuJ3Sw&r=JEKigqAv3f2lWHmA02pq9MDT5naXLkEStB4d4n0NQmk&m=32wc_2NhBYmEbZduqD6ejal16V-6fZSaV8nMKc4DXlo&s=Lk7Nz72vIqLozdzuEhGqTm49719bax0k3wzX9Os-cu8&e=
>
> And submitted PR #3023
> https://github.com/apache/geode/pull/3023
>
> Please review and/or discuss further if needed.
>
> Thanks,
> Kirk
>
> On Thu, Mar 15, 2018 at 12:00 PM Bruce Schuchardt <bschuchardt@pivotal.io>
> wrote:
>
>> +0.5 I think we can turn this off (back to the default) now since the
>> AnalyzeSerializables tests for other modules have been created. These
>> tests ensure that serializable objects are properly white-listed or
>> excluded and are able to be serialized/deserialized.
>>
>> Excluded classes are not tested, though, so if you add an excluded class
>> and are wrong about whether it is sent over the wire you will break
>> Geode. Having serialization validation enabled in dunit tests protects
>> against this if you write tests that use the excluded class.
>>
>> We also have non-default values for cluster configuration in dunit runs.
>>
>>
>> On 3/13/18 10:03 AM, Kirk Lund wrote:
>>> I want to propose using the default value for
>> validate-serializable-object
>>> in dunit tests instead of forcing it on for all dunit tests. I'm
>>> sympathetic to the reason why this was done: ensure that all existing
>> code
>>> and future code will function properly with this feature enabled.
>>> Unfortunately running all dunit tests with it turned on is not a 
>>> good way
>>> to achieve this.
>>>
>>> Here are my reasons:
>>>
>>> 1) All tests should start out with the same defaults that Users have. If
>> we
>>> don't do this, we are going to goof up sometime and release something
>> that
>>> only works with this feature turned on or worsen the User experience of
>>> Geode in some way.
>>>
>>> 2) All tests should have sovereign control over their own configuration.
>> We
>>> should strive towards being able to look at a test and determine its
>> config
>>> at a glance without having to dig through the framework or other classes
>>> for hidden configuration. We continue to improve dunit in this area 
>>> but I
>>> think adding to the problem is going in the wrong direction.
>>>
>>> 3) It sets a bad precedent. Do we follow this approach once or keep
>> adding
>>> additional non-default features when we need to test them too? Next one
>> is
>>> GEODE-4769 "Serialize region entry before putting in local cache" which
>>> will be disabled by default in the next Geode release and yet by turning
>> it
>>> on by default for all of precheckin we were able to find lots of 
>>> problems
>>> in both the product code and test code.
>>>
>>> 4) This is already starting to cause confusion for developers thinking
>> its
>>> actually a product default or expecting it to be enabled in other
>>> (non-dunit) tests.
>>>
>>> Alternatives for test coverage:
>>>
>>> There really are no reasonable shortcuts for end-to-end test coverage of
>>> any feature. We need to write new tests or identify existing tests to
>>> subclass with the feature enabled.
>>>
>>> 1) Subclass specific tests to turn validate-serializable-object on for
>> that
>>> test case. Examples of this include a) dunit tests that execute Region
>>> tests with OffHeap enabled, b) dunit tests that flip on HTTP over GFSH,
>> c)
>>> dunit tests that run with SSL or additional security enabled.
>>>
>>> 2) Write new tests that cover all features with
>> validate-serializable-object
>>> enabled.
>>>
>>> 3) Rerun all of dunit with and without the option. This doesn't sound
>> very
>>> reasonable to me, but it's the closest to what we really want or need.
>>>
>>> Any other ideas or suggestions other than forcing all dunit tests to run
>>> with a non-default value?
>>>
>>> Thanks,
>>> Kirk
>>>
>>

Mime
View raw message