8264010: Add Gradle dependency verification #437
Conversation
|
👋 Welcome back jgneff! A progress list of the required criteria for merging this PR into |
Webrevs
|
|
/reviewers 2 |
|
@kevinrushforth |
|
This seems like a good idea to do. I have a couple overall questions before reviewing / testing.
|
|
Thanks, Kevin. I added a README file and updated the Lucene instructions, as you suggested. I'm open to any other suggestions on the wording or formatting, no matter how minor.
The Gradle command, now documented in the |
|
Thanks for providing / updating the instructions. My internal test build failed right off the bat, since we have a supplemental closed gradle file that augments the build and downloads additional build tools for our internal CI machines. I don't yet know to handle this, since there is a single, global
|
Would any of the following options work?
I think the protection from the verification file is worth having as a default in the public repository. Gluon, Oracle, BellSoft, and anyone else building JavaFX can decide, based on their own security assessment, whether or not they want to use it. The point of including the file in the repository is to make that decision explicit. |
|
The three options you listed are roughly what I had come up with as well. Option 1 would otherwise be ideal, but it would violate the best practice of not writing to an SCM-managed file during the build. If gradle were to provide a way to specify an alternate location -- even if it were a single global file -- then that's what we'd do. I'll check gradle 7 and see if they've added something like this, since we will want to update at some point in the not-too-distant future (so we can support JDK 16 as a boot JDK). Option 2 could be easily achieved by setting system property Option 3 is what I was leaning towards. I'll take a look (not this week, though) at generating the checksums for the internal files and see what it looks like. It's really only cmake (for WebKit build), Ninja (for WebKit build on Windows), and the compiler toolchain "devkit". Unless I'm forgetting something. |
Just to follow up on that question, all of them are in fact downloaded during the build, at least. I removed the Gradle directory $ rm -r $HOME/.gradle
$ bash gradlew sdk jmods javadoc apps test -x :web:test
...
$ find ~/.gradle/caches/modules-2 \( -name "*.jar" -o -name "*.pom" \) \
-exec basename {} \; | sort > downloaded.log
$ grep '<artifact' gradle/verification-metadata.xml | sed 's/.*name="\(.*\)">/\1/' \
| sort > verified.log
$ diff downloaded.log verified.log
31a32
> org.eclipse.swt.cocoa.macosx.x86_64_3.105.3.v20170228-0512-.jar
32a34
> org.eclipse.swt.win32.win32.x86_64_3.105.3.v20170228-0512-.jarA total of 36 artifacts (14 JAR files and 22 POM files) are downloaded during the build. The SWT libraries for macOS and Windows were not downloaded because I ran the build on Linux. |
|
We have a few in-flight or imminent updates that will impact this PR. There is a tight deadline on one of them (an ICU data file dependency), so I'd prefer to wait on integrating this until after they are done. It's still worth continuing the review in the mean time. I noticed that the libav bundles are missing on Linux. To ensure that you aren't missing any dependencies, can you add the following gradle flags to your build? This will build the native media libraries, including the libav stubs (the latter is Linux only). Eventually, you will need to include WebKit, but that's not needed for now. |
|
When you build media with libav stubs on Linux, you should see the following 5 new entries: And I was right about the additional internal tools that I would need to add. Here is the list: When you build media with libav stubs on Linux, you should see the following 5 new entries: And I was right about the additional tools that I would need to add (as a bonus I found an unused tool that I will eliminate). Here is the list: And here is one that will show up after PR #450 is integrated: And here is one that will show up after PR #450 is integrated: |
|
When you build media with libav stubs on Linux, you should see the following 5 new entries: I'll let you add them. And I was right about the additional internal tools that I would need to add. Here is the list: I'll provide the sha256 sums once I've tested this (probably next week). Speaking of which, we will do a compiler update shortly after JDK 17 does theirs (should be pretty soon for Mac and Windows anyway), which will update the devkits. (as a bonus I found an unused tool that I will eliminate) And here is one more for WebKit that will show up after PR #450 is integrated: This, and then its successor when we update ICU, are the main reason I want to hold off on this for 2-3 more weeks. |
|
Yes, there are two updates:
Since this should be settled down for now, I'll send you the checksums some time later this week (presuming you have added the media and WebKit artifacts by then). |
|
There's an odd thing happening with some of the artifact names in the dependency file. For example: <component group="" name="ffmpeg-3.3.3" version="">
<artifact name="ffmpeg-3.3.3-.tar.gz">
<sha256 value="6600...bf3c" origin="Generated by Gradle"/>
</artifact>
</component>Gradle creates the artifact name from the component name and version, separated by a hyphen. Because the version attribute is empty, we get an artifact name ending in dependencies {
if (IS_BUILD_LIBAV_STUBS) {
media name: "libav-9.14", ext: "tar.gz"
media name: "libav-11.4", ext: "tar.gz"
media name: "libav-12.1", ext: "tar.gz"
media name: "ffmpeg-3.3.3", ext: "tar.gz"
media name: "ffmpeg-4.0.2", ext: "tar.gz"
}
compile project(":base")
compile project(":graphics")
}Other places in the build file seem to depend on having the version as part of the name string, and my initial attempt to separate the two failed. The build works with the names as they are, and these are in fact the names of the files in the Gradle cache. Some day, though, we might want to have better component elements in the dependency file, seeing as it functions also like a software bill of materials. |
|
Yeah, I noticed that about the names, too. Not sure it's worth worrying about, although it is a little odd. Btw, I have the final list of internal downloads that I'll pass on to you, so you can add them to the PR. I've done a CI build with this version of You can see the diffs here: kevinrushforth/jfx@ce1aefa |
|
Thanks, Kevin. I added your list to the file and ran just the Linux build followed by tests, all successful: $ gradle -PCONF=Release -PPROMOTED_BUILD_NUMBER=7 \
-PHUDSON_BUILD_NUMBER=101 -PHUDSON_JOB_NAME=jfx \
-PCOMPILE_WEBKIT=true -PCOMPILE_MEDIA=true -PBUILD_LIBAV_STUBS=true \
sdk jmods javadoc test
...
BUILD SUCCESSFUL in 4m 26s
224 actionable tasks: 93 executed, 131 up-to-date |
|
/contributor add kcr |
|
@jgneff |
gradle/README.txt
Outdated
| When upgrading an external dependency to a newer version, update the | ||
| dependency verification file as follows: | ||
|
|
||
| $ gradle --write-verification-metadata sha256 help |
There was a problem hiding this comment.
This isn't sufficient for many of the dependencies. Gradle won't try to download external dependencies until the point they are used. For example: the junit dependency is downloaded only when running gradle test, the icu data dependency is downloaded only when building the sdk with -PCOMPILE_WEBKIT=true, the libav media libraries (for Linux) are downloaded only when building the sdk with -PCOMPILE_MEDIA=true -PBUILD_LIBAV_STUBS=true, etc.
|
@jgneff Except for the README, I think this is ready. I'm about to send PRs for compiler updates on Windows and Linux, so if they go in first (which seems likely), I'll need to send you an additional entry for each. @johanvos can you also review this? By way of general comment, here is another article highlighting the importance of doing this: |
|
Here are the two additional internal downloads for the new versions of the gcc-10.3 and vs2019-16.9.3 download bundles; can add them to the PR? I've done a CI build with this version of verification.xml and it passes. You can see the diffs here: kevinrushforth/jfx@c0db736 Thanks. |
Thanks, Kevin. I'll assume those replace the older versions shown in the excerpt below, and I'll remove them. Let me know otherwise. <component group="javafx" name="devkit-linux_x64-gcc10.2.0" version="OL6.4+1.0.tar">
<component group="javafx" name="devkit-linux_x64-gcc10.3.0" version="OL6.4+1.0.tar">
<component group="javafx" name="devkit-windows_x64-VS2019" version="16.7.2+1.0.tar">
<component group="javafx" name="devkit-windows_x64-VS2019" version="16.9.3+1.0.tar"> |
|
They do replace the older ones, so it would be OK to remove them. If you are going to do that, then you might also remove the Xcode 11.3 entry. |
Add more details to the file 'gradle/README.txt' on how to create and update the dependency verification file for Linux, macOS, Windows, and the internal Oracle builds.
|
The changes look good. I'm doing one last CI build to double-check everything. One thing I just thought of: Since you removed the old devkit entries, this will need to wait for PR #482. That PR needs one more reviewer and should be ready soon, so I don't think this will cause a delay. |
|
PR #482 is now integrated, so once you get a second review, from Johan, you can integrate this. |
|
Looks good, I'll do a deeper inspection tomorrow. |
|
@jgneff This change now passes all automated pre-integration checks. ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details. After integration, the commit message for the final commit will be: You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed. At the time when this comment was updated there had been 2 new commits pushed to the
Please see this link for an up-to-date comparison between the source branch of this pull request and the ➡️ To integrate this PR with the above commit message to the |
|
/integrate |
openjdk
bot
added
integrated
|
@jgneff Since your change was applied there have been 2 commits pushed to the
Your commit was automatically rebased without conflicts. Pushed as commit a9f6035. 💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored. |
This pull request adds dependency verification to the Gradle builds of JavaFX on Linux, macOS, and Windows. It is the third of three changes that close the gaps in the JavaFX build security:
"Without dependency verification it's easy for an attacker to compromise your supply chain," warns the Gradle User Guide. All three changes come from conference talks by members of the Gradle team, available as PDF slides or on YouTube in the following two videos:
"We all run in a crazy-unsafe environment, in a way," says Louis Jacomet at the end of his talk. These three changes make it just a little less crazy-unsafe for all of us building JavaFX, regardless of our system, network, or country.
Progress
Issue
Reviewers
Contributors
<kcr@openjdk.org>Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.java.net/jfx pull/437/head:pull/437$ git checkout pull/437Update a local copy of the PR:
$ git checkout pull/437$ git pull https://git.openjdk.java.net/jfx pull/437/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 437View PR using the GUI difftool:
$ git pr show -t 437Using diff file
Download this PR as a diff file:
https://git.openjdk.java.net/jfx/pull/437.diff