8292033: Move jdk.X509Certificate event logic to JCA layer #10422

coffeys · 2022-09-26T13:45:45Z

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)
Change must not contain extraneous whitespace
Commit message must refer to an issue

Issue

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

Reviewers

Sean Mullan (@seanjmullan - Reviewer)

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 · 2022-09-26T13:48:01Z

👋 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 · 2022-09-26T13:50:59Z

@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 · 2022-10-14T15:45:23Z

Webrevs

AlanBateman · 2022-10-14T15:50:11Z

/label remove core-libs

openjdk · 2022-10-14T15:51:59Z

@AlanBateman
The core-libs label was successfully removed.

turbanoff · 2022-10-14T19:18:58Z

src/java.base/share/classes/jdk/internal/event/X509CertificateEvent.java

@@ -31,6 +31,15 @@
 */

 public final class X509CertificateEvent extends Event {
+    private final static X509CertificateEvent EVENT = new X509CertificateEvent();


Use blessed modifiers order

Suggested change

private final static X509CertificateEvent EVENT = new X509CertificateEvent();

private static final X509CertificateEvent EVENT = new X509CertificateEvent();

wangweij · 2022-10-25T13:51:06Z

test/lib/jdk/test/lib/security/TestCertificate.java

-        "CN=SSLCertificate, O=SomeCompany",
-        "CN=Intermediate CA Cert, O=SomeCompany",
+        "CN=SSLCertificate,O=SomeCompany",
+        "CN=Intermediate CA Cert,O=SomeCompany",


Why remove the spaces? X500Name::toString always has them.

Code was modified to use the X509Certificate API (rather than X509CertImpl)

it's using the X500Principal#getName() call now. Would it be better to use X500Principal#toString() ?

If you are only writing there and not parsing the string, then I assume either is OK. Still, less change is better.

fair point. I updated patch to use X500Principal#toString()

seanjmullan · 2022-10-25T20:11:33Z

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 · 2022-10-25T20:22:29Z

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 · 2022-10-27T10:04:07Z

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 · 2022-10-27T14:16:39Z

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 · 2022-11-01T18:19:57Z

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 · 2022-11-02T15:45:29Z

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 · 2022-11-03T17:40:53Z

src/java.base/share/classes/sun/security/x509/X509CertImpl.java

@@ -285,6 +286,7 @@ private DerValue readRFC1421Cert(InputStream in) throws IOException {
     */
    public X509CertImpl(X509CertInfo certInfo) {
        this.info = certInfo;
+        JCAUtil.tryCommitCertEvent(this);


Why do we need to record this as an event? This is an incomplete (unsigned) certificate.

good point. this constructor is mainly used by keytool. Would it make sense if I committed the event after the cert details are signed ?

seanjmullan · 2022-11-03T17:54:35Z

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 · 2022-11-03T23:25:29Z

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 · 2022-11-04T20:00:01Z

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 · 2022-11-04T20:30:22Z

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 · 2022-11-08T16:22:55Z

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 · 2022-11-09T15:49:46Z

@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 · 2022-11-09T15:49:48Z

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 · 2022-11-09T21:28:13Z

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

1b94ae1: 8296139: Make GrowableBitMap the base class of all implementations
cc8bf95: 8296718: Refactor bootstrap Test Common Functionalities to test/lib/Utils
17e3412: 8296615: use of undeclared identifier 'IPV6_DONTFRAG'
a5d838c: 8296591: Signature benchmark
fa8a866: 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 · 2022-11-09T21:45:25Z

/integrate

openjdk · 2022-11-09T21:47:07Z

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

1b94ae1: 8296139: Make GrowableBitMap the base class of all implementations
cc8bf95: 8296718: Refactor bootstrap Test Common Functionalities to test/lib/Utils
17e3412: 8296615: use of undeclared identifier 'IPV6_DONTFRAG'
a5d838c: 8296591: Signature benchmark
fa8a866: 8296675: Exclude linux-aarch64 in NSS tests

Your commit was automatically rebased without conflicts.

openjdk · 2022-11-09T21:47:30Z

@coffeys Pushed as commit 102b2b3.

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

coffeys added 3 commits August 10, 2022 12:59

8292033

2eb644c

merge with master

5894cd7

fix up merge

7e1baf3

openjdk bot added security security-dev@openjdk.org core-libs core-libs-dev@openjdk.org labels Sep 26, 2022

coffeys added 6 commits September 28, 2022 16:00

Record all X509 generate events

bcd4012

update commit method name

3b73d1c

Merge branch 'master' into 8292033-x509Event

def4a96

Minor clean up

b6036f3

Merge branch 'master' into 8292033-x509Event

2bdec01

restore import style

e145a1a

coffeys changed the title ~~8292033 x509 event~~ 8292033: Move jdk.X509Certificate event logic to JCA layer Oct 14, 2022

coffeys marked this pull request as ready for review October 14, 2022 15:39

openjdk bot added the rfr Pull request is ready for review label Oct 14, 2022

openjdk bot removed the core-libs core-libs-dev@openjdk.org label Oct 14, 2022

turbanoff reviewed Oct 14, 2022

View reviewed changes

coffeys added 2 commits October 25, 2022 10:59

Merge branch 'master' into 8292033-x509Event

3025d41

blessed modifiers

c58e0fa

wangweij reviewed Oct 25, 2022

View reviewed changes

Use X500Principal#toString()

ff67470

record events for internal constructor calls. Expand testing

423e2d7

Revert use of x509 constructor helper in some areas. Clean up tests

c26762b

coffeys added 2 commits November 2, 2022 15:11

funnel cert events via generateCertificate only

f51d262

code clean up

f430a3e

seanjmullan reviewed Nov 3, 2022

View reviewed changes

Further code review comments and new keytool test coverage with JFR

e363564

Don't record certs directly from CertAndGen/keytool

dda445f

openjdk bot added the merge-conflict Pull request has merge conflict with target branch label Nov 9, 2022

coffeys added 3 commits November 9, 2022 15:51

Merge branch 'master' into 8292033-x509Event

ee941cc

Merge with master

a87e6ac

revert recording events in keytool

d6faa84

openjdk bot removed the merge-conflict Pull request has merge conflict with target branch label Nov 9, 2022

seanjmullan approved these changes Nov 9, 2022

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Nov 9, 2022

openjdk bot added the integrated Pull request has been integrated label Nov 9, 2022

openjdk bot closed this Nov 9, 2022

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Nov 9, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8292033: Move jdk.X509Certificate event logic to JCA layer #10422

8292033: Move jdk.X509Certificate event logic to JCA layer #10422

coffeys commented Sep 26, 2022 •

edited by openjdk bot

Loading

bridgekeeper bot commented Sep 26, 2022

openjdk bot commented Sep 26, 2022

mlbridge bot commented Oct 14, 2022 •

edited

Loading

AlanBateman commented Oct 14, 2022

openjdk bot commented Oct 14, 2022

turbanoff Oct 14, 2022

wangweij Oct 25, 2022 •

edited

Loading

coffeys Oct 25, 2022

wangweij Oct 25, 2022

coffeys Oct 25, 2022

seanjmullan commented Oct 25, 2022

seanjmullan commented Oct 25, 2022

coffeys commented Oct 27, 2022

wangweij commented Oct 27, 2022

seanjmullan commented Nov 1, 2022

coffeys commented Nov 2, 2022

seanjmullan Nov 3, 2022

coffeys Nov 3, 2022

seanjmullan commented Nov 3, 2022

coffeys commented Nov 3, 2022

seanjmullan commented Nov 4, 2022

coffeys commented Nov 4, 2022

seanjmullan commented Nov 8, 2022

openjdk bot commented Nov 9, 2022

coffeys commented Nov 9, 2022

openjdk bot commented Nov 9, 2022

coffeys commented Nov 9, 2022

openjdk bot commented Nov 9, 2022

openjdk bot commented Nov 9, 2022

	private final static X509CertificateEvent EVENT = new X509CertificateEvent();
	private static final X509CertificateEvent EVENT = new X509CertificateEvent();

8292033: Move jdk.X509Certificate event logic to JCA layer #10422

8292033: Move jdk.X509Certificate event logic to JCA layer #10422

Conversation

coffeys commented Sep 26, 2022 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Reviewing

bridgekeeper bot commented Sep 26, 2022

openjdk bot commented Sep 26, 2022

mlbridge bot commented Oct 14, 2022 • edited Loading

Webrevs

AlanBateman commented Oct 14, 2022

openjdk bot commented Oct 14, 2022

turbanoff Oct 14, 2022

Choose a reason for hiding this comment

wangweij Oct 25, 2022 • edited Loading

Choose a reason for hiding this comment

coffeys Oct 25, 2022

Choose a reason for hiding this comment

wangweij Oct 25, 2022

Choose a reason for hiding this comment

coffeys Oct 25, 2022

Choose a reason for hiding this comment

seanjmullan commented Oct 25, 2022

seanjmullan commented Oct 25, 2022

coffeys commented Oct 27, 2022

wangweij commented Oct 27, 2022

seanjmullan commented Nov 1, 2022

coffeys commented Nov 2, 2022

seanjmullan Nov 3, 2022

Choose a reason for hiding this comment

coffeys Nov 3, 2022

Choose a reason for hiding this comment

seanjmullan commented Nov 3, 2022

coffeys commented Nov 3, 2022

seanjmullan commented Nov 4, 2022

coffeys commented Nov 4, 2022

seanjmullan commented Nov 8, 2022

openjdk bot commented Nov 9, 2022

coffeys commented Nov 9, 2022

openjdk bot commented Nov 9, 2022

coffeys commented Nov 9, 2022

openjdk bot commented Nov 9, 2022

openjdk bot commented Nov 9, 2022

coffeys commented Sep 26, 2022 •

edited by openjdk bot

Loading

mlbridge bot commented Oct 14, 2022 •

edited

Loading

wangweij Oct 25, 2022 •

edited

Loading