You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@maven.apache.org by GitBox <gi...@apache.org> on 2022/08/03 07:10:26 UTC
[GitHub] [maven-wrapper] raphw opened a new pull request, #58: [MWRAPPER-75] Allow for sha256 checksum verification of downloaded artifacts.
raphw opened a new pull request, #58:
URL: https://github.com/apache/maven-wrapper/pull/58
Optionally verify checksums of downloaded binaries, both for Maven wrapper jar and actual Maven distribution.
The verification is optional and is activated by adding checksums to the maven.properties file, either as
'wrapperSha256Sum' (Maven wrapper) or as 'distributionSha256Sum' (Maven distribution).
Following this checklist to help us incorporate your
contribution quickly and easily:
- [ ] Make sure there is a [JIRA issue](https://issues.apache.org/jira/browse/MWRAPPER) filed
for the change (usually before you start working on it). Trivial changes like typos do not
require a JIRA issue. Your pull request should address just this issue, without
pulling in other changes.
- [ ] Each commit in the pull request should have a meaningful subject line and body.
- [ ] Format the pull request title like `[MWRAPPER-XXX] - Fixes bug in ApproximateQuantiles`,
where you replace `MWRAPPER-XXX` with the appropriate JIRA issue. Best practice
is to use the JIRA issue title in the pull request title and in the first line of the
commit message.
- [ ] Write a pull request description that is detailed enough to understand what the pull request does, how, and why.
- [ ] Run `mvn clean verify` to make sure basic checks pass. A more thorough check will
be performed on your pull request automatically.
- [ ] You have run the integration tests successfully (`mvn -Prun-its clean verify`).
If your pull request is about ~20 lines of code you don't need to sign an
[Individual Contributor License Agreement](https://www.apache.org/licenses/icla.pdf) if you are unsure
please ask on the developers list.
To make clear that you license your contribution under
the [Apache License Version 2.0, January 2004](http://www.apache.org/licenses/LICENSE-2.0)
you have to acknowledge this by using the following check-box.
- [ ] I hereby declare this contribution to be licenced under the [Apache License Version 2.0, January 2004](http://www.apache.org/licenses/LICENSE-2.0)
- [ ] In any other case, please file an [Apache Individual Contributor License Agreement](https://www.apache.org/licenses/icla.pdf).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-wrapper] raphw commented on pull request #58: [MWRAPPER-75] Allow for sha256 checksum verification of downloaded artifacts.
Posted by GitBox <gi...@apache.org>.
raphw commented on PR #58:
URL: https://github.com/apache/maven-wrapper/pull/58#issuecomment-1263892672
I used a `@REM` in a PowerShell script where it's supposed to be `#`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-wrapper] michael-o commented on a diff in pull request #58: [MWRAPPER-75] Allow for sha256 checksum verification of downloaded artifacts.
Posted by GitBox <gi...@apache.org>.
michael-o commented on code in PR #58:
URL: https://github.com/apache/maven-wrapper/pull/58#discussion_r936418056
##########
maven-wrapper-distribution/src/resources/mvnw:
##########
@@ -247,6 +247,21 @@ fi
# End of extension
##########################################################################################
+# If specified, validate the SHA-256 sum of the Maven wrapper jar file
+wrapperSha256Sum=""
+while IFS="=" read key value; do
+ case "$key" in (wrapperSha256Sum) wrapperSha256Sum=$value; break ;;
+ esac
+done < "$MAVEN_PROJECTBASEDIR/.mvn/wrapper/maven-wrapper.properties"
Review Comment:
I recommend to test this with a POSIX only shell.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-wrapper] michael-o commented on pull request #58: [MWRAPPER-75] Allow for sha256 checksum verification of downloaded artifacts.
Posted by GitBox <gi...@apache.org>.
michael-o commented on PR #58:
URL: https://github.com/apache/maven-wrapper/pull/58#issuecomment-1259583149
I have expressed my concerns, but will refrain from any further actions. Still referring to: https://github.com/apache/maven-wrapper/pull/58#issuecomment-1259505296
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-wrapper] michael-o commented on a diff in pull request #58: [MWRAPPER-75] Allow for sha256 checksum verification of downloaded artifacts.
Posted by GitBox <gi...@apache.org>.
michael-o commented on code in PR #58:
URL: https://github.com/apache/maven-wrapper/pull/58#discussion_r936336907
##########
maven-wrapper-distribution/src/resources/mvnw:
##########
@@ -247,6 +247,21 @@ fi
# End of extension
##########################################################################################
+# If specified, validate the SHA-256 sum of the Maven wrapper jar file
+wrapperSha256Sum=""
+while IFS="=" read key value; do
+ case "$key" in (wrapperSha256Sum) wrapperSha256Sum=$value; break ;;
+ esac
+done < "$MAVEN_PROJECTBASEDIR/.mvn/wrapper/maven-wrapper.properties"
+if [ -n "$wrapperSha256Sum" ]; then
+ if ! echo "$wrapperSha256Sum $wrapperJarPath" | shasum -a 256 -c > /dev/null 2>&1; then
+ echo "Error: Failed to validate Maven wrapper SHA-256, your Maven wrapper might be compromised." >&2
Review Comment:
I don't like this wording because this implies some security aspect.
##########
maven-wrapper-distribution/src/resources/mvnw.cmd:
##########
@@ -153,6 +153,24 @@ if exist %WRAPPER_JAR% (
)
@REM End of extension
+@REM If specified, validate the SHA-256 sum of the Maven wrapper jar file
+SET WRAPPER_SHA_256_SUM=""
+FOR /F "usebackq tokens=1,2 delims==" %%A IN ("%MAVEN_PROJECTBASEDIR%\.mvn\wrapper\maven-wrapper.properties") DO (
+ IF "%%A"=="wrapperSha256Sum" SET WRAPPER_SHA_256_SUM=%%B
+)
+IF NOT %WRAPPER_SHA_256_SUM%=="" (
+ FOR /F "usebackq tokens=*" %%A in ('certUtil -hashfile "%WRAPPER_JAR%" SHA256') do (
+ echo %%A | findstr /C:"hash" 1>nul || (
+ IF NOT %%A==%WRAPPER_SHA_256_SUM% (
+ echo Error: Failed to validate Maven wrapper SHA-256, your Maven wrapper might be compromised. >&2
Review Comment:
ditto
##########
maven-wrapper-distribution/src/resources/mvnw:
##########
@@ -247,6 +247,21 @@ fi
# End of extension
##########################################################################################
+# If specified, validate the SHA-256 sum of the Maven wrapper jar file
+wrapperSha256Sum=""
+while IFS="=" read key value; do
+ case "$key" in (wrapperSha256Sum) wrapperSha256Sum=$value; break ;;
+ esac
+done < "$MAVEN_PROJECTBASEDIR/.mvn/wrapper/maven-wrapper.properties"
+if [ -n "$wrapperSha256Sum" ]; then
+ if ! echo "$wrapperSha256Sum $wrapperJarPath" | shasum -a 256 -c > /dev/null 2>&1; then
Review Comment:
You should check whether `shasum` is available.
##########
maven-wrapper-distribution/src/resources/mvnw:
##########
@@ -247,6 +247,21 @@ fi
# End of extension
##########################################################################################
+# If specified, validate the SHA-256 sum of the Maven wrapper jar file
+wrapperSha256Sum=""
+while IFS="=" read key value; do
+ case "$key" in (wrapperSha256Sum) wrapperSha256Sum=$value; break ;;
+ esac
+done < "$MAVEN_PROJECTBASEDIR/.mvn/wrapper/maven-wrapper.properties"
Review Comment:
Are you certain that this is POSIX compliant?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-wrapper] raphw commented on pull request #58: [MWRAPPER-75] Allow for sha256 checksum verification of downloaded artifacts.
Posted by "raphw (via GitHub)" <gi...@apache.org>.
raphw commented on PR #58:
URL: https://github.com/apache/maven-wrapper/pull/58#issuecomment-1405016348
It adds a millisecond to the build on my machine. I don't think it's an issue.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-wrapper] jorsol commented on pull request #58: [MWRAPPER-75] Allow for sha256 checksum verification of downloaded artifacts.
Posted by "jorsol (via GitHub)" <gi...@apache.org>.
jorsol commented on PR #58:
URL: https://github.com/apache/maven-wrapper/pull/58#issuecomment-1404041977
Hi @raphw, just found this behavior: https://issues.apache.org/jira/browse/MWRAPPER-93
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-wrapper] slawekjaranowski commented on pull request #58: [MWRAPPER-75] Allow for sha256 checksum verification of downloaded artifacts.
Posted by GitBox <gi...@apache.org>.
slawekjaranowski commented on PR #58:
URL: https://github.com/apache/maven-wrapper/pull/58#issuecomment-1264625016
@raphw - next round ... please look at build result
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-wrapper] raphw commented on pull request #58: [MWRAPPER-75] Allow for sha256 checksum verification of downloaded artifacts.
Posted by GitBox <gi...@apache.org>.
raphw commented on PR #58:
URL: https://github.com/apache/maven-wrapper/pull/58#issuecomment-1259573788
You requested changes on this PR @michael-o so at least you had a previous interest. But I imply that you do not have any concerns then and that this can be merged and released.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-wrapper] szpak commented on pull request #58: [MWRAPPER-75] Allow for sha256 checksum verification of downloaded artifacts.
Posted by GitBox <gi...@apache.org>.
szpak commented on PR #58:
URL: https://github.com/apache/maven-wrapper/pull/58#issuecomment-1264734631
@raphw Unfortunately, there still seems to be some problems on Windows :-/
https://github.com/apache/maven-wrapper/actions/runs/3169982576/jobs/5162469060#step:8:2570
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-wrapper] slawekjaranowski commented on pull request #58: [MWRAPPER-75] Allow for sha256 checksum verification of downloaded artifacts.
Posted by GitBox <gi...@apache.org>.
slawekjaranowski commented on PR #58:
URL: https://github.com/apache/maven-wrapper/pull/58#issuecomment-1264738379
Only one test is failing: `sha256_type_only-script`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-wrapper] raphw commented on pull request #58: [MWRAPPER-75] Allow for sha256 checksum verification of downloaded artifacts.
Posted by GitBox <gi...@apache.org>.
raphw commented on PR #58:
URL: https://github.com/apache/maven-wrapper/pull/58#issuecomment-1265059184
Missed to update the printed error message from the wrapper application this time. Fixed this now.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-wrapper] raphw commented on a diff in pull request #58: [MWRAPPER-75] Allow for sha256 checksum verification of downloaded artifacts.
Posted by GitBox <gi...@apache.org>.
raphw commented on code in PR #58:
URL: https://github.com/apache/maven-wrapper/pull/58#discussion_r936434537
##########
maven-wrapper-distribution/src/resources/mvnw:
##########
@@ -247,6 +247,21 @@ fi
# End of extension
##########################################################################################
+# If specified, validate the SHA-256 sum of the Maven wrapper jar file
+wrapperSha256Sum=""
+while IFS="=" read key value; do
+ case "$key" in (wrapperSha256Sum) wrapperSha256Sum=$value; break ;;
+ esac
+done < "$MAVEN_PROJECTBASEDIR/.mvn/wrapper/maven-wrapper.properties"
+if [ -n "$wrapperSha256Sum" ]; then
+ if ! echo "$wrapperSha256Sum $wrapperJarPath" | shasum -a 256 -c > /dev/null 2>&1; then
+ echo "Error: Failed to validate Maven wrapper SHA-256, your Maven wrapper might be compromised." >&2
Review Comment:
It does imply a security aspect.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-wrapper] slawekjaranowski commented on pull request #58: [MWRAPPER-75] Allow for sha256 checksum verification of downloaded artifacts.
Posted by GitBox <gi...@apache.org>.
slawekjaranowski commented on PR #58:
URL: https://github.com/apache/maven-wrapper/pull/58#issuecomment-1268956087
I will fix
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-wrapper] raphw commented on pull request #58: [MWRAPPER-75] Allow for sha256 checksum verification of downloaded artifacts.
Posted by GitBox <gi...@apache.org>.
raphw commented on PR #58:
URL: https://github.com/apache/maven-wrapper/pull/58#issuecomment-1264726403
Alright, I got hold of a Windows machine. I did the final clean ups without one and of course I broke things after they already worked. Now it should be functional again.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-wrapper] raphw commented on pull request #58: [MWRAPPER-75] Allow for sha256 checksum verification of downloaded artifacts.
Posted by GitBox <gi...@apache.org>.
raphw commented on PR #58:
URL: https://github.com/apache/maven-wrapper/pull/58#issuecomment-1265742206
@slawekjaranowski As for the "validation on download only": The problem with this approach is that it cannot be tested as normally, the distribution will already be downloaded from another build. If this other build did not validate a checksum, the corrupted version is now shared. Also, I am not sure if this is such a good idea from a procedural point of view, if the download succeeds but the validation fails; if the file cannot be deleted afterwards, the corrupted version will continue to exist at the target location.
I have reverted the validation to be executed only during downloads. Also, the SHA-256 computation is not overly expensive, so I do not think this is an actual problem.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-wrapper] raphw commented on pull request #58: [MWRAPPER-75] Allow for sha256 checksum verification of downloaded artifacts.
Posted by "raphw (via GitHub)" <gi...@apache.org>.
raphw commented on PR #58:
URL: https://github.com/apache/maven-wrapper/pull/58#issuecomment-1404975208
This was the original behavior and I pointed it out in this comment: https://github.com/apache/maven-wrapper/pull/58#issuecomment-1265742206 - I was asked to change it here: https://github.com/apache/maven-wrapper/pull/58#discussion_r985951900
I am more than happy to change this back, but wanted to double-check that this is wanted now?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-wrapper] raphw commented on pull request #58: [MWRAPPER-75] Allow for sha256 checksum verification of downloaded artifacts.
Posted by GitBox <gi...@apache.org>.
raphw commented on PR #58:
URL: https://github.com/apache/maven-wrapper/pull/58#issuecomment-1265206947
The problem seems to be that the distribution-only script does not propagate the power shell error level. Not sure how to solve this yet, the script is rather cryptic, but I am still trying to figure it out. If you have a pointer, please let me know!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-wrapper] raphw commented on pull request #58: [MWRAPPER-75] Allow for sha256 checksum verification of downloaded artifacts.
Posted by GitBox <gi...@apache.org>.
raphw commented on PR #58:
URL: https://github.com/apache/maven-wrapper/pull/58#issuecomment-1264750025
> I see the same value of hash for wrapper jar and for Maven distribution ... I miss something.
To avoid such dependencies, the tests check for a checksum mismatch. The SHA-256 is not matching any Maven file.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-wrapper] raphw commented on a diff in pull request #58: [MWRAPPER-75] Allow for sha256 checksum verification of downloaded artifacts.
Posted by GitBox <gi...@apache.org>.
raphw commented on code in PR #58:
URL: https://github.com/apache/maven-wrapper/pull/58#discussion_r936388381
##########
maven-wrapper-distribution/src/resources/mvnw:
##########
@@ -247,6 +247,21 @@ fi
# End of extension
##########################################################################################
+# If specified, validate the SHA-256 sum of the Maven wrapper jar file
+wrapperSha256Sum=""
+while IFS="=" read key value; do
+ case "$key" in (wrapperSha256Sum) wrapperSha256Sum=$value; break ;;
+ esac
+done < "$MAVEN_PROJECTBASEDIR/.mvn/wrapper/maven-wrapper.properties"
Review Comment:
I would think so, that's pretty basic. Also, the same logic is already applied in the script during download.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-wrapper] raphw commented on a diff in pull request #58: [MWRAPPER-75] Allow for sha256 checksum verification of downloaded artifacts.
Posted by GitBox <gi...@apache.org>.
raphw commented on code in PR #58:
URL: https://github.com/apache/maven-wrapper/pull/58#discussion_r936529321
##########
maven-wrapper-distribution/src/resources/mvnw:
##########
@@ -247,6 +247,25 @@ fi
# End of extension
##########################################################################################
+# If specified, validate the SHA-256 sum of the Maven wrapper jar file
+wrapperSha256Sum=""
+while IFS="=" read key value; do
+ case "$key" in (wrapperSha256Sum) wrapperSha256Sum=$value; break ;;
+ esac
+done < "$MAVEN_PROJECTBASEDIR/.mvn/wrapper/maven-wrapper.properties"
+if [ -n "$wrapperSha256Sum" ]; then
+ if ! command -v shasum > /dev/null; then
+ echo "Checksum validation was requested but the 'shasum' command is not available in the current environment."
+ echo "Please install 'shasum' or disable validation by removing 'wrapperSha256Sum' from your maven.properties file."
Review Comment:
Indeed, I chose it as it ships with most Linux distributions and is available on Mac which *sha256sum* is not. I added a switch on the command's availability.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-wrapper] jorsol commented on a diff in pull request #58: [MWRAPPER-75] Allow for sha256 checksum verification of downloaded artifacts.
Posted by GitBox <gi...@apache.org>.
jorsol commented on code in PR #58:
URL: https://github.com/apache/maven-wrapper/pull/58#discussion_r936495287
##########
maven-wrapper-distribution/src/resources/mvnw:
##########
@@ -247,6 +247,25 @@ fi
# End of extension
##########################################################################################
+# If specified, validate the SHA-256 sum of the Maven wrapper jar file
+wrapperSha256Sum=""
+while IFS="=" read key value; do
+ case "$key" in (wrapperSha256Sum) wrapperSha256Sum=$value; break ;;
+ esac
+done < "$MAVEN_PROJECTBASEDIR/.mvn/wrapper/maven-wrapper.properties"
+if [ -n "$wrapperSha256Sum" ]; then
+ if ! command -v shasum > /dev/null; then
+ echo "Checksum validation was requested but the 'shasum' command is not available in the current environment."
+ echo "Please install 'shasum' or disable validation by removing 'wrapperSha256Sum' from your maven.properties file."
Review Comment:
On Linux the `shasum` is a Perl package, so it might not be a good idea depending on this command.
To allow for more compatibility is better to test for the existence of the coreutils version `sha256sum` first, and if not found, fallback to `shasum` (which I doubt will exist on normal installations), not sure if it's the opposite in Mac or others OS, yet on Linux, the `sha256sum` command is the way to go.
Also, the file is maven-wrapper.properties, not maven.properties
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-wrapper] jorsol commented on a diff in pull request #58: [MWRAPPER-75] Allow for sha256 checksum verification of downloaded artifacts.
Posted by GitBox <gi...@apache.org>.
jorsol commented on code in PR #58:
URL: https://github.com/apache/maven-wrapper/pull/58#discussion_r936473189
##########
maven-wrapper-distribution/src/resources/mvnw:
##########
@@ -247,6 +247,21 @@ fi
# End of extension
##########################################################################################
+# If specified, validate the SHA-256 sum of the Maven wrapper jar file
+wrapperSha256Sum=""
+while IFS="=" read key value; do
+ case "$key" in (wrapperSha256Sum) wrapperSha256Sum=$value; break ;;
+ esac
+done < "$MAVEN_PROJECTBASEDIR/.mvn/wrapper/maven-wrapper.properties"
+if [ -n "$wrapperSha256Sum" ]; then
+ if ! echo "$wrapperSha256Sum $wrapperJarPath" | shasum -a 256 -c > /dev/null 2>&1; then
+ echo "Error: Failed to validate Maven wrapper SHA-256, your Maven wrapper might be compromised." >&2
Review Comment:
It *might* imply a security aspect, but also an incomplete download, a failure in the network, a cosmic ray, etc.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-wrapper] jorsol commented on pull request #58: [MWRAPPER-75] Allow for sha256 checksum verification of downloaded artifacts.
Posted by "jorsol (via GitHub)" <gi...@apache.org>.
jorsol commented on PR #58:
URL: https://github.com/apache/maven-wrapper/pull/58#issuecomment-1405007910
Ohhh, I missed that comment, anyway I genuinely think that this should be checked even at the expense of calculating the checksum on every Maven execute.
@slawekjaranowski maybe this need to be revisited since the recent Gradle Wrapper attack (https://blog.gradle.org/wrapper-attack-report) WDYT?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-wrapper] slawekjaranowski commented on pull request #58: [MWRAPPER-75] Allow for sha256 checksum verification of downloaded artifacts.
Posted by GitBox <gi...@apache.org>.
slawekjaranowski commented on PR #58:
URL: https://github.com/apache/maven-wrapper/pull/58#issuecomment-1259639733
@michael-o please remove your change request as last action 😄 to not introduce next questions
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-wrapper] raphw commented on pull request #58: [MWRAPPER-75] Allow for sha256 checksum verification of downloaded artifacts.
Posted by GitBox <gi...@apache.org>.
raphw commented on PR #58:
URL: https://github.com/apache/maven-wrapper/pull/58#issuecomment-1264750306
> @raphw Unfortunately, there still seem to be some problems on Windows :-/ https://github.com/apache/maven-wrapper/actions/runs/3169982576/jobs/5162469060#step:8:2570
I only ran the script on a Windows machine but forgot that there are two. Now it works, hopefully.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-wrapper] raphw commented on pull request #58: [MWRAPPER-75] Allow for sha256 checksum verification of downloaded artifacts.
Posted by GitBox <gi...@apache.org>.
raphw commented on PR #58:
URL: https://github.com/apache/maven-wrapper/pull/58#issuecomment-1265470684
Generally, I agree. Currently, the command script parses itself, extracts the PowerShell script from its own file and executes that, reads the output of it, and then jumps to the end of the file. That's certainly not a very intuitive solution.
I do however see such a rewrite as beyond of the scope of this ticket, but I'd appreciate such a rewrite.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-wrapper] slawekjaranowski commented on pull request #58: [MWRAPPER-75] Allow for sha256 checksum verification of downloaded artifacts.
Posted by GitBox <gi...@apache.org>.
slawekjaranowski commented on PR #58:
URL: https://github.com/apache/maven-wrapper/pull/58#issuecomment-1265822433
Ok, even if local distribution will be corrupted after first download and verification - wrapper will not use it if has already unpacked version unless alwaysUnpack is true.
What do you think to verify before unpacking?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-wrapper] slawekjaranowski commented on pull request #58: [MWRAPPER-75] Allow for sha256 checksum verification of downloaded artifacts.
Posted by GitBox <gi...@apache.org>.
slawekjaranowski commented on PR #58:
URL: https://github.com/apache/maven-wrapper/pull/58#issuecomment-1262679119
@raphw please check build result on windows
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-wrapper] slawekjaranowski commented on a diff in pull request #58: [MWRAPPER-75] Allow for sha256 checksum verification of downloaded artifacts.
Posted by GitBox <gi...@apache.org>.
slawekjaranowski commented on code in PR #58:
URL: https://github.com/apache/maven-wrapper/pull/58#discussion_r981329867
##########
maven-wrapper-plugin/src/main/java/org/apache/maven/plugins/wrapper/WrapperMojo.java:
##########
@@ -98,6 +98,12 @@
@Parameter( defaultValue = "false", property = "includeDebug" )
private boolean includeDebugScript;
+ @Parameter( property = "wrapperSha256Sum" )
+ private String wrapperSha256Sum;
+
+ @Parameter( property = "distributionSha256Sum" )
Review Comment:
Please add documentation
##########
maven-wrapper-plugin/src/main/java/org/apache/maven/plugins/wrapper/WrapperMojo.java:
##########
@@ -98,6 +98,12 @@
@Parameter( defaultValue = "false", property = "includeDebug" )
private boolean includeDebugScript;
+ @Parameter( property = "wrapperSha256Sum" )
Review Comment:
Please add documentation
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-wrapper] raphw commented on pull request #58: [MWRAPPER-75] Allow for sha256 checksum verification of downloaded artifacts.
Posted by GitBox <gi...@apache.org>.
raphw commented on PR #58:
URL: https://github.com/apache/maven-wrapper/pull/58#issuecomment-1265296342
Had to make some adjustments as its not straight-forward to capture the status code. Should work now, but happy to accept improvement suggestions.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-wrapper] jorsol commented on pull request #58: [MWRAPPER-75] Allow for sha256 checksum verification of downloaded artifacts.
Posted by GitBox <gi...@apache.org>.
jorsol commented on PR #58:
URL: https://github.com/apache/maven-wrapper/pull/58#issuecomment-1265309532
> Had to make some adjustments as its not straight-forward to capture the status code. Should work now, but happy to accept improvement suggestions.
Don't know anything about powershell, but my improvement suggestion would be to fully rewrite the `.cmd` scripts using pure PowerShell `.ps1`, this might be a completely different issue (https://issues.apache.org/jira/browse/MWRAPPER-78), but this might help to properly handle such scenario.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-wrapper] slawekjaranowski commented on a diff in pull request #58: [MWRAPPER-75] Allow for sha256 checksum verification of downloaded artifacts.
Posted by GitBox <gi...@apache.org>.
slawekjaranowski commented on code in PR #58:
URL: https://github.com/apache/maven-wrapper/pull/58#discussion_r985951900
##########
maven-wrapper/src/main/java/org/apache/maven/wrapper/Installer.java:
##########
@@ -87,6 +91,14 @@ public Path createDist( WrapperConfiguration configuration )
downloaded = Files.exists( localZipFile );
}
+ if ( verifyDistributionSha256Sum )
+ {
+ verifier.verify( localZipFile,
+ "distributionSha256Sum",
+ Verifier.SHA_256_ALGORITHM,
+ configuration.getDistributionSha256Sum() );
+ }
+
Review Comment:
I would like to execute this block only when new file is downloaded, now we will calculate checksum on every Maven execute.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-wrapper] slawekjaranowski commented on pull request #58: [MWRAPPER-75] Allow for sha256 checksum verification of downloaded artifacts.
Posted by GitBox <gi...@apache.org>.
slawekjaranowski commented on PR #58:
URL: https://github.com/apache/maven-wrapper/pull/58#issuecomment-1268977608
build green
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-wrapper] raphw commented on pull request #58: [MWRAPPER-75] Allow for sha256 checksum verification of downloaded artifacts.
Posted by GitBox <gi...@apache.org>.
raphw commented on PR #58:
URL: https://github.com/apache/maven-wrapper/pull/58#issuecomment-1260068277
I added the missing documentation.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-wrapper] raphw commented on pull request #58: [MWRAPPER-75] Allow for sha256 checksum verification of downloaded artifacts.
Posted by GitBox <gi...@apache.org>.
raphw commented on PR #58:
URL: https://github.com/apache/maven-wrapper/pull/58#issuecomment-1268954444
I'm confused. It builds locally. What would need to be changed?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-wrapper] slawekjaranowski commented on pull request #58: [MWRAPPER-75] Allow for sha256 checksum verification of downloaded artifacts.
Posted by GitBox <gi...@apache.org>.
slawekjaranowski commented on PR #58:
URL: https://github.com/apache/maven-wrapper/pull/58#issuecomment-1268946256
@raphw can you look at https://ci-maven.apache.org/job/Maven/job/maven-box/job/maven-wrapper/job/master/
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-wrapper] slawekjaranowski commented on pull request #58: [MWRAPPER-75] Allow for sha256 checksum verification of downloaded artifacts.
Posted by GitBox <gi...@apache.org>.
slawekjaranowski commented on PR #58:
URL: https://github.com/apache/maven-wrapper/pull/58#issuecomment-1268955664
Maven version
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-wrapper] jorsol commented on a diff in pull request #58: [MWRAPPER-75] Allow for sha256 checksum verification of downloaded artifacts.
Posted by GitBox <gi...@apache.org>.
jorsol commented on code in PR #58:
URL: https://github.com/apache/maven-wrapper/pull/58#discussion_r936495287
##########
maven-wrapper-distribution/src/resources/mvnw:
##########
@@ -247,6 +247,25 @@ fi
# End of extension
##########################################################################################
+# If specified, validate the SHA-256 sum of the Maven wrapper jar file
+wrapperSha256Sum=""
+while IFS="=" read key value; do
+ case "$key" in (wrapperSha256Sum) wrapperSha256Sum=$value; break ;;
+ esac
+done < "$MAVEN_PROJECTBASEDIR/.mvn/wrapper/maven-wrapper.properties"
+if [ -n "$wrapperSha256Sum" ]; then
+ if ! command -v shasum > /dev/null; then
+ echo "Checksum validation was requested but the 'shasum' command is not available in the current environment."
+ echo "Please install 'shasum' or disable validation by removing 'wrapperSha256Sum' from your maven.properties file."
Review Comment:
On Linux the `shasum` is a Perl package, so it might not be a good idea depending on this command.
To allow for more compatibility is better to test for the existence of the coreutils version `sha256sum` first, and if not found, fallback to `shasum` (which I doubt will exist on normal installations), not sure if it's the opposite on Mac or others OS, yet on Linux, the `sha256sum` command is the way to go.
Also, the file is maven-wrapper.properties, not maven.properties
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-wrapper] michael-o commented on pull request #58: [MWRAPPER-75] Allow for sha256 checksum verification of downloaded artifacts.
Posted by GitBox <gi...@apache.org>.
michael-o commented on PR #58:
URL: https://github.com/apache/maven-wrapper/pull/58#issuecomment-1259505296
> What is a status of this PR? @michael-o, @jorsol Do you have any more requests about the provided code?
I am nit interested in Wrapper at all. Therefore, I will neither bless nor merge something.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-wrapper] szpak commented on pull request #58: [MWRAPPER-75] Allow for sha256 checksum verification of downloaded artifacts.
Posted by GitBox <gi...@apache.org>.
szpak commented on PR #58:
URL: https://github.com/apache/maven-wrapper/pull/58#issuecomment-1259487391
What is a status of this PR? @michael-o, @jorsol Do you have any more requests about the provided code?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-wrapper] michael-o commented on pull request #58: [MWRAPPER-75] Allow for sha256 checksum verification of downloaded artifacts.
Posted by GitBox <gi...@apache.org>.
michael-o commented on PR #58:
URL: https://github.com/apache/maven-wrapper/pull/58#issuecomment-1259666362
> @michael-o please remove your change request as last action 😄 to not introduce next questions
Tried. Doesn't work. They have been resolved anyway
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-wrapper] slawekjaranowski commented on pull request #58: [MWRAPPER-75] Allow for sha256 checksum verification of downloaded artifacts.
Posted by GitBox <gi...@apache.org>.
slawekjaranowski commented on PR #58:
URL: https://github.com/apache/maven-wrapper/pull/58#issuecomment-1268951563
hehe i see:
```
Cannot assign configuration entry 'successCodes' with value '1' of type java.lang.String to property of type java.lang.Integer[]
```
`successCodes` is array
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-wrapper] raphw commented on a diff in pull request #58: [MWRAPPER-75] Allow for sha256 checksum verification of downloaded artifacts.
Posted by GitBox <gi...@apache.org>.
raphw commented on code in PR #58:
URL: https://github.com/apache/maven-wrapper/pull/58#discussion_r936434311
##########
maven-wrapper-distribution/src/resources/mvnw:
##########
@@ -247,6 +247,21 @@ fi
# End of extension
##########################################################################################
+# If specified, validate the SHA-256 sum of the Maven wrapper jar file
+wrapperSha256Sum=""
+while IFS="=" read key value; do
+ case "$key" in (wrapperSha256Sum) wrapperSha256Sum=$value; break ;;
+ esac
+done < "$MAVEN_PROJECTBASEDIR/.mvn/wrapper/maven-wrapper.properties"
Review Comment:
Ran it through shellcheck and none of my code is flagged there.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-wrapper] michael-o commented on a diff in pull request #58: [MWRAPPER-75] Allow for sha256 checksum verification of downloaded artifacts.
Posted by GitBox <gi...@apache.org>.
michael-o commented on code in PR #58:
URL: https://github.com/apache/maven-wrapper/pull/58#discussion_r936524786
##########
maven-wrapper-distribution/src/resources/mvnw:
##########
@@ -247,6 +247,25 @@ fi
# End of extension
##########################################################################################
+# If specified, validate the SHA-256 sum of the Maven wrapper jar file
+wrapperSha256Sum=""
+while IFS="=" read key value; do
+ case "$key" in (wrapperSha256Sum) wrapperSha256Sum=$value; break ;;
+ esac
+done < "$MAVEN_PROJECTBASEDIR/.mvn/wrapper/maven-wrapper.properties"
+if [ -n "$wrapperSha256Sum" ]; then
+ if ! command -v shasum > /dev/null; then
+ echo "Checksum validation was requested but the 'shasum' command is not available in the current environment."
+ echo "Please install 'shasum' or disable validation by removing 'wrapperSha256Sum' from your maven.properties file."
Review Comment:
Same here, agree with Jorge.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-wrapper] slawekjaranowski commented on pull request #58: [MWRAPPER-75] Allow for sha256 checksum verification of downloaded artifacts.
Posted by GitBox <gi...@apache.org>.
slawekjaranowski commented on PR #58:
URL: https://github.com/apache/maven-wrapper/pull/58#issuecomment-1264745343
More investigation is need here, eg you use in IT tests
```
distributionSha256Sum=7e0c63c6a99639e57cc64375d6717d72e301d8ab829fef2e145ee860317bc3cb
wrapperSha256Sum =7e0c63c6a99639e57cc64375d6717d72e301d8ab829fef2e145ee860317bc3cb
```
I see the same value of hash for wrapper jar and for Maven distribution ... I miss something.
- Currently IT test use the latest Maven versions, so what will be happen when new version of Maven will be released
- Wrapper jar under test is one of produced by current build, next build can produce another file ...
- Wrapper jar contains eg in MANIFEST.MF information abut jdk used by build ... so when we have the same hash test should filed ....
I not checked it yet ... I only starting to think ...
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org