Dependency upgrade: xstream 1.4.19, jettison 1.4.1 #5552

groldan · 2022-01-03T13:41:09Z

Current XStream version has several (29) vulnerabilities, though they won't affect GeoServer, since it properly configures the security subsystem, an upgrade seems to be in place anyways.

This upgrades to the latest 1.4.19 version and Jettison 1.4.1 with a patch to preserve Jettison's legacy
behaviour when encoding single-element arrays.

Checklist

I have read the contribution guidelines.
I have sent a Contribution Licence Agreement (not required for small changes, e.g., fixing typos in documentation).
First PR targets the main branch (backports managed later; ignore for branch specific issues).
All the build checks are green (see automated QA checks).

For core and extension modules:

New unit tests have been added covering the changes.
Documentation has been updated (if change is visible to end users).
The REST API docs have been updated (when changing configuration objects or the REST controllers).
There is an issue in the GeoServer Jira (except for changes that do not affect administrators or end users in any way).
Commit message(s) must be in the form [GEOS-XYZWV] Title of the Jira ticket.
Bug fixes and small new features are presented as a single commit.
Each commit has a single objective (if there are multiple commits, each has a separate JIRA ticket describing its goal).

groldan · 2022-01-03T13:41:37Z

Supersedes #4572

aaime · 2022-01-03T13:44:40Z

AFAIK the vulnerabilities occur only when the XStream security subsystem is not configured. The GeoServer code does initialize it (although I cannot guarantee it happens everywhere), so it should not actually be vulnerable? Communication about potential vulnerabilities should be done in a responsible way, without spreading panic, and done only once fixes are released to the public, please.

groldan · 2022-01-03T13:55:06Z

Spreading panic? sigh. Ok, I'm sorry for mentioning XStream has vulnerabilities although it's public information, and even more sorry for not realizing they don't affect GeoServer, wouldn't go through all 29 of them to verify though.

groldan · 2022-01-03T14:08:31Z

Build's failing on CoverageControllerWCSTest.testPostAsJSON().

I wonder why CoverageControllerWCSTest.testPostAsJSON() expects a single-element-array:

        JSONArray coverages = json.getJSONObject("coverages").getJSONArray("coverage");
        assertThat(coverages.size(), is(1));

While CoverageControllerTest.testGetAsJSON() expects an object:

        JSONObject coverage = ((JSONObject) json).getJSONObject("coverage");
        assertNotNull(coverage);

aaime · 2022-01-03T14:11:58Z

@groldan yes spreading panic, it should be obvious to anyone that has had exposure to customers, which go nuts over the off-chance possibility anything is wrong. The Log4J2 case should serve as an emblematic example, we got grilled over a case of "run a RCE with any HTTP request" when that was not even affecting GeoServer, and ended up fixing one that required pre-existing access to the server file system in order to be triggered. Why that? Because people panic before checking if they are even affected. Even just mentioning "vulnerability" makes some people trigger... and those same people typically don't check CVEs and don't do research (the latter kind normally checks if an issue is an actual one, and how it can be triggered, before taking out the big guns).

We have multiple sections about responsible disclosure:

Please read them and be more careful in the future, even for issues that are already listed (and even more so for the ones that are not, or for which you have an attack vector), the more they are discussed in public, the more the nutjobs mentioned above will start panicking.

aaime · 2022-01-03T14:20:59Z

Build's failing on CoverageControllerWCSTest.testPostAsJSON().

I wonder why CoverageControllerWCSTest.testPostAsJSON() expects a single-element-array:
        JSONArray coverages = json.getJSONObject("coverages").getJSONArray("coverage");
        assertThat(coverages.size(), is(1));
While CoverageControllerTest.testGetAsJSON() expects an object:
        JSONObject coverage = ((JSONObject) json).getJSONObject("coverage");
        assertNotNull(coverage);

I guessing it's a simple "because XStream used to generate outputs like this"... historically XStream was used for the configuration, the REST API came some time after tha, and that's when we also got a JSON encoding... I believe it was accepted as-is, and tests kept on being written using that "untouchable" behavior.

In #4572 I could not find a set of settings that made the output match the previous one. As weird as it is, and as much as I don't like the inconsistency, the test spotted a backwards incompatible change. We can have such change, it just requires discussion with the PSC and an indication to the users that the upgrade might break some REST automation. I'll be +1 on breaking the JSON encoding expectations for more correct output, btw.

