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

Add CRIU security provider #494

Closed
wants to merge 1 commit into from
Closed

Conversation

ZainabF92
Copy link

Specialized security provider needed for CRIU

Signed-off-by: Zainab Fatmi zainab@ibm.com

@alon-sh
Copy link
Contributor

alon-sh commented Mar 31, 2022

@mstoodle some of the code for CRIU security provider is taken from other OpenJDK security providers (specifically SUN security provider). Is this OK in term of licensing?

The code was changed to remove some state and add methods that will wipe out any state forcibly during CRIU checkpoint.

fyi @vijaysun-omr

@pshipton
Copy link
Member

pshipton commented Mar 31, 2022

some of the code for CRIU security provider is taken from other OpenJDK security providers

You need to retain the Oracle copyright & license in new files where this code is used, and add an IBM copyright for the IBM changes.

@pshipton
Copy link
Member

pshipton commented Mar 31, 2022

If there are new files which are 100% IBM code, there is a similar IBM license that should be used. Somebody can find an example if you need that.

@pshipton
Copy link
Member

pshipton commented Mar 31, 2022

@pshipton
Copy link
Member

To explain about the closed directory, we put files there which are added by IBM to avoid merge conflicts and make it easier to find IBM additions. Files under closed can also override files of the same name provided by OpenJDK.

@mstoodle
Copy link
Member

@mstoodle some of the code for CRIU security provider is taken from other OpenJDK security providers (specifically SUN security provider). Is this OK in term of licensing?

It is ok so long as you follow the steps Peter listed above.

@ZainabF92
Copy link
Author

Thank you! I moved the new files to the closed directory and added the IBM license.

@ZainabF92 ZainabF92 force-pushed the criu-sec branch 2 times, most recently from a96482b to cc38c31 Compare April 5, 2022 16:13
@ZainabF92 ZainabF92 force-pushed the criu-sec branch 3 times, most recently from fa0d2f7 to a5fcea6 Compare April 7, 2022 13:06
@keithc-ca
Copy link
Member

Variants of this will be needed for other Java versions, otherwise I think builds configured with --enable-criu-support will break if eclipse-openj9/openj9#14824 merged.

@alon-sh
Copy link
Contributor

alon-sh commented Apr 8, 2022

@keithc-ca jdk8 will not support --enable-criu-support for now as per @tajila.

@keithc-ca
Copy link
Member

Perhaps Tobi forgot about ibmruntimes/openj9-openjdk-jdk8#508 ?

@JasonFengJ9
Copy link
Member

Maybe in a separated PR, CRIU specific code need to be decorated w/ JPP flag CRIU_SUPPORT, hence these additions won't spread into normal product streams.
Please refer #498 for CRIU JPP in extension repo.

@tajila
Copy link
Member

tajila commented Apr 8, 2022

Perhaps Tobi forgot about ibmruntimes/openj9-openjdk-jdk8#508 ?

Im working on a PR to disable it on JDK8 for the time being.

Copy link
Member

@tajila tajila left a comment

Choose a reason for hiding this comment

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

Looks good, just need to add the JPP flags. Talk to @JasonFengJ9 for more info on this

Copy link
Member

@tajila tajila left a comment

Choose a reason for hiding this comment

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

@JasonFengJ9 please double check the usage of JPP flags

@JasonFengJ9
Copy link
Member

JasonFengJ9 commented Apr 12, 2022

JPP flag usage looks good. The java files have to be added explicitly for JPP processing like

# OpenJ9 CRIU only supports Linux, so we only need to consider the unix sub-directory.
OPENJDK_SOURCE_PROCESS_ENVIRONMENT := $(TOPDIR)/src/java.base/unix/classes/java/lang/ProcessEnvironment.java
OPENJDK_STAGED_PROCESS_ENVIRONMENT := $(patsubst $(TOPDIR)/%,$(SUPPORT_OUTPUTDIR)/overlay/%,$(OPENJDK_SOURCE_PROCESS_ENVIRONMENT))
$(OPENJDK_STAGED_PROCESS_ENVIRONMENT) : $(OPENJDK_SOURCE_PROCESS_ENVIRONMENT)
$(call install-file)

CRIUSECProvider.java, DigestBase.java, NativePRNG.java, and SHA.java are copied into $(SUPPORT_OUTPUTDIR)/overlay/src/java.base/share/classes/com/ibm/security/criu/

CRIUConfigurator.java and Security.java are copied into $(SUPPORT_OUTPUTDIR)/overlay/src/java.base/share/classes/java/security

@JasonFengJ9
Copy link
Member

CRIUSECProvider.java, DigestBase.java, NativePRNG.java, and SHA.java are copied into $(SUPPORT_OUTPUTDIR)/overlay/src/java.base/share/classes/com/ibm/security/criu/
CRIUConfigurator.java and Security.java are copied into $(SUPPORT_OUTPUTDIR)/overlay/src/java.base/share/classes/java/security

A draft change - https://github.com/JasonFengJ9/openj9-openjdk-jdk11/blob/criu-sec/closed/GensrcJ9JCL.gmk
@keithc-ca any suggestions?

@JasonFengJ9
Copy link
Member

JasonFengJ9 commented Apr 13, 2022

Just on second thought, following files can be moved into openj9 repo, and take advantage of JPP w/o coping into $(SUPPORT_OUTPUTDIR)/overlay.
CRIUSECProvider.java
DigestBase.java
NativePRNG.java
SHA.java
CRIUConfigurator.java

Edit: OpenJDK based files can't be put into OpenJ9 due to license incompatibility.

@ZainabF92

@keithc-ca
Copy link
Member

draft change
@keithc-ca any suggestions

There's a lot of repetition that should be captured in a macro or via use of $(foreach...), but more importantly, the use of the phony target overlayFiles means rules that depend on it will always execute - we want the rules to execute only when necessary.

@keithc-ca
Copy link
Member

Perhaps a simpler longer-term solution would be to feed all of the Java source within closed/src through the preprocessor (rather than include that directory in TOP_SRC_DIRS).

@JasonFengJ9
Copy link
Member

feed all of the Java source within closed/src through the preprocessor (rather than include that directory in TOP_SRC_DIRS).

Created Apply JPP to JCL patch files within $(TOPDIR)/closed.
W/ that change, this PR only need copy Security.java into $(SUPPORT_OUTPUTDIR)/overlay.

Signed-off-by: Zainab Fatmi <zainab@ibm.com>
@JasonFengJ9
Copy link
Member

#501 has been merged.
Now only src/java.base/share/classes/java/security/Security.java is required to be added for JPP process, @ZainabF92 pls refer https://github.com/ibmruntimes/openj9-openjdk-jdk11/compare/openj9...JasonFengJ9:criu-sec?expand=1

@alon-sh
Copy link
Contributor

alon-sh commented Apr 28, 2022

Zainab is out sick

@keithc-ca
Copy link
Member

Superseded by #510.

@keithc-ca keithc-ca closed this Apr 29, 2022
@ZainabF92 ZainabF92 deleted the criu-sec branch May 19, 2022 19:48
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.

7 participants