8311546: Certificate name constraints improperly validated with leading period

[shipilev]

Backporting this due to wider customer interest in aligning JDK behavior with other SSL implementations. Both patches apply cleanly. First patch does the fix. Second patch fixes the test.

Additional testing:

• macos-aarch64-server-release, new test fails without the change, passes with it
• macos-aarch64-server-release, sun/security/x509/
• linux-x86_64-server-release, jdk_security

---

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
• JDK-8320372 needs maintainer approval
• JDK-8347424 needs maintainer approval
• JDK-8311546 needs maintainer approval

Issues

• JDK-8311546: Certificate name constraints improperly validated with leading period (Bug - P3 - Approved)
• JDK-8320372: test/jdk/sun/security/x509/DNSName/LeadingPeriod.java validity check failed (Bug - P2 - Approved)
• JDK-8347424: Fix and rewrite sun/security/x509/DNSName/LeadingPeriod.java test (Bug - P4 - Approved)

Reviewers

• Volker Simonis (@simonis - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk21u-dev.git pull/1268/head:pull/1268
$ git checkout pull/1268

Update a local copy of the PR:
$ git checkout pull/1268
$ git pull https://git.openjdk.org/jdk21u-dev.git pull/1268/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 1268

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk21u-dev/pull/1268.diff

Using Webrev

Link to Webrev Comment

[bridgekeeper]

👋 Welcome back shade! 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]

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

8311546: Certificate name constraints improperly validated with leading period
8320372: test/jdk/sun/security/x509/DNSName/LeadingPeriod.java validity check failed
8347424: Fix and rewrite sun/security/x509/DNSName/LeadingPeriod.java test

Reviewed-by: simonis

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

• 97119a9: 8346887: DrawFocusRect() may cause an assertion failure
• 4068754: 8342508: Use latch in BasicMenuUI/bug4983388.java instead of delay
• 0dadc44: 8342498: Add test for Allocation elimination after use as alignment reference by SuperWord
• a9a563d: 8333403: Write a test to check various components events are triggered properly
• e2385eb: 8340687: Open source closed frame tests 8313082: Enable CreateCoredumpOnCrash for testing in makefiles #1
• 4e0c9ee: 8324807: Manual printer tests have no Pass/Fail buttons, instructions close set 2
• 455763b: 8328819: Remove applet usage from JFileChooser tests bug6698013
• f9d2fb8: 8227529: With malformed --app-image the error messages are awful
• ddddc1a: 8304701: Request with timeout aborts later in-flight request on HTTP/1.1 cxn
• 4035d85: 8325529: Remove unused imports from ModuleGenerator test file
• ... and 69 more: https://git.openjdk.org/jdk21u-dev/compare/e1b5f3c56d02c6738726106d3d069fea7a633245...master

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.

[openjdk]

This backport pull request has now been updated with issue from the original commit.

[mlbridge]

Webrevs

• 01: Full - Incremental (6a700088)
• 00: Full (6770ee56)

[shipilev]

/issue add JDK-8320372

[openjdk]

@shipilev
Adding additional issue to issue list: 8320372: test/jdk/sun/security/x509/DNSName/LeadingPeriod.java validity check failed.

[shipilev]

Both backports are actually clean.

/clean

[openjdk]

@shipilev The /clean pull request command is not enabled for this repository

[shipilev]

@shipilev The /clean pull request command is not enabled for this repository

:( Ok, then I need some reviews, please.

[simonis]

Looks good.

Just for my education: is it now common/recommended to bundle several downports into a single PR? From my memory (which might be outdated) we used dependent PRs for that purpose. The latter is obviously more complicated and work intensive so I'll probably follow your style in the future if that's OK?

[openjdk]

⚠️ @shipilev This change is now ready for you to apply for maintainer approval. This can be done directly in each associated issue or by using the /approval command.

[shipilev]

Looks good.

Thanks!

Just for my education: is it now common/recommended to bundle several downports into a single PR? From my memory (which might be outdated) we used dependent PRs for that purpose. The latter is obviously more complicated and work intensive so I'll probably follow your style in the future if that's OK?

I think we prefer multi-issue backports if we know there are breakages that are fixed by follow-up changes. This would introduce an atomic commit that would transit the repository from a good state to a good state, without exposing any known bad state in between. This is both cleaner and more convenient for eventual bisects.

[GoeLin]

Yes, I also combine changes with immediate fixes. Especially if one of them needs a review anyways, so it does not cause an unnecessary review.
Further it simplifies backports to later releases, as the change pushed to 21 can directly be backported.
Last, dependent pull request often don't resolve automatically, causing the dilemma to either push right on top, or to wait for the testing of the resolved change.

[shipilev]

I finally figured out why regression test is not working: JDK-8347424 -- I think I'll wait for that and mix the fix here.

[shipilev]

/issue add JDK-8347424

[openjdk]

@shipilev
Adding additional issue to issue list: 8347424: Fix and rewrite sun/security/x509/DNSName/LeadingPeriod.java test.

[shipilev]

Thanks for reviews! I mixed in the test rewrite that fixes the regression test. Now I can confirm that retracting the product fix shows up as new regression test failure. Unfortunately, this invalidates prior reviews, please review again.

[simonis]

Still good! Thanks.

[shipilev]

/approval 8311546 request Fixes the compatibility bug in certificate checking. Seen in the wild. The patch was in JDK 22 without product bugtail. The only problems are with the test, which requires additional followups, JDK-8320372, JDK-8347424. Applies cleanly, all tests pass, new test fails without a fix, passes with it. Risk is medium-low: touches certpath code, but the fix is simple and already shipping in existing JDKs.

/approval 8320372 request Fixes the test introduced by JDK-8311546. Applies cleanly, test passes. Risk is low: Test-only change, the patch was in JDK 22.

/approval 8347424 request Fixes the test introduced by JDK-8311546. Applies cleanly, test passes. Risk is medium-low: Test-only change, but only a recent one, so there might be follow-up test bugs later.

[openjdk]

@shipilev
8311546: The approval request has been created successfully.

[openjdk]

@shipilev
8320372: The approval request has been created successfully.

[openjdk]

@shipilev
8347424: The approval request has been created successfully.

[shipilev]

Thanks for reviews!

/integrate

[openjdk]

Going to push as commit 99a9299.
Since your change was applied there have been 84 commits pushed to the master branch:

• 5b377a3: 8334305: Remove all code for nsk.share.Log verbose mode
• abd8902: 8329322: Convert PageFormat/Orient.java to use PassFailJFrame
• fe321bb: 8320676: Manual printer tests have no Pass/Fail buttons, instructions close set 1
• ded4113: 8326525: com/sun/tools/attach/BasicTests.java does not verify AgentLoadException case
• aed5fd3: 8313633: [macOS] java/awt/dnd/NextDropActionTest/NextDropActionTest.java fails with java.lang.RuntimeException: wrong next drop action!
• 97119a9: 8346887: DrawFocusRect() may cause an assertion failure
• 4068754: 8342508: Use latch in BasicMenuUI/bug4983388.java instead of delay
• 0dadc44: 8342498: Add test for Allocation elimination after use as alignment reference by SuperWord
• a9a563d: 8333403: Write a test to check various components events are triggered properly
• e2385eb: 8340687: Open source closed frame tests 8313082: Enable CreateCoredumpOnCrash for testing in makefiles #1
• ... and 74 more: https://git.openjdk.org/jdk21u-dev/compare/e1b5f3c56d02c6738726106d3d069fea7a633245...master

Your commit was automatically rebased without conflicts.

[openjdk]

@shipilev Pushed as commit 99a9299.

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