RH2117972 - Extend the support for NSS DBs (PKCS11) in FIPS mode

[martinuy]

Search this PR in Red Hat Jira

8u backport of the RH2117972 enhancement. See the 11u PR for further reference: rh-openjdk/jdk11u#4

Differences with the 11u patch (in addition to paths conversion):

• jdk/src/share/classes/sun/security/pkcs11/Config.java

  • The code fragment that sets the System property with the nssdb path in case that it's available as a Security property only has been moved to Config::getConfig because the SunPKCS11::configure method is not available in 8u and it's not possible to execute code before calling a constructor here [1]. Config::getConfig is the next (and last) point before the Config instance is created and the property in the nss.fips.cfg configuration file has to be expanded.

• jdk/src/share/classes/sun/security/pkcs11/FIPSTokenLoginHandler.java

  • sun.misc.IOUtils has to be used in 8u to "readNBytes" from an InputStream.

• jdk/src/share/classes/sun/security/pkcs11/Token.java

  • SharedSecrets is in sun.misc.SharedSecrets in 8u.

• jdk/src/share/lib/security/java.security-linux

  • Hook context is a bit different. No changes in the text, though.

• jdk/test/sun/security/pkcs11/fips/NssdbPin.java

  • The method Files::writeString is not available in 8u. Found a BufferedWriter replacement.

--
[1] - https://github.com/openjdk/jdk8u/blob/jdk8u352-b02/jdk/src/share/classes/sun/security/pkcs11/SunPKCS11.java#L103

[martinuy]

A couple of dependencies which I intend to upstream have been pushed as part of this PR:

• 8195607: sun/security/pkcs11/Secmod/TestNssDbSqlite.java failed with "NSS initialization failed" on NSS 3.34.1

  • 8195607: sun/security/pkcs11/Secmod/TestNssDbSqlite.java failed with "NSS initialization failed" on NSS 3.34.1 openjdk/jdk8u-dev#106

• JDK-8292688: Support Security properties in security.testlibrary.Proc

  • JDK-8292688: Support Security properties in security.testlibrary.Proc openjdk/jdk8u-dev#107

If these dependencies are accepted upstream, I'll revert these commits and rebase as needed.

[martinuy]

In the process of proposing JDK-8195607 to 8u upstream, it has been decided to split the fix into the following: JDK-8195607 and JDK-8292998. Both fixes are review-approved now and waiting for maintainers approval. My understanding is that, due to the current timelines, they won't make it to 8u352 but to 8u362.

The reviewer for this PR can verify that the patches now separated into JDK-8195607 and JDK-8292998 are the same than the one proposed as part of this change. If this PR is merged before the upstream patches, we should include them as proposed here and rebase eventually.

In regards to JDK-8292688, it has been integrated to 8u-dev already and I believe that it will make it to 8u352. No changes introduced compared to the proposal in this PR. We will eventually need to rebase this PR.

[franferrax]

Hi @martinuy, here is my in-advance review for this draft pull request.

Upstream changes review

6ad6252

