Skip to content

8271868: Warn user when using mac-sign option with unsigned app-image. #5004

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

Closed
wants to merge 5 commits into from

Conversation

andyherrick
Copy link

@andyherrick andyherrick commented Aug 4, 2021


Progress

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

Issue

  • JDK-8271868: Warn user when using mac-sign option with unsigned app-image.

Reviewers

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/5004/head:pull/5004
$ git checkout pull/5004

Update a local copy of the PR:
$ git checkout pull/5004
$ git pull https://git.openjdk.java.net/jdk pull/5004/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 5004

View PR using the GUI difftool:
$ git pr show -t 5004

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jdk/pull/5004.diff

@bridgekeeper
Copy link

bridgekeeper bot commented Aug 4, 2021

👋 Welcome back herrick! 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 changed the title JDK-8271868: Warn user when using mac-sign option with unsigned app-image 8271868: Warn user when using mac-sign option with unsigned app-image. Aug 4, 2021
@openjdk
Copy link

openjdk bot commented Aug 4, 2021

@andyherrick The following label will be automatically applied to this pull request:

  • core-libs

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

@openjdk openjdk bot added the core-libs core-libs-dev@openjdk.org label Aug 4, 2021
@andyherrick andyherrick marked this pull request as ready for review August 4, 2021 20:17
@openjdk openjdk bot added the rfr Pull request is ready for review label Aug 4, 2021
@mlbridge
Copy link

mlbridge bot commented Aug 4, 2021

Webrevs

// if signing bundle with app-image, warn user if app-image
// is not allready signed.
Path launcher = applicationImage.resolve("Contents/MacOS")
.resolve(APP_NAME.fetchFrom(params));
Copy link
Member

Choose a reason for hiding this comment

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

As far as I remember launcher can be signed, but entire app image might not be signed. So, in this case check will pass, but notarization will fail. I think we should run check on app image itself.

Copy link
Author

Choose a reason for hiding this comment

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

jpackage will either copy the launcher from resource unmodified and unsigned, or sign all the sign-able elements in the app-image (including the launcher).

public static boolean isFileSigned(Path file) {
try {
IOUtils.exec(new ProcessBuilder("/usr/bin/codesign",
"--verify", file.toString()));
Copy link
Member

Choose a reason for hiding this comment

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

--deep should be added, so check is run on nested frameworks and helpers.

@alexeysemenyukoracle
Copy link
Member

What if we save "signed/not signed" flag in ".jpackage.xml" when building app image and read its value instead of guessing if they signed app image or not using codesign?
.jpackage.xml is designed specifically to pass data between multiple invocations of jpackage.

@sashamatveev
Copy link
Member

I think it will not work. User might modify app image after it was created and it will invalidate signature.

@alexeysemenyukoracle
Copy link
Member

What options exist for building a package from app image with invalid signature?

@andyherrick
Copy link
Author

The purpose of this change is to catch the case (and emit a warning) when user creates an app-image w/o using --mac-sign, and then uses that app image to build a pkg or dmg bundle using --mac-sign. For this purpose, checking if the main launcher is signed is sufficient. There is no reason to check all the executables, libraries, and/or Frameworks in the app image., and there is no such thing as signing the app-image itself, also using --deep arg to codesigner is specifically recommended against in all codesigner documentation I have read.
The user can modify the app-image in any way he chooses, possibly invalidating the signing, before using it to create dmg or pkg (or he may create the app-image unsigned, and manually sign all or any part of it. This really has nothing to do with this change. The app-image can still be used to create a dmg or pkg using --mac-sign or not.

The alternative of recording if the app-image was created with --app-sign in the AppImageFile is a reasonable alternative to verifying the signing of the main launchers

@sashamatveev
Copy link
Member

First of all why we only want to cover case when app-image was produced without --mac-sign? If we want to cover such case only, then we need to use approach suggested by Alexey and record signing flag in .jpackage.xml. Otherwise, user will receive false warning if unsigned app-image is provided which was generated without using jpackage or in case if app-image was modified after it was generated and signed.

@alexeysemenyukoracle
Copy link
Member

alexeysemenyukoracle commented Aug 4, 2021

My understanding of this enhancement is to warn user when app image created without --mac-sign is used in building a signed package. I.e. to warn user they misused --mac-sign option. We don't want to check the quality of app image and its suitability for packaging in signed package as a part of this enhancement. For this limited scope checking the value of signing flag recorded in .jpackage.xml seems reasonable approach.

@andyherrick
Copy link
Author

now recording in AppImageFile if signing used to create the app-image, and showing warning only if signing an app using an app-image that is not so recorded as signed.

if (Optional.ofNullable(
SIGN_BUNDLE.fetchFrom(params)).orElse(Boolean.FALSE)) {
// if signing bundle with app-image, warn user if app-image
// is not allready signed.
Copy link

@danielpeintner danielpeintner Aug 5, 2021

Choose a reason for hiding this comment

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

nitpicking: typo "allready" -> "already"

Copy link
Author

Choose a reason for hiding this comment

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

will fix this spelling error

@openjdk
Copy link

openjdk bot commented Aug 5, 2021

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

8271868: Warn user when using mac-sign option with unsigned app-image.

Reviewed-by: almatvee, asemenyuk

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 29 new commits pushed to the master branch:

  • 62e72ad: 8271293: Monitor class should use ThreadBlockInVMPreprocess
  • cb36880: 8270116: Expand ButtonGroupLayoutTraversalTest.java to run in all LaFs, including Aqua on macOS
  • 3ab95d1: 8271905: mark hotspot runtime/Metaspace tests which ignore external VM flags
  • e2c5bfe: 8271308: (fc) FileChannel.transferTo() transfers no more than Integer.MAX_VALUE bytes in one call
  • 7234a43: 8271953: fix mis-merge in JDK-8271878
  • d7fc9e4: 8267840: Improve URLStreamHandler.parseURL()
  • 55bd52a: 8271840: Add simple Integer.toString microbenchmarks
  • 18dd4d4: 8271121: ZGC: stack overflow (segv) when -Xlog:gc+start=debug
  • 685fc3c: 8270903: sun.net.httpserver.HttpConnection: Improve toString
  • 4abe531: 8271722: [TESTBUG] gc/g1/TestMixedGCLiveThreshold.java can fail if G1 Full GC uses >1 workers
  • ... and 19 more: https://git.openjdk.java.net/jdk/compare/452f7d764fc0112cabf0be944e4233173d63f933...master

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.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

@openjdk openjdk bot added the ready Pull request is ready to be integrated label Aug 5, 2021
@andyherrick
Copy link
Author

/integrate

@openjdk
Copy link

openjdk bot commented Aug 6, 2021

Going to push as commit 0aca4f7.
Since your change was applied there have been 39 commits pushed to the master branch:

Your commit was automatically rebased without conflicts.

@openjdk openjdk bot closed this Aug 6, 2021
@openjdk openjdk bot added integrated Pull request has been integrated and removed ready Pull request is ready to be integrated labels Aug 6, 2021
@openjdk
Copy link

openjdk bot commented Aug 6, 2021

@andyherrick Pushed as commit 0aca4f7.

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

@openjdk openjdk bot removed the rfr Pull request is ready for review label Aug 6, 2021
@andyherrick andyherrick deleted the JDK-8271868 branch September 15, 2021 16:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core-libs core-libs-dev@openjdk.org integrated Pull request has been integrated
Development

Successfully merging this pull request may close these issues.

4 participants