-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>
> > >>
> >>
> >>
>