• Upstream PR: 8195607: sun/security/pkcs11/Secmod/TestNssDbSqlite.java failed with "NSS initialization failed" on NSS 3.34.1 openjdk/jdk8u-dev#106
• 6ad6252 is NOT a true copy of openjdk/jdk8u-dev@f0ac319, verified as follows in a jdk8u clean repository:

  # Checkout the PR and fetch the required changes
  git fetch -q https://github.com/rh-openjdk/jdk8u pull/3/head && git switch -c pr/3 FETCH_HEAD
  git fetch -q https://github.com/openjdk/jdk8u-dev master
  # Revert 6ad6252, without committing
  git revert --no-commit 6ad6252b7110e17412f9bd3fbd5b9d54c714898c
  # Apply f0ac319, without committing
  git cherry-pick --no-commit f0ac31998d8396d92b4ce99aa345c05e6fd0f02a
  # Show changes from 6ad6252 not present in f0ac319
  git diff -R --staged
  # Cleanup
  git revert --abort && git switch master && git branch -D pr/3
  git -c gc.pruneExpire=now -c gc.reflogExpire=now -c gc.reflogExpireUnreachable=now gc -q

  • ❌ The diff command shows changes from 6ad6252 not present in openjdk/jdk8u-dev@f0ac319
    • This is because the upstream PR has split into JDK-8292998: Clean Secmod temporary NSS DB directory before test execution openjdk/jdk8u-dev#115, so 6ad6252 is a true copy of openjdk/jdk8u-dev@f0ac319 (which has been duly reviewed, approved and merged) + openjdk/jdk8u-dev@1a2fdc0 (whose review is still pending, but shouldn't been repeated here)

      # Checkout the PR and fetch the required changes
      git fetch -q https://github.com/rh-openjdk/jdk8u pull/3/head && git switch -c pr/3 FETCH_HEAD
      git fetch -q https://github.com/openjdk/jdk8u-dev master pr/115
      # Revert 6ad6252, without committing
      git revert --no-commit 6ad6252b7110e17412f9bd3fbd5b9d54c714898c
      # Apply f0ac319 + 1a2fdc0, without committing
      git cherry-pick --no-commit f0ac31998d8396d92b4ce99aa345c05e6fd0f02a 1a2fdc08e39606982425886ee7fd9d3945336658
      # Show changes from 6ad6252 not present in f0ac319 + 1a2fdc0
      git diff -R --staged
      # Cleanup
      git revert --abort && git switch master && git branch -D pr/3
      git -c gc.pruneExpire=now -c gc.reflogExpire=now -c gc.reflogExpireUnreachable=now gc -q

      • ✔️ The diff command shows no output

bd910b8

• Upstream PR: JDK-8292688: Support Security properties in security.testlibrary.Proc openjdk/jdk8u-dev#107
• bd910b8 is a true copy of openjdk/jdk8u-dev@0d5ea9d, which has been duly reviewed, approved and merged:

  # Checkout the PR and fetch the required changes
  git fetch -q https://github.com/rh-openjdk/jdk8u pull/3/head && git switch -c pr/3 FETCH_HEAD
  git fetch -q https://github.com/openjdk/jdk8u-dev master
  # Revert bd910b8, without committing
  git revert --no-commit bd910b898381f3f587e2f9bf7ced678d02763bc5
  # Apply 0d5ea9d, without committing
  git cherry-pick --no-commit 0d5ea9d29e97f1e4adcc1e1d36bc109fc5cee506
  # Show changes from bd910b8 not present in 0d5ea9d
  git diff -R --staged
  # Cleanup
  git revert --abort && git switch master && git branch -D pr/3
  git -c gc.pruneExpire=now -c gc.reflogExpire=now -c gc.reflogExpireUnreachable=now gc -q

  • ✔️ The diff command shows no output

Downstream changes review

I've left 3 comments in-line with the code, the most importan one is the suggestion to move the Config.java code to SunPKCS11.java.

[martinuy]

@franferrax I've pushed a new changeset after your review. Can you please take a look? Thanks.-

[martinuy]

@franferrax looks to me like there is an issue with the hook as it was suggested because we need the property to be set by the time Config::getConfig gets called and does the expansion. Please take a look at the new proposal.

[franferrax]

@franferrax looks to me like there is an issue with the hook as it was suggested because we need the property to be set by the time Config::getConfig gets called and does the expansion. Please take a look at the new proposal.

@martinuy: you are right, I've reviewed the new changes and, to me, this PR is now ready.

• JDK-8292998: Clean Secmod temporary NSS DB directory before test execution openjdk/jdk8u-dev#115
  • Regarding this, which is included here as part of 6ad6252, I suggest the following:
    1. Wait for the upstream approval of that change
    2. If that work doesn't reach our fips branch on time, introduce the latest version in this PR (if it differs from what we already have)
    3. Only then merge this PR
  • On the other hand, being a test-only change already approved by one of the reviewers, I'm not blocking this PR just because of that (I agree with some commented suggestions, but I don't see them as blockers)

[martinuy]

Thanks @franferrax for your review. I'll move this PR from draft to open.

[franferrax]

Removing approval, as we need to:

• Backport Add nss.fips.cfg support to OpenJDK tree jdk#22 to this repository, so we can make the nss.fips.cfg changes
• Apply f9afab1...69cabe4 from RH2117972 - Extend the support for NSS DBs (PKCS11) in FIPS mode jdk#17
• Decide what to do about JDK-8292998: Clean Secmod temporary NSS DB directory before test execution openjdk/jdk8u-dev#115 (closed because of inactivity)