Skip to content

Conversation

@franferrax
Copy link

@franferrax franferrax commented Oct 28, 2022

Remove forgotten dead code from #13 and #14

#13 isn't a perfect revert of 0af22dc limited to SSLContextImpl.java and SunJSSE.java, since it doesn't remove the SharedSecrets import. This was already in this way in rh2020290-support_tls_1_3_in_fips.v1.patch, I'm now realizing this when doing the OpenJDK 11 backport (rh-openjdk/jdk11u#7) and retrying the same approach in OpenJDK 17:

# Revert #13
git show 0bd5ca9ccc5890b009b927b541424d0aacbb0463 | git apply -R
# Redo #13 by reverting 0af22dc in SSLContextImpl.java and SunJSSE.java
git show 0af22dcc391ff38f65ff7d7cfcc2f933e29e9126 |
  git apply -R --include=src/java.base/share/classes/sun/security/ssl/*

In #14, I forgot to delete the DHKF and DHKFLock static variables from FIPSKeyImporter, which are no longer used, see this #14 comment.

openjdk#13 isn't a perfect revert of 0af22dc limited to SSLContextImpl.java and
SunJSSE.java, since it doesn't remove the SharedSecrets import. This was
already in this way in rh2020290-support_tls_1_3_in_fips.v1.patch, I'm
now realizing this when doing the OpenJDK 11 backport and retrying the
same approach in OpenJDK 17:
~~~
# Revert openjdk#13
git show 0bd5ca9 | git apply -R
# Redo openjdk#13 by reverting 0af22dc in SSLContextImpl.java and SunJSSE.java
git show 0af22dc |
  git apply -R --include=src/java.base/share/classes/sun/security/ssl/*
~~~

In openjdk#14, I forgot to delete the DHKF and DHKFLock static
variables from FIPSKeyImporter, which are no longer used,
see rh-openjdk#14 (comment).
Copy link

@gnu-andrew gnu-andrew left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks fine to me.

@gnu-andrew
Copy link

Any idea why we still have a test failure on Linux? Seems unrelated to this change.

@franferrax
Copy link
Author

Any idea why we still have a test failure on Linux? Seems unrelated to this change.

Yes, the Linux failure is the currently known one (-Djava.security.debug=properties makes SimpleFormatterFormat fail, see the 1ˢᵗ comment of #11).

Windows MineField.sh and wcMineField.sh failures were already present in bb46af0, but I haven't analyzed them:

@gnu-andrew
Copy link

Any idea why we still have a test failure on Linux? Seems unrelated to this change.

Yes, the Linux failure is the currently known one (-Djava.security.debug=properties makes SimpleFormatterFormat fail, see the 1ˢᵗ comment of #11).

Ah, for some reason, I thought we'd fixed that. I'll open a PR that removes it. We can always add the line temporarily if we need the debugging info for a specific case.

Windows MineField.sh and wcMineField.sh failures were already present in bb46af0, but I haven't analyzed them:

As both were present before, I don't see them as a blocker for this. I don't see this failure in upstream 17u so I'll sync with the latest release and see if that fixes it.

We may as well get this change in along with the NSS DB support.

@gnu-andrew gnu-andrew merged commit e41cd9c into rh-openjdk:fips-17u Nov 23, 2022
@gnu-andrew
Copy link

I've removed the -Djava.security.debug=properties as part of the 17.0.5 merge. That refactored all the GitHub files so it didn't seem worth trying to get the argument working again in a new place, just to remove it again.

@gnu-andrew
Copy link

From the 17.0.5 run, all the tests seem to be passing: https://github.com/gnu-andrew/jdk/actions/runs/3528808451

@franferrax franferrax deleted the remove-dead-code branch December 1, 2022 15:49
gnu-andrew pushed a commit that referenced this pull request Aug 22, 2023
#13 isn't a perfect revert of 0af22dc limited to SSLContextImpl.java and
SunJSSE.java, since it doesn't remove the SharedSecrets import. This was
already in this way in rh2020290-support_tls_1_3_in_fips.v1.patch, I'm
now realizing this when doing the OpenJDK 11 backport and retrying the
same approach in OpenJDK 17:
~~~
# Revert #13
git show 0bd5ca9 | git apply -R
# Redo #13 by reverting 0af22dc in SSLContextImpl.java and SunJSSE.java
git show 0af22dc |
  git apply -R --include=src/java.base/share/classes/sun/security/ssl/*
~~~

In #14, I forgot to delete the DHKF and DHKFLock static
variables from FIPSKeyImporter, which are no longer used,
see #14 (comment).

Reviewed-by: @gnu-andrew
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants