8268349: Provide clear run-time warnings about Security Manager deprecation #13
Conversation
|
/label add security |
@wangweij |
@wangweij this pull request will not be integrated until the CSR request JDK-8268392 for issue JDK-8268349 has been approved. |
Webrevs
|
@@ -31,6 +31,8 @@ | |||
import jdk.test.lib.process.OutputAnalyzer; | |||
import jdk.test.lib.process.ProcessTools; | |||
|
|||
import java.io.ByteArrayOutputStream; | |||
import java.io.PrintStream; | |||
import java.security.Permission; | |||
|
|||
public class SecurityManagerWarnings { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should probably have 8268349 in the @bug line.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in new commit.
@@ -87,7 +87,5 @@ public static void main(String[] args) throws Exception { | |||
throw new Exception | |||
("Test with custom non-bootclass SecurityManager failed"); | |||
} | |||
|
|||
System.setSecurityManager(null); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why did this line need to be removed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is where the setSecurityManager
method is called again when a security manager has already been installed, and the getProtectionDomain
permission is needed. While I can add the new permission into the policy file, my understanding is that this line was just a clean-up and the test was about implementation of ProtectionDomain::toString
(esp the seeAllp
method called by it) when debug is on.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, thanks for the explanation. I think it should be ok to remove this line then.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Implementation changes/text is okay.
@@ -324,6 +326,16 @@ private static void checkIO() { | |||
private static native void setOut0(PrintStream out); | |||
private static native void setErr0(PrintStream err); | |||
|
|||
// Remember original System.err. setSecurityManager() warning goes here | |||
private static volatile @Stable PrintStream originalErrStream = null; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd probably use "initial" rather than "original" here but okay. You can drop setting it to null as that is its default value.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will do. Thanks.
A new commit. Much more exact checking of various warning messages. |
System.err.println("WARNING: java.lang.System::setSecurityManager" + | ||
" is deprecated and will be removed in a future release."); | ||
var caller = Reflection.getCallerClass(); | ||
String signature = caller.getName() + " (" + codeSource(caller) + ")"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hello Weijun,
Given that the codeSource()
method above is allowed to return null
, there could be a case where the warning message printed would be something like:
WARNING: A terminally deprecated method in java.lang.System has been called
WARNING: System::setSecurityManager has been called by foo.bar.Hello (null)
WARNING: Please consider reporting this to the maintainers of foo.bar.Hello
WARNING: System::setSecurityManager will be removed in a future release
Would that be OK or do you think the presence of "(null)" be unnecessary and confusing? Maybe in such cases that line should just say "System::setSecurityManager has been called by foo.bar.Hello"?
Another minor nit - the variable is named signature
which initially gave me an impression that it was the method signature of the caller code, but that isn't the case. Should this variable be renamed perhaps?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catches. Will fix it. Thanks.
Hello Sean, Weijung, From what I have known, the Java/JDK code has always taken extra precaution when it comes to printing out potentially sensitive details like IP addresses and paths to file, like jar files in the log messages or exception stacktraces. In fact, one of the annoying things about some of the error messages that the JarFile API throws is that it doesn't even print out the jar file name, let alone the full path of the jar file which ran into issues. At least that was the case, unless that has changed in recent times. Furthermore, as you will surely know, to print out these details there's an security property which needs to be explicitly enabled ("jdk.includeInExceptions") with the right values. Given all that, do you think that we should be printing the jar file paths in this WARNING message by default? I read the linked CSR, but I didn't see why the location of the jar or the name of the jar would be useful in this warning message. As long as the caller class (and perhaps the caller method) is printed, I think that should be enough of a summary on what's triggering this warning. |
Mailing list message from Alan Bateman on security-dev: On 18/06/2021 03:51, Jaikiran Pai wrote:
It's not hugely different to the "Illegal reflective access" warnings -Alan. |
/csr needed |
/integrate |
@wangweij This PR has not yet been marked as ready for integration. |
/csr unneeded |
/integrate |
@wangweij This change now passes all automated pre-integration checks. After integration, the commit message for the final commit will be:
You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed. At the time when this comment was updated there had been 84 new commits pushed to the
As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.
|
Going to push as commit ef4ba22.
Your commit was automatically rebased without conflicts. |
More loudly and precise warning messages when a security manager is either enabled at startup or installed at runtime.
This is new PR for the
openjdk/jdk17
repo copied from openjdk/jdk#4400. A new commit is added.Progress
Issue
Reviewers
Reviewing
Using
git
Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk17 pull/13/head:pull/13
$ git checkout pull/13
Update a local copy of the PR:
$ git checkout pull/13
$ git pull https://git.openjdk.java.net/jdk17 pull/13/head
Using Skara CLI tools
Checkout this PR locally:
$ git pr checkout 13
View PR using the GUI difftool:
$ git pr show -t 13
Using diff file
Download this PR as a diff file:
https://git.openjdk.java.net/jdk17/pull/13.diff