RH2090378: Revert to disabling system security properties and FIPS mode support together #4
Conversation
…system Introduce security.useFIPSProviders as a security property equivalent of the system property com.redhat.fips. Improve debug output of all four properties for FIPS mode and system security property support.
Run JDK tests in GHA with system security properties and FIPS mode support enabled in java.security
…rmer always takes precedence. Fix spacing in submit.yml
|
Reworked the handling of the new property so it -Dcom.redhat.fips takes precedence and can be used to override what is in |
gnu-andrew
changed the title
|
We have looked in detail into the proposed set of changes with @franferrax. For convenience, here it's a diff with all changes squashed. We will separate our observations into 3 categories: Semantic equivalence to the old behaviorThere seems to be semantic equivalence, to the best of our understanding. Visible changesWith the proposed changes, the method At a smaller scale, there can also be savings if checking the FIPS property ( Code readability and debug outputsWe think that the overall code readability and debug outputs are not necessarily an improvement over the previous state but can, in some instances, add issues. These are some of the improvement possibilities that we see: Security.java
SystemConfigurator.java
Finally, we think that the old/existing code should be improved but we have some concerns about the direction in which it's going with the current series of proposed changes. Ideally, we would like to redefine the set of requirements for the FIPS functionality and refactor the old code with that clear. In example, if decoupling FIPS from system security properties is the way forward at a functional level, the code should not be in a hybrid state (as proposed with the last change) but fully aligned to that. Conversely, if the way forward is not decoupling, the code should also reflect that. |
How to see PR changes on top of a base commit (including it)For our own future reference, this is how we generated 4ac1a03...afff64a. rh_repo='https://github.com/rh-openjdk/jdk'
base_commit='2028344a5c354ac14b8c6ea4ca5a71622cd5e524'
pr_number=4
branch="PR4onTopOf${base_commit:0:7}"
# Get the fips-17u and this PR branches
git fetch ${rh_repo} fips-17u:fips-17u
git fetch ${rh_repo} pull/$pr_number/head:pr-branch
# Create a branch from the base commit
git switch -c ${branch} ${base_commit}
# Cherry pick the PR changes, in this case ignoring merges
git cherry-pick --no-merges fips-17u..pr-branch
# Push the branch to my GitHub fork
git push origin ${branch}
# Generate comparison URL: parent of ${base_commit} -> ${branch}
bold_green="$(tput setaf 2)$(tput bold)" && reset="$(tput sgr0)"
range="$(git rev-parse ${base_commit}^)...$(git rev-parse ${branch})"
echo "${bold_green}See changes in $rh_repo/compare/${range}${reset}"
# Cleanup local changes (keeping commits in GitHub for the record)
git switch master && git branch -D ${branch} pr-branch fips-17u
git reflog expire --expire-unreachable=now --all && git gc --prune=nowNOTE: as long as the |
This isn't correct. It includes changes which are not part of this PR. You can see the changes in this PR by clicking the "Files Changed" tab or running |
I think this is the key point; the goal of the patch is achieved. Many of the suggestions below are worthwhile improvements, but would change the behaviour. I'm well aware that the result of this patch is not perfect, but the intention is to do a fairly minimal reversion, not to rename properties or change their behaviour.
The intention was to be able to tell the user as much as possible (your system is in FIPS mode, but we can't turn on FIPS support), but re-evaluating this, I agree the performance cost isn't worth it. I'll move the check to wrap the entire FIPS configuration instead.
As above.
The reason I avoided this was because the length of the properties makes the line length too long. I've changed them to constants to avoid this, which also allows them to be reused in the debugging output.
I've renamed them as suggested. I think inverting the semantics in the code would create even more confusion. It's already hard enough for me to follow. I looked briefly at
The The FIPS property check is already after the system security properties alignment check. You already said part of this above, I believe. I've moved the variable into
I've removed the first line. I guess it's a legacy of when there were originally two properties.
Done
Maybe, but this is not added or altered by this patch at all
I've taken it out, but I tend to disagree. If you're relying on the system security property system to limit the algorithms available, then you should really check that this is actually in place.
Can you elaborate? I don't see how.
This is definitely out of scope of this PR. I have no strong feelings either way as to whether FIPS depends on system security properties; that is an implementation choice for yourself and Francisco. It is certain that system security properties are used outside of FIPS mode and so should be entangled with that code as little as possible. As previously stated, I intend to shortly rework that code and take it upstream. |
… plus general code cleanup
|
Changes pushed. Please use the "Files changed" tab to review. |
|
Current output once property is set to true in |
Correctness depends on the context, we intentionally included 2028344 as part of the diff, and this is the only additional change outside of the PR. We did it behind the following rationale:
Updated diff view: 4ac1a03...5e92115 |
src/java.base/share/classes/java/security/SystemConfigurator.java
Outdated
Show resolved
Hide resolved
It's been reviewed back in January and been shipped. This PR is not the place to review it as it is not contained within this PR.
If this is useful to you, then good. But I want to look at the changes I'm proposing to commit, not a bunch of other changes that are already committed as well. I don't find it remotely helpful, it's just confusing. |
Thank you! |
I wondered about that, but the last run didn't seem to get to the tests, failing with some odd download error, so this is the first time I've seen the output. The doc you referenced suggests we can unquote and use |
Yes, I was going to mention that, but it also says "This can be useful if you have layers of scripts and have trouble getting proper quoting of command line arguments through". I don't think it describes our case, since it looks like the issue is caused by the inner quotation (single quotes), and we are using the outer quotation without issues (double quotes). I would first try |
I've tried |
…de support together (#4) - Improve debug output of all properties for FIPS mode and system security property support. - Run JDK tests in GHA with system security properties both disabled and enabled in java.security - General code cleanup Reviewed-by: @martinuy Reviewed-by: @franferrax
…de support together (#4) - Improve debug output of all properties for FIPS mode and system security property support. - Run JDK tests in GHA with system security properties both disabled and enabled in java.security - General code cleanup Reviewed-by: @martinuy Reviewed-by: @franferrax
…de support together (#4) - Improve debug output of all properties for FIPS mode and system security property support. - Run JDK tests in GHA with system security properties both disabled and enabled in java.security - General code cleanup Reviewed-by: @martinuy Reviewed-by: @franferrax
…de support together (#4) - Improve debug output of all properties for FIPS mode and system security property support. - Run JDK tests in GHA with system security properties both disabled and enabled in java.security - General code cleanup Reviewed-by: @martinuy Reviewed-by: @franferrax
…de support together (#4) - Improve debug output of all properties for FIPS mode and system security property support. - Run JDK tests in GHA with system security properties both disabled and enabled in java.security - General code cleanup Reviewed-by: @martinuy Reviewed-by: @franferrax
Search this PR in Red Hat Jira
Introduce
security.useFIPSProvidersas a security property equivalent of the system propertycom.redhat.fips. Improve debug output of all four properties for FIPS mode and system security property support.Note that the security properties are both now turned off by default, so behaviour is initially like upstream. We will enable them in the RPM. Once both are turned on in the file, the behaviour is as shown below (
TestProvidersis a simple piece of code to list the security providers installed)If FIPS mode is detected but system security property support is off, the following is printed and FIPS mode support is disabled. This allows
-Djava.security.disableSystemPropertiesFile=trueor thejava.securityequivalent alone to turn it off as before.