8267485: Remove the dependency on SecurityManager in JceSecurityManager.java #4150

bradfordwetmore · 2021-05-22T00:20:11Z

The JceSecurityManager is currently a subclass of java.security.SecurityManager. Now that JEP 411 has been integrated, this class should be updated to no longer subclass SecurityManager.

The only reason for using SecurityManager to easily get the Class Context (call stack), but we can achieve the same effect by using the JDK 9 API java.lang.StackWalkeer. None of the other SecurityManager API are used.

I have run mach5 tier1/tier2 plus --test jck:api/java_security,jck:api/javax_crypto,jck:api/javax_net,jck:api/javax_security,jck:api/org_ietf,jck:api/javax_xml/crypto with all green.

Progress

Change must not contain extraneous whitespace
Commit message must refer to an issue
Change must be properly reviewed

Issue

JDK-8267485: Remove the dependency on SecurityManager in JceSecurityManager.java

Reviewers

Mandy Chung (@mlchung - Reviewer) ⚠️ Review applies to 65f4a2e

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/4150/head:pull/4150
$ git checkout pull/4150

Update a local copy of the PR:
$ git checkout pull/4150
$ git pull https://git.openjdk.java.net/jdk pull/4150/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 4150

View PR using the GUI difftool:
$ git pr show -t 4150

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jdk/pull/4150.diff

…er.java

bridgekeeper · 2021-05-22T00:20:53Z

👋 Welcome back wetmore! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

openjdk · 2021-05-22T00:23:14Z

@bradfordwetmore The following label will be automatically applied to this pull request:

security

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

mlbridge · 2021-06-02T17:58:45Z

Webrevs

dfuch · 2021-06-02T18:18:46Z

src/java.base/share/classes/javax/crypto/JceSecurityManager.java

+        List<StackFrame> stack = StackWalker.getInstance(
+                StackWalker.Option.RETAIN_CLASS_REFERENCE)
+                .walk((s) -> s.collect(Collectors.toList()));


StackWalker.getInstance(StackWalker.Option.RETAIN_CLASS_REFERENCE) will require a permission check.
As long as the SecurityManager is still functional, doesn't this mean that creating the StackWalker should be performed in a doPrivileged? If so maybe it should be done in a (possibly static) initializer. Or is it intentional to check that the caller (and the whole calling stack) posses the RuntimePermission("getStackWalkerWithClassReference")?

Good catch, thanks.

dfuch · 2021-06-03T08:27:14Z

src/java.base/share/classes/javax/crypto/JceSecurityManager.java

+        final List<StackFrame> stack = AccessController.doPrivileged(
+                (PrivilegedAction<List<StackFrame>>)
+                        () -> StackWalker.getInstance(
+                                Option.RETAIN_CLASS_REFERENCE)
+                                .walk((s) -> s.collect(Collectors.toList())));
+


Note: StackWalker is a stateless capability object. It's not the walk() method that requires a permission, but the creation of the StackWalker itself (hence my suggestion to create it in the constructor, or in a static initializer). If you walk the stack from within a doPrivileged call then the doPrivileged frame will appear in the returned List<StackFrame>; this may (or may not) be OK - depending on the logic that processes the stack.

You could consider simplifying:

PrivilegedAction<StackWalker> pa = () -> StackWalker.getInstance(Option.RETAIN_CLASS_REFERENCE); final List<StackFrame> stack = AccessController.doPrivileged(pa).walk(Stream::toList);

Thanks. I was going to step through this code more thoroughly today, hopefully I would have caught that.

This code is only needed in certain deployment and Cipher creation situations, so would rather not create a static CodeWalker that is not normally used.

Fair enough.

seanjmullan · 2021-06-03T14:20:37Z

src/java.base/share/classes/javax/crypto/JceSecurityManager.java

@@ -45,9 +47,8 @@
 *
 * @since 1.4
 */
-
 @SuppressWarnings("removal")


You should remove this annotation now that the dependency on SecurityManager has been removed.

Unfortunately, we are still calling AccessController, thus the annotation needs to remain.

But if you follow my suggestion you can simply apply it to this line:

@SuppressWarnings("removal") final List<StackFrame> stack = AccessController.doPrivileged(pa).walk(Stream::toList);

For the static initializer that needs updating: I could move the code out of the initializer up to the declaration, or I could create a dummy declaration and then assign to INSTANCE.

The latter is probably better so you don't have to move the code; @wangweij used this technique quite a bit for JEP 411 refactoring, see 508cec7

@seanjmullan , that's what I ended up doing. I'll have a new revision out as soon as the mach5 finishes.

In this case, I name the variable tmp. There are cases where the action is a PrivlegedAction<Void> and you still have to create a variable for the return value, and then I call it dummy.

seanjmullan

I would remove line 44 from the comments as it is no longer applicable:

Note that this security manager is never installed, only instantiated.

Also, you may want to rename this class to something other than JceSecurityManager to avoid future confusion, maybe "JcePolicyManager".

mlchung · 2021-06-03T22:43:12Z

src/java.base/share/classes/javax/crypto/JceSecurityManager.java

+                () -> StackWalker.getInstance(Option.RETAIN_CLASS_REFERENCE);
+        @SuppressWarnings("removal")
+        Optional<StackFrame> stackFrame = AccessController.doPrivileged(pa)
+                .walk((s) -> s.skip(2).findFirst());


You can use the same StackWalker instance in multiple places.

StackWalker::getCallerClass is the API to get the caller class. You want to get the caller of the subclass of Cipher in this case. So Cipher constructor will call walker.getCallerClass() and then pass it to isCallerTrusted which will take an additional caller class parameter for validation.

mlchung · 2021-06-03T22:48:45Z

src/java.base/share/classes/javax/crypto/JceSecurityManager.java

+                () -> StackWalker.getInstance(Option.RETAIN_CLASS_REFERENCE);
+        @SuppressWarnings("removal")
+        List<StackFrame> stack =
+                AccessController.doPrivileged(pa).walk(Stream::toList);


You can replace line 108-125 with something like this:

StackWalker walker = AccessController.doPrivileged(pa); Optional<URL> callerCodeBase = walker.walk(s -> { s.map(f -> JceSecurity.getCodeBase(f.getDeclaringClass())) .findFirst(); });

@mlchung Maybe there should be a .filter(cb -> cb != null) inserted before .findFirst()?

Daniel is right and it needs the filter to find the first non-null code base.

bridgekeeper · 2021-07-14T01:42:03Z

@bradfordwetmore This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

mlchung

Looks good with some minor comments.

src/java.base/share/classes/javax/crypto/JceSecurityManager.java

openjdk · 2021-07-27T03:54:24Z

@bradfordwetmore This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8267485: Remove the dependency on SecurityManager in JceSecurityManager.java

Reviewed-by: mchung

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 14 new commits pushed to the master branch:

45d277f: 8270308: Arena::Amalloc may return misaligned address on 32-bit
fde1831: 8212961: [TESTBUG] vmTestbase/nsk/stress/jni/ native code cleanup
bb508e1: 8269753: Misplaced caret in PatternSyntaxException's detail message
c3d8e92: 8190753: (zipfs): Accessing a large entry (> 2^31 bytes) leads to a negative initial size for ByteArrayOutputStream
eb6da88: Merge
b76a838: 8269150: UnicodeReader not translating \u005c\u005d to \]
7ddabbf: 8271175: runtime/jni/FindClassUtf8/FindClassUtf8.java doesn't have to be run in othervm
3c27f91: 8271222: two runtime/Monitor tests don't check exit code
049b2ad: 8015886: java/awt/Focus/DeiconifiedFrameLoosesFocus/DeiconifiedFrameLoosesFocus.java sometimes failed on ubuntu
fcc7d59: 8269342: CICrashAt=1 does not always catch first Java method
... and 4 more: https://git.openjdk.java.net/jdk/compare/efa63dc1c64db357eeb497d2e1fefd170ca22d98...master

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.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

seanjmullan · 2021-07-27T13:17:35Z

The JBS issue should have an applicable noreg label. Perhaps noreg-other with a comment about the existing tests that have been run.

bradfordwetmore · 2021-07-27T17:10:23Z

/integrate

openjdk · 2021-07-27T17:11:22Z

Going to push as commit c8af823.
Since your change was applied there have been 16 commits pushed to the master branch:

ea49691: 8270794: Avoid loading Klass* twice in TypeArrayKlass::oop_size()
fc80a6b: 8270946: X509CertImpl.getFingerprint should not return the empty String
45d277f: 8270308: Arena::Amalloc may return misaligned address on 32-bit
fde1831: 8212961: [TESTBUG] vmTestbase/nsk/stress/jni/ native code cleanup
bb508e1: 8269753: Misplaced caret in PatternSyntaxException's detail message
c3d8e92: 8190753: (zipfs): Accessing a large entry (> 2^31 bytes) leads to a negative initial size for ByteArrayOutputStream
eb6da88: Merge
b76a838: 8269150: UnicodeReader not translating \u005c\u005d to \]
7ddabbf: 8271175: runtime/jni/FindClassUtf8/FindClassUtf8.java doesn't have to be run in othervm
3c27f91: 8271222: two runtime/Monitor tests don't check exit code
... and 6 more: https://git.openjdk.java.net/jdk/compare/efa63dc1c64db357eeb497d2e1fefd170ca22d98...master

