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.

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

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

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.

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.

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

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

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?

/issue JDK-8347067

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

/issue JDK-8347067

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

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

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

Webrevs

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

/reviewers 2

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

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

Possible to get a second reviewer? potentially @wangweij?

Unless @seanjmullan you could suggest someone?

@timja This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

Hi @seanjmullan can you recommend anyone else for review please?

Hi @seanjmullan can you recommend anyone else for review please?

Hi, I was at the JavaOne conference this week. I'll get back to you when I get a chance to review this again.

I am dubious that this is the right thing to do. There is a distinct difference between a certificate that is trusted and one that requires additional validation to determine if it is trusted. Blindly trusting self-signed certificates is very dangeorous, and unless I am mistaken, this seems to be what this code may do. I do not think lumping both of these certificates into "trusted" certificates is the right thing to do.

It may mean that we need to enhance the KeyStore API to be able to store arbitrary certificates. Or you could consider storing these additional certificates in a CertStore.

I am also not sure the use case is common. Typically a web server will send its entire chain, and not just the server certificate.

Certainly open to hearing alternate opinions if I have overlooked something.

I am dubious that this is the right thing to do. There is a distinct difference between a certificate that is trusted and one that requires additional validation to determine if it is trusted. Blindly trusting self-signed certificates is very dangeorous, and unless I am mistaken, this seems to be what this code may do. I do not think lumping both of these certificates into "trusted" certificates is the right thing to do.

There is no 'blind' trust in this pull request. Any 'non trusted' certificate is validated via the OS APIs, once it has been validated by the OS then it is a trusted certificate it just needed its chain verified to the root.

https://github.com/openjdk/jdk/pull/22911/files#diff-b0b2f959f0fa18e568c0688bd233882283bca94f4ff6dce886a21510ff82d64dR475

https://github.com/timja/jdk/blob/59ceebc4acf2c39c86c45b1aa6746bf619417cea/src/java.base/macosx/native/libosxsecurity/KeystoreImpl.m#L475

It may mean that we need to enhance the KeyStore API to be able to store arbitrary certificates. Or you could consider storing these additional certificates in a CertStore.

The KeyStore can store them arbitrarily right now can't it but is it that currently anything in there is 'trusted'?

I am also not sure the use case is common. Typically a web server will send its entire chain, and not just the server certificate.

I don't think its very common either but I think its becoming more common with ZTNA providers (Zero Trust Network Access) being deployed in a lot of organisations for compliance and security reasons.

When these are third party providers they often won't have your root cert and they may or may not provide the complete chain (I don't know if its configurable to do that) my organisation's one certainly doesn't provide the whole chain and there are other bug reports out there from other organisations too.

Certainly open to hearing alternate opinions if I have overlooked something.

An alternative option would be a truststore or some other mechanism that just does SecTrustEvaluateWithError without any of the complexity of providing the actual certificates but just checks the trust for the presented chain. Not sure how complex that would be.

I am dubious that this is the right thing to do. There is a distinct difference between a certificate that is trusted and one that requires additional validation to determine if it is trusted. Blindly trusting self-signed certificates is very dangeorous, and unless I am mistaken, this seems to be what this code may do. I do not think lumping both of these certificates into "trusted" certificates is the right thing to do.

There is no 'blind' trust in this pull request. Any 'non trusted' certificate is validated via the OS APIs, once it has been validated by the OS then it is a trusted certificate it just needed its chain verified to the root.

That helps, but just because it successfully validated once does not mean it should be treated as a trusted certificate (essentially as a trust anchor). It may later be revoked/expired, its key or signature algorithm may weaken or it may fail validation for some other reason, or CA certificates further up the chain may change and affect the trustworthiness of this certificate.

It may mean that we need to enhance the KeyStore API to be able to store arbitrary certificates. Or you could consider storing these additional certificates in a CertStore.

The KeyStore can store them arbitrarily right now can't it but is it that currently anything in there is 'trusted'?

I am also not sure the use case is common. Typically a web server will send its entire chain, and not just the server certificate.

I don't think its very common either but I think its becoming more common with ZTNA providers (Zero Trust Network Access) being deployed in a lot of organisations for compliance and security reasons.

When these are third party providers they often won't have your root cert and they may or may not provide the complete chain (I don't know if its configurable to do that) my organisation's one certainly doesn't provide the whole chain and there are other bug reports out there from other organisations too.

Certainly open to hearing alternate opinions if I have overlooked something.

An alternative option would be a truststore or some other mechanism that just does SecTrustEvaluateWithError without any of the complexity of providing the actual certificates but just checks the trust for the presented chain. Not sure how complex that would be.

I think at this point if this is important enough we would need to explore other options. I haven't had time to really think about the different use cases you mention above, but providing more rationale and pointers to issues where users are having issues with Java because of this issue would help.

Thanks,
Sean

I am dubious that this is the right thing to do. There is a distinct difference between a certificate that is trusted and one that requires additional validation to determine if it is trusted. Blindly trusting self-signed certificates is very dangeorous, and unless I am mistaken, this seems to be what this code may do. I do not think lumping both of these certificates into "trusted" certificates is the right thing to do.

There is no 'blind' trust in this pull request. Any 'non trusted' certificate is validated via the OS APIs, once it has been validated by the OS then it is a trusted certificate it just needed its chain verified to the root.

That helps, but just because it successfully validated once does not mean it should be treated as a trusted certificate (essentially as a trust anchor). It may later be revoked/expired, its key or signature algorithm may weaken or it may fail validation for some other reason, or CA certificates further up the chain may change and affect the trustworthiness of this certificate.

Exactly the same happens with other certificates with trust settings. They are loaded on start and not verified for trust settings later. They are still trusted, If the Trust Settings are updated manually in the native KeyChainStore.
Also, it affects KeychainStore only and does not affect KeychainStore-ROOT certificates, as it is static.

Isnt that the Same for Windows? Not all Stores are explicitely trusted, some contain intermediate certificates. It would certainly be helpful to reflect trust in Keystore API (if only to not Trust the ones which are not marked as such). For example CurrentUser\Root or LocalMachine\CA are such Locations which could ne marking as trusted/not trusted respektively.

We need to be really careful here. With this fix we are deciding at runtime that these intermediate certificates should be treated as KeyStore.TrustedCertificateEntry objects just because they validated ok, and without any interaction with the user or application.

Also, the JDK does not rely on certificate chain validation from the OS. The JDK has its own PKIX CertPathValidator implementation and has its own restrictions on weak algorithms, etc which is a key part of establishing trust in certificates used in TLS, signed JARs, etc. You are now delegating this to MacOS as a mostly invisible change which brings in a new set of security concerns which may make it less secure or at a minimum requires resources to ensure this code is properly reviewed, audited, etc.

We need to be really careful here. With this fix we are deciding at runtime that these intermediate certificates should be treated as KeyStore.TrustedCertificateEntry objects just because they validated ok, and without any interaction with the user or application.

Also, the JDK does not rely on certificate chain validation from the OS. The JDK has its own PKIX CertPathValidator implementation and has its own restrictions on weak algorithms, etc which is a key part of establishing trust in certificates used in TLS, signed JARs, etc. You are now delegating this to MacOS as a mostly invisible change which brings in a new set of security concerns which may make it less secure or at a minimum requires resources to ensure this code is properly reviewed, audited, etc.

This isn't being delegated to macOS anymore so than it already was, trust settings are still checked as the OS does not validate non trusted certificates, this is just in KeychainStore which is opt-in only.

CertPathValidator is still used in finding the path and I assume could still veto weak algorithms etc.
It would be good if it did check the path and not assume all certificates are Trusted Roots.

Yes, exactly the same on Windows, I implemented it in node.js here: nodejs/node#57164 (node's certificate validation does check the path)