-
Notifications
You must be signed in to change notification settings - Fork 6.1k
8293462: [macos] app image signature invalid when creating DMG or PKG from post processed signed image #10316
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
Conversation
… from post processed signed image
|
👋 Welcome back almatvee! A progress list of the required criteria for merging this PR into |
|
@sashamatveev The following label will be automatically applied to this pull request:
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. |
Webrevs
|
|
Mailing list message from Michael Hall on core-libs-dev:
Alexander, I think I figured out how to include PR?s in my build and this appears good. codesign -v --verbose=4 "/Volumes/BlackJack Blastoff_Unsigned/BlackJack Blastoff_Unsigned.app" This was new? I?m not quite sure what it?s saying but it doesn?t seem to impact what I?m doing. Thanks, FWIW, I ran into Adding this which I noticed in the source? To the two erroring methods? Seemed a fix. I could build. |
… from post processed signed image [v2]
|
Mailing list message from Alexander Matveev on core-libs-dev: Hi Michael, On Sep 16, 2022, at 6:19 PM, Michael Hall <mik3hall at gmail.com<mailto:mik3hall at gmail.com>> wrote: On Sep 16, 2022, at 6:02 PM, Alexander Matveev <almatvee at openjdk.org<mailto:almatvee at openjdk.org>> wrote: Problem is that [JDK-8286850](https://bugs.openjdk.org/browse/JDK-8286850) never set correct value in .jpackage.xml to mark image as signed. [JDK-8289030](https://bugs.openjdk.org/browse/JDK-8289030) reads this value to check if we can add per-user and system wide configuration to packaged app. Fixed by setting correct value in .jpackage.xml when we signing predefine application image. Alexander, I think I figured out how to include PR?s in my build and this appears good. codesign -v --verbose=4 "/Volumes/BlackJack Blastoff_Unsigned/BlackJack Blastoff_Unsigned.app" This was new? I?m not quite sure what it?s saying but it doesn?t seem to impact what I?m doing. This warning means that support for per-user configuration of the installed application will be disabled. We added support for per-user configuration with https://bugs.openjdk.org/browse/JDK-8250950 (Allow per-user and system wide configuration of a jpackaged app). Thanks, Thanks, FWIW, I ran into Adding this which I noticed in the source? To the two erroring methods? Seemed a fix. I could build. -------------- next part -------------- |
|
Mailing list message from Michael Hall on core-libs-dev:
Alexander, I?ll take a look. It sounds like it could be a useful feature at times. The last update still works for me. Thanks again, |
|
Mailing list message from Michael Hall on core-libs-dev:
Alexander, Since both of these locations are external to the application bundle I?m not understanding why they would no longer work with whatever you changed. Mike -------------- next part -------------- |
|
Mailing list message from Michael Hall on core-libs-dev:
Never mind on this. That is no different really from the .cfg file and doesn?t help with changing settings on the installed machine, user or system. |
|
Mailing list message from Michael Hall on core-libs-dev:
Alexander, If you don?t mind a little more opinion on this. When I started trying to figure out what to do with application external files on OS/X I began with the ?Application Support? directories. But it occurred to me that most users rarely look at or often possibly aren?t even aware of the ?Application Support? directory. So if I thought if I want the user to look at or actively manage a file I'd put it in their Documents directory. I also thought it might be nice going for different file types possibly on different platforms if you didn?t have to think about what would be the best place to put things. So I tried to come up with an API that would determine the locations for me and all I would indicate would be the data type. Like? public enum DataTypes { Example usages: Or Where the API determines the best location for log files and you don?t have to be concerned. -------------- next part -------------- |
src/jdk.jpackage/macosx/classes/jdk/jpackage/internal/MacAppImageBuilder.java
Show resolved
Hide resolved
src/jdk.jpackage/share/classes/jdk/jpackage/internal/AppImageFile.java
Outdated
Show resolved
Hide resolved
… from post processed signed image [v3]
|
@sashamatveev 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 161 new commits pushed to the
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 |
|
/integrate |
|
Going to push as commit 1e222bc.
Your commit was automatically rebased without conflicts. |
|
@sashamatveev Pushed as commit 1e222bc. 💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored. |
|
Mailing list message from Michael Hall on core-libs-dev:
Thinking about this I looked at my application that includes java commands and saw that currently I include all. And all appear to be of fixed size. So I assume some kind of launcher stub? I then remembered [macos]: App bundle cannot upload to Mac App Store due to info.plist embedded in java exe The bug being due?
This because the launcher stub includes it?s own Info.pliat?s always using the same CFBundleIdentifier If I understand the resolution correctly? This effectively prohibits jpackage applications going to the App Store from using java native commands by disallowing the jlink option. But wouldn?t it be possible to simply make the CFBundleIdentifier unique? I don?t think the identifiers for the embedded commands need to be meaningful to the developer or anyone else. A good hash should pretty much eliminate collisions. My apologies if I?m simply repeating something dismissed in prior discussion. -------------- next part -------------- |
|
Mailing list message from Michael Hall on core-libs-dev:
It might be an idea, if coming up with a unique or hashed CFBundleIdentifier in the Info.plist isn?t seen as a workable alternative, for jpackage to issue a warning anytime jlink parameters are passed without ?strip-native-commands to issue a warning message that the application will not be eligible for the Mac App Store. -------------- next part -------------- |
|
Mailing list message from Alexander Matveev on core-libs-dev: Hi Michael, It is not possible to provide a unique or hashed CFBundleIdentifier. We already implemented to throw error if ?strip-native-commands are not provided to jlink or if provided runtime contain bin directory. Thanks, On Sep 27, 2022, at 4:44 AM, Michael Hall <mik3hall at gmail.com<mailto:mik3hall at gmail.com>> wrote: On Sep 26, 2022, at 9:24 PM, Michael Hall <mik3hall at gmail.com<mailto:mik3hall at gmail.com>> wrote: On Sep 20, 2022, at 5:50 PM, Michael Hall <mik3hall at gmail.com<mailto:mik3hall at gmail.com>> wrote: Still you could use post-processing to add whatever java binary executable commands you wanted. This again would mean changes to the embedded jdk that might have signing side effects. I haven?t tested. Thinking about this I looked at my application that includes java commands and saw that currently I include all. And all appear to be of fixed size. So I assume some kind of launcher stub? I then remembered [macos]: App bundle cannot upload to Mac App Store due to info.plist embedded in java exe It might be an idea, if coming up with a unique or hashed CFBundleIdentifier in the Info.plist isn?t seen as a workable alternative, for jpackage to issue a warning anytime jlink parameters are passed without ?strip-native-commands to issue a warning message that the application will not be eligible for the Mac App Store. -------------- next part -------------- |
|
Mailing list message from Michael Hall on core-libs-dev:
Alexander Thanks set java.home |
|
Mailing list message from Michael Hall on core-libs-dev:
Alexander, java -cp /Users/mjh PlistZap -i HalfPipe.app/Contents/runtime/Contents/Home/bin/java Showing signature is still valid after signing? And from the application it still works? I realize there are possibly not enough applications involved for this to be a major concern but a hash like fix would be possible. -------------- next part -------------- |
|
Mailing list message from Michael Hall on core-libs-dev:
Alexander You may want to consider if this opens up a security vulnerability that doesn?t currently exist. -------------- next part -------------- |
Problem is that JDK-8286850 never set correct value in .jpackage.xml to mark image as signed. JDK-8289030 reads this value to check if we can add per-user and system wide configuration to packaged app. Fixed by setting correct value in .jpackage.xml when we signing predefine application image.
Added tests to cover this case and added check for values inside .jpackage.xml to validate signed and Mac App Store values.
Progress
Issue
Reviewers
Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.org/jdk pull/10316/head:pull/10316$ git checkout pull/10316Update a local copy of the PR:
$ git checkout pull/10316$ git pull https://git.openjdk.org/jdk pull/10316/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 10316View PR using the GUI difftool:
$ git pr show -t 10316Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/10316.diff