8347067: Load certificates without explicit trust settings in KeyChainStore

[timja]

The change

Without this change intermediate certificates that don't have explicit trust settings are ignored not added to the truststore.

Reproducer

See https://github.com/timja/openjdk-intermediate-ca-reproducer

Without this change the reproducer fails, and with this change it succeeds.

Example failing architecture

Root CA -> Intermediate 1 -> Intermediate 2 -> Leaf

Where:

• All certs are in admin domain kSecTrustSettingsDomainAdmin
• Root CA is marked as always trust
• Intermediate 1 and 2 are Unspecified

Previously Root CA would be found but intermediate 1 and 2 would be skipped when verifying trust settings.

Background reading

Rust

see also Rust Lib that is used throughout Rust ecosystem for this:
https://github.com/rustls/rustls-native-certs/blob/efe7b1d77bf6080851486535664d1dc7ef0dea68/src/macos.rs#L39-L58

e.g. in Deno https://github.com/denoland/deno/pull/11491 where I've verified it is correctly implemented and works in my setup

Python

I also looked at the Python implementation for inspiration as well (which also works on my system): https://github.com/sethmlarson/truststore/blob/main/src/truststore/_macos.py

---

Progress

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

Issue

• JDK-8347067: Load certificates without explicit trust settings in KeyChainStore (Enhancement - P4)

Reviewers

