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 <bschucha...@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 < > bschucha...@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 > >>> > >> >
