RFR: 8319332: Security properties files inclusion [v19]

[Martin Balao]

On Wed, 7 Aug 2024 20:09:09 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:
> 
>   Code review and additional changes
>   
>   Throw an IllegalArgumentException exception if Security.getProperty() is
>   invoked with "include" as key, also add a check in the test case.
>   
>   Initialize java.security.Security::props in a single place, and make it
>   final.
>   
>   Make sun.security.util.PropertyExpander::expandNonStrict() throw
>   AssertionError instead of RuntimeException.
>   
>   Remove "this file" reference in java.security.
>   
>   Additionally, use java.security.Security::check() to check the
>   'getProperty.<key>' permission, as with 'setProperty.<key>'.
>   
>   Co-authored-by: Martin Balao <mbalao at redhat.com>
>   Co-authored-by: Francisco Ferrari Bihurriet <fferrari at redhat.com>

Our original proposal was to ignore the call because it would be the programmatic equivalent of adding the security property `include` to the `java.security` file and not seeing it on the properties map. For one who has not read any of the documentation available, it's unexpected indeed. Realistically, we don't expect anybody to use `include` as a Security property name and the potential risk of confusion should be minimal.

While throwing an exception was not our first preference, we agreed on the basis of giving immediate feedback to the user in such unlikely case. The current exceptions thrown in the `setProperty` and `getProperty` APIs do not really fit the new case and that's why we proposed IAE. We can document the _reserved_ words in the API if that helps. While the reader may not fully understand what `include` is, it will be evident that it won't be affected by the exception because it's probably not a security property name in use.

In any case, we prefer not to have `include` as a property on the map. We also considered triggering an inclusion action from a `setProperty` call but the mix of properties and statements under the same API would be confusing. Thus, if we want the include programmatic behavior at some point in the future, there should be a different API instead of overloading `setProperty`.

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

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