Hi,
The value of SOURCE_DATE_EPOCH is not so sensitive that it needs the
protections you are applying.
The doPriv only exposes the value of that specific environment variable
and in the usual case, it is undefined.
The complexity in the specification and implementation seem unnecessary
in this case.
Though java.util.Date is used in the current implementation, its use is
discouraged in new code
in favor of java.time.ZonedDateTime.
Consider if java.time.format.DateTimeFormatter.RFC_1123_DATE_TIME can be
used to parse and format the time.
Thanks, Roger
On 9/5/21 8:31 AM, Jaikiran Pai wrote:
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
