Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

chore: address testcontainers vulnerability by replacing with docker-java #1088

Merged
merged 9 commits into from
May 3, 2024

Conversation

ianbotsf
Copy link
Contributor

@ianbotsf ianbotsf commented May 2, 2024

Issue #

https://github.com/smithy-lang/smithy-kotlin/security/dependabot/9

Description of changes

The Testcontainers utility we use for proxy testing has a dependency on a vulnerable version of Apache Commons Compress (see testcontainers/testcontainers-java#8338, CVE-2024-25710, and CVE-2024-26308). The library maintainers refuse to upgrade to a safe version because of API compatibility, which indicates their maintenance policy isn't robust enough for our usage.

This PR replaces Testcontainers with the Docker Java client and adds utility classes MitmContainer, Docker, and Poller to replace the functionality provided by Testcontainers's abstractions.

Note: I've only tested this on my dev box (EC2 instance running AL2) so far...we'll want to test in a few places to ensure it's as available as the previous solution.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@ianbotsf ianbotsf added the no-changelog Indicates that a changelog entry isn't required for a pull request. Use sparingly. label May 2, 2024
@ianbotsf ianbotsf requested a review from a team as a code owner May 2, 2024 19:03

This comment has been minimized.

1 similar comment

This comment has been minimized.

@ianbotsf ianbotsf marked this pull request as draft May 2, 2024 19:21

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

@ianbotsf ianbotsf marked this pull request as ready for review May 3, 2024 14:00

This comment has been minimized.

@@ -83,6 +83,9 @@ allprojects {
)
}
}

// Enables running `./gradlew allDeps` to get a comprehensive list of dependencies for every subproject
tasks.register<DependencyReportTask>("allDeps") { }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this different from ./gradlew dependencies?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, ./gradlew dependencies only shows the dependencies of the target project (root if otherwise unspecified). I initially ran ./gradlew dependencies to grep for where we transitively consumed the Apache Commons Compress library but couldn't find it because it only showed me the root project dependencies.

Running ./gradlew :runtime:protocol:http-client-engines:test-suite:dependencies shows our use of Apache Commons Compress but then you have to know that :runtime:protocol:http-client-engines:test-suite is where to look. This new task registers an effective alias of dependencies for every subproject in such a way that it can be invoked at the root project and yield the dependencies for root and every subproject.

@@ -24,7 +24,7 @@ kotest-version = "5.8.0"
kotlin-compile-testing-version = "1.5.0"
kotlinx-benchmark-version = "0.4.9"
kotlinx-serialization-version = "1.6.0"
testcontainers-version = "1.19.1"
docker-client-version = "3.3.6"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: docker-java-version might be more accurate

// Port used for communication with container
private val exposedPort = ExposedPort.tcp(CONTAINER_PORT)

class MitmContainer(vararg options: String) : Closeable {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: missing KDocs

Copy link

github-actions bot commented May 3, 2024

Affected Artifacts

Significantly increased in size

Artifact Pull Request (bytes) Latest Release (bytes) Delta (bytes) Delta (percentage)
telemetry-api-jvm.jar 121,407 101,389 20,018 19.74%
aws-protocol-core-jvm.jar 24,042 22,705 1,337 5.89%
Changed in size
Artifact Pull Request (bytes) Latest Release (bytes) Delta (bytes) Delta (percentage)
runtime-core-jvm.jar 878,947 878,670 277 0.03%
http-client-jvm.jar 330,654 330,607 47 0.01%

@ianbotsf ianbotsf merged commit 7d2012a into main May 3, 2024
15 checks passed
@ianbotsf ianbotsf deleted the chore-testcontainers-vuln branch May 3, 2024 16:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
acknowledge-artifact-size-increase no-changelog Indicates that a changelog entry isn't required for a pull request. Use sparingly.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants