On Thu, 2 May 2024 21:24:19 GMT, Francisco Ferrari Bihurriet <fferr...@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: > > Profiles documentation adjustments. > > Co-authored-by: Francisco Ferrari <fferr...@redhat.com> > Co-authored-by: Martin Balao <mba...@redhat.com> src/java.base/share/conf/security/java.security line 50: > 48: # switch between predefined security profiles on a per-execution basis. > 49: # Profile selection may be required or optional depending on the files > 50: # layout, as shown in example #3 below. I feel like this paragraph is better suited to a programmer's guide. I think the text for this property is already quite long, and we should focus mostly on specification and behavior and leave text that sounds like advice or use cases to a programmer's guide, or release note. src/java.base/share/conf/security/java.security line 70: > 68: # an error is thrown and explicit profile selection is required. In order > 69: # to override properties defined in this file, the include statement may > be > 70: # placed at the end. Same with this paragraph. I think it is ok/useful to include the 3 examples but explaining how one might deploy the include files with FIPS as an example is more for a programming guide. Also, some text is repeated - you already indicated an error is thrown if a file does not exist and the property defaults to the empty string. The one sentence I found useful is the last one - but I think you could move this as the last sentence of the first paragraph and it would have more impact. I would change "may" to "should". ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/16483#discussion_r1589654525 PR Review Comment: https://git.openjdk.org/jdk/pull/16483#discussion_r1589658468