groldan · 2022-01-03T14:24:40Z

Hi, for the moment I fixed the test to expect an object, under the assumption that since it's already the expected behavior, seems more like a fix than anything else, like in we at least have a consistent output? Let me know what you think.

aaime · 2022-01-03T14:29:45Z

The read side is safer... it all depends on the specific client, on whether it has been built to match specific behavior, or written to be general... but in Javascript, it's easy to just read JSON into a variable and then hard-code an access path to the bit of information required, which might cross one of these weird points.

The write side is more problematic, will the change make the REST API stop accepting JSON it used to read?

Again, not against the change, but it should be managed properly: PSC vote and warning to the users in the documentation (there is an upgrade section in the docs). I don't think we need a proposal, just a mail on devel explaining the situation (that the change actually brings better, more consistent behavior) and a set of votes.

groldan · 2022-01-03T14:41:05Z

The write side is more problematic, will the change make the REST API stop accepting JSON it used to read?

Haven't changed anything related to writes (POST requests), relying on the existing set of tests.
The only thing I've changed is what that test expects after the POST was processed, and reads the coverage created by the POST request. For some strange reason it used to expect a single-element array, whereas everywhere else, a GET request with a single element returns an object.

Is that what you mean? just checking we're on the same page

aaime · 2022-01-03T14:51:04Z

I believe we are not on the same page. The change in encoding happens because XStream + Jettison behavior changed, right?
I would expect them to be symmetric, if they encode something in a different way, they would also expect to read it back in that same different way. So if a REST automatic client is posting a JSON built the old way, I would expect to see an error, XStream + Jettison would not read it anymore. Does that make sense?

A deployed system is normally a complex beast, made of different components, built by different people (and sometimes different companies, on contract). Upgrading GeoServer (maybe to get a security fix?) should not require changing the other components of the deployment. If it does, then it should be clearly advertised in the docs (and discussed by the PSC).

groldan · 2022-01-03T15:55:07Z

The change in encoding happens because XStream + Jettison behavior changed, right?

I'm not sure if you realize what I'm trying to do is to preserve the old behavior?

working on that XStreamCatalogListConverter issue which seems to be the only problematic one now

aaime · 2022-01-03T16:53:45Z

The change in encoding happens because XStream + Jettison behavior changed, right?

I'm not sure if you realize what I'm trying to do is to preserve the old behavior?

I am. But tests failing means there is currently a breakage in backward compatibility.

working on that XStreamCatalogListConverter issue which seems to be the only problematic one now

I was not aware it was limited to that converter, no previous message mentioned it. It's good news though, that's a converter under our control, so hopefully, it can be fixed.

aaime · 2022-01-03T20:17:10Z

src/rest/src/main/java/org/geoserver/rest/converters/XStreamCatalogListConverter.java

