Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Removing SecurityManager in prep of JEP411 #7562

Closed
wants to merge 31 commits into from
Closed

Removing SecurityManager in prep of JEP411 #7562

wants to merge 31 commits into from

Conversation

joakime
Copy link
Contributor

@joakime joakime commented Feb 10, 2022

Removing the usages of deprecated (and slated for removal) methods and classes defined in JEP-411.

joakime and others added 30 commits January 21, 2022 09:36
@joakime
Initial rearrangement of Maven Structure
587c5a3 
+ Introduce new top levels
  /build/
  /jetty-core/
  /jetty-integrations/
  /jetty-ee9/
+ Ensure `mvn validate` is happy

Signed-off-by: Joakim Erdfelt <joakim.erdfelt@gmail.com>
@joakime
Update to version 12.0.0-SNAPSHOT
a938d1f 
+ Change artifactIds in /jetty-ee9/ to
  include jetty-ee9 prefix
+ Ensure 'mvn validate' is sane

Signed-off-by: Joakim Erdfelt <joakim.erdfelt@gmail.com>
@joakime
Fixing license header validation configuration
0b27781 
Signed-off-by: Joakim Erdfelt <joakim.erdfelt@gmail.com>
@joakime
Working compilation of new structure
6732e67 
Signed-off-by: Joakim Erdfelt <joakim.erdfelt@gmail.com>
@joakime
Fixing parent module names to be consistent
de3d86a 
Signed-off-by: Joakim Erdfelt <joakim.erdfelt@gmail.com>
@joakime
Update projects changed in eclipse/jetty.project@jetty-12.0.x
e2abdea
@joakime
More refactoring
58e7d49 
+ Moving /demos/ to /jetty-ee9/jetty-ee9-demos/
+ Moving more /tests/ projects to /jetty-ee9/
+ Isolating ee9 dependencies properties to /jetty-ee9/
+ Establishing jetty-ee9-bom
+ Removing IO classes that were deleted in jetty-12.0.x

