Re: RFR (12): 8191053: Provide a mechanism to make system's security manager immutable
Hi Sean,
Thanks for the updates. Looks like the details are all there now. One minor
point:
87 * If a class name is specified, it must be {@code
java.lang.SecurityManager}
88 * or a public subclass and have a public no-arg constructor. The class is
89 * loaded by the {@linkplain ClassLoader#getSystemClassLoader()
90 * system class loader}.
The class is loaded by the system class loader, only if the class name is
something other than "java.lang.SecurityManager".
Editorial:
83 * the system property "{@code java.security.manager}" on the command line
to
Here and in several places, I don't think it's necessary to have the property
name both in code font and in quotation marks. I think the code font is sufficient.
Thanks,
s'marks
On 10/4/18 2:04 PM, Sean Mullan wrote:
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 "<>".)
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
Re: RFR (12): 8191053: Provide a mechanism to make system's security manager immutable
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 ">".)
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,
Re: JGSS Enhancements (contribution by Two Sigma Open Source)
On Thu, Oct 04, 2018 at 11:19:06AM +0100, Alan Bateman wrote: > On 03/10/2018 21:49, Nico Williams wrote: > >: > >A lot of these changes are interrelated. Reviewing them in order of > >size might require rebasing our stack of commits, and may not be > >entirely possible. > > > >We're extremely familiar with this code as we have been patching the > >JGSS stack this way for years (we have developed these patches for JDKs > >7, 8, 9, 10, 11, and the current 12 master), and we have been running > >this in production (with JDKs 7, 8, and 9, and soon 11) > Just a few high-level points on the patches that you attached: > > 1. It's important to take sponsor/Reviewer effort into account. I > skimmed through some of the 25 patches and they lack a detailed > description on what the issue is about. JGSS gurus might recognize > some of these issues from the diffs but I suspect you (or Victor) > will need to match the patches to existing issues in JIRA or else > get bugs submitted so that there is a description for each issue in > the bug database. We've pointed out that we need bugs filed. We'll work with Weijun to get them filed when he's back from vacation. > 2. I skimmed the patches and didn't see any tests or changes to > existing tests. This may come up in the discussion of each change as > the default position is for all bug fixes should have tests where > feasible. That's an existing problem in the codebase. The tests are very limited as it is, and testing GSS/Kerberos requires mocking up a KDC, which requires more complex test infrastructure. > 3. I see the patches include at least some API changes so the > sponsor will need to submit a CSR for approval. API changes are only > allowed in feature releases. That's fine. Nico --
Re: Fwd: Re: RFR (12): 8191053: Provide a mechanism to make system's security manager immutable
On Wed, Oct 3, 2018 at 7:53 PM Sergey Bylokhov wrote: > Hi, Sean. > One question related to SecurityManager and performance, is it possible > to provide a special version of AccessController.doPrivileged which will > be noop if SecurityManager is not present? TBH that method (at least, the no-arg variant) should *always* have been a no-op. All it really accomplishes, in practice, is to place a mark on the stack where the stack crawl to build the access control context should stop (plus one frame), and this effect is something that is already a natural consequence of a method being called in the JVM. The doPrivileged variant that accepts an ACC parameter should likewise always have been no-op other than stashing the nested ACC into some sort of thread-local (or better, a field on Thread) which can be referred to by the aforementioned stack crawl. The pure-java AccessController I prototyped late last year relies on these ideas, among other things. -- - DML
Re: Fwd: Re: RFR (12): 8191053: Provide a mechanism to make system's security manager immutable
On 10/3/18 8:52 PM, Sergey Bylokhov wrote:
Hi, Sean.
One question related to SecurityManager and performance, is it possible
to provide a special version of AccessController.doPrivileged which will
be noop if SecurityManager is not present?
Yes, it may be possible, and we have prototyped it. But I didn't want to
mix it up with this one as it has some different specification
challenges -- for example, the AccessController APIs can be used
independently of the SecurityManager. I plan to get back to this one
soon and hope to have something that can be reviewed a bit later.
Thanks,
Sean
On 03/10/2018 13:12, Sean Mullan wrote:
For those of you that are not also subscribed to security-dev, this is
mostly FYI, as the review is winding down, but if you have any
comments, let me know.
This change will add new token options ("allow" and "disallow") to the
java.security.manager system property. The "disallow" option is
intended to provide a potential performance boost for applications
that don't enable a SecurityManager, as there is a cost associated
with allowing a SecurityManager to enabled at runtime, even if it is
never enabled. The CSR provides a good summary of the issue and spec
changes: https://bugs.openjdk.java.net/browse/JDK-8203316
Thanks,
Sean
Forwarded Message
Subject: Re: RFR (12): 8191053: Provide a mechanism to make system's
security manager immutable
Date: Tue, 2 Oct 2018 11:34:09 -0400
From: Sean Mullan
Organization: Oracle Corporation
To: security Dev OpenJDK
Hello,
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
Re: JGSS Enhancements (contribution by Two Sigma Open Source)
On 03/10/2018 21:49, Nico Williams wrote: : A lot of these changes are interrelated. Reviewing them in order of size might require rebasing our stack of commits, and may not be entirely possible. We're extremely familiar with this code as we have been patching the JGSS stack this way for years (we have developed these patches for JDKs 7, 8, 9, 10, 11, and the current 12 master), and we have been running this in production (with JDKs 7, 8, and 9, and soon 11) Just a few high-level points on the patches that you attached: 1. It's important to take sponsor/Reviewer effort into account. I skimmed through some of the 25 patches and they lack a detailed description on what the issue is about. JGSS gurus might recognize some of these issues from the diffs but I suspect you (or Victor) will need to match the patches to existing issues in JIRA or else get bugs submitted so that there is a description for each issue in the bug database. 2. I skimmed the patches and didn't see any tests or changes to existing tests. This may come up in the discussion of each change as the default position is for all bug fixes should have tests where feasible. 3. I see the patches include at least some API changes so the sponsor will need to submit a CSR for approval. API changes are only allowed in feature releases. -Alan
