8308042: [macos] Developer ID Application Certificate not picked up by jpackage if it contains UNICODE characters #15394

sashamatveev · 2023-08-22T21:59:50Z

Added support for certificates with UNICODE characters.
Added new approach to find certificate using "security" and "openssl" commands. Just "security" does not works, since it can truncate certificates name. "security" is used to dump certificate in PEM format and then "openssl" to get subject form PEM.
New approach works with UNICODE and ASCII, but I left old approach to avoid regressions. If old approach fails to find certificate (UNICODE or very long subject case) we will fallback to new approach.
Added tests for UNICODE to SigningAppImageTest and SigningPackageTest.java. I do not think that we need to cover other signing cases for UNICODE.

Progress

Change must be properly reviewed (1 review required, with at least 1 Reviewer)
Change must not contain extraneous whitespace
Commit message must refer to an issue

Issue

JDK-8308042: [macos] Developer ID Application Certificate not picked up by jpackage if it contains UNICODE characters (Bug - P3)

Reviewers

Alexey Semenyuk (@alexeysemenyukoracle - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/15394/head:pull/15394
$ git checkout pull/15394

Update a local copy of the PR:
$ git checkout pull/15394
$ git pull https://git.openjdk.org/jdk.git pull/15394/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 15394

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/15394.diff

Webrev

Link to Webrev Comment

…y jpackage if it contains UNICODE characters

bridgekeeper · 2023-08-22T22:01:07Z

👋 Welcome back almatvee! 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 · 2023-08-22T22:02:44Z

@sashamatveev 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.

mlbridge · 2023-08-22T22:14:48Z

Webrevs

00: Full (4cba4b2c)

mlbridge · 2023-08-22T23:17:52Z

Mailing list message from Alan Snyder on core-libs-dev:

I?m confused by this.

The issue is marked as macOS, but on macOS you don?t need to ?find? the certificate, codesign finds it using the text supplied by the user. jpackage does not need to understand this text.

Surely codesign can handle certificates with unicode names, can?t it?

openjdk · 2023-08-23T17:45:01Z

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

8308042: [macos] Developer ID Application Certificate not picked up by jpackage if it contains UNICODE characters

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

fae3b02: 8314746: Remove unused private put* methods from DirectByteBufferR
096b7ff: 8314810: (fs) java/nio/file/Files/CopyInterference.java should use TestUtil::supportsLinks
6261020: 8312555: Ideographic characters aren't stretched by AffineTransform.scale(2, 1)
703817d: 8314517: some tests fail in case ipv6 is disabled on the machine
742e319: 8314157: G1: "yielded" is not initialized on some paths after JDK-8140326
1cee3b9: 8313262: C2: Sinking node may cause required cast to be dropped
f8203cb: 8313626: C2 crash due to unexpected exception control flow
2be469f: 8314743: Use of uninitialized local in SR_initialize after JDK-8314114
571c435: 8313374: --enable-ccache's CCACHE_BASEDIR breaks builds
d1de3d0: 8313901: [TESTBUG] test/hotspot/jtreg/compiler/codecache/CodeCacheFullCountTest.java fails with java.lang.VirtualMachineError
... and 119 more: https://git.openjdk.org/jdk/compare/ec0cc6300a02dd92b25d9072b8b3859dab583bbd...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.

sashamatveev · 2023-08-23T20:19:34Z

/integrate

openjdk · 2023-08-23T20:22:15Z

Going to push as commit 57a322d.
Since your change was applied there have been 133 commits pushed to the master branch:

38a9edf: 8314679: SA fails to properly attach to JVM after having just detached from a different JVM
2c60cad: 8280743: HSDB "Monitor Cache Dump" command might throw NPE
9435cd1: 8175874: Update Security.insertProviderAt to specify behavior when requested position is out of range.
dbb788f: 8294535: Add screen capture functionality to PassFailJFrame
fae3b02: 8314746: Remove unused private put* methods from DirectByteBufferR
096b7ff: 8314810: (fs) java/nio/file/Files/CopyInterference.java should use TestUtil::supportsLinks
6261020: 8312555: Ideographic characters aren't stretched by AffineTransform.scale(2, 1)
703817d: 8314517: some tests fail in case ipv6 is disabled on the machine
742e319: 8314157: G1: "yielded" is not initialized on some paths after JDK-8140326
1cee3b9: 8313262: C2: Sinking node may cause required cast to be dropped
... and 123 more: https://git.openjdk.org/jdk/compare/ec0cc6300a02dd92b25d9072b8b3859dab583bbd...master

Your commit was automatically rebased without conflicts.

openjdk · 2023-08-23T20:22:24Z

@sashamatveev Pushed as commit 57a322d.

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

mlbridge · 2023-08-23T20:26:08Z

Mailing list message from Alexander Matveev on core-libs-dev:

Hi Alan,

On Aug 22, 2023, at 3:35 PM, Alan Snyder <javalists at cbfiddle.com> wrote:

I?m confused by this.

The issue is marked as macOS, but on macOS you don?t need to ?find? the certificate, codesign finds it using the text supplied by the user. jpackage does not need to understand this text.

This is done for error handling. If signing is requested jpackage tries to find the certificate to make sure it is exist and is not expired and will exit with error if it does not exist or expired. In case if we just pass user provided information to codesign, then jpackage will fail during signing and after app image was generated. jpackage will fail faster if user mistyped certificate name in case when jpackage checks for it first.

Second reason is that both jpackage and codesign will find certificate if it contains provided key name/identity. codesign will fail if it finds two or more certificates, but jpackage will use first one with warning message.

Surely codesign can handle certificates with unicode names, can?t it?

Yes it can, but problem was is that our certificate validation code was not able to handle certificates with Unicode names.

Thanks,
Alexander

mlbridge · 2023-08-23T20:26:10Z

Mailing list message from Alan Snyder on core-libs-dev:

On Aug 22, 2023, at 4:42 PM, Alexander Matveev <alexander.matveev at oracle.com> wrote:

Hi Alan,

On Aug 22, 2023, at 3:35 PM, Alan Snyder <javalists at cbfiddle.com> wrote:

I?m confused by this.

The issue is marked as macOS, but on macOS you don?t need to ?find? the certificate, codesign finds it using the text supplied by the user. jpackage does not need to understand this text.

This is done for error handling. If signing is requested jpackage tries to find the certificate to make sure it is exist and is not expired and will exit with error if it does not exist or expired. In case if we just pass user provided information to codesign, then jpackage will fail during signing and after app image was generated. jpackage will fail faster if user mistyped certificate name in case when jpackage checks for it first.

The problem with this solution is that it introduces bugs. This bug and JDK-8311877 both result from jpackage trying to perform its own certificate search instead of letting codesign do the search.

The claimed advantage of failing ?faster" is negligible (it is a small difference and only happens when the user has made a mistake) and not worth the (proven) risk of introducing bugs.

If you think you can do a better job of diagnosing the failure to find a certificate than codesign, then you should post-process the failure of codesign. But I don?t see this as a worthwhile investment.

Second reason is that both jpackage and codesign will find certificate if it contains provided key name/identity. codesign will fail if it finds two or more certificates, but jpackage will use first one with warning message.

Surely codesign can handle certificates with unicode names, can?t it?

Yes it can, but problem was is that our certificate validation code was not able to handle certificates with Unicode names.

Exactly my point. By doing your own certificate validation you risk doing it wrong.

Thanks,
Alexander

mlbridge · 2023-08-23T20:26:12Z

Mailing list message from Michael Hall on core-libs-dev:

On Aug 22, 2023, at 9:40 PM, Alan Snyder <javalists at cbfiddle.com> wrote:

On Aug 22, 2023, at 4:42 PM, Alexander Matveev <alexander.matveev at oracle.com> wrote:

Hi Alan,

On Aug 22, 2023, at 3:35 PM, Alan Snyder <javalists at cbfiddle.com> wrote:

I?m confused by this.

The issue is marked as macOS, but on macOS you don?t need to ?find? the certificate, codesign finds it using the text supplied by the user. jpackage does not need to understand this text.

This is done for error handling. If signing is requested jpackage tries to find the certificate to make sure it is exist and is not expired and will exit with error if it does not exist or expired. In case if we just pass user provided information to codesign, then jpackage will fail during signing and after app image was generated. jpackage will fail faster if user mistyped certificate name in case when jpackage checks for it first.

The problem with this solution is that it introduces bugs. This bug and JDK-8311877 both result from jpackage trying to perform its own certificate search instead of letting codesign do the search.

The claimed advantage of failing ?faster" is negligible (it is a small difference and only happens when the user has made a mistake) and not worth the (proven) risk of introducing bugs.

If you think you can do a better job of diagnosing the failure to find a certificate than codesign, then you should post-process the failure of codesign. But I don?t see this as a worthwhile investment.

Second reason is that both jpackage and codesign will find certificate if it contains provided key name/identity. codesign will fail if it finds two or more certificates, but jpackage will use first one with warning message.

Surely codesign can handle certificates with unicode names, can?t it?

Yes it can, but problem was is that our certificate validation code was not able to handle certificates with Unicode names.

Exactly my point. By doing your own certificate validation you risk doing it wrong.

Assuming code sign will catch the errors discussed or even not.

Would it make sense to do post validation of the entire app after completion?

echo '*******************'
echo 'verifying signature'
echo '*******************'
codesign -v --verbose=4 outputdir/HalfPipe.app
echo '********************'
echo 'spctl assess install'
echo '********************'
#spctl --assess --type install --verbose=4 outputdir/HalfPipe.app
echo '********************'
echo 'spctl assess execute'
echo '********************'
#spctl --assess --type execute --verbose=4 outputdir/HalfPipe.app

I don?t remember why I killed the spctl checks. Maybe they were flagging errors that weren?t?

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.org/pipermail/core-libs-dev/attachments/20230823/5d2b7562/attachment.htm>

mlbridge · 2023-08-24T05:59:09Z

Mailing list message from Alan Snyder on core-libs-dev:

What?s the point of reviewing if the reviews are ignored?

mlbridge · 2023-08-24T05:59:10Z

Mailing list message from Alexander Matveev on core-libs-dev:

Hi Alan,

On Aug 22, 2023, at 7:40 PM, Alan Snyder <javalists at cbfiddle.com<mailto:javalists at cbfiddle.com>> wrote:

On Aug 22, 2023, at 4:42 PM, Alexander Matveev <alexander.matveev at oracle.com<mailto:alexander.matveev at oracle.com>> wrote:

Hi Alan,

On Aug 22, 2023, at 3:35 PM, Alan Snyder <javalists at cbfiddle.com<mailto:javalists at cbfiddle.com>> wrote:

I?m confused by this.

The issue is marked as macOS, but on macOS you don?t need to ?find? the certificate, codesign finds it using the text supplied by the user. jpackage does not need to understand this text.

This is done for error handling. If signing is requested jpackage tries to find the certificate to make sure it is exist and is not expired and will exit with error if it does not exist or expired. In case if we just pass user provided information to codesign, then jpackage will fail during signing and after app image was generated. jpackage will fail faster if user mistyped certificate name in case when jpackage checks for it first.

The problem with this solution is that it introduces bugs. This bug and JDK-8311877 both result from jpackage trying to perform its own certificate search instead of letting codesign do the search.

The claimed advantage of failing ?faster" is negligible (it is a small difference and only happens when the user has made a mistake) and not worth the (proven) risk of introducing bugs.

If you think you can do a better job of diagnosing the failure to find a certificate than codesign, then you should post-process the failure of codesign. But I don?t see this as a worthwhile investment.

JDK-8308042 is about fixing bug in current approach used by jpackage. You suggestion is to passthrough value from ?--mac-signing-key-user-name? to codesign and use it as value for "--sign? argument in codesign will change behavior of ?--mac-signing-key-user-name? and I do not think that we should be doing this as part of this bug fix.From documentation:
https://docs.oracle.com/en/java/javase/20/jpackage/support-application-features.html#GUID-8D9F0607-91F4-4070-8823-02FCAB12238D

If you want to distribute your application outside the Mac App Store, then you'll need the certificates "Developer ID Application: <user or team name>" and "Developer ID Installer: <user or team name>".

If you want to deploy your application through the Mac App Store, then you'll need the certificates "3rd Party Mac Developer Application: <user or team name>" and "3rd Party Mac Developer Installer: <user or team name>?.

If --mac-app-store is set we will look for "3rd Party Mac Developer Application: <user or team name>? and if we cannot find it we will fallback to "Developer ID Application: <user or team name>? and if it is not found we will fail. If --mac-app-store is not set we will look for "Developer ID Application: <user or team name>?. <user or team name> is value provided via ?--mac-signing-key-user-name?.

I think you suggestion is to add additional argument to jpackage which will be used as passthrough value for "--sign? argument in codesign. I am still not sure if we should be doing this, but I am currently considering it as a solution for https://bugs.openjdk.org/browse/JDK-8311877 instead of adding "Apple Development? as fallback if we cannot find "Developer ID Application?.

Thanks,
Alexander

Second reason is that both jpackage and codesign will find certificate if it contains provided key name/identity. codesign will fail if it finds two or more certificates, but jpackage will use first one with warning message.

Surely codesign can handle certificates with unicode names, can?t it?

Yes it can, but problem was is that our certificate validation code was not able to handle certificates with Unicode names.

Exactly my point. By doing your own certificate validation you risk doing it wrong.

Thanks,
Alexander

On Aug 22, 2023, at 3:15 PM, Alexander Matveev <almatvee at openjdk.org<mailto:almatvee at openjdk.org>> wrote:

- Added support for certificates with UNICODE characters.
- Added new approach to find certificate using "security" and "openssl" commands. Just "security" does not works, since it can truncate certificates name. "security" is used to dump certificate in PEM format and then "openssl" to get subject form PEM.
- New approach works with UNICODE and ASCII, but I left old approach to avoid regressions. If old approach fails to find certificate (UNICODE or very long subject case) we will fallback to new approach.
- Added tests for UNICODE to SigningAppImageTest and SigningPackageTest.java. I do not think that we need to cover other signing cases for UNICODE.

-------------

Commit messages:
- 8308042: [macos] Developer ID Application Certificate not picked up by jpackage if it contains UNICODE characters

Changes: https://git.openjdk.org/jdk/pull/15394/files
Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=15394&range=00
Issue: https://bugs.openjdk.org/browse/JDK-8308042
Stats: 444 lines in 11 files changed: 248 ins; 60 del; 136 mod
Patch: https://git.openjdk.org/jdk/pull/15394.diff
Fetch: git fetch https://git.openjdk.org/jdk.git pull/15394/head:pull/15394

PR: https://git.openjdk.org/jdk/pull/15394

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.org/pipermail/core-libs-dev/attachments/20230823/20942884/attachment.htm>

mlbridge · 2023-08-24T05:59:12Z

Mailing list message from Alexander Matveev on core-libs-dev:

Hi Michael,

On Aug 22, 2023, at 11:33 PM, Michael Hall <mik3hall at gmail.com<mailto:mik3hall at gmail.com>> wrote:

On Aug 22, 2023, at 9:40 PM, Alan Snyder <javalists at cbfiddle.com<mailto:javalists at cbfiddle.com>> wrote: