Excellent comments, Stuart. Thanks for taking the time to review this.
I have posted another review that should address most of your comments,
but also responded inline with replies below.
http://cr.openjdk.java.net/~mullan/webrevs/8191053/webrev.02/
I also posted the javadoc so you can see what it looks like, esp. the table:
http://cr.openjdk.java.net/~mullan/webrevs/8191053/webrev.02/docs/api/java.base/java/lang/SecurityManager.html
http://cr.openjdk.java.net/~mullan/webrevs/8191053/webrev.02/docs/api/java.base/java/lang/System.html
On 10/3/18 7:11 PM, Stuart Marks wrote:
Hi Sean,
The new arrangement of using special tokens for java.security.manager
makes a lot more sense than having a separate property. Overall, the
proposed semantics seem reasonable to me.
I have some suggestions to clarify the proposed specification. (But also
see below.) From webrev.01:
81 * Environments using a security manager will typically set the
security
82 * manager at startup. In the JDK implementation, this is done by
setting
83 * the system property "{@code java.security.manager}" on the
command line to
84 * the class name of the security manager, or to the empty String
("")
85 * or "{@code default}" to utilize the default security manager.
86 * If the "{@code java.security.manager}" system property is not
set, the
87 * default value is {@code null}, which means a security manager
will not be
88 * installed at startup.
I'd suggest using the term "special token" to describe the string
"default" here, to make it clear that this string is interpreted
specially and is not interpreted as a security manager class name. (The
FilePermission docs use the term "special token" to describe "<<ALL
FILES>>".)
Similarly, I'd suggest "special token" be used to describe "allow" and
"disallow" below.
Good suggestion, I added "special token" to those places as well as to
the reference to "disallow" in System.setSecurityManager. I also broke
up the first sentence above on lines 81-85 in two sentences to make it
easier to read.
93 * In the JDK implementation, if the Java virtual machine is
started with
94 * the "{@code java.security.manager}" system property set to
95 * "{@code =disallow}" then the
96 * {@link System#setSecurityManager(SecurityManager)
setSecurityManager}
97 * method cannot be used to set a security manager and will throw an
98 * {@code UnsupportedOperationException}. The ability to
dynamically set the
(I assume this will be changed from "=disallow" to "disallow" and
similar for "allow" below.)
Oops. Yes. I had fixed that but forgot to upload it in the webrev. Fixed
now.
This should clarify that if "disallow" is used then no security manager
will be installed, in addition to preventing one from being installed in
the future.
Yes, fixed.
98 * The ability to
dynamically set the
99 * security manager in a running system is an impediment to some
performance
100 * optimizations, even if the method is never called.
While I think this is true, it's a bit of rationale stuck into the
middle of the specification for the values of the system property, and
as such it sticks out. It kind of begs for more explanation. I'd suggest
removing it.
I was on the fence about including that as well. I have removed it (and
also from the implNote in System.setSecurityManager). I think the JBS
issue is the best place to keep this type of information for now.
100 * If a security
manager is
101 * set at startup, or if the property is set to "{@code =allow}",
then a
102 * security manager can be set dynamically.
I'd split this into two (or multiple) sentences, because there's
actually a lot going on here.
If the property is set to the special token "allow", no security manager
is installed at startup, but one can be set dynamically using the
setSecurityManager method. (Right?)
Correct.
I think there are more cases that need to be covered than just "allow".
If the property is set to "allow", the class name of a security manager,
the empty string "", or the special token "default", then the
setSecurityManager() method can be used to attempt to set the security
manager dynamically. However, an already-installed security manager may
refuse this request. (Right?)
Correct. Initially I was a bit reluctant to document the behavior of
System.setSecurityManager for all the different property values. It also
depends on whether you are calling it for the first time or not. The
only real difference in behavior is if "disallow" is set. Otherwise, it
works exactly as the API is specified. But I can see how it can cause
confusion since there are many different options for
java.security.manager now.
**
Whew, this is kind of a lot. The reason is that there are several
different values that the property can be set to, and they have an
effect on the initial SM that's set AND an effect on the behavior of
calls to setSecurityManager() at runtime. To get this straight, I wrote
down a table:
Prop Value SM installed initially setSecurityManager()
works
------------------------------------------------------------------------------
null none yes
empty string java.lang.SecurityManager maybe
"default" java.lang.SecurityManager maybe
"disallow" none no
"allow" none yes
a class name the named class maybe
Note: "maybe" means that setSecurityManager() will attempt to set the
SM, in that it won't throw UOE; however, the current SM might disallow
it depending upon its policies and the privilege of the caller.
From this table one can see that setting the property to the empty
string and to "default" have identical effects. Also, not setting the
property (i.e., its value is null) and setting it to "allow" have the
same effects. Did I get this right?
Yes, more or less. For the "yes" boxes, it is only if you assume it is
the first time it is called and null is not the value of SM.
Anyway, you can describe all of this in prose, but it has to be worded
very carefully in order to get all the details right. Or, you could put
a table directly into the spec. Or both! Up to you how you want to proceed.
I did both! For the table, my main issue was how to document the last
column, and whether to assume setSecurityManager was being called for
the first time or not. I assumed it was not, because this is really
about static vs dynamic and not about how many times an SM can be set.
**
An aside, possibly off topic. If the j.s.m property names a class that's
to be used as the security manager, presumably it must be a
j.l.SecurityManager or a subclass. Are there other requirements, such as
having a public no-arg constructor? Does the class need to be public,
and does it need to be in exported package or anything? Is the classpath
or the module path searched, and if the alternative SM is in a named
module, is there a syntax for naming it?
I don't think the package needs to be exported - it looks like the code
tries to work around that by using reflection. It can be in a named or
unnamed module.
I have kept it simple and added these 2 sentences: "If a class name is
specified, it must be java.lang.SecurityManager or a public subclass and
have a public no-arg constructor. The class is loaded by the system
class loader." We could revisit this later if it should be more precise.
--Sean
Sorry for all the impertinent questions; I can't find where this is
documented. Feel free to redirect.
s'marks
On 10/2/18 8:34 AM, Sean Mullan wrote:
Thanks for all the comments so far, and the interesting discussions
about the future of the SecurityManager. We will definitely return to
those discussions in the near future, but for now I have a second
webrev ready for review for this enhancement:
http://cr.openjdk.java.net/~mullan/webrevs/8191053/webrev.01/
The main changes since the initial revision are:
1. System.setSecurityManager is no longer deprecated. This type of
change clearly needs more discussion and is not an essential part of
this RFE.
2. After further thought, I took Bernd's suggestion [1] and instead of
adding a new property to disallow the setting of a SecurityManager at
runtime, have added new tokens to the existing "java.security.manager"
system property, named "=disallow", and "=allow" to toggle this
behavior. The "=" is to avoid any potential clashes with custom SM
classes with those names. I think this is a cleaner approach. There
are a couple of new paragraphs in the SecurityManager class
description describing the "java.security.manager" property and how
the new tokens work.
3. I also added a comment that Bernd had requested [2] on why
System.setSecurityManager calls checkPackageAccess("java.lang").
Also, the CSR has been updated:
https://bugs.openjdk.java.net/browse/JDK-8203316
Thanks,
Sean
[1]
http://mail.openjdk.java.net/pipermail/security-dev/2018-September/018217.html
[2]
http://mail.openjdk.java.net/pipermail/security-dev/2018-September/018193.html
On 9/13/18 4:02 PM, Sean Mullan wrote:
This is a SecurityManager related change which warrants some
additional details for its motivation.
The current System.setSecurityManager() API allows a SecurityManager
to be set at run-time. However, because of this mutability, it incurs
a performance overhead even for applications that never call it and
do not enable a SecurityManager dynamically, which is probably the
majority of applications.
For example, there are lots of "SecurityManager sm =
System.getSecurityManager(); if (sm != null) ..." checks in the JDK.
If it was known that a SecurityManager could never be set at
run-time, these checks could be optimized using constant-folding.
There are essentially two main parts to this change:
1. Deprecation of System.securityManager()
Going forward, we want to discourage applications from calling
System.setSecurityManager(). Instead they should enable a
SecurityManager using the java.security.manager system property on
the command-line.
2. A new JDK-specific system property to disallow the setting of the
security manager at run-time: jdk.allowSecurityManager
If set to false, it allows the run-time to optimize the code and
improve performance when it is known that an application will never
run with a SecurityManager. To support this behavior, the
System.setSecurityManager() API has been updated such that it can
throw an UnsupportedOperationException if it does not allow a
security manager to be set dynamically.
webrev: http://cr.openjdk.java.net/~mullan/webrevs/8191053/webrev.00/
CSR: https://bugs.openjdk.java.net/browse/JDK-8203316
JBS: https://bugs.openjdk.java.net/browse/JDK-8191053
(I will likely also send this to core-libs for additional review later)
--Sean
