8292033: Move jdk.X509Certificate event logic to JCA layer

[coffeys]

By moving the JFR event up to the java.security.cert.CertificateFactory class, we can record all generate cert events, including those from 3rd party providers. I've also altered the logic so that an event is genertate for every generate cert call (not just ones missing from the JDK provider implementation cache)

test case also updated to capture new logic

---

Progress

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

Issue

• JDK-8292033: Move jdk.X509Certificate event logic to JCA layer

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 10422

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

Using diff file

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

[bridgekeeper[bot]]

:wave: Welcome back coffeys! 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[bot]]

@coffeys The following labels will be automatically applied to this pull request:

• core-libs
• security

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

[mlbridge[bot]]

Webrevs

• 06: Full (d6faa84a)
• 05: Full - Incremental (dda445fd)
• 04: Full - Incremental (e3635647)
• 03: Full (f430a3ee)
• 02: Full - Incremental (ff67470a)
• 01: Full (c58e0fa6)
• 00: Full (e145a1ae)

[AlanBateman]

/label remove core-libs

[openjdk[bot]]

@AlanBateman The core-libs label was successfully removed.

[seanjmullan]

I think this will miss cases where the certificates are part of a chain, and the application (or JDK code) is calling CertificateFactory.generateCertPath or generateCertificates, whereas the previous code would not have missed it (if not using a 3rd-party provider) as it was firing the event at a lower layer in the provider code.

I think this is fixable though. In these methods, you can iterate over the certificates that are in the Collection or CertPath and log an event for each.

[seanjmullan]

I think this will miss cases where the certificates are part of a chain, and the application (or JDK code) is calling CertificateFactory.generateCertPath or generateCertificates, whereas the previous code would not have missed it (if not using a 3rd-party provider) as it was firing the event at a lower layer in the provider code.

Actually, I think the previous code missed these cases as well. But I think it is important to try to fix this. If not as part of this issue, then a separate issue. It is just for the cases where the above methods take an InputStream.

[coffeys]

Thanks for the feedback Sean. Yes - this event should also cater for the internal new X509CertImpl type calls that are sprinkled through some of the security libraries.

Some look a bit suspicious perhaps ? I see OCSP/CertPath type calls to new X509CertImpl --- given that CertPath and CertificateFactory are viewed as two different services at the JCA level, I wonder if they should be routing calls back to java.security.cert.CertificateFactory#generateCertificate when generating certs ?

I'll study further and see if we can maximize the number of X509Certificate JFR events that are captured.

[wangweij]

If this event is mainly for public access through the CertificateFactory API, then there is no need to care about internal creation of X509CertImpl directly.

[seanjmullan]

Thanks for the feedback Sean. Yes - this event should also cater for the internal new X509CertImpl type calls that are sprinkled through some of the security libraries.

Some look a bit suspicious perhaps ? I see OCSP/CertPath type calls to new X509CertImpl --- given that CertPath and CertificateFactory are viewed as two different services at the JCA level, I wonder if they should be routing calls back to java.security.cert.CertificateFactory#generateCertificate when generating certs ?

Yes, that code should ideally go through CertificateFactory and not call new X509CertImpl directly.

There's some old code in sun.security.pkcs.PKCS7 that also calls new X509CertImpl if it cannot instantiate an X.509 CertificateFactory, but I think that code can be removed since an "X.509" CertificateFactory is a requirement.

[coffeys]

on further reading, it turns out that code like CertificateFactory.generateCertPath or generateCertificates need not have an explicit X509Cert event recording. In theory, that implementation should call into CertificateFactory.generateCertificate to generate the underlying certificates. Some of the JDK implementation doesn't go down that route and I've added an X509CertImpl getter method to help in those scenarios. (to construct and record an X509CertImpl instance)

Recording cert events from CertificateFactory.generateCertPath or generateCertificates would most likely lead to duplicate certificates being recorded. It depends on how the 3rd party providers are coded of course and we have no control over that.

I've beefed up the test logic to cover the various CertificateFactory methods that raised concern. I also included a CertAndGen example to cover what keytool might do in such scenarios.

@seanjmullan - I'll log a new bug to cover the sun.security.pkcs.PKCS7 code issues you highlight

[seanjmullan]

Do you think it is that useful to have keytool record events? Ok, I guess some apps could be execing keytool, but that would be in a separate process, and probably wouldn't have JFR enabled. Also, these certs, if used for authentication usages will eventually come back into the runtime through CertificateFactory.

[coffeys]

Do you think it is that useful to have keytool record events? Ok, I guess some apps could be execing keytool, but that would be in a separate process, and probably wouldn't have JFR enabled. Also, these certs, if used for authentication usages will eventually come back into the runtime through CertificateFactory.

I figured it would be useful. keytool is an important generator of X509 certs. Why not give the opportunity to record them if JFR is enabled etc ? -J-XX:StartFlightRecording passed to keytool is sufficient to capture a recording.

The certs could be deployed out to any software stack I guess. Java being one possibility.

I see your point about recording of constructor with X509CertInfo now. The keytool eventually re-loads the newly generated cert. I'll look at editing. (duplicate record)

jdk.X509Certificate {
  startTime = 23:16:53.687 (2022-11-03)
  algorithm = N/A
  serialNumber = "44ffbec5b6f38b64"
  subject = "CN=test.oracle.com, OU=JPG, C=US"
  issuer = "CN=test.oracle.com, OU=JPG, C=US"
  keyType = "RSA"
  keyLength = 2048
  certificateId = 0
  validFrom = 23:16:53.686 (2022-11-03)
  validUntil = 23:16:53.686 (2023-11-03)
  eventThread = "main" (javaThreadId = 1)
  stackTrace = [
    sun.security.jca.JCAUtil.tryCommitCertEvent(Certificate) line: 129
    sun.security.x509.X509CertImpl.<init>(X509CertInfo) line: 290
    sun.security.tools.keytool.CertAndKeyGen.getSelfCertificate(X500Name, Date, long, CertificateExtensions) line: 340
    sun.security.tools.keytool.Main.doGenKeyPair(String, String, String, int, String, String, String) line: 2013
    sun.security.tools.keytool.Main.doCommands(PrintStream) line: 1180
    ...
  ]
}

jdk.X509Certificate {
  startTime = 23:16:53.901 (2022-11-03)
  algorithm = "SHA384withRSA"
  serialNumber = "44ffbec5b6f38b64"
  subject = "CN=test.oracle.com, OU=JPG, C=US"
  issuer = "CN=test.oracle.com, OU=JPG, C=US"
  keyType = "RSA"
  keyLength = 2048
  certificateId = 1683785197
  validFrom = 23:16:53.000 (2022-11-03)
  validUntil = 23:16:53.000 (2023-11-03)
  eventThread = "main" (javaThreadId = 1)
  stackTrace = [
    sun.security.jca.JCAUtil.tryCommitCertEvent(Certificate) line: 129
    java.security.cert.CertificateFactory.generateCertificate(InputStream) line: 356
    sun.security.pkcs12.PKCS12KeyStore.loadSafeContents(DerInputStream) line: 2428
    sun.security.pkcs12.PKCS12KeyStore.lambda$engineLoad$1(AlgorithmParameters, byte[], char[]) line: 2127
    sun.security.pkcs12.PKCS12KeyStore$RetryWithZero.run(PKCS12KeyStore$RetryWithZero, char[]) line: 257
  ]

[seanjmullan]

Do you think it is that useful to have keytool record events? Ok, I guess some apps could be execing keytool, but that would be in a separate process, and probably wouldn't have JFR enabled. Also, these certs, if used for authentication usages will eventually come back into the runtime through CertificateFactory.

I figured it would be useful. keytool is an important generator of X509 certs. Why not give the opportunity to record them if JFR is enabled etc ? -J-XX:StartFlightRecording passed to keytool is sufficient to capture a recording.

The certs could be deployed out to any software stack I guess. Java being one possibility.

I think the threat level is a bit different than certificates that are actually parsed and potentially being used by applications to authenticate servers, etc, so I would be wary of treating these events with equivalence. These certificates may never be used by applications, and if they are, then there will be an event for that.

Also with keytool, you have direct control over what algorithms, key sizes, validity, etc are being used, whereas in an app case, you don't really know until you parse the certificate and see what it contains.

[coffeys]

I'd agree with your thoughts. While it may not be a threat level, it's still a useful information point, especially in environments where hard coded values might get embedded in some type of key generation tool. Not many might be interested but there's a option there now with JFR to view this data at least. I don't think many will configure keytool to run with JFR.

Happy to revert the keytool change but I don't see it being too invasive in code changes.

[seanjmullan]

My vote would be to leave it out. keytool already emits warnings when weak algorithms are used. It seems we both agree that few users, will likely enable JFR on keytool. We could always add these events later, but it is harder to remove them once they are in there.

[openjdk[bot]]

@coffeys this pull request can not be integrated into master due to one or more merge conflicts. To resolve these merge conflicts and update this pull request you can run the following commands in the local repository for your personal fork:

git checkout 8292033-x509Event
git fetch https://git.openjdk.org/jdk master
git merge FETCH_HEAD
# resolve conflicts and follow the instructions given by git merge
git commit -m "Merge master"
git push

[coffeys]

My vote would be to leave it out. keytool already emits warnings when weak algorithms are used. It seems we both agree that few users, will likely enable JFR on keytool. We could always add these events later, but it is harder to remove them once they are in there.

I'm fine with that suggestion Sean. I've removed the event form the CertAndGen class. Turns out that the keytool will load the new cert via the standard CertificateFactory.generateCertificate call at a later stage anyhow! [1]

Tests modified also to capture this.

[1]

jdk.X509Certificate {
  startTime = 11:36:48.208 (2022-11-09)
  algorithm = "SHA384withRSA"
  serialNumber = "fe9b213c1345aadd"
  subject = "CN=8292033.oracle.com, OU=JPG, C=US"
  issuer = "CN=8292033.oracle.com, OU=JPG, C=US"
  keyType = "RSA"
  keyLength = 2048
  certificateId = -749360774
  validFrom = 11:36:48.000 (2022-11-09)
  validUntil = 11:36:48.000 (2023-11-09)
  eventThread = "main" (javaThreadId = 1)
  stackTrace = [
    sun.security.jca.JCAUtil.tryCommitCertEvent(Certificate) line: 126
    java.security.cert.CertificateFactory.generateCertificate(InputStream) line: 356
    sun.security.pkcs12.PKCS12KeyStore.loadSafeContents(DerInputStream) line: 2428
    sun.security.pkcs12.PKCS12KeyStore.lambda$engineLoad$1(AlgorithmParameters, byte[], char[]) line: 2127
    sun.security.pkcs12.PKCS12KeyStore$RetryWithZero.run(PKCS12KeyStore$RetryWithZero, char[]) line: 257
    sun.security.pkcs12.PKCS12KeyStore.engineLoad(InputStream, char[]) line: 2118
    sun.security.util.KeyStoreDelegator.engineLoad(InputStream, char[]) line: 228
    java.security.KeyStore.load(InputStream, char[]) line: 1500
    java.security.KeyStore.getInstance(File, char[], KeyStore$LoadStoreParameter, boolean) line: 1828
    java.security.KeyStore.getInstance(File, char[]) line: 1709
    sun.security.tools.keytool.Main.doCommands(PrintStream) line: 1390
    sun.security.tools.keytool.Main.run(String[], PrintStream) line: 419
    sun.security.tools.keytool.Main.main(String[]) line: 412

[openjdk[bot]]

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

8292033: Move jdk.X509Certificate event logic to JCA layer

Reviewed-by: mullan

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

• 1b94ae13d3940951134a1010500fd95001b8cb15: 8296139: Make GrowableBitMap the base class of all implementations
• cc8bf95046d1fba0f88b0e17481f36b2be870659: 8296718: Refactor bootstrap Test Common Functionalities to test/lib/Utils
• 17e3412363bf5263c26d9bf1dfbef1fecc3d11a9: 8296615: use of undeclared identifier 'IPV6_DONTFRAG'
• a5d838c337599d740e7940d7303b327199f4d07b: 8296591: Signature benchmark
• fa8a8668a6656046d713a6b09244adfc81556d63: 8296675: Exclude linux-aarch64 in NSS tests

Please see this link for an up-to-date comparison between the source branch of this pull request and the master branch. 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.

[coffeys]

/integrate

[openjdk[bot]]

Going to push as commit 102b2b32feec4727145be4814eb1a69ef462ff16. Since your change was applied there have been 5 commits pushed to the master branch:

• 1b94ae13d3940951134a1010500fd95001b8cb15: 8296139: Make GrowableBitMap the base class of all implementations
• cc8bf95046d1fba0f88b0e17481f36b2be870659: 8296718: Refactor bootstrap Test Common Functionalities to test/lib/Utils
• 17e3412363bf5263c26d9bf1dfbef1fecc3d11a9: 8296615: use of undeclared identifier 'IPV6_DONTFRAG'
• a5d838c337599d740e7940d7303b327199f4d07b: 8296591: Signature benchmark
• fa8a8668a6656046d713a6b09244adfc81556d63: 8296675: Exclude linux-aarch64 in NSS tests

Your commit was automatically rebased without conflicts.

[openjdk[bot]]

@coffeys Pushed as commit 102b2b32feec4727145be4814eb1a69ef462ff16.

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