• Alexey Bakhtin (@alexeybakhtin - Committer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 22911

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

Using diff file

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

Using Webrev

Link to Webrev Comment

[bridgekeeper]

Hi @timja, welcome to this OpenJDK project and thanks for contributing!

We do not recognize you as Contributor and need to ensure you have signed the Oracle Contributor Agreement (OCA). If you have not signed the OCA, please follow the instructions. Please fill in your GitHub username in the "Username" field of the application. Once you have signed the OCA, please let us know by writing /signed in a comment in this pull request.

If you already are an OpenJDK Author, Committer or Reviewer, please click here to open a new issue so that we can record that fact. Please use "Add GitHub user timja" as summary for the issue.

If you are contributing this work on behalf of your employer and your employer has signed the OCA, please let us know by writing /covered in a comment in this pull request.

[openjdk]

❗ This change is not yet ready to be integrated.
See the Progress checklist in the description for automated requirements.

[openjdk]

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

• security

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.

[timja]

moved so that the empty ArrayList is always created

[alexeybakhtin]

I'm afraid but it looks like this implementation violates the https://developer.apple.com/documentation/security/sectrustsettingscopytrustsettings(_:_:_:) specification.

It always creates an empty input trust and fills it with the provided non-null trustSettings. So, the certificate always has at least empty trustSettings. However, according to this specification, two different scenarios should be considered:

1. Empty trustSettings means that the certificate is trusted if it is self-signed.
2. Null trustSettings means that the certificate must be verified before adding to the trust store.

The current implementation adds Root certificate to the TrustStore because it has "Always Trust" settings.
Implementation does not add the intermediate certificate to the TrustStore because it has "System Defaults" settings ( SecTrustSettingsCopyTrustSettings returns null trustSettings), and the current implementation only adds certificates with trust settings.

I think, in this particular case, we need two iterations to add certificates into the trust store. The first iteration will add certificates with non-null trust settings, and the second iteration should verify and add certificates with null trust settings.

[timja]

I think, in this particular case, we need two iterations to add certificates into the trust store. The first iteration will add certificates with non-null trust settings, and the second iteration should verify and add certificates with null trust settings.

Thanks for the feedback it was very helpful, I had missed the bottom note on https://developer.apple.com/documentation/security/sectrustsettingscopytrustsettings(_:_:_:) before this.

I've implemented the recommendation based on the docs in 0052cd0.

All my test cases are now passing.

I've added a second intermediate CA to my test setup as well although it only uses 1 by default:
https://github.com/timja/openjdk-intermediate-ca-reproducer?rgh-link-date=2025-01-03T11%3A28%3A01Z

I've tested by revoking trust along each part of the chain and its behaving correctly now.

[alexeybakhtin]

Thank you for this patch. It looks correct now (see my comment about subjCerts above)

Is it possible to add jtreg test for this scenario?
Also, You'll need a jbs issue to submit this PR

[timja]

Thank you for this patch. It looks correct now (see my comment about subjCerts above)

Thanks, will look into that

Is it possible to add jtreg test for this scenario?

I'll look at that ~tomorrow

Also, You'll need a jbs issue to submit this PR

Would it be possible for you to do it on my behalf please? I don't have access

[timja]

Is it possible to add jtreg test for this scenario?

I've done some research.

I think it would only be possible with manual intervention to run it.
The certificates could be generated with a script, similar to the existing https://github.com/openjdk/jdk/blob/master/test/jdk/sun/security/x509/DNSName/certs/generate-certs.sh and then checked in.

The certificates could be added to the truststore using /usr/bin/security add-trusted-cert, like in https://github.com/JetBrains/jvm-native-trusted-roots/blob/trunk/src/test/java/org/jetbrains/nativecerts/mac/SecurityFrameworkUtilTest.java#L114-L120

but marking the root certificate as trusted would need the user to confirm an OS prompt, https://github.com/JetBrains/jvm-native-trusted-roots#testing, i.e. I need to approve via Touch ID when I make changes to a certs trust level.

Does that add value to add a test so someone could run it manually?

[alexeybakhtin]

/issue JDK-8347067

[openjdk]

@alexeybakhtin Only the author (@timja) is allowed to issue the /issue command.

[timja]

/issue JDK-8347067

[openjdk]

@timja The primary solved issue for a PR is set through the PR title. Since the current title does not contain an issue reference, it will now be updated.

[alexeybakhtin]

Does that add value to add a test so someone could run it manually?

Yes, I think a manual test is better than nothing.

[timja]

@alexeybakhtin quick question on how this should be marked as manual.

I see all tests in https://github.com/openjdk/jdk/blob/master/test/jdk/TEST.groups#L256-L259 are manual ones.
Is this test automatically included in that?

Or should it be added here?
https://github.com/openjdk/jdk/blob/master/test/jdk/TEST.groups#L657

[alexeybakhtin]

The test should be marked as @run junit/manual and added to the jdk_security_manual_interactive part of the TEST.groups

[timja]

Any idea how I can run the test after making those changes?

The test just gets skipped with:

CONF=release make test TEST=test/jdk/java/security/KeyStore/CheckMacOSKeyChainIntermediateCATrust.java

I've checked through:
https://openjdk.org/jtreg/faq.html#how-do-i-run-only-tests-requiring-manual-intervention

and I saw somewhere mention of passing -manual but I can't get that to work with make test

[timja]

I was able to run it by setting up jtreg and avoiding the make test task.

jtreg.sh -manual test/jdk/java/security/KeyStore/CheckMacOSKeyChainIntermediateCATrust.java

Although it seems it ran anyway without passing manual:

jtreg.sh test/jdk/java/security/KeyStore/CheckMacOSKeyChainIntermediateCATrust.java

Test results: passed: 1

[timja]

Attaching logs are requested on mailing list,

debug-all.txt

cert-path.txt

[timja]

(OCA has been signed by my employer, so should be able to move forward soon)

[mlbridge]

Webrevs

• 04: Full - Incremental (59ceebc4)
• 03: Full - Incremental (2125a8e7)
• 02: Full - Incremental (7d22f46f)
• 01: Full - Incremental (d9605e12)
• 00: Full (2d955702)

[seanjmullan]

/reviewers 2

[openjdk]

@seanjmullan
The total number of required reviews for this PR (including the jcheck configuration and the last /reviewers command) is now set to 2 (with at least 1 Reviewer, 1 Author).

[seanjmullan]

This change is significant and should be reviewed by at least 2 Reviewers.

[alexeybakhtin]

Thank you for the fixes. Looks good to me

[timja]

Possible to get a second reviewer? potentially @wangweij?

Unless @seanjmullan you could suggest someone?