Your commit was automatically rebased without conflicts.

openjdk · 2021-07-27T17:11:37Z

@bradfordwetmore Pushed as commit c8af823.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

bradfordwetmore added 2 commits May 21, 2021 16:02

8267485: Remove the dependency on SecurityManager in JceSecurityManag…

a8ab489

…er.java

Updated copyright date.

68a78c9

openjdk bot added the security security-dev@openjdk.org label May 22, 2021

Merge branch 'master' into JDK-8267485

f0a8155

bradfordwetmore marked this pull request as ready for review June 2, 2021 17:53

openjdk bot added the rfr Pull request is ready for review label Jun 2, 2021

bradfordwetmore marked this pull request as draft June 2, 2021 18:08

openjdk bot removed the rfr Pull request is ready for review label Jun 2, 2021

dfuch reviewed Jun 2, 2021

View reviewed changes

bradfordwetmore added 5 commits June 2, 2021 14:10

Replace missing annotation

a93ad92

Merge branch 'master' into JDK-8267485

54a7561

Merge branch 'master' into JDK-8267485

c971cb0

Merge branch 'master' into JDK-8267485

5040df3

Address codereview comments

d5ac890

bradfordwetmore marked this pull request as ready for review June 2, 2021 23:29

openjdk bot added the rfr Pull request is ready for review label Jun 2, 2021

dfuch reviewed Jun 3, 2021

View reviewed changes

seanjmullan reviewed Jun 3, 2021

View reviewed changes

bradfordwetmore added 6 commits June 3, 2021 09:28

Merge branch 'master' into JDK-8267485

b2cee34

Codereview Comments #2

491defb

Reduced SuppressWarnings scope

b3ac5cf

Minor typo

aa4fa6e

Merge branch 'master' into JDK-8267485

bf1df1a

More Codereview Comments

a441778

mlchung reviewed Jun 3, 2021

View reviewed changes

Merge branch 'master' into JDK-8267485

c542d9d

bradfordwetmore added 5 commits June 21, 2021 09:26

Switching tasks

28bc7bb

Merge branch 'master' into JDK-8267485

d9e0238

Merge branch 'master' into JDK-8267485

fa90828

Merge branch 'master' into JDK-8267485

668c8f3

Merge branch 'master' into JDK-8267485

7911594

bradfordwetmore added 3 commits July 26, 2021 09:54

Merge branch 'master' into JDK-8267485

3b6ab9f

More codereview comments

2ee40c1

Additional codereview comments

65f4a2e

mlchung approved these changes Jul 27, 2021

View reviewed changes

src/java.base/share/classes/javax/crypto/JceSecurityManager.java Outdated Show resolved Hide resolved

src/java.base/share/classes/javax/crypto/JceSecurityManager.java Show resolved Hide resolved

openjdk bot added the ready Pull request is ready to be integrated label Jul 27, 2021

More codereview comments

40b0de0

openjdk bot closed this Jul 27, 2021

openjdk bot added integrated Pull request has been integrated and removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Jul 27, 2021

8267485: Remove the dependency on SecurityManager in JceSecurityManager.java #4150

8267485: Remove the dependency on SecurityManager in JceSecurityManager.java #4150

Uh oh!

Conversation

bradfordwetmore commented May 22, 2021 • edited by openjdk bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Progress

Issue

Reviewers

Reviewing

Uh oh!

bridgekeeper bot commented May 22, 2021

Uh oh!

openjdk bot commented May 22, 2021

Uh oh!

mlbridge bot commented Jun 2, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Webrevs

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dfuch Jun 3, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

seanjmullan left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bridgekeeper bot commented Jul 14, 2021

Uh oh!

mlchung left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

openjdk bot commented Jul 27, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

seanjmullan commented Jul 27, 2021

Uh oh!

bradfordwetmore commented Jul 27, 2021

Uh oh!

openjdk bot commented Jul 27, 2021

Uh oh!

openjdk bot commented Jul 27, 2021

Uh oh!

Uh oh!

bradfordwetmore commented May 22, 2021 •

edited by openjdk bot

Loading

mlbridge bot commented Jun 2, 2021 •

edited

Loading

dfuch Jun 3, 2021 •

edited

Loading

openjdk bot commented Jul 27, 2021 •

edited

Loading