Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Enable the -XX:[+|-]CRIUSecProvider jvm option #18354

Merged

Conversation

WilburZjh
Copy link
Contributor

Whenever we are performing a checkpoint
this is typically done by enabling
-XX:+EnableCRIUSupport. It will remove
all the security providers and the
CRIUSecProvider is inserted to the
runtime enviroment. This provides a more
locked down approach to what cryptography
is allowed while taking a checkpoint.

Therefore, this new flag is introduced by
passing it when -XX:+EnableCRIUSupport is
active. When this option is used, we no
longer make use of CRIUSecProvider and
instead use normal provider loading to
enable all security algorithms in an out
of the box state. This can allow users to
make use of any algorithms in the various
providers available.

@WilburZjh
Copy link
Contributor Author

Marking as a draft. Doing the tests, once they are finished, will open it for review.

FYI @jasonkatonica

@WilburZjh WilburZjh force-pushed the simpleenablefullproviderincriu branch from 6d814e5 to e6acbe4 Compare October 26, 2023 16:18
@pshipton
Copy link
Member

@keithc-ca a pre-review may be helpful, I believe we will attempt to put this change into the 0.41 release, time permitting.

@keithc-ca
Copy link
Contributor

Please explain how avoiding use of CRIUSECProvider doesn't just reintroduce the problems it was intended to address. See the javadoc from that class:

/**
 * The CRIUSECProvider is a security provider that is used as follows when CRIU
 * is enabled. During the checkpoint phase, all other security providers are
 * removed, except CRIUSECProvider, and the digests are cleared, to ensure that
 * no state is saved during checkpoint that is then restored during the restore
 * phase. During the resore phase, CRIUSECProvider is removed and the other
 * security providers are added back.
 */

@WilburZjh
Copy link
Contributor Author

Please explain how avoiding use of CRIUSECProvider doesn't just reintroduce the problems it was intended to address. See the javadoc from that class:

/**
 * The CRIUSECProvider is a security provider that is used as follows when CRIU
 * is enabled. During the checkpoint phase, all other security providers are
 * removed, except CRIUSECProvider, and the digests are cleared, to ensure that
 * no state is saved during checkpoint that is then restored during the restore
 * phase. During the resore phase, CRIUSECProvider is removed and the other
 * security providers are added back.
 */

When CRIU is enabled, during the checkpoint phase, currently, only the CRIUSecProvider is used. However, CRUISecProvider only supports a limited number of services. Some clients may have other requirements which might need to use other security services from other JCE providers. In order to avoid using of CRIUSecProvider, we leave on this non default option -XX:[+|-]EnableFullSecurityEnv to those clients so that they can use other security providers. In this scenario, this non default option is being made available to those clients who have alternative approaches to protecting their files such as initialization prior to checkpoints being done with non sensitive data, feature parity with other CRIUS JDKs such as Azul which do not have such restrictions in place, and for use of other development and testing activities that are being done in Liberty.

@keithc-ca
Copy link
Contributor

You've described some motivation for this, but not offered an explanation for how we will avoid capturing state in a snapshot that should not be present after a restore. Do you have draft changes for the extension repositories you can share that would illustrate how that problem is avoided?

@WilburZjh
Copy link
Contributor Author

Test is passed. Open draft to ready for review.

FYI @jasonkatonica

@WilburZjh WilburZjh marked this pull request as ready for review October 30, 2023 14:52
@jasonkatonica
Copy link
Contributor

Hi @keithc-ca I read through through @WilburZjh explanation here as to our use cases we are supporting with this feature. I find his description accurate and it does describe the motivation here to disable the CRIUSecProvider in certain use cases.

In general there have never been any guarantees that all user application memory is absent of sensitive data when making use of the CRIU provider solution. Currently yes we take additional precautions of running resets on some algorithms although we share responsibility with liberty to ensure that the startup code in liberty consists of trusted / groomed code to support CRIU in a secure fashion. Liberty must clear their user supplied byte buffers and avoid creating immutable strings containing sensitive data AND the CRIU provider must force a few additional cleanups of potentially sensitive values from the CRIU provider perspective. Both of these actions must be in place in order to avoid the check-pointed files from containing sensitive information. If CRIU is used from a generalized caller ( non trusted non liberty code ) then there are no guarantees that all memory has been zerod of sensitive data.

This being said supporting the approach of allowing the following use cases makes sense:

  1. Allow for users to make use of sensitive fields that are NOT real prior to checkpoint such that no real sensitive information can potentially land on disk. In this case they no longer need any additional processing in the CRIU provider nor restrictions on algorithms in use as no real sensitive information was used that can potentially land within checkpointed files.
  2. Deployments may choose to make use of disk encryption to contain checkpoint files at rest. In this case all values are encrypted at rest and cannot be exposed by design to others.

We do acknowledge that these are both corner cases. For this reason this option is implemented as a non default option for users.

@keithc-ca
Copy link
Contributor

In my view there are (at least) two classes of sensitive information - things like keys (private or symmetric) and things like seeds of random number generation. It's not clear that applications can fully avoid generating all forms of sensitive information if they're free to use all security algorithms, particularly those which we haven't (yet?) integrated with CRIU.

Liberty may be an important use case, but we shouldn't restrict our considerations to their goals. I'm still waiting for an "explanation for how we will avoid capturing state" and for related changes that would benefit from this. You suggest they might use not-real parameters, but I don't understand the value of that as compared with avoiding algorithms that won't be available until after the restore.

