8312383: Log X509ExtendedKeyManager implementation class name in TLS/SSL connection #17742

pkumaraswamy · 2024-02-07T06:36:10Z

During the time of server certificate validation, users have the flexibility to use a custom X509 Key Manager implementation by extending "X509ExtendedKeyManager.".
In such cases, printing the class name in X509Authentication.java will be helpful to trace any failure of the SSL connection due to a certificate issue.

I've tested the code by running the custom X509 manager, the default X509 manager, and passing the null key manager.
The screen shots are attached here.
x509_log_testing.zip

Also, the internal test runs against this fix are green

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-8312383: Log X509ExtendedKeyManager implementation class name in TLS/SSL connection (Enhancement - P4)

Reviewers

Sean Coffey (@coffeys - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 17742

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

Using diff file

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

Webrev

Link to Webrev Comment

bridgekeeper · 2024-02-07T06:36:32Z

👋 Welcome back pkumaraswamy! 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 · 2024-02-07T06:36:52Z

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

mlbridge · 2024-02-07T06:43:04Z

Webrevs

coffeys · 2024-02-07T14:19:03Z

src/java.base/share/classes/sun/security/ssl/X509Authentication.java

@@ -201,6 +201,10 @@ public static SSLPossession createPossession(
    private static SSLPossession createClientPossession(
            ClientHandshakeContext chc, String[] keyTypes) {
        X509ExtendedKeyManager km = chc.sslContext.getX509KeyManager();
+        if (SSLLogger.isOn && SSLLogger.isOn("ssl")) {
+            SSLLogger.finest("X509ExtendedKeyManager being used: " +


Could the JBS title be made more descriptive ? It's quite vague.

I wonder if "X509KeyManager class: " would be better for displaying.

createServerPossession would also benefit from this logging enhancement. I wonder if this belongs in logging during SSLContext creation time instead. Other security-dev engineers may have opinion on that.

IIRC, there's another issue open where we iterate over the certificate contexts of custom tm/km types. The JDK src does it at the moment for the default tm/km but no output given for custom impl. Will be good to have that tied up at some stage also.

I have changed the title to reflect the specific change made here.
I'll make changes in createServerPossession as well.

I have made changes and attached the testing snapshots in a zip file attached in bug description

I have made changes to log statement

coffeys · 2024-02-07T14:19:27Z

src/java.base/share/classes/sun/security/ssl/X509Authentication.java

@@ -201,6 +201,10 @@ public static SSLPossession createPossession(
    private static SSLPossession createClientPossession(
            ClientHandshakeContext chc, String[] keyTypes) {
        X509ExtendedKeyManager km = chc.sslContext.getX509KeyManager();
+        if (SSLLogger.isOn && SSLLogger.isOn("ssl")) {
+            SSLLogger.finest("X509ExtendedKeyManager being used: " +
+                    (km == null ? "null" : km.getClass().getName()));


do you need to cater for null ? I thought a Dummy manager is returned in such scenarios.

No, I did verify that it always returns Dummy Manager even when key manager is initialized as null. I have added this as a safety net. I'll remove this.

bridgekeeper · 2024-03-07T13:32:31Z

@pkumaraswamy 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!

pkumaraswamy · 2024-03-07T13:34:19Z

Adding comment to avoid auto closure of PR

pkumaraswamy · 2024-03-08T06:09:25Z

I have tested the latest changes and attached is the snapshot
x509kmprintlogs_v3.zip

coffeys

Looks good.

openjdk · 2024-03-08T08:04:16Z

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

8312383: Log X509ExtendedKeyManager implementation class name in TLS/SSL connection

Reviewed-by: coffeys

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 no new commits pushed to the master branch. If another commit should be pushed before you perform the /integrate command, your PR will be automatically rebased. If you prefer to avoid any potential automatic rebasing, please check the documentation for the /integrate command for further details.

As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@coffeys) but any other Committer may sponsor as well.

➡️ To flag this PR as ready for integration with the above commit message, type /integrate in a new comment. (Afterwards, your sponsor types /sponsor in a new comment to perform the integration).

pkumaraswamy · 2024-03-08T08:11:57Z

/integrate

openjdk · 2024-03-08T08:13:00Z

@pkumaraswamy
Your change (at version bb0a54e) is now ready to be sponsored by a Committer.

coffeys · 2024-03-08T09:03:27Z

/sponsor

openjdk · 2024-03-08T09:04:17Z

Going to push as commit bdd1aeb.
Since your change was applied there has been 1 commit pushed to the master branch:

fb4610e: 8327444: simplify RESTARTABLE macro usage in JDK codebase

Your commit was automatically rebased without conflicts.

openjdk · 2024-03-08T09:04:23Z

@coffeys @pkumaraswamy Pushed as commit bdd1aeb.

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

8312383: Improve SSL debug log

75b6ed4

openjdk bot added security security-dev@openjdk.org rfr Pull request is ready for review labels Feb 7, 2024

coffeys reviewed Feb 7, 2024

View reviewed changes

pkumaraswamy changed the title ~~8312383: Improve SSL debug log~~ 8312383: Log X509ExtendedKeyManager implementation class name in TLS/SSL connection Feb 7, 2024

pkumaraswamy added 2 commits February 8, 2024 07:24

Merge remote-tracking branch 'origin/master' into JDK-8312383

ad1654e

Add log for client auth

bd8449b

pkumaraswamy added 3 commits March 8, 2024 04:54

Merge remote-tracking branch 'origin/master' into JDK-8312383

c15712f

Merge remote-tracking branch 'origin/master' into JDK-8312383

3110899

change log message

bb0a54e

coffeys approved these changes Mar 8, 2024

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Mar 8, 2024

openjdk bot added the sponsor Pull request is ready to be sponsored label Mar 8, 2024

openjdk bot added the integrated Pull request has been integrated label Mar 8, 2024

openjdk bot closed this Mar 8, 2024

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review sponsor Pull request is ready to be sponsored labels Mar 8, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8312383: Log X509ExtendedKeyManager implementation class name in TLS/SSL connection #17742

8312383: Log X509ExtendedKeyManager implementation class name in TLS/SSL connection #17742

pkumaraswamy commented Feb 7, 2024 •

edited by openjdk bot

Loading

bridgekeeper bot commented Feb 7, 2024

openjdk bot commented Feb 7, 2024

mlbridge bot commented Feb 7, 2024 •

edited

Loading

coffeys Feb 7, 2024

pkumaraswamy Feb 7, 2024

pkumaraswamy Feb 8, 2024

pkumaraswamy Mar 8, 2024

coffeys Feb 7, 2024

pkumaraswamy Feb 7, 2024

bridgekeeper bot commented Mar 7, 2024

pkumaraswamy commented Mar 7, 2024

pkumaraswamy commented Mar 8, 2024 •

edited

Loading

coffeys left a comment

openjdk bot commented Mar 8, 2024

pkumaraswamy commented Mar 8, 2024

openjdk bot commented Mar 8, 2024

coffeys commented Mar 8, 2024

openjdk bot commented Mar 8, 2024

openjdk bot commented Mar 8, 2024

8312383: Log X509ExtendedKeyManager implementation class name in TLS/SSL connection #17742

8312383: Log X509ExtendedKeyManager implementation class name in TLS/SSL connection #17742

Conversation

pkumaraswamy commented Feb 7, 2024 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Reviewing

Webrev

bridgekeeper bot commented Feb 7, 2024

openjdk bot commented Feb 7, 2024

mlbridge bot commented Feb 7, 2024 • edited Loading

Webrevs

coffeys Feb 7, 2024

Choose a reason for hiding this comment

pkumaraswamy Feb 7, 2024

Choose a reason for hiding this comment

pkumaraswamy Feb 8, 2024

Choose a reason for hiding this comment

pkumaraswamy Mar 8, 2024

Choose a reason for hiding this comment

coffeys Feb 7, 2024

Choose a reason for hiding this comment

pkumaraswamy Feb 7, 2024

Choose a reason for hiding this comment

bridgekeeper bot commented Mar 7, 2024

pkumaraswamy commented Mar 7, 2024

pkumaraswamy commented Mar 8, 2024 • edited Loading

coffeys left a comment

Choose a reason for hiding this comment

openjdk bot commented Mar 8, 2024

pkumaraswamy commented Mar 8, 2024

openjdk bot commented Mar 8, 2024

coffeys commented Mar 8, 2024

openjdk bot commented Mar 8, 2024

openjdk bot commented Mar 8, 2024

pkumaraswamy commented Feb 7, 2024 •

edited by openjdk bot

Loading

mlbridge bot commented Feb 7, 2024 •

edited

Loading

pkumaraswamy commented Mar 8, 2024 •

edited

Loading