8377819: Security properties jlink plugin#30635
8377819: Security properties jlink plugin#30635seanjmullan wants to merge 8 commits intoopenjdk:masterfrom
Conversation
|
👋 Welcome back mullan! A progress list of the required criteria for merging this PR into |
|
❗ This change is not yet ready to be integrated. |
|
@seanjmullan The following label will be automatically applied to this pull request:
When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command. |
|
/label add security-libs |
|
@seanjmullan
|
|
/label add security |
|
@seanjmullan |
|
|
||
| Description | ||
| : Override the security properties in the `java.security` configuration | ||
| file with the properties in the specified file. |
There was a problem hiding this comment.
I think the description will need to be expanded a bit to make it very clear than the contents of the given file are used to override or add to the java.security that goes into the generated run-time image. Just trying to avoid anyone thinking it is somehow related to -Djava.security.properties to override the location of the file.
There was a problem hiding this comment.
+1. It would make sense to also document that that the include directive is not supported.
| file with the properties in the specified file. | |
| Override the security properties - if they exist - in the `conf/security/java.security` | |
| configuration file with the properties in the specified file. Appends properties not | |
| previously present in `java.security` at the end. The `include` directive is not supported. |
There was a problem hiding this comment.
Agree that we should be more clear to avoid confusion. Thanks for this suggested change. I may tweak it slightly but it looks good.
| ### Plugin `security-properties` | ||
|
|
||
| Options | ||
| : `--security-properties=*filename*` |
There was a problem hiding this comment.
I think the approach to use the contents as overrides is good. I'm just wondering if --security-properties is the best name as it initially looked like this sets the security properties when it's really more like an overlay/patch to override the values of existing properties, or add new properties, if you see what I mean.
There was a problem hiding this comment.
I think with the suggested improved description by @jerboaa it will be more clear and we don't necessarily need to change the option name.
| if (overrideProps.containsKey("include")) { | ||
| throw new IllegalArgumentException( | ||
| "the include property is not supported"); | ||
| } |
There was a problem hiding this comment.
Why is this being treated specially? It makes the use-cases of using this jlink plugin more limited to editing the file in place (or using some post-processor). It would break some of the use cases we have with system crypto policies where this plugin would come in handy:
Consider this use case:
Take a generic JDK build that is about to being used to integrate with system crypto policies (see JDK-8319332) on some systems. The generic JDK build is also being used elsewhere where this isn't being done. The system policies are maintained by the OS and live in /etc/crypto-policies/back-ends/java.config, say. A custom jlink invocation including all modules with --security-properties extra could achieve this when extra contained include=/etc/crypto-policies/back-ends/java.config. The benefit of using this over --security-properties /etc/crypto-policies/back-ends/java.config is that the system policy file could be updated async to the jlink command being run.
There was a problem hiding this comment.
The main issue I see with supporting the include directive is that its position relative to other properties is important. This is described in the CSR:
The effect of each definition is to include a
referred security properties file inline, adding all its properties.
Security properties defined before an include statement may be overridden
by properties in the included file, if their names match. Conversely,
properties defined after an include statement may override properties in
the included file.
There was a problem hiding this comment.
Hmm, I see. I would think the main use-case for include is to include it last. Perhaps it could be considered to allow includes only at the end and a single time instead of outright failing? A warning on include processing could be added too.
There was a problem hiding this comment.
Hi @seanjmullan, sorry if you already discussed this, but have you considered just appending the contents of <filename> at the end of java.security? Is there any other downside, besides the increased size of the resource file? If the most likely use-case doesn't override a bunch of properties, the size waste could remain moderate.
That approach would automatically address the properties overriding (<filename> content goes after java.security content) and support include directives, without the need to parse the properties files and manually track the overrides.
There was a problem hiding this comment.
Do you mean the file in the include directive? The problem with that is that file may not exist on the system you are running the jlink command from.
Or did you mean the file containing the properties you want to override? The latter would work, although it could be confusing to have more than one property with different values to someone reading the file manually. Perhaps it would be ok if I inserted a comment stating that properties below this line will override the above properties with the same name.
There was a problem hiding this comment.
I meant the latter, the problem we observed with
cat props >> conf/security/java.securityis while linking a jmod-less image. The SHA-512 checksum of the originalconf/security/java.securityfile is stored in the jimage file (lib/modules). So if we modifyconf/security/java.securityand runjlink(in a jmod-less JDK), we getError: .../conf/security/java.security has been modified.
Right. Note that there is also a concept of "upgradable files" and java.security could be considered upgradable just like cacerts or tzdata. See: https://bugs.openjdk.org/browse/JDK-8353185. Both would work, but I tend to think the jlink plugin would be better suited for this.
There was a problem hiding this comment.
Just on the ">>" discussion. I think the downside is that the resulting java.security may have duplicate keys with different values. The files in $JAVA_HOME/conf are user editable so if someone were to edit java.security then they could easily change the value of the first key and the change would not be effective.
There was a problem hiding this comment.
I meant the latter, the problem we observed with
cat props >> conf/security/java.securityis while linking a jmod-less image. The SHA-512 checksum of the originalconf/security/java.securityfile is stored in the jimage file (lib/modules). So if we modifyconf/security/java.securityand runjlink(in a jmod-less JDK), we getError: .../conf/security/java.security has been modified.
Why don't you get the error in a jmod JDK? Just trying to understand the distinction.
There was a problem hiding this comment.
Just on the ">>" discussion. I think the downside is that the resulting java.security may have duplicate keys with different values. The files in $JAVA_HOME/conf are user editable so if someone were to edit java.security then they could easily change the value of the first key and the change would not be effective.
Yes, that is a downside, and one that should not be taken lightly.
In an actual deployment, in order for the include directive to be used, I think it must be included at least once in the java.security file (assuming you are not using -Djava.security.properties). Let's take 3 cases where it might be included:
- at the beginning of the file. This doesn't make any sense as all properties after that would be overridden. It would have no impact.
- at the end of the file. This is the easiest case, and one that could be supported equivalently by adding at the end.
- somewhere in the middle. I don't know if this is common since the properties don't have any specific order, and we could change the order if we wanted. I think this would be more common in the included files themselves.
Plus the include directive could also be somewhere in the beginning or middle of the properties file and that makes it more like how it is handled currently if specified with the -Djava.security.properties option.
All in all, this becomes very complex to support all of these cases within the current implementation. And adding the whole file at the end has downsides as mentioned above.
I think a possible middle ground is to have a separate option that adds a single include directive to the end of the java.security file, and that's the only case that is supported. Include directives in the properties file are still treated as an error. I would have to experiment with this, but something like:
jlink --security-properties props=<filename>:include=/etc/crypto-policies/back-ends/java.config
There was a problem hiding this comment.
Why don't you get the error in a jmod JDK? Just trying to understand the distinction.
In a jmod JDK, the lib/modules image doesn't contain the checksums. Instead, jlink obtains a pristine java.security copy from jmods/java.base.jmod, ignoring any change to conf/security/java.security. So we don't get an error, but, as with a jmod-less JDK, there's no way to propagate the conf/security/java.security changes to the image.
I think an alternative to point to a whole java.security replacement, or an option to pass-trhough conf/security/java.security with its modifications into the image would be great for our use-case.
| String propName = line.substring(0, index); | ||
| String propValue = props.remove(propName.trim()); | ||
| if (propValue != null) { | ||
| // skip multi-line values |
There was a problem hiding this comment.
| // skip multi-line values | |
| // skip multi-line values in original |
|
|
||
| Description | ||
| : Override the security properties in the `java.security` configuration | ||
| file with the properties in the specified file. |
There was a problem hiding this comment.
+1. It would make sense to also document that that the include directive is not supported.
| file with the properties in the specified file. | |
| Override the security properties - if they exist - in the `conf/security/java.security` | |
| configuration file with the properties in the specified file. Appends properties not | |
| previously present in `java.security` at the end. The `include` directive is not supported. |
There was a problem hiding this comment.
With this patch to the file the test becomes runnable with JEP 493 enabled as well (it's currently skipped).
diff --git a/test/jdk/tools/jlink/plugins/SecurityPropertiesPluginTest.java b/test/jdk/tools/jlink/plugins/SecurityPropertiesPluginTest.java
index 697816a2dc3..8d40dc60048 100644
--- a/test/jdk/tools/jlink/plugins/SecurityPropertiesPluginTest.java
+++ b/test/jdk/tools/jlink/plugins/SecurityPropertiesPluginTest.java
@@ -27,6 +27,7 @@
import java.util.Map;
import java.util.Properties;
+import jdk.tools.jlink.internal.LinkableRuntimeImage;
import jtreg.SkippedException;
import jdk.test.lib.Asserts;
import tests.Helper;
@@ -35,10 +36,11 @@
* @bug 8377819
* @summary Test the --security-properties plugin
* @library ../../lib /test/lib
- * @modules java.base/jdk.internal.jimage
+ * @modules jdk.jlink/jdk.tools.jlink.internal
+ * java.base/jdk.internal.jimage
* jdk.jlink/jdk.tools.jimage
* @build tests.*
- * @run main SecurityPropertiesPluginTest
+ * @run main/othervm SecurityPropertiesPluginTest
*/
public class SecurityPropertiesPluginTest {
@@ -47,10 +49,11 @@ public class SecurityPropertiesPluginTest {
private static String SECPROPS_PATH = "conf/security/java.security";
private static String TEST_DIR = System.getProperty("test.dir", ".");
+ private static final boolean LINKABLE_RUNTIME = LinkableRuntimeImage.isLinkableRuntime();
public static void main(String[] args) throws Throwable {
- helper = Helper.newHelper();
+ helper = Helper.newHelper(LINKABLE_RUNTIME);
if (helper == null) {
throw new SkippedException("Test not run");
}
|
/template append |
|
@seanjmullan The pull request template has been appended to the pull request body |
4 participants
This is a new jlink plugin which allows the user to specify values of security properties it wants to override in the
conf/security/java.securityconfiguration file in a custom runtime image. This enhancement, along with thecacertsjlink plugin allow users to more easily create runtimes that address the specific security requirements of their applications.The command-line syntax takes a file containing properties that the user wants to override.
For example:
where
props.securityis a file containing one more more properties in thejava.securityfile syntax.Progress
Issues
Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/30635/head:pull/30635$ git checkout pull/30635Update a local copy of the PR:
$ git checkout pull/30635$ git pull https://git.openjdk.org/jdk.git pull/30635/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 30635View PR using the GUI difftool:
$ git pr show -t 30635Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/30635.diff
Using Webrev
Link to Webrev Comment