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

Add maven wrapper #1530

Merged
merged 1 commit into from
Apr 29, 2023
Merged

Add maven wrapper #1530

merged 1 commit into from
Apr 29, 2023

Conversation

tegorov
Copy link
Contributor

@tegorov tegorov commented Apr 29, 2023

Hi! I want to add the maven wrapper to make it easier to build https://maven.apache.org/wrapper/

@ggrossetie
Copy link
Member

Looking good, thanks 👍🏻

@ggrossetie ggrossetie merged commit cadffdd into yuzutech:main Apr 29, 2023
@tegorov tegorov deleted the add-maven-wrapper branch April 30, 2023 03:24
# specific language governing permissions and limitations
# under the License.
distributionUrl=https://repo.maven.apache.org/maven2/org/apache/maven/apache-maven/3.8.1/apache-maven-3.8.1-bin.zip
wrapperUrl=https://repo.maven.apache.org/maven2/org/apache/maven/wrapper/maven-wrapper/3.2.0/maven-wrapper-3.2.0.jar
Copy link

@vermeeren vermeeren May 17, 2023

Choose a reason for hiding this comment

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

@tegorov @ggrossetie Should the checksums not be added here as well? From https://maven.apache.org/wrapper/

To avoid supply-chain-attacks by downloading a corrupted artifact, it is possible to specify checksums for both the maven-wrapper.jar and the downloaded distribution. To apply verification, add the expected file's SHA-256 sum in hex notation, using only small caps, to maven-wrapper.properties. The property for validating the maven-wrapper.jar file is named wrapperSha256Sum whereas the distribution file property is named distributionSha256Sum.

Without this, if the repo/site is compromised the mvnw will effectively download and execute arbitrary code on the machine without any questions, which not only can compromise the machine building Kroki but also can inject malicious code into the built Kroki server.

I haven't checked other parts of Kroki for dependency checksum verification or similar, so there may also be other locations where resources are fetched and used/executed without any verification.

Copy link
Member

Choose a reason for hiding this comment

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

That's a good point 👍🏻
Does Maven provide an option to generate/get the wrapperSha256Sum and distributionSha256Sum?

Choose a reason for hiding this comment

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

@ggrossetie I looked but couldn't find anything, I believe one has to manually set these when updating the maven-wrapper.properties, making it effectively trust-on-first-use.

I do notice they PGP-sign the binaries as well, the .asc file is available from https://repo.maven.apache.org/maven2/org/apache/maven/wrapper/maven-wrapper/3.2.0/, but how to use this properly seems not-so-easy to implement compared to pinning checksums.

In general I also tried looking into dependency checksum verification for the pom.xml, but this appears to be a real rabbit hole with Maven. Ideally all dependencies that are pinned have their checksum pinned in the Kroki repository too to prevent man-in-the-middle supply chain attacks. package-lock.json does this for node, but I couldn't really find info about this with Maven and only some vague hits with Gradle.

Personally not familiar with Java ecosystem at all, this might be worth a follow-up issue though. Your thoughts on this?

Copy link
Member

Choose a reason for hiding this comment

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

Personally not familiar with Java ecosystem at all, this might be worth a follow-up issue though. Your thoughts on this?

Yes please, open a new issue so we can discuss how to address this concern 👍🏻

Choose a reason for hiding this comment

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

@ggrossetie Done, ended up a bit broader/bigger than expected but hopefully will be of good use! :p

See #1551

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants