quarkiverse / quarkus-wiremock

Quarkus extension for launching an in-process Wiremock server
https://wiremock.org/
Apache License 2.0
15 stars 8 forks source link

feat: switch to wiremock-jre8-standalone #56

Closed wjglerum closed 8 months ago

wjglerum commented 8 months ago

WireMock pulls in a bunch of (vulnerable) dependencies that we don't need, so simplifying the dependency tree by switching to the standalone version, see https://wiremock.org/docs/standalone/

Quarkus also uses the standalone version in their tests, see for example https://github.com/quarkusio/quarkus/blob/7dcbe6793d431ce140131550775c406ea9e2d74e/extensions/resteasy-reactive/rest-client-reactive/deployment/pom.xml#L73

I'll do the same for the main branch next.

chberger commented 8 months ago

According to the documentation the standalone-jar is just a uber-jar:

WireMock Java is distributed in two flavours - a standard JAR containing just WireMock, and a standalone uber JAR containing WireMock plus all its dependencies

https://wiremock.org/docs/download-and-installation/

So yes it reduces the dependency tree, but how does it help regarding security? If it just contains the vulnerable classes than there is not much benefit.

Quarkus also uses the standard version as referenced in this offical guide: https://quarkus.io/guides/getting-started-testing#quarkus-test-resource https://github.com/geoand/quarkus-test-demo/blob/main/pom.xml#L69

So, I'm not convinced yet whether this is a good idea or not.

wjglerum commented 8 months ago

According to the documentation the standalone-jar is just a uber-jar:

WireMock Java is distributed in two flavours - a standard JAR containing just WireMock, and a standalone uber JAR containing WireMock plus all its dependencies

https://wiremock.org/docs/download-and-installation/

So yes it reduces the dependency tree, but how does it help regarding security? If it just contains the vulnerable classes than there is not much benefit.

Good point! Was looking for this piece of documentation about what's included in the standalone version but hadn't seen that page. It won't fix the vulnerabilities then no, only not annoy the CVE scanner in my project ;)

Quarkus also uses the standard version as referenced in this offical guide: https://quarkus.io/guides/getting-started-testing#quarkus-test-resource https://github.com/geoand/quarkus-test-demo/blob/main/pom.xml#L69

So, I'm not convinced yet whether this is a good idea or not.

Afaik that's an outdated example (it was last updated 3 years aga), in the repo I can only find usages of wiremock-standalone, see https://github.com/search?q=repo%3Aquarkusio%2Fquarkus+wiremock+language%3A%22Maven+POM%22&type=code&l=Maven+POM

chberger commented 8 months ago

Good point! Was looking for this piece of documentation about what's included in the standalone version but hadn't seen that page. It won't fix the vulnerabilities then no, only not annoy the CVE scanner in my project ;)

I mean maybe your CVE scanner should skip test dependencies anyway. They are not part of the production build and might not risk your deployments. But of course it depends on your project/environment setup.

Afaik that's an outdated example (it was last updated 3 years aga), in the repo I can only find usages of wiremock-standalone, see https://github.com/search?q=repo%3Aquarkusio%2Fquarkus+wiremock+language%3A%22Maven+POM%22&type=code&l=Maven+POM

Good catch. I've just checked the official documentation of rest-client-reactive: https://quarkus.io/guides/rest-client-reactive#using-a-mock-http-server-for-tests

There they point to the legacy-rest-client where also the standard version is being mentioned: https://quarkus.io/guides/rest-client#using-a-mock-http-server-for-tests

However, obviously they have switched internally.

To be honest, I'm not a fan of hiding vulnerabilities. If the standalone version is actually more secure (because less dependencies or whatever), then for sure, let's upgrade.

Maybe we should consult the upstream project what they would recommend.

@oleg-nenashev Can you advice us here?

wjglerum commented 8 months ago

Good catch. I've just checked the official documentation of rest-client-reactive: https://quarkus.io/guides/rest-client-reactive#using-a-mock-http-server-for-tests

There they point to the legacy-rest-client where also the standard version is being mentioned: https://quarkus.io/guides/rest-client#using-a-mock-http-server-for-tests

However, obviously they have switched internally.

Yeah seems like that doesn't align indeed. After we have an agreement here, I can create some PRs to update the docs too.

eijckron commented 8 months ago

The use of wiremock is during dev & test so although avoiding vulnerabilities is a good goal it has less relevance during testing. The standalone version at least has the advantage that it will never cause outdated versions into your project production build because of conflicting versions for transitive dependencies

chberger commented 8 months ago

Thanks @eijckron; I got your point.

But would the standalone version actually help in your scenario? I mean the insecure and incompatible dependency would be still part of the uber-jar and so part of the test-classpath. I don't know the exact test-classpath behaviour of Quarkus, but I would assume that you would have to deal with the incompatible libraries at this point as well. So you would start to exclude the compatible Quarkus libraries as well. Or do I miss something here?

eijckron commented 8 months ago

We are still using our own wiremock code until the extension is a bit more mature and we have upgraded to Quarkus 3.2.x. Some time ago we replaced Wiremock (tomakehurst) 2.35.1 with the standalone 3.x (wiremock.org) version and that removed interference with other components from the production build.

chberger commented 8 months ago

@wjglerum Since we don't get more feedback, I would suggest to continue with your implementation proposal. Right now, it looks like the standalone version might have some benefits when it comes to the management of incompatible libraries. At least a user would not need to deal with it at the dependency management level, but rather at the test-classpath level. So, if we figure out this switch is a bad idea, we could switch back anyway...