RFR: 8319332: Security properties files inclusion [v19]

[Sean Mullan]

On Fri, 16 Aug 2024 16:13:17 GMT, Weijun Wang <weijun at openjdk.org> wrote:

>> 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>
>
> I don't like the silent mode. If no one uses that key name, then everything is fine anyway. Otherwise, if someone really sets it, it's very likely they will want to read it somewhere and expect a non `null` value.
> 
> Can we just support it as a real security property? i.e. if it appears in `java.security`, besides loading the content, we can also read it as a security property. And then we also allow it in `setProperty` and `getProperty`. I can see some behavior change:
> 
> 1. If it's in `java.security` and not point to a real file, there will be an exception.
> 2. If it appears in an included file, the value will overwrite the previous one.
> 
> For 1, I don't think you want to be silent about the missing file. For 2, we can ignore it as a security property, but this becomes a little complex.

I'm ok with proceeding to integrate. @wangweij do you have any further comments or concerns?

If Weijun is ok with proceeding, you will need to finalize the CSR before you can integrate. You also need to write a release note, unless I missed that.

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

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