A more minor complaint I have is with the name of the option; I'm not sure that "Env" contributes anything helpful, and as it relates to CRIU, that should form part of the name (or something more general, like "Snapshot" or "Checkpoint").

@ymanton
Copy link
Member

ymanton commented Nov 7, 2023

I'm still waiting for an "explanation for how we will avoid capturing state" and for related changes that would benefit from this.

@keithc-ca I don't think this change is intending to provide any such guarantees. Anyone who uses this option is expected to assume the responsibility of protecting their secrets on disk (or invalidating them on restore) if they deem them sensitive.

I will say that the name of the option is a little counter-intuitive. I would expect enabling something called "FullSecurityEnv" to be more secure, relative to the default, but this goes in the opposite direction. Maybe a better name is warranted?

@jasonkatonica
Copy link
Contributor

Hi Keith. As for the name of the option we were following along with a set pattern of other CRIU environment names but we are fine with considering other options. Are you aware of any non Env CRIU option names / patterns we could adopt?

In terms of MD5 support yes agreed that for any serious cryptography applications are not recommended to make use of MD5. MD5 certainly has long been deprecated as a good hash algorithm to make use of. This being said MD5 is still appropriate for other uses such as quickly checking if a configuration file has changed, checking if a file matches known values, interacting with legacy systems, and perhaps some other usages. Id recommend to users to use other hashing mechanisms although this is not always possible or needed.

@keithc-ca
Copy link
Contributor

In terms of MD5 support

I don't know why you bring up MD5 here; perhaps you meant to make that comment in ibmruntimes/openj9-openjdk-jdk#685?

Again, I'm still waiting to see something that would benefit from this change. If there's no code that calls CRIUSupport.isEnableFullSecurityEnvInCRIU() (or whatever better name we might agree upon), then I don't understand the point of this change. Should ibmruntimes/openj9-openjdk-jdk#685 include such a call?

@WilburZjh
Copy link
Contributor Author

Hi @keithc-ca , there will be a call CRIUSupport.isEnableFullSecurityEnvInCRIU() in Semeru JDK, but that call is based upon this PR. I just open a draft PR in Semeru JDKnext for your reference.

@jasonkatonica
Copy link
Contributor

I don't know why you bring up MD5 here; perhaps you meant to make that comment in ibmruntimes/openj9-openjdk-jdk#685?

Sorry yes i meant to make this comment in that review. I will duplicate this comment there for completeness.

@keithc-ca
Copy link
Contributor

Please update this, including the commit message and the description here, to reflect the option name agreed upon in #18422 (-XX:-CRIUSecProvider).

@WilburZjh WilburZjh force-pushed the simpleenablefullproviderincriu branch 2 times, most recently from 7eb9814 to b352caf Compare November 14, 2023 15:55
@WilburZjh WilburZjh changed the title Enable -XX:[+|-]EnableFullSecurityEnv jvm options Enable -XX:-CRIUSecProvider jvm options Nov 14, 2023
@WilburZjh
Copy link
Contributor Author

WilburZjh commented Nov 14, 2023

Please update this, including the commit message and the description here, to reflect the option name agreed upon in #18422 (-XX:-CRIUSecProvider).

Updated, also update the code based upon the discussion in #18422 accordingly. Please help to review.

runtime/jcl/exports.cmake Outdated Show resolved Hide resolved
runtime/jcl/uma/criu_exports.xml Outdated Show resolved Hide resolved
runtime/oti/jclprots.h Outdated Show resolved Hide resolved
runtime/oti/vm_api.h Outdated Show resolved Hide resolved
runtime/vm/CRIUHelpers.cpp Outdated Show resolved Hide resolved
runtime/vm/jvminit.c Outdated Show resolved Hide resolved
@WilburZjh WilburZjh force-pushed the simpleenablefullproviderincriu branch 2 times, most recently from ccf1c4a to 9397259 Compare November 20, 2023 06:37
runtime/vm/CRIUHelpers.cpp Outdated Show resolved Hide resolved
runtime/vm/jvminit.c Outdated Show resolved Hide resolved
@WilburZjh WilburZjh force-pushed the simpleenablefullproviderincriu branch 2 times, most recently from e65eaed to f747a7b Compare November 21, 2023 05:05
Whenever we are performing a checkpoint
this is typically done by enabling
-XX:+EnableCRIUSupport. It will remove
all the security providers and the
CRIUSecProvider is inserted to the
runtime enviroment. This provides a more
locked down approach to what cryptography
is allowed while taking a checkpoint.

Therefore, this new flag is introduced by
passing it after -XX:+EnableCRIUSupport is
enabled. When this option is used, we no
longer make use of CRIUSecProvider and
instead use normal provider loading to
enable all security algorithms in an out
of the box state. This can allow users to
make use of any algorithms in the various
providers available.
@WilburZjh WilburZjh force-pushed the simpleenablefullproviderincriu branch from f747a7b to df6acd4 Compare November 21, 2023 14:53
@WilburZjh WilburZjh changed the title Enable -XX:-CRIUSecProvider jvm options Enable the -XX:[+|-]CRIUSecProvider jvm option Nov 21, 2023
@keithc-ca
Copy link
Contributor

Jenkins test sanity alinux jdk17

@keithc-ca keithc-ca merged commit d5e17dd into eclipse-openj9:master Nov 21, 2023
5 checks passed
@pshipton
Copy link
Member

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.

5 participants