geode-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Bruce Schuchardt <bschucha...@pivotal.io>
Subject Re: [PROPOSAL] use default value for validate-serializable-objects in dunit
Date Fri, 21 Dec 2018 22:54:46 GMT
-1

I'm fearful that removing full testing of serialization validation 
leaves the project exposed to java-serialization bombs.  We need to 
ensure that our servers and clients are protected from malicious 
deserialization attacks.


On 2018/12/21 22:42:23, Kirk Lund <k...@apache.org> wrote:
 > <bump>>
 >
 > I filed GEODE-6202: DUnit should not enable 
VALIDATE_SERIALIZABLE_OBJECTS>
 > by default>
 > https://issues.apache.org/jira/browse/GEODE-6202>
 >
 > 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 <bs...@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