RFR: 8319332: Security properties files inclusion [v8]

Tue Apr 23 01:48:34 UTC 2024

On Mon, 22 Apr 2024 20:42:44 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 with a new target base due to a merge or a rebase. The pull request now contains 13 commits:
> 
>  - Extend backward compatibility support
>    
>    Co-authored-by: Francisco Ferrari <fferrari at redhat.com>
>    Co-authored-by: Martin Balao <mbalao at redhat.com>
>  - Merge 'openjdk/master' into JDK-8319332
>  - Merge 'openjdk/master' into JDK-8319332
>  - Merge 'openjdk/master' into JDK-8319332
>    
>    Conflict in ConfigFileTest.java solved by keeping our file, which had
>    been previously adjusted.
>    
>    Commands:
>      git merge upstream/master
>      git restore --ours -- test/jdk/java/security/Security/ConfigFileTest.java
>      git add test/jdk/java/security/Security/ConfigFileTest.java
>      git merge --continue
>  - 8319332: Adjust code for JDK-8319673 changes
>    
>    JDK-8319673: Few security tests ignore VM flags
>    
>    Next, we will merge the openjdk/master branch and ignore the conflict in
>    this file.
>    
>    Co-authored-by: Martin Balao <mbalao at redhat.com>
>    Co-authored-by: Francisco Ferrari Bihurriet <fferrari at redhat.com>
>  - 8319332: Update copyright and ConfigFileTest.java.
>    
>    Bump copyright year to 2024 in all the modified files.
>    
>    Remove leaked host name from children JVMs debug command.
>    
>    Extract Executor::addSystemPropertiesAsJvmArgs from Executor::execute
>    and rename 'allJvmArgs' to 'command'. Also split class name and
>    RUNNER_ARG addition to 'command' as two separated command.add() calls.
>    
>    Co-authored-by: Martin Balao <mbalao at redhat.com>
>    Co-authored-by: Francisco Ferrari Bihurriet <fferrari at redhat.com>
>  - Merge 'openjdk/master' into JDK-8319332
>  - 8319332: Fix corner-case regression with bash pipe
>    
>    Extra properties files provided through bash pipes used to work before
>    this enhancement, restore their behaviour.
>    
>    Also take advantage to use Files::isRegularFile, Files::isDirectory and
>    Files::exists APIs instead of converting from Path to File.
>    
>    Linux reproducers (sub-shell, stdin, and combination of both):
>    
>    java -XshowSettings:security:properties                      \
>         -Djava.security.properties==<(echo name=value)          \
>         -Djava.security.debug=properties -version
>    
>    echo name=value | java -XshowSettings:security:properties    \
>         -Djava.security.properties==/dev/stdin                  \
>         -Djava.security.debug=properties -version
>    
>    echo name=value | java -XshowSettings:security:properties    \
>         -Djava.secu...

Thanks. I really appreciate it. I’m on vacation this week and will read more when I’m back.

On Apr 22, 2024, at 18:34, Martin Balao Alonso ***@***.***> wrote:

Hi @wangweij<https://urldefense.com/v3/__https://github.com/wangweij__;!!ACWV5N9M2RV99hQ!PLfmz4Pj-Wb17DCRZva4wDWu3nWdbLz1skiojB-1WAGnJ8PgdU8Eg_Xv53oGhCOT8qu_cVpRYLGi0TuC61ayzndAFSo$> ,

We have pushed a change to support malformed URLs as discussed before. We introduced changes to the ConfigFileTest test so the backward-compatible scenarios are asserted. This has been tested on both Windows and Linux. In summary, our tests show no regression compared to the previous java.security.properties behavior: file: works, file:/ works and file:/// works. file:// does not work, because it tries to establish an FTP connection to a host with the empty string hostname. Notice that the latter behavior comes from java.net.URL::openStream and was there before.

We have also introduced the following changes to the CSR<https://bugs.openjdk.org/browse/JDK-8319333>:

  1.  Removed file:/// note from Compatibility Risk Description

  2.  Added the discussion about empty string expansion of non-existent system properties in include paths. See section Solution and subsections Syntax and Examples of Specification.

Looking forward to your thoughts.

—
Reply to this email directly, view it on GitHub<https://urldefense.com/v3/__https://github.com/openjdk/jdk/pull/16483*issuecomment-2071248623__;Iw!!ACWV5N9M2RV99hQ!PLfmz4Pj-Wb17DCRZva4wDWu3nWdbLz1skiojB-1WAGnJ8PgdU8Eg_Xv53oGhCOT8qu_cVpRYLGi0TuC61ay3fAX3fk$>, or unsubscribe<https://urldefense.com/v3/__https://github.com/notifications/unsubscribe-auth/AAEMEPJZRKH3I43STJDLVV3Y6W3AHAVCNFSM6AAAAAA63MC2XCVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDANZRGI2DQNRSGM__;!!ACWV5N9M2RV99hQ!PLfmz4Pj-Wb17DCRZva4wDWu3nWdbLz1skiojB-1WAGnJ8PgdU8Eg_Xv53oGhCOT8qu_cVpRYLGi0TuC61ayh5LNOdE$>.
You are receiving this because you were mentioned.Message ID: ***@***.***>

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

PR Comment: https://git.openjdk.org/jdk/pull/16483#issuecomment-2071258992