RFR: 8319332: Security properties files inclusion [v5]

Mon Jan 8 18:59:54 UTC 2024

> 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 properties file can be loaded from a URL. ...

Francisco Ferrari Bihurriet has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains seven additional commits since the last revision:

 - 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.security.properties==<(echo include /dev/stdin)  \
        -Djava.security.debug=properties -version

   Co-authored-by: Martin Balao <mbalao at redhat.com>
   Co-authored-by: Francisco Ferrari Bihurriet <fferrari at redhat.com>
 - 8319332: Remove wildcard imports, adjust test case

   Test case adjustments: add missing check in PropsFile::assertApplied(),
   and fix typo in FilesManager::handleRequest() method name.

   Also, add clarifying comments to Executor::assertSuccess() explaining
   how we use the special properties to make the assertion. Make 'lastFile'
   assignation latter, so it's separated from the first group of asserts.

   Co-authored-by: Martin Balao <mbalao at redhat.com>
   Co-authored-by: Francisco Ferrari Bihurriet <fferrari at redhat.com>
 - 8319332: use Path::of(URI) to deal with file URLs

   Instead of the previously introduced FileURLConnection::getFile(), use
   Path::of(URI), leaving some file URL corner-cases without relative
   imports support.

   Further details in https://github.com/openjdk/jdk/pull/16483#discussion_r1382111155

   Co-authored-by: Martin Balao <mbalao at redhat.com>
   Co-authored-by: Francisco Ferrari Bihurriet <fferrari at redhat.com>
 - 8319332: Implement security properties 'include'

   Co-authored-by: Martin Balao <mbalao at redhat.com>
   Co-authored-by: Francisco Ferrari Bihurriet <fferrari at redhat.com>
 - 8319332: Refactor security properties file test

   Co-authored-by: Martin Balao <mbalao at redhat.com>
   Co-authored-by: Francisco Ferrari Bihurriet <fferrari at redhat.com>
 - 8319332: Refactor properties file loading code

   Co-authored-by: Martin Balao <mbalao at redhat.com>
   Co-authored-by: Francisco Ferrari Bihurriet <fferrari at redhat.com>

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

Changes:
  - all: https://git.openjdk.org/jdk/pull/16483/files
  - new: https://git.openjdk.org/jdk/pull/16483/files/90df17c2..40afe1f4

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jdk&pr=16483&range=04
 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=16483&range=03-04

  Stats: 809876 lines in 4524 files changed: 189789 ins; 541418 del; 78669 mod
  Patch: https://git.openjdk.org/jdk/pull/16483.diff
  Fetch: git fetch https://git.openjdk.org/jdk.git pull/16483/head:pull/16483

PR: https://git.openjdk.org/jdk/pull/16483