RFR: 8319332: Security properties files inclusion [v18]

[Weijun Wang]

On Tue, 6 Aug 2024 23:55:06 GMT, Francisco Ferrari Bihurriet <fferrari at openjdk.org> wrote:

>> The implementation of this proposal is based on the requirements, specification and design choices described in the [JDK-8319332] ticket and its respective CSR [JDK-8319333]. What follows are implementation notes organized per functional component, with the purpose of assisting to navigate the code changes in this pull-request.
>> 
>> ## Security properties loading (overview)
>> 
>> A new static class named `SecPropLoader` (nested within `java.security.Security`) is introduced to handle the loading of all security properties. Its method `loadAll` is the first one to be called, at `java.security.Security` static class initialization. The master security properties file is then loaded by `loadMaster`. When additional security properties files are allowed (the security property `security.overridePropertiesFile` is set to `true`) and the `java.security.properties` system property is passed, the method `loadExtra` handles the extra load.
>> 
>> The master properties file is loaded in `OVERRIDE` mode, meaning that the map of properties is originally empty. Any failure occurred while loading these properties is considered fatal. The extra properties file (`java.security.properties`) may be loaded in `OVERRIDE` or `APPEND` mode. Any failure in this case is ignored. This behavior maintains compatibility with the previous implementation.
>> 
>> While the `java.security.properties` system property is documented to accept an URL type of value, filesystem path values are supported in the same way that they were prior to this enhancement. Values are then interpreted as paths and, only if that fails, are considered URLs. In the latter case, there is one more attempt after opening the stream to check if there is a local file path underneath (e.g. the URL has the form of `file:///path/to/a/local/file`). The reason for preferring paths over URLs is to support relative path file inclusion in properties files.
>> 
>> ## Loading security properties from paths (`loadFromPath` method)
>> 
>> When loading a properties file from a path, the normalized file location is stored in the static field `currentPath`. This value is the current base to resolve any relative path encountered while handling an _include_ definition. Normalized paths are also saved in the `activePaths` set to detect recursive cycles. As we move down or up in the _includes_ stack, `currentPath` and `activePaths` values are updated.
>> 
>> ## Loading security properties from URLs (`loadFromUrl` method)
>> 
>> The extra properti...
>
> Francisco Ferrari Bihurriet has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Throw an IllegalArgumentException exception if Security.setProperty("include", ...) is invoked.
>   
>   Co-authored-by: Martin Balao <mbalao at redhat.com>
>   Co-authored-by: Francisco Ferrari Bihurriet <fferrari at redhat.com>

Some last comments. Thanks.

src/java.base/share/classes/java/security/Security.java line 118:

> 116:         }
> 117: 
> 118:         private static void loadMaster() {

Maybe you can rename to `loadMain`. There have been some voices against the usage of the "master" word.

src/java.base/share/classes/java/security/Security.java line 211:

> 209:         }
> 210: 
> 211:         private static void reset(LoadingMode mode) {

The method here looks like there is a chance that `props` does not get assigned. I know when the main file is loaded the mode is OVERRIDE, so this will not actually happen. Do you want to add a comment on this?

Or, maybe we can assign `props` at the beginning and use APPEND mode when loading the main file?

src/java.base/share/classes/java/security/Security.java line 241:

> 239:             try {
> 240:                 Path path = Path.of(expPropFile);
> 241:                 if (!path.isAbsolute()) {

So you allow a properties file on the net to include a local absolute path file. Is this intended?

src/java.base/share/classes/sun/security/util/PropertyExpander.java line 72:

> 70:         } catch (ExpandException e) {
> 71:             // should not happen
> 72:             throw new RuntimeException("unexpected expansion error: when " +

Is an `AssertionError` better here?

src/java.base/share/conf/security/java.security line 33:

> 31: #
> 32: # The special "include" property can be defined one or multiple times in
> 33: # this file with a filesystem path value. The effect of each definition

"this file". Is this precise? As described below, It can also appear in included files and the file "java.security.properties" points to.

Maybe not an issue since no one will be confused. You decide whether to make any update.

src/java.base/share/conf/security/java.security line 45:

> 43: # "include" definition, if local. Paths may contain system properties for
> 44: # expansion in the form of ${system.property}. If a system property does
> 45: # not have a value, it expands to the empty string.

I mentioned this in a previous comment, but if java.security.properties points to an HTTP URL, can it still include a local file with absolute path?

-------------

PR Review: https://git.openjdk.org/jdk/pull/16483#pullrequestreview-2225820362
PR Review Comment: https://git.openjdk.org/jdk/pull/16483#discussion_r1707553139
PR Review Comment: https://git.openjdk.org/jdk/pull/16483#discussion_r1707561234
PR Review Comment: https://git.openjdk.org/jdk/pull/16483#discussion_r1707547937
PR Review Comment: https://git.openjdk.org/jdk/pull/16483#discussion_r1707573692
PR Review Comment: https://git.openjdk.org/jdk/pull/16483#discussion_r1707578252
PR Review Comment: https://git.openjdk.org/jdk/pull/16483#discussion_r1707590811