geode-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Kirk Lund <kl...@apache.org>
Subject Re: Re: [PROPOSAL] use default value for validate-serializable-objects in dunit
Date Sat, 22 Dec 2018 00:00:20 GMT
Thanks for the input Bruce. I'm going to close the PR for now and do some
more thinking about how we test it.

On Fri, Dec 21, 2018 at 3:26 PM Bruce Schuchardt <bschuchardt@pivotal.io>
wrote:

> 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
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message