Re: RFR: 8229773: Resolve permissions for code source URLs lazily
Hi Alan, Your suspicion is correct. :) Thanks for the leads, I'll look into it further. Currently the policy implementation finds policy url's in system properties, "java.security.policy" and numbered policy locations with the prefix "policy.url." if the "java.security.policy" property doesn't begin with "=" (which represents java.security.policy==). Cheers, Peter. On 15/09/2019 10:58 PM, Alan Bateman wrote: On 14/09/2019 21:21, Peter Firmstone wrote: Hi Alan, We've got a bunch of very old policy files in our test suite, so they still had policy grants using the extension directory property. The grant for the extension directory property was followed by a forward slash and asterix. Oddly when the property was missing the grant became a wildcard URL. Note this isn't the sun PolicyFile implementation, but our policy provider also augments, rather than replace, maybe there's a new policy file our provider isn't aware of? From memory there was something special about the way the extension directory property was treated by the policy provider, but I don't recall the details, the same problems don't appear to exist when other properties in policy files cannot be resolved. Modules that required permissions, seem to be service providers: In jdk/jdk repo, the following policy files are merged in the build to create the default policy: src/java.base/windows/lib/security/default.policy src/java.base/solaris/lib/security/default.policy src/java.base/share/lib/security/default.policy The default policy goes into a JDK internal location in the run-time image and used by the PolicyFile implementation. If you look in there you should see the permissions that are granted to the modules that map to the platform class loader. The intention is that deployments that are setting their own policy files don't need to be concerned about the permissions of modules in the run-time image. I suspect you are looking for a custom PolicyFile implementation to make use of these defaults to avoid needing to be concerned with the specific permissions that the modules in the run-time image. -Alan
Re: RFR: 8229773: Resolve permissions for code source URLs lazily
On 14/09/2019 21:21, Peter Firmstone wrote: Hi Alan, We've got a bunch of very old policy files in our test suite, so they still had policy grants using the extension directory property. The grant for the extension directory property was followed by a forward slash and asterix. Oddly when the property was missing the grant became a wildcard URL. Note this isn't the sun PolicyFile implementation, but our policy provider also augments, rather than replace, maybe there's a new policy file our provider isn't aware of? From memory there was something special about the way the extension directory property was treated by the policy provider, but I don't recall the details, the same problems don't appear to exist when other properties in policy files cannot be resolved. Modules that required permissions, seem to be service providers: In jdk/jdk repo, the following policy files are merged in the build to create the default policy: src/java.base/windows/lib/security/default.policy src/java.base/solaris/lib/security/default.policy src/java.base/share/lib/security/default.policy The default policy goes into a JDK internal location in the run-time image and used by the PolicyFile implementation. If you look in there you should see the permissions that are granted to the modules that map to the platform class loader. The intention is that deployments that are setting their own policy files don't need to be concerned about the permissions of modules in the run-time image. I suspect you are looking for a custom PolicyFile implementation to make use of these defaults to avoid needing to be concerned with the specific permissions that the modules in the run-time image. -Alan
Re: RFR: 8229773: Resolve permissions for code source URLs lazily
Hi Alan,
We've got a bunch of very old policy files in our test suite, so they
still had policy grants using the extension directory property. The
grant for the extension directory property was followed by a forward
slash and asterix. Oddly when the property was missing the grant became
a wildcard URL. Note this isn't the sun PolicyFile implementation, but
our policy provider also augments, rather than replace, maybe there's a
new policy file our provider isn't aware of?
From memory there was something special about the way the extension
directory property was treated by the policy provider, but I don't
recall the details, the same problems don't appear to exist when other
properties in policy files cannot be resolved.
Modules that required permissions, seem to be service providers:
grant codebase "jrt:/jdk.security.auth" {
permission java.security.AllPermission "", "";
};
grant codebase "jrt:/jdk.crypto.cryptoki" {
permission java.security.AllPermission "", "";
};
grant codebase "jrt:/java.smartcardio" {
permission java.security.AllPermission "", "";
};
grant codebase "jrt:/java.xml.crypto" {
permission java.security.AllPermission "", "";
};
grant codebase "jrt:/java.security.jgss" {
permission java.security.AllPermission "", "";
};
grant codebase "jrt:/jdk.crypto.ec" {
permission java.security.AllPermission "", "";
};
grant codebase "jrt:/jdk.crypto.mscapi" {
permission java.security.AllPermission "", "";
};
grant codebase "jrt:/jdk.localedata" {
permission java.security.AllPermission "", "";
};
grant codebase "jrt:/jdk.security.jgss" {
permission java.security.AllPermission "", "";
};
Regards,
Peter.
On 14/09/2019 5:22 PM, Alan Bateman wrote:
On 13/09/2019 23:07, Peter Firmstone wrote:
:
One change I noticed is permissions granted to the java extension
directory are now granted to every domain in our policy provider as
the java.ext.dirs property is now blank, I also had to grant
permissions to a number of jdk modules, after fixing these, everthing
running as expected, except for a few minor test failures.
The extension mechanism was deprecated in one of the maintenance
releases of the JSR for Java SE 8 so hopefully not a surprise that the
system property java.ext.dirs is no longer set. Since JDK 9, the VM
will will not start if this system property is set on the command line
and there is also a XX option in JDK 8 to help find usages of this
mechanism. So I think I'm surprised that permissions are being granted
when the property isn't set.
Also surprised to hear that you need to grant permissions to the
java.* or jdk.* modules. Are you saying that that the permissions
granted in the default JDK policy file are incomplete or that there
are code is missing a call to doPrivileged somewhere? Part of the
reason for asking is that the both "-Djava.security.policy=" and
"-Djava.security.policy==" will augment rather than override the
default permissions. Maybe you are doing something that doesn't use
this mechanism and you are always overriding the permissions granted
to the modules in the run-time image?
-Alan
Re: RFR: 8229773: Resolve permissions for code source URLs lazily
On 13/09/2019 23:07, Peter Firmstone wrote: : One change I noticed is permissions granted to the java extension directory are now granted to every domain in our policy provider as the java.ext.dirs property is now blank, I also had to grant permissions to a number of jdk modules, after fixing these, everthing running as expected, except for a few minor test failures. The extension mechanism was deprecated in one of the maintenance releases of the JSR for Java SE 8 so hopefully not a surprise that the system property java.ext.dirs is no longer set. Since JDK 9, the VM will will not start if this system property is set on the command line and there is also a XX option in JDK 8 to help find usages of this mechanism. So I think I'm surprised that permissions are being granted when the property isn't set. Also surprised to hear that you need to grant permissions to the java.* or jdk.* modules. Are you saying that that the permissions granted in the default JDK policy file are incomplete or that there are code is missing a call to doPrivileged somewhere? Part of the reason for asking is that the both "-Djava.security.policy=" and "-Djava.security.policy==" will augment rather than override the default permissions. Maybe you are doing something that doesn't use this mechanism and you are always overriding the permissions granted to the modules in the run-time image? -Alan
Re: RFR: 8229773: Resolve permissions for code source URLs lazily
Hi Claes,
So this security manager is part of a much larger program, (a fork of
Jini / Apache River), I've almost finished the transition from Java 8 to
Java 11...
One change I noticed is permissions granted to the java extension
directory are now granted to every domain in our policy provider as the
java.ext.dirs property is now blank, I also had to grant permissions to
a number of jdk modules, after fixing these, everthing running as
expected, except for a few minor test failures.
Next step will be to test against the EA builds.
On 17/08/2019 7:24 AM, Peter Firmstone wrote:
Thanks Claes,
I'll run some tests :)
Cheers,
Peter.
On 16/08/2019 9:14 PM, Claes Redestad wrote:
Hi Peter,
by explicitly ensuring the file system has been initialized before
installing a SecurityManager using a hook in System.setSecurityManager,
the patch at hand takes step to ensure things stay neutral w.r.t.
Permission initialization order when using any SecurityManager. It's not
perfectly identical, so while unlikely, there's a theoretical
possibility some implementation scenario not covered by our regression
tests might run into issues. Any help testing custom implementation on
coming EA builds would be greatly appreciated!
One thing we could do on the JDK end to reduce fragility somewhat in
this area is to provoke initialization of
sun.security.util.SecurityConstants before installing the first SM.
/Claes
On 2019-08-16 12:40, Peter Firmstone wrote:
Hello Alan,
Yes, we are aware of those issues.
I mean documenting that system Permission classes should be loaded
before setting a custom SecurityManager, accessing the file system
is important, so if you haven't loaded the necessary classes before
setting a custom SecurityManager, it won't be gracefull... The
simplest way of ensuring classes are loaded is by creating object
instances of them.
Perhaps just a note to beware of ensuring necessary classes are
loaded and let developers figure out what they need.
The recursive calls are easy enough to deal with to avoid any stack
overflows using ThreadLocal.
inTrustedCodeRecursiveCall.set(Boolean.TRUE);
try {
delegateContext = AccessController.doPrivileged(
new PrivilegedAction(){
public AccessControlContext run() {
return new
AccessControlContext(finalExecutionContext, dc);
}
}
);
}finally {
inTrustedCodeRecursiveCall.set(Boolean.FALSE); //
Must always happen, no matter what.
}
We've only really been caught out once with some jvm bootstrap
changes, otherwise it's been rock solid.
The other thing we do is once we've got more than three
ProtectionDomains on the stack is execute the ProtectionDomain
implies checks in parallel. Really helps when you're making a lot
of network calls.
Cheers,
Peter.
On 16/08/2019 5:04 PM, Alan Bateman wrote:
On 15/08/2019 23:20, Peter Firmstone wrote:
:
The following code is included in the constructor of our
SecurityManager implementation, I suspect we may need to add some
classes to this list, perhaps this is something that needs
documenting?
The checkPermission method of custom security manager can run
arbitrary code so recursive initialization, stack overflow,
bootstrap method errors, ... are always hazards. I don't know what
documentation you have in mind but I don't think there is a
definite list of classes that need to be loaded/initialized before
the custom security manager is set because it will come down to the
code that it executes in its checkPermission method.
-Alan.
Re: RFR: 8229773: Resolve permissions for code source URLs lazily
Thanks everyone. Pushed. /Claes Roger Riggs skrev: (16 augusti 2019 19:00:29 CEST) >+1 > >On 8/16/19 12:51 PM, Sean Mullan wrote: >> +1 from me as well. >> >> --Sean >> >> On 8/16/19 12:38 PM, Alan Bateman wrote: >>> On 16/08/2019 13:30, Claes Redestad wrote: How about this: http://cr.openjdk.java.net/~redestad/8229773/webrev.03/ Also simplified BuiltinClassLoader#getPermissions since the jrt-specific optimization is now redundant. >>> Looks good. >>> >>> -Alan >>> -- Skickat från min Android-enhet med K-9 Mail. Ursäkta min fåordighet.
Re: RFR: 8229773: Resolve permissions for code source URLs lazily
Thanks Claes,
I'll run some tests :)
Cheers,
Peter.
On 16/08/2019 9:14 PM, Claes Redestad wrote:
Hi Peter,
by explicitly ensuring the file system has been initialized before
installing a SecurityManager using a hook in System.setSecurityManager,
the patch at hand takes step to ensure things stay neutral w.r.t.
Permission initialization order when using any SecurityManager. It's not
perfectly identical, so while unlikely, there's a theoretical
possibility some implementation scenario not covered by our regression
tests might run into issues. Any help testing custom implementation on
coming EA builds would be greatly appreciated!
One thing we could do on the JDK end to reduce fragility somewhat in
this area is to provoke initialization of
sun.security.util.SecurityConstants before installing the first SM.
/Claes
On 2019-08-16 12:40, Peter Firmstone wrote:
Hello Alan,
Yes, we are aware of those issues.
I mean documenting that system Permission classes should be loaded
before setting a custom SecurityManager, accessing the file system is
important, so if you haven't loaded the necessary classes before
setting a custom SecurityManager, it won't be gracefull... The
simplest way of ensuring classes are loaded is by creating object
instances of them.
Perhaps just a note to beware of ensuring necessary classes are
loaded and let developers figure out what they need.
The recursive calls are easy enough to deal with to avoid any stack
overflows using ThreadLocal.
inTrustedCodeRecursiveCall.set(Boolean.TRUE);
try {
delegateContext = AccessController.doPrivileged(
new PrivilegedAction(){
public AccessControlContext run() {
return new
AccessControlContext(finalExecutionContext, dc);
}
}
);
}finally {
inTrustedCodeRecursiveCall.set(Boolean.FALSE); //
Must always happen, no matter what.
}
We've only really been caught out once with some jvm bootstrap
changes, otherwise it's been rock solid.
The other thing we do is once we've got more than three
ProtectionDomains on the stack is execute the ProtectionDomain
implies checks in parallel. Really helps when you're making a lot of
network calls.
Cheers,
Peter.
On 16/08/2019 5:04 PM, Alan Bateman wrote:
On 15/08/2019 23:20, Peter Firmstone wrote:
:
The following code is included in the constructor of our
SecurityManager implementation, I suspect we may need to add some
classes to this list, perhaps this is something that needs
documenting?
The checkPermission method of custom security manager can run
arbitrary code so recursive initialization, stack overflow,
bootstrap method errors, ... are always hazards. I don't know what
documentation you have in mind but I don't think there is a definite
list of classes that need to be loaded/initialized before the custom
security manager is set because it will come down to the code that
it executes in its checkPermission method.
-Alan.
Re: RFR: 8229773: Resolve permissions for code source URLs lazily
+1 On 8/16/19 12:51 PM, Sean Mullan wrote: +1 from me as well. --Sean On 8/16/19 12:38 PM, Alan Bateman wrote: On 16/08/2019 13:30, Claes Redestad wrote: How about this: http://cr.openjdk.java.net/~redestad/8229773/webrev.03/ Also simplified BuiltinClassLoader#getPermissions since the jrt-specific optimization is now redundant. Looks good. -Alan
Re: RFR: 8229773: Resolve permissions for code source URLs lazily
+1 from me as well. --Sean On 8/16/19 12:38 PM, Alan Bateman wrote: On 16/08/2019 13:30, Claes Redestad wrote: How about this: http://cr.openjdk.java.net/~redestad/8229773/webrev.03/ Also simplified BuiltinClassLoader#getPermissions since the jrt-specific optimization is now redundant. Looks good. -Alan
Re: RFR: 8229773: Resolve permissions for code source URLs lazily
On 16/08/2019 13:30, Claes Redestad wrote: How about this: http://cr.openjdk.java.net/~redestad/8229773/webrev.03/ Also simplified BuiltinClassLoader#getPermissions since the jrt-specific optimization is now redundant. Looks good. -Alan
Re: RFR: 8229773: Resolve permissions for code source URLs lazily
On 2019-08-15 21:21, Alan Bateman wrote: On 15/08/2019 16:22, Claes Redestad wrote: (adding back core-libs-dev) Hi Roger, seems easy enough to add a writeReplace: http://cr.openjdk.java.net/~redestad/8229773/webrev.02 This mostly looks good. In LazyCodeSourcePermissionCollection it think "initialize" should be renamed to "ensureAdded" as that more accurately describes what it does. Also the class description could do with a bit of word smiting to make it clear that permissions for a CodeSource are lazily added to the underlying permission collection. How about this: http://cr.openjdk.java.net/~redestad/8229773/webrev.03/ Also simplified BuiltinClassLoader#getPermissions since the jrt-specific optimization is now redundant. /Claes
Re: RFR: 8229773: Resolve permissions for code source URLs lazily
Hi Peter,
by explicitly ensuring the file system has been initialized before
installing a SecurityManager using a hook in System.setSecurityManager,
the patch at hand takes step to ensure things stay neutral w.r.t.
Permission initialization order when using any SecurityManager. It's not
perfectly identical, so while unlikely, there's a theoretical
possibility some implementation scenario not covered by our regression
tests might run into issues. Any help testing custom implementation on
coming EA builds would be greatly appreciated!
One thing we could do on the JDK end to reduce fragility somewhat in
this area is to provoke initialization of
sun.security.util.SecurityConstants before installing the first SM.
/Claes
On 2019-08-16 12:40, Peter Firmstone wrote:
Hello Alan,
Yes, we are aware of those issues.
I mean documenting that system Permission classes should be loaded
before setting a custom SecurityManager, accessing the file system is
important, so if you haven't loaded the necessary classes before setting
a custom SecurityManager, it won't be gracefull... The simplest way of
ensuring classes are loaded is by creating object instances of them.
Perhaps just a note to beware of ensuring necessary classes are loaded
and let developers figure out what they need.
The recursive calls are easy enough to deal with to avoid any stack
overflows using ThreadLocal.
inTrustedCodeRecursiveCall.set(Boolean.TRUE);
try {
delegateContext = AccessController.doPrivileged(
new PrivilegedAction(){
public AccessControlContext run() {
return new
AccessControlContext(finalExecutionContext, dc);
}
}
);
}finally {
inTrustedCodeRecursiveCall.set(Boolean.FALSE); // Must
always happen, no matter what.
}
We've only really been caught out once with some jvm bootstrap changes,
otherwise it's been rock solid.
The other thing we do is once we've got more than three
ProtectionDomains on the stack is execute the ProtectionDomain implies
checks in parallel. Really helps when you're making a lot of network
calls.
Cheers,
Peter.
On 16/08/2019 5:04 PM, Alan Bateman wrote:
On 15/08/2019 23:20, Peter Firmstone wrote:
:
The following code is included in the constructor of our
SecurityManager implementation, I suspect we may need to add some
classes to this list, perhaps this is something that needs documenting?
The checkPermission method of custom security manager can run
arbitrary code so recursive initialization, stack overflow, bootstrap
method errors, ... are always hazards. I don't know what documentation
you have in mind but I don't think there is a definite list of classes
that need to be loaded/initialized before the custom security manager
is set because it will come down to the code that it executes in its
checkPermission method.
-Alan.
Re: RFR: 8229773: Resolve permissions for code source URLs lazily
Hello Alan,
Yes, we are aware of those issues.
I mean documenting that system Permission classes should be loaded
before setting a custom SecurityManager, accessing the file system is
important, so if you haven't loaded the necessary classes before setting
a custom SecurityManager, it won't be gracefull... The simplest way of
ensuring classes are loaded is by creating object instances of them.
Perhaps just a note to beware of ensuring necessary classes are loaded
and let developers figure out what they need.
The recursive calls are easy enough to deal with to avoid any stack
overflows using ThreadLocal.
inTrustedCodeRecursiveCall.set(Boolean.TRUE);
try {
delegateContext = AccessController.doPrivileged(
new PrivilegedAction(){
public AccessControlContext run() {
return new
AccessControlContext(finalExecutionContext, dc);
}
}
);
}finally {
inTrustedCodeRecursiveCall.set(Boolean.FALSE); // Must
always happen, no matter what.
}
We've only really been caught out once with some jvm bootstrap changes,
otherwise it's been rock solid.
The other thing we do is once we've got more than three
ProtectionDomains on the stack is execute the ProtectionDomain implies
checks in parallel. Really helps when you're making a lot of network calls.
Cheers,
Peter.
On 16/08/2019 5:04 PM, Alan Bateman wrote:
On 15/08/2019 23:20, Peter Firmstone wrote:
:
The following code is included in the constructor of our
SecurityManager implementation, I suspect we may need to add some
classes to this list, perhaps this is something that needs documenting?
The checkPermission method of custom security manager can run
arbitrary code so recursive initialization, stack overflow, bootstrap
method errors, ... are always hazards. I don't know what documentation
you have in mind but I don't think there is a definite list of classes
that need to be loaded/initialized before the custom security manager
is set because it will come down to the code that it executes in its
checkPermission method.
-Alan.
Re: RFR: 8229773: Resolve permissions for code source URLs lazily
On 15/08/2019 23:20, Peter Firmstone wrote: : The following code is included in the constructor of our SecurityManager implementation, I suspect we may need to add some classes to this list, perhaps this is something that needs documenting? The checkPermission method of custom security manager can run arbitrary code so recursive initialization, stack overflow, bootstrap method errors, ... are always hazards. I don't know what documentation you have in mind but I don't think there is a definite list of classes that need to be loaded/initialized before the custom security manager is set because it will come down to the code that it executes in its checkPermission method. -Alan.
Re: RFR: 8229773: Resolve permissions for code source URLs lazily
Hello Alan, This is related to URL and CodeSource and might be worth making a note of for future reference. Our software uses delayed dynamically assigned permissions via a policy provider, but for privileged domains that have AllPermission we make sure to assign this up front (We also utilise an RFC3986URIClassLoader and override CodeSource, so we're using our RFC3986 compliant URI instead of URL). The former because we dynamically download CodeSource's and there's no way of predicting up front which will be downloaded and the latter as a performance optimisation of ProtectionDomain. So we have a RFC3986 URI implementation, similar to Java's URI, it is not Serializable for security reasons. In addition to RFC3896 normalization, we have also recently added the ability to normalize IPv6 address conformant to "RFC 5952 A Recommendation for IPv6 Address Text Representation." The class also normalizes file system paths in a platform dependant manner, eg Upper Case for MS Windows, but not Unix. We have a URI::implies method that is similar to CodeSource::implies, with matching rules. We do this to avoid DNS calls or accessing the file system unnecessarily. Also, to avoid synchronization / locking overhead of PermissionCollection's and Permissions, we have a Policy provider that generates a thread confined Permissions and PermissionCollection instances on demand, allowing them to be garbage collected as soon as the implies call returns (Permission objects are initialized up front and effectively immutable and cached) a PermissionComparator also arranges the Permissions in an order that improves performance wen creating PermissionCollection instances. Our Security overhead is less than 1% as a result and the delays and blocking we had due to DNS calls have been eliminated. Regards, Peter. On 15/08/2019 8:56 PM, Alan Bateman wrote: On 15/08/2019 11:03, Claes Redestad wrote: Hi, by resolving permissions for code source URLs lazily, we can reduce early class loading during bootstrap, which improves footprint, startup and reduces the typical bootstrap dependency graph. Bug:https://bugs.openjdk.java.net/browse/JDK-8229773 Webrev: http://cr.openjdk.java.net/~redestad/8229773/webrev.00/ : I think the approach is okay as URL::openConnection doesn't actually open the connection to the resource and the creation of the URLStreamdHandler shouldn't depend on permissions granted to the caller. If a handler needs permissions when creating the URLConnection then it should do so in a privileged block. I think it would be a bit cleaner if the supporting class would lazily add the permissions for a CodeSource to the collection. That is, create it with a permissions collection and code source rather than a URL to match SecureClassLoader::getPermissions. You could potentially use it in URLClassLoader getPermission(CodeSource) method too. In System.setSecurityManager then you might need DefaultFileSystemProvider.theFileSystem() to ensure that the default file system is fully initialized before setting the SM. A minor nit this adds a spurious import BuiltinClassLoader. Also it can import the sun.security.uti class to be consistent with the existing code. -Alan
Re: RFR: 8229773: Resolve permissions for code source URLs lazily
Hello Claes,
The following code is included in the constructor of our SecurityManager
implementation, I suspect we may need to add some classes to this list,
perhaps this is something that needs documenting?
Regards,
Peter.
/* The following ensures the classes we need are loaded early to avoid
* class loading deadlock during permission checks */
try {
checkPermission(new RuntimePermission("setIO"),
SMPrivilegedContext);
} catch (ExceptionInInitializerError er){
Error e = new ExceptionInInitializerError(
"All domains on stack when starting a security manager must
have AllPermission");
e.initCause(er);
throw e;
}
constructed = Security.class != null;
On 15/08/2019 8:03 PM, Claes Redestad wrote:
Hi,
by resolving permissions for code source URLs lazily, we can reduce
early class loading during bootstrap, which improves footprint, startup
and reduces the typical bootstrap dependency graph.
Bug:https://bugs.openjdk.java.net/browse/JDK-8229773
Webrev: http://cr.openjdk.java.net/~redestad/8229773/webrev.00/
A note on the System.java changes:
The laziness makes it so that FilePermission isn't initialized
eagerly, which has an implicit bootstrap dependency on that the default
file system provider has been initialized before a SecurityManager has
been installed (since initializing FilePermission will otherwise do
recursive calls into FilePermission). We already force load of the image
reader on SecurityManager due similar bootstrap issues, which
transitively loads the DefaultFileSystemProvider.instance(), but
explicitly adding the dependency on the file system provider to
System::setSecurityManager seems prudent: it's effectively a no-op on
jdk/jdk right now, but documents the dependency and safeguards against
future implementation changes to image reader subsystem.
A note on the HelloClasslist changes:
The patch drops a number of classes from being loaded on typical
bootstrap and small apps, including the HelloClasslist tool. Since the
HelloClasslist establishes what's to be included in the default CDS
archive this can lead to a small regression on apps which actually do
use the (nio) file system provider, so explicitly adding it in avoids
tiny regression on those while not diminishing the speed-up to other
apps.
Testing: tier1-3
Thanks!
/Claes
Re: RFR: 8229773: Resolve permissions for code source URLs lazily
On 15/08/2019 16:22, Claes Redestad wrote: (adding back core-libs-dev) Hi Roger, seems easy enough to add a writeReplace: http://cr.openjdk.java.net/~redestad/8229773/webrev.02 This mostly looks good. In LazyCodeSourcePermissionCollection it think "initialize" should be renamed to "ensureAdded" as that more accurately describes what it does. Also the class description could do with a bit of word smiting to make it clear that permissions for a CodeSource are lazily added to the underlying permission collection. -Alan
Re: RFR: 8229773: Resolve permissions for code source URLs lazily
Thanks Claes, Looks good to me too. best regards, -- daniel On 15/08/2019 16:27, Roger Riggs wrote: Looks good, Thanks, Roger On 8/15/19 11:22 AM, Claes Redestad wrote: (adding back core-libs-dev) Hi Roger, seems easy enough to add a writeReplace: http://cr.openjdk.java.net/~redestad/8229773/webrev.02 /Claes
Re: RFR: 8229773: Resolve permissions for code source URLs lazily
Looks good, Thanks, Roger On 8/15/19 11:22 AM, Claes Redestad wrote: (adding back core-libs-dev) Hi Roger, seems easy enough to add a writeReplace: http://cr.openjdk.java.net/~redestad/8229773/webrev.02 /Claes On 2019-08-15 16:54, Roger Riggs wrote: Hi Claes, I would recommend using writeReplace to serialize the PermissionCollection so the serialized form does not change. Though these are unlikely to be serialized, it will be less likely to trigger some interoperability issue between different version. It may need to be documented that serializing/deserializing does not retain the lazyness. Roger On 8/15/19 10:30 AM, Claes Redestad wrote: Hi Sean, On 2019-08-15 15:07, Sean Mullan wrote: Hi Claes, I already reviewed an earlier version of this and this is pretty similar. I did have a question about whether the default serialization was ok - did you look into that more? ah, yes.. all the constituents are serializable (whether we wrap the CodeSource or its URL) so the new Lazy..Collection should be too. I was entertaining the idea that we could writeReplace the lazy collection with a PermissionCollection to avoid polluting serialization use cases with new type, but couldn't think of any real upside to that. /Clae
Re: RFR: 8229773: Resolve permissions for code source URLs lazily
(adding back core-libs-dev) Hi Roger, seems easy enough to add a writeReplace: http://cr.openjdk.java.net/~redestad/8229773/webrev.02 /Claes On 2019-08-15 16:54, Roger Riggs wrote: Hi Claes, I would recommend using writeReplace to serialize the PermissionCollection so the serialized form does not change. Though these are unlikely to be serialized, it will be less likely to trigger some interoperability issue between different version. It may need to be documented that serializing/deserializing does not retain the lazyness. Roger On 8/15/19 10:30 AM, Claes Redestad wrote: Hi Sean, On 2019-08-15 15:07, Sean Mullan wrote: Hi Claes, I already reviewed an earlier version of this and this is pretty similar. I did have a question about whether the default serialization was ok - did you look into that more? ah, yes.. all the constituents are serializable (whether we wrap the CodeSource or its URL) so the new Lazy..Collection should be too. I was entertaining the idea that we could writeReplace the lazy collection with a PermissionCollection to avoid polluting serialization use cases with new type, but couldn't think of any real upside to that. /Clae
Re: RFR: 8229773: Resolve permissions for code source URLs lazily
Hi Daniel, seems prudent, especially if we are to writeReplace the underlying collection on serialization. /Claes On 2019-08-15 17:10, Daniel Fuchs wrote: Hi Claes, I wonder if initialize() should check the state of the readOnly() flag - and if that's true, call perms.setReadOnly() ? see SecureClassLoader::getProtectionDomain(..) best regards, -- daniel
Re: RFR: 8229773: Resolve permissions for code source URLs lazily
Hi Claes, I wonder if initialize() should check the state of the readOnly() flag - and if that's true, call perms.setReadOnly() ? see SecureClassLoader::getProtectionDomain(..) best regards, -- daniel On 15/08/2019 13:44, Claes Redestad wrote: Hi, On 2019-08-15 12:56, Alan Bateman wrote: On 15/08/2019 11:03, Claes Redestad wrote: Hi, by resolving permissions for code source URLs lazily, we can reduce early class loading during bootstrap, which improves footprint, startup and reduces the typical bootstrap dependency graph. Bug: https://bugs.openjdk.java.net/browse/JDK-8229773 Webrev: ... : I think the approach is okay as URL::openConnection doesn't actually open the connection to the resource and the creation of the URLStreamdHandler shouldn't depend on permissions granted to the caller. If a handler needs permissions when creating the URLConnection then it should do so in a privileged block. thanks! I checked most of the handlers and the openConnection implementations and couldn't find any path that isn't either free of permission checks or doing permission sensitive steps in a privileged block already. Many handlers are already potentially created lazily after a SM has already been installed, so I don't think we need additional tests for this. I think it would be a bit cleaner if the supporting class would lazily add the permissions for a CodeSource to the collection. That is, create it with a permissions collection and code source rather than a URL to match SecureClassLoader::getPermissions. You could potentially use it in URLClassLoader getPermission(CodeSource) method too. That'd be cool. The logic in URLClassLoader is mostly a super-set of the logic I'm hoisting from BuiltinClassLoader here, but the logic in URLClassLoader also has a security check acting on the ACC of the classloader which would be hard to move to the supporting class, and it seems that check need to happen eagerly. I'll readjust the API to wrap the CodeSource rather than the URL, but I think we should leave URLClassLoader alone for now. In System.setSecurityManager then you might need DefaultFileSystemProvider.theFileSystem() to ensure that the default file system is fully initialized before setting the SM. Right, doesn't make much of a difference since all providers currently set up their singleton file system on creation, but using theFileSystem better captures intent. A minor nit this adds a spurious import BuiltinClassLoader. Also it can import the sun.security.uti class to be consistent with the existing code. Cleaned up imports, too: http://cr.openjdk.java.net/~redestad/8229773/webrev.01/ /Claes
Re: RFR: 8229773: Resolve permissions for code source URLs lazily
Hi Sean, On 2019-08-15 15:07, Sean Mullan wrote: Hi Claes, I already reviewed an earlier version of this and this is pretty similar. I did have a question about whether the default serialization was ok - did you look into that more? ah, yes.. all the constituents are serializable (whether we wrap the CodeSource or its URL) so the new Lazy..Collection should be too. I was entertaining the idea that we could writeReplace the lazy collection with a PermissionCollection to avoid polluting serialization use cases with new type, but couldn't think of any real upside to that. /Clae
Re: RFR: 8229773: Resolve permissions for code source URLs lazily
Hi Claes, I already reviewed an earlier version of this and this is pretty similar. I did have a question about whether the default serialization was ok - did you look into that more? --Sean On 8/15/19 6:03 AM, Claes Redestad wrote: Hi, by resolving permissions for code source URLs lazily, we can reduce early class loading during bootstrap, which improves footprint, startup and reduces the typical bootstrap dependency graph. Bug: https://bugs.openjdk.java.net/browse/JDK-8229773 Webrev: http://cr.openjdk.java.net/~redestad/8229773/webrev.00/ A note on the System.java changes: The laziness makes it so that FilePermission isn't initialized eagerly, which has an implicit bootstrap dependency on that the default file system provider has been initialized before a SecurityManager has been installed (since initializing FilePermission will otherwise do recursive calls into FilePermission). We already force load of the image reader on SecurityManager due similar bootstrap issues, which transitively loads the DefaultFileSystemProvider.instance(), but explicitly adding the dependency on the file system provider to System::setSecurityManager seems prudent: it's effectively a no-op on jdk/jdk right now, but documents the dependency and safeguards against future implementation changes to image reader subsystem. A note on the HelloClasslist changes: The patch drops a number of classes from being loaded on typical bootstrap and small apps, including the HelloClasslist tool. Since the HelloClasslist establishes what's to be included in the default CDS archive this can lead to a small regression on apps which actually do use the (nio) file system provider, so explicitly adding it in avoids tiny regression on those while not diminishing the speed-up to other apps. Testing: tier1-3 Thanks! /Claes
Re: RFR: 8229773: Resolve permissions for code source URLs lazily
Hi, On 2019-08-15 12:56, Alan Bateman wrote: On 15/08/2019 11:03, Claes Redestad wrote: Hi, by resolving permissions for code source URLs lazily, we can reduce early class loading during bootstrap, which improves footprint, startup and reduces the typical bootstrap dependency graph. Bug: https://bugs.openjdk.java.net/browse/JDK-8229773 Webrev: ... : I think the approach is okay as URL::openConnection doesn't actually open the connection to the resource and the creation of the URLStreamdHandler shouldn't depend on permissions granted to the caller. If a handler needs permissions when creating the URLConnection then it should do so in a privileged block. thanks! I checked most of the handlers and the openConnection implementations and couldn't find any path that isn't either free of permission checks or doing permission sensitive steps in a privileged block already. Many handlers are already potentially created lazily after a SM has already been installed, so I don't think we need additional tests for this. I think it would be a bit cleaner if the supporting class would lazily add the permissions for a CodeSource to the collection. That is, create it with a permissions collection and code source rather than a URL to match SecureClassLoader::getPermissions. You could potentially use it in URLClassLoader getPermission(CodeSource) method too. That'd be cool. The logic in URLClassLoader is mostly a super-set of the logic I'm hoisting from BuiltinClassLoader here, but the logic in URLClassLoader also has a security check acting on the ACC of the classloader which would be hard to move to the supporting class, and it seems that check need to happen eagerly. I'll readjust the API to wrap the CodeSource rather than the URL, but I think we should leave URLClassLoader alone for now. In System.setSecurityManager then you might need DefaultFileSystemProvider.theFileSystem() to ensure that the default file system is fully initialized before setting the SM. Right, doesn't make much of a difference since all providers currently set up their singleton file system on creation, but using theFileSystem better captures intent. A minor nit this adds a spurious import BuiltinClassLoader. Also it can import the sun.security.uti class to be consistent with the existing code. Cleaned up imports, too: http://cr.openjdk.java.net/~redestad/8229773/webrev.01/ /Claes
Re: RFR: 8229773: Resolve permissions for code source URLs lazily
On 15/08/2019 11:03, Claes Redestad wrote: Hi, by resolving permissions for code source URLs lazily, we can reduce early class loading during bootstrap, which improves footprint, startup and reduces the typical bootstrap dependency graph. Bug: https://bugs.openjdk.java.net/browse/JDK-8229773 Webrev: http://cr.openjdk.java.net/~redestad/8229773/webrev.00/ : I think the approach is okay as URL::openConnection doesn't actually open the connection to the resource and the creation of the URLStreamdHandler shouldn't depend on permissions granted to the caller. If a handler needs permissions when creating the URLConnection then it should do so in a privileged block. I think it would be a bit cleaner if the supporting class would lazily add the permissions for a CodeSource to the collection. That is, create it with a permissions collection and code source rather than a URL to match SecureClassLoader::getPermissions. You could potentially use it in URLClassLoader getPermission(CodeSource) method too. In System.setSecurityManager then you might need DefaultFileSystemProvider.theFileSystem() to ensure that the default file system is fully initialized before setting the SM. A minor nit this adds a spurious import BuiltinClassLoader. Also it can import the sun.security.uti class to be consistent with the existing code. -Alan