Signed-off-by: Joakim Erdfelt <joakim.erdfelt@gmail.com>
@joakime
Only build with JDK17
cfed610 
Signed-off-by: Joakim Erdfelt <joakim.erdfelt@gmail.com>
@joakime
Disable javadoc module (for now)
f9c042b 
Signed-off-by: Joakim Erdfelt <joakim.erdfelt@gmail.com>
@joakime
More tweaks to maven structure
8added4 
Signed-off-by: Joakim Erdfelt <joakim.erdfelt@gmail.com>
@joakime
Fixing rewrite
79017d7 
Signed-off-by: Joakim Erdfelt <joakim.erdfelt@gmail.com>
@joakime
Disable errorprone (doesn't work with JDK 17)
2343fe7 
Signed-off-by: Joakim Erdfelt <joakim.erdfelt@gmail.com>
@joakime
Revert jetty-server changes from 12.0.x (will be placed into jetty-co…
c28d210 
…re-server instead)
@joakime
Create jetty-core-server with changes in jetty-12.0.x by @gregw and crew
fda49fd
@joakime
Moving to package org.eclipse.jetty.core.server
6833af0
@joakime
Import changes from 12.0.x
08c33ff
@joakime
Import changes from 12.0.x
13fb2fc
@joakime
Import changes from 12.0.x
34e890a
@joakime
Starting to integrate http3 changes
64b1e31 
Signed-off-by: Joakim Erdfelt <joakim.erdfelt@gmail.com>
@joakime
jetty-core now compiles
5e26f4d
@joakime
jetty-ee9 now compiles
2de02e1
@joakime
Skip maven test failures (let jenkins collect list of test failures)
07fdf44
@joakime
Found working shade plugin config for Java 17
9d3f9fd 
Signed-off-by: Joakim Erdfelt <joakim.erdfelt@gmail.com>
@joakime
Fixing test dep references
4d1774f 
Signed-off-by: Joakim Erdfelt <joakim.erdfelt@gmail.com>
@joakime
Ignore IT failures (still tracked in Test failure CI UI)
060697a
@joakime
Ignore IT failures (still tracked in Test failure CI UI)
2fdc353
@joakime
Fixing maven.version & java.version enforcer rules
da2604e
@joakime
Removing "-parent" suffix from artifactId's
e3a64b1
@joakime
Aligning maven project names
25992e7
@gregw
Updated NG with latest from gregw's jetty-12.0.x
317f33b
@joakime joakime added the Build label Feb 10, 2022
@joakime joakime added this to the 12.0.x milestone Feb 10, 2022
@joakime joakime self-assigned this Feb 10, 2022
@joakime joakime added this to In progress in Jetty 12.0.ALPHA1 via automation Feb 10, 2022
@joakime joakime mentioned this pull request Feb 11, 2022
Closed
sbordet
sbordet reviewed Feb 11, 2022
View reviewed changes
Copy link
Contributor

@sbordet sbordet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are SecurityManager direct usages that it's ok to remove; fine with that.

There are doPrivileged() calls that I think were only used to "hide" operations that required permissions (e.g. load the logging properties) and that is ok to remove; fine with that.

There are doPrivileged() calls that were done to "cut" the AccessControlContext that was captured, which otherwise was retaining ClassLoader instances; I don't think it's ok to remove those, but I may be wrong.
If it's not ok, then perhaps try reflection?

@janbartel?

@janbartel
Copy link
Contributor

@sbordet @joakime see my comment over here #6184 (comment).

The java.security classes were essential in fixing the implementation of the Thread' constructor pinning of the classloader.

@sbordet
Copy link
Contributor

sbordet commented Feb 14, 2022

@janbartel I agree, Thread pinning the ClassLoader is category 3 of my comment above and I think it should remain, so that people using Java 17 will get the right effect, and those using a Java version without SecurityManager will have a modified Thread class that does not pin the ClassLoader, hopefully.

@sbordet
Copy link
Contributor

sbordet commented Feb 14, 2022

@janbartel looking at this in more details, I am dubious about the fix for the ClassLoader pinning.

See how Executors.privilegedThreadFactory() is doing things differently from our PrivilegedThreadFactory, in particular it does not replace the AccessControlContext, which pins a ProtectionDomain which pins the ClassLoader.

@janbartel
Copy link
Contributor

@sbordet Executors.privilegedThreadFactory will pin the AccessControlContext, thus the ProtectionDomain and the ClassLoader to be the same as the calling Thread. Thus, depending on when jetty called Executors.privilegedThreadFactory, we could be just pinning the server class loader, which would be fine. However, if it was called by code that was already in a context, then it would pin the context's classloader.

Our solution executes in a privileged block, which means that the AccessControlContext is effectively not inherited, thus the ProtectionDomain and attendant ClassLoader cannot be pinned.

@joakime
Copy link
Contributor Author

joakime commented Mar 1, 2022

@janbartel the AccessController is one of the classes deprecated by JEP-411, it's role in JDK-17 is a mere shell now, it barely does anything anymore as you cannot have a SecurityManager (most of the remaining methods now do nothing because of this, compared to previous JDKs).
A future JDK will just delete/remove AccessController entirely.

@gregw gregw force-pushed the jetty-12.0.x branch 3 times, most recently from fdbae02 to 0a32147 Compare May 3, 2022 13:56
@joakime
Copy link
Contributor Author

joakime commented Jun 28, 2022

Way out of date.
Will redo if there is a desire for it.
Closing.

@joakime joakime closed this Jun 28, 2022
Jetty 12.0.ALPHA1 automation moved this from In progress to Done Jun 28, 2022
@joakime joakime deleted the jetty-12.0.x-remove-security-manager branch June 28, 2022 19:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sbordet sbordet sbordet left review comments

@gregw gregw Awaiting requested review from gregw

@janbartel janbartel Awaiting requested review from janbartel

Assignees

@joakime joakime

Labels
Build
Projects
No open projects
Jetty 12.0.ALPHA1
  
Done
Milestone
12.0.x
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@joakime @janbartel @sbordet @gregw