On Thu, 2 Nov 2023 20:07:28 GMT, Alan Bateman <al...@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...
>
> src/java.base/share/classes/java/security/Security.java line 243:
>
>> 241: if (connection instanceof FileURLConnection fileConnection)
>> {
>> 242: // A local file URL can be interpreted as a Path
>> 243: loadFromPath(fileConnection.getFile().toPath(), mode);
>
> Ugh, shouldn't be direct using FileURLConnection here. Instead I think you
> should check if the url scheme is "file" (equalsIgnoreCase). If it is then
> use `Path.of(url.toURI())`.
Checking for _file_ in the URL scheme is not conclusive evidence that there is
a local file path behind. I'll give a couple of examples. In Unix/Linux
platforms, an URL of the form `file://example.com/path/to/some/file.txt` is
processed with a remote FTP request (see Unix
`sun.net.www.protocol.file.Handler`). In Windows, file URLs may be interpreted
as UNCs but, if not possible, there is an FTP fallback (see Windows
`sun.net.www.protocol.file.Handler`). While checking the host name in the URL
is possible, there are three types of drawbacks: 1) a DNS query during the
Security class initialization process should be avoided, 2) looking for
hardcoded host names such as _localhost_ might lead to false negatives (i.e. a
host is considered remote when it is not), and 3) there will be
platform-specific and duplicated logic to deal with UNC file URLs. In addition,
OpenJDK supports ill-formed relative path file URLs such as
`file:some/relative/path`. In these cases, there is not a host name bu
t there is a local file path underneath (relative to the current working
directory). We did not find normative elements in [RFC
8089](https://www.rfc-editor.org/rfc/rfc8089) for all previously described
behaviors, that would have been helpful for a URL-based check. Misinterpreting
a file URL as remote will unnecessarily block the possibility of relative path
includes.
We think that `FileURLConnection` is the most accurate indicator of a local
file path because it includes the decision logic that is specific to OpenJDK
and varies depending on the platform.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/16483#discussion_r1380842630
