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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

8262236: Configure Gradle checksum verification #411

Closed
wants to merge 1 commit into from

Conversation

jgneff
Copy link
Member

@jgneff jgneff commented Feb 23, 2021

The recent supply-chain attacks in the news are making me nervous! 馃槦

The Gradle 6.3 distribution is the only software on my OpenJFX build system that doesn't come from an Ubuntu package or a GitHub repository. Ubuntu uses digital signatures to authenticate each package, and Git uses a secure hash algorithm to ensure the integrity of each file, but there is no such check of the Gradle distribution before running it. During my OpenJFX builds, Gradle is downloaded from a Cloudflare server through an HTTPS proxy server, and there's no guarantee that it's the same file as the one published by the Gradle developers.

This pull requests adds the additional step of verifying the Gradle distribution on the build system before extracting its archive and running it.

We might also consider adding the Gradle Wrapper Validation GitHub Action to the OpenJFX repository.


Progress

  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Change must be properly reviewed

Issue

Reviewers

Download

$ git fetch https://git.openjdk.java.net/jfx pull/411/head:pull/411
$ git checkout pull/411

@bridgekeeper
Copy link

bridgekeeper bot commented Feb 23, 2021

馃憢 Welcome back jgneff! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

@openjdk openjdk bot added the rfr Ready for review label Feb 23, 2021
@mlbridge
Copy link

mlbridge bot commented Feb 23, 2021

Webrevs

@kevinrushforth
Copy link
Member

kevinrushforth commented Feb 23, 2021

I presume that just adding the checksum will enable the validation? This sounds like a very good idea. I'll review / test it.

@kevinrushforth
Copy link
Member

We might also consider adding the Gradle Wrapper Validation GitHub Action to the OpenJFX repository.

Feel free to file a bug and create a PR, if you are interested. I agree that this sounds like a good idea.

@jgneff
Copy link
Member Author

jgneff commented Feb 23, 2021

I presume that just adding the checksum will enable the validation?

It does in my tests. See the section "Steps to Reproduce" in the bug report for details. It would be nice to see a message that the distribution was validated successfully, but I can't find any command option to make the Gradle Wrapper do that.

@jgneff
Copy link
Member Author

jgneff commented Feb 23, 2021

We might also consider adding the Gradle Wrapper Validation GitHub Action to the OpenJFX repository.

Feel free to file a bug and create a PR, if you are interested. I agree that this sounds like a good idea.

Isn't this configured directly through GitHub rather than with a pull request? Once that GitHub Action is added, I was thinking of following up with a pull request that upgrades the Gradle Wrapper to version 6.3. The older wrapper is probably fine, but I think we should keep the Wrapper at the same version as the distribution it downloads.

For example, when I followed the instructions for Verifying the integrity of the Gradle Wrapper JAR in the OpenJFX repository, it failed. Only after a brief panic did I reverse the check and discovered that we're still using the older Gradle 4.8 wrapper.

Copy link
Member

@kevinrushforth kevinrushforth left a comment

Choose a reason for hiding this comment

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

Looks good. I confirmed that the checksum is correct, and that a bad checksum will fail the build.

@openjdk
Copy link

openjdk bot commented Feb 23, 2021

@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:

8262236: Configure Gradle checksum verification

Reviewed-by: kcr

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 1 new commit pushed to the master branch:

  • e25d39b: 8260468: Wrong behavior of LocalDateTimeStringConverter

Please see this link for an up-to-date comparison between the source branch of this pull request and the master branch.
As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@kevinrushforth) but any other Committer may sponsor as well.

鉃★笍 To flag this PR as ready for integration with the above commit message, type /integrate in a new comment. (Afterwards, your sponsor types /sponsor in a new comment to perform the integration).

@openjdk openjdk bot added the ready Ready to be integrated label Feb 23, 2021
@kevinrushforth
Copy link
Member

We might also consider adding the Gradle Wrapper Validation GitHub Action to the OpenJFX repository.

Feel free to file a bug and create a PR, if you are interested. I agree that this sounds like a good idea.

Isn't this configured directly through GitHub rather than with a pull request?

My reading of it here is that we would add this action as a step to our workflow script, which is in .github/workflows/submit.yml.

Once that GitHub Action is added, I was thinking of following up with a pull request that upgrades the Gradle Wrapper to version 6.3. The older wrapper is probably fine, but I think we should keep the Wrapper at the same version as the distribution it downloads.

As you noticed, I generally haven't done that when updating gradle versions, but I can see the value in doing so. Since the gradle wrapper is a third-party file that needs to be checked into the repo, someone from Oracle needs to integrate it. As long as it's not causing any problems, I think I'd rather wait until the next time this comes up.

@jgneff
Copy link
Member Author

jgneff commented Feb 23, 2021

My reading of it here is that we would add this action as a step to our workflow script, which is in .github/workflows/submit.yml.

Thanks, Kevin. I'll look into it and follow up with a bug report and pull request. Maybe I'll even add a commit with a bogus Gradle Wrapper to the pull request for testing, which we can then remove.

I think I'd rather wait until the next time this comes up.

That's fine by me.

@jgneff
Copy link
Member Author

jgneff commented Feb 23, 2021

/integrate

@openjdk openjdk bot added the sponsor Ready to sponsor label Feb 23, 2021
@openjdk
Copy link

openjdk bot commented Feb 23, 2021

@jgneff
Your change (at version 209c9d2) is now ready to be sponsored by a Committer.

@kevinrushforth
Copy link
Member

/sponsor

@openjdk openjdk bot closed this Feb 23, 2021
@openjdk openjdk bot added integrated Pull request has been integrated and removed sponsor Ready to sponsor ready Ready to be integrated rfr Ready for review labels Feb 23, 2021
@openjdk
Copy link

openjdk bot commented Feb 23, 2021

@kevinrushforth @jgneff Since your change was applied there has been 1 commit pushed to the master branch:

  • e25d39b: 8260468: Wrong behavior of LocalDateTimeStringConverter

Your commit was automatically rebased without conflicts.

Pushed as commit dc342d3.

馃挕 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

@jgneff jgneff deleted the add-gradle-checksum branch February 23, 2021 19:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
integrated Pull request has been integrated
2 participants