@@ -216,7 +217,13 @@ public void encodeCollectionLink(String link, HierarchicalStreamWriter writer) {

        @Override
        protected XStream createXStreamInstance() {
-            return new SecureXStream(new JettisonMappedXmlDriver());
+            // needed for Jettison 1.4.1


aaime · 2022-01-03T20:18:05Z

All green, well done! I can't do a review right now, will try to look at this in the next few days.

groldan · 2022-01-03T20:34:32Z

Actually I'm trying to revert that test case to its original form and get exactly the same output as before out of XStreamCatalogListConverter. Added test cases to CoverageControllerTest for zero and single element lists, because those never returned unwrapped single-element results.

Problem is I either get

{"coverages":  {  "coverage": { "name": "tazdem"} } }

or

{"coverages": [ {"coverage": {"name": "tazdem"} } ] }

But not the expected

{"coverages": {"coverage": [  { "name": "tazdem" } ] } }

groldan · 2022-01-04T02:13:52Z

ok, managed to preserve XStreamCatalogListConverter's behavior as of 75925d3 (replaces previous commit that changed the failing test instead, and adds a specific test case for the single-element case)

mprins

I see boolean useSerializeAsArray = false; in several classes and I'm wondering if it would be a good idea to move that into a public final in eg. XStreamPersister.java (not sure how that would work out dependency-wise

mprins · 2022-01-07T15:33:49Z

src/restconfig/src/test/java/org/geoserver/rest/resources/ResourceControllerTest.java

@@ -310,6 +311,7 @@ public void testDirectoryXML() throws Exception {
    }

    @Test
+    @Ignore


Can you add a comment as to why this test should be ignored?

Suggested change

@Ignore

@Ignore("why is this ignored....")

I think it wasn't intentional, met me check

No, I missed that one. Was added by @aaime at 8045f62
I'll see what's going on

Yep it was part of the work in progress, that branch was not meant to be committed as-is

aaime

Looks pretty good in general! Agree with the two points raised by @mprins (ignored test, and sharing duplicated code if possible).

aaime · 2022-01-10T11:50:23Z

src/restconfig/src/test/java/org/geoserver/rest/catalog/DataStoreControllerTest.java

@@ -72,6 +72,7 @@ public void testGetAsJSON() throws Exception {
    @Test
    public void testGetAllAsJSON() throws Exception {
        JSON json = getAsJSON(ROOT_PATH + "/workspaces/sf/datastores.json");
+        print(json);


I'd get rid of all the extra "print" in this PR.

aaime · 2022-01-18T14:15:56Z

Looks like there are legit test failures (at one point I believe the build was green though? maybe I'm confusing it with something else...)

groldan · 2022-01-24T01:38:11Z

Looks like there are legit test failures (at one point I believe the build was green though? maybe I'm confusing it with something else...)

It was green, but haven't seen that one that was @Ignored. I'll get to it asap

groldan · 2022-02-14T13:57:09Z

just a heads up that I'm resuming work on this

aaime · 2022-02-21T10:35:17Z

How are things going @groldan ?

groldan · 2022-02-23T16:32:04Z

How are things going @groldan ?

dancing with priorities @aaime, but I think I can finally get to this before the end of the week

groldan · 2022-03-14T13:22:52Z

back from vacations, finally working on this for good

…worse...

Pass `false` to `JettisonMappedXmlDriver` constructor's `useSerializeAsArray` parameter to preserve the legacy (1.1) single-element-arrar-as-object behavior after upgrading to Jettison 1.4.1.

… single-element collections The upgrade to Jettison 1.4.1 makes it impossible to preserve the single-collection JSON encoding as-is. Use a helper class to ensure single-element collections are encoded as it used to be before the upgrade, and add tests.

…and multiple children cases

groldan · 2022-03-18T14:25:38Z

Closing as superseded by #5730 with a single commit and corrected branch name.

groldan requested a review from aaime January 3, 2022 13:41

groldan mentioned this pull request Jan 3, 2022

Bump xstream from 1.4.11.1 to 1.4.18 in /src #4572

Closed

aaime reviewed Jan 3, 2022

View reviewed changes

groldan marked this pull request as draft January 3, 2022 20:25

groldan force-pushed the dependencies/xstream_1418 branch from b1b11da to 75925d3 Compare January 4, 2022 01:01

groldan marked this pull request as ready for review January 4, 2022 02:11

groldan requested a review from aaime January 4, 2022 15:06

mprins reviewed Jan 7, 2022

View reviewed changes

aaime reviewed Jan 10, 2022

View reviewed changes

groldan marked this pull request as draft January 14, 2022 20:29

groldan force-pushed the dependencies/xstream_1418 branch from 75925d3 to cbd1ade Compare January 14, 2022 20:31

mprins mentioned this pull request Jan 19, 2022

Bump xstream from 1.4.11.1 to 1.4.18 in /src #5594

Closed

This comment has been minimized.

Sign in to view

mprins mentioned this pull request Feb 1, 2022

Bump xstream from 1.4.11.1 to 1.4.19 in /src #5636

Closed

mprins changed the title ~~Dependency upgrade: xstream 1.4.18, jettison 1.4.1~~ Dependency upgrade: xstream 1.4.19, jettison 1.4.1 Feb 1, 2022

groldan force-pushed the dependencies/xstream_1418 branch from e9afe67 to a06a726 Compare March 1, 2022 23:13

aaime and others added 7 commits March 15, 2022 14:02

Moving on, not much progress. Try to disable force array in the driver?

f9c2206

Trying to configure Jettison driver as in XStream head. Makes things …

ecc5daf

…worse...

Preserve legacy JSON single-element-array-as-object serialization

65d9368

Pass `false` to `JettisonMappedXmlDriver` constructor's `useSerializeAsArray` parameter to preserve the legacy (1.1) single-element-arrar-as-object behavior after upgrading to Jettison 1.4.1.

Split ResourceControllerTest.testDirectoryJSON() into empty, single, …

784a1fa

…and multiple children cases

Dependency upgrade: xstream 1.4.18 -> 1.4.19

f8ae850

Preserve legacy encoding of collections for ResourceDirectoryInfo

acca593

groldan force-pushed the dependencies/xstream_1418 branch from a70400c to acca593 Compare March 17, 2022 18:52

groldan self-assigned this Mar 17, 2022

ResourceController: return resources in a predictable order

efc4e30

groldan force-pushed the dependencies/xstream_1418 branch from c0555fa to efc4e30 Compare March 18, 2022 05:00

groldan marked this pull request as ready for review March 18, 2022 05:02

groldan closed this Mar 18, 2022

groldan deleted the dependencies/xstream_1418 branch March 20, 2022 10:29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Dependency upgrade: xstream 1.4.19, jettison 1.4.1 #5552

Dependency upgrade: xstream 1.4.19, jettison 1.4.1 #5552

groldan commented Jan 3, 2022 •

edited

groldan commented Jan 3, 2022

aaime commented Jan 3, 2022 •

edited

groldan commented Jan 3, 2022

groldan commented Jan 3, 2022

aaime commented Jan 3, 2022

aaime commented Jan 3, 2022

groldan commented Jan 3, 2022

aaime commented Jan 3, 2022

groldan commented Jan 3, 2022

aaime commented Jan 3, 2022

groldan commented Jan 3, 2022

aaime commented Jan 3, 2022

aaime Jan 3, 2022

aaime commented Jan 3, 2022

groldan commented Jan 3, 2022

groldan commented Jan 4, 2022

mprins left a comment

mprins Jan 7, 2022

groldan Jan 7, 2022

groldan Jan 7, 2022 •

edited

aaime Jan 10, 2022

aaime left a comment

aaime Jan 10, 2022

aaime commented Jan 18, 2022

groldan commented Jan 24, 2022

This comment has been minimized.

groldan commented Feb 14, 2022

aaime commented Feb 21, 2022

groldan commented Feb 23, 2022

groldan commented Mar 14, 2022

groldan commented Mar 18, 2022

@@ @@ -310,6 +311,7 @@ public void testDirectoryXML() throws Exception { @@
                   }
                   @Test
+                  @Ignore

Dependency upgrade: xstream 1.4.19, jettison 1.4.1 #5552

Dependency upgrade: xstream 1.4.19, jettison 1.4.1 #5552

Conversation

groldan commented Jan 3, 2022 • edited

Checklist

groldan commented Jan 3, 2022

aaime commented Jan 3, 2022 • edited

groldan commented Jan 3, 2022

groldan commented Jan 3, 2022

aaime commented Jan 3, 2022

aaime commented Jan 3, 2022

groldan commented Jan 3, 2022

aaime commented Jan 3, 2022

groldan commented Jan 3, 2022

aaime commented Jan 3, 2022

groldan commented Jan 3, 2022

aaime commented Jan 3, 2022

aaime Jan 3, 2022

Choose a reason for hiding this comment

aaime commented Jan 3, 2022

groldan commented Jan 3, 2022

groldan commented Jan 4, 2022

mprins left a comment

Choose a reason for hiding this comment

mprins Jan 7, 2022

Choose a reason for hiding this comment

groldan Jan 7, 2022

Choose a reason for hiding this comment

groldan Jan 7, 2022 • edited

Choose a reason for hiding this comment

aaime Jan 10, 2022

Choose a reason for hiding this comment

aaime left a comment

Choose a reason for hiding this comment

aaime Jan 10, 2022

Choose a reason for hiding this comment

aaime commented Jan 18, 2022

groldan commented Jan 24, 2022

This comment has been minimized.

groldan commented Feb 14, 2022

aaime commented Feb 21, 2022

groldan commented Feb 23, 2022

groldan commented Mar 14, 2022

groldan commented Mar 18, 2022

groldan commented Jan 3, 2022 •

edited

aaime commented Jan 3, 2022 •

edited

groldan Jan 7, 2022 •

edited