RFR: 8231640: (prop) Canonical property storage

Jaikiran Pai jai.forums2013 at gmail.com
Sun Sep 5 12:31:32 UTC 2021


Hello Alan,

On 05/09/21 1:46 pm, Alan Bateman wrote:
> On 04/09/2021 16:50, Jaikiran Pai wrote:
>> The commit in this PR implements the proposal for enhancement that 
>> was discussed in the core-libs-dev mailing list recently[1], for 
>> https://bugs.openjdk.java.net/browse/JDK-8231640
>>
>> At a high level - the `store()` APIs in `Properties` have been 
>> modified to now look for the `SOURCE_DATE_EPOCH` environment 
>> variable[2]. If that env variable is set, then instead of writing out 
>> the current date time as a date comment, the `store()` APIs instead 
>> will use the value set for this env variable to parse it to a `Date` 
>> and write out the string form of such a date. The implementation here 
>> uses the `d MMM yyyy HH:mm:ss 'GMT'` date format and `Locale.ROOT` to 
>> format and write out such a date. This should provide reproducibility 
>> whenever the `SOURCE_DATE_EPOCH` is set. Furthermore, intentionally, 
>> no changes in the date format of the "current date" have been done.
>>
>> These  modified `store()` APIs work in the presence of the 
>> `SecurityManager` too. The caller is expected to have a read 
>> permission on the `SOURCE_DATE_EPOCH` environment variable. If the 
>> caller doesn't have that permission, then the implementation of these 
>> `store()` APIs will write out the "current date" and will ignore any 
>> value that has been set for the `SOURCE_DATE_EPOCH` env variable. 
>> This should allow for backward compatibility of existing 
>> applications, where, when they run under a `SecurityManager` and 
>> perhaps with an existing restrictive policy file, the presence of 
>> `SOURCE_DATE_EPOCH` shouldn't impact their calls to the `store()` APIs.
>>
>> The modified `store()` APIs will also ignore any value for 
>> `SOURCE_DATE_EPOCH` that  cannot be parsed to an `long` value. In 
>> such cases, the `store()` APIs will write out the "current date" and 
>> ignore the value set for this environment variable. No exceptions 
>> will be thrown for such invalid values. This is an additional 
>> backward compatibility precaution to prevent any rogue value for 
>> `SOURCE_DATE_EPOCH` from breaking applications.
> In the discussion on this option then I think we were talking about 
> changing the specification of the store methods to allow for an 
> implementation specific means to override the date and put the 
> discussion on SOURCE_DATE_EPOCH in an @implNote.
>
I will move the updated javadoc to an @implNote then. I guess, the 
existing part where it explains how the current date comment is written 
out, should stay where it is currently?


> The SM case probably needs discussion as I was expecting to see 
> something like this:
>
> PrivilegedAction<String> pa = () -> System.getenv("SOURCE_DATE_EPOCH");
> String value = AccessController.doPrivileged(pa);
>
> as a caller of the store method (in a library for example) is unlikely 
> to have this permission. I assume your concern is that untrusted code 
> would have a way to read this specific env variable without a 
> permission (by way of creating and storing an empty Properties)?  In 
> this case I'm mostly wondering if it's really worth having a behavior 
> difference in this mode.

I had initially started off with using a doPrivileged code in this 
change. However, I soon realized that it might be a security hole, like 
in the example you state. Unlike other parts of the JDK code where the 
doPrivileged makes sense, since the "action" part of it does things that 
are internal implementation details to the JDK, this new piece of code 
that we are introducing, does a System.getenv(...) in its "action" but 
will then additionally hand out the value (in a formatted manner of 
course) of that environment variable to the callers, through the 
OutputStream/Writer instances that the callers pass in to the store() 
APIs. Effectively, this means that even when getenv.SOURCE_DATE_EPOCH 
(or getenv.* for that matter) haven't been granted to the callers, they 
will _always_ be able to get hold of that environment variable's value 
through this approach. Like you state, they could just call 
Properties.store() (which by the way, doesn't necessarily have to be 
empty) to bypass the security checks that are put in place for the 
direct System.getenv(...) calls from their code.

In such a doPrivelged case, the only way the administrator can prevent 
this security bypass, would then be to _not_ grant this permission to 
the JDK code itself, which then effectively means that this enhancement 
for using SOURCE_DATE_EPOCH will not take effect at all and the date 
comment will always write out the current date.

So the doPriveleged case, IMO, will not allow selective granting of 
permissions to callers. The alternate approach that's used in this PR, 
on the other hand allows for selective granting of permissions. I'm not 
too aware of how things stand with the application server and security 
manager integration these days, but I would guess that in such use cases 
where you probably would want to control which deployed applications 
(within the same JVM) are granted what permissions, this alternate 
approach of not using the doPriveleged would make it feasible to control 
these permissions.

-Jaikiran



More information about the core-libs-dev mailing list