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.

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

Remove SecurityManager layer #7928

Draft
wants to merge 6 commits into
base: master
Choose a base branch
from
Draft

Remove SecurityManager layer #7928

wants to merge 6 commits into from

Conversation

mbien
Copy link
Member

@mbien mbien commented Oct 31, 2024

JDK 24 on the horizon we will have to find a solution for the upcoming removal of the deprecated SecurityManager mechanism.

JEP 486 is a candidate for JDK 24, PR is here openjdk/jdk#21498 (still open merged, in build 24+)

This PR is an alternative to #3386 and proposes to remove the SM layer entirely, trying to avoid additional complexity of a tracking agent.

To simulate this, I am running NB 23 with -DTopSecurityManager.disable=true -Djava.lang.Runtime.level=FINE for several weeks month now without issues, this PR goes one step further and removes the SM layer for a test build - which I tested too.

What is SM used for? Why could we likely simply remove it?

System exit blocking:

  • trusted dependencies should not exit JVMs! (OpenJDK essentially came to the same conclusion if you read the JEP)
  • there are many other ways to take down JVMs, the basic question is: is the dependency trusted or not, there is no way to simply run untrusted code today
  • JVM exit is trackable with -J-Djava.lang.Runtime.level=FINE (will log full stack trace) and also via JFR's jdk.Shutdown event
    • if something exits it can be found in the log and the issue resolved
    • there is no ambiguity between JVM hard crashes and faulty dependencies
  • problematic plugins can be removed from the portal until fixed (we have to do this anyway if something causes issues)
  • tested several scenarios I could come up with
    • e.g form designer preview windows with EXIT_ON_CLOSE
    • or debugger eval window running System.exit(0)) but nothing did actually exit NB
    • the "waiting for process to finish" window does also still work before exit
  • (other modular projects like maven do also not block System.exit even though they dynamically download and run code)

swing clipboard swap:

  • NB swapped swing's clipboard impl by hooking into JDK's AppContext which was originally used for webstart sandboxing
    • this did grab sun.awt.AppContext and write TransferHandler#SandboxClipboardKey using reflection, which was only read by Swing if a SM blocked access to the system clipboard (!)
    • with webstart and SM removed, the clipboard swap trick will likely quit working too at some point (why would they keep this impl detail?)
  • we might be able to remove an --add-opens here
  • TODO: more data needed - i tested this for over a month now and clipboard works perfectly fine so far - but I am no clipboard power user and only tested on linux - for convenient testing on windows we have to release a new launcher first Remove java.security.manager=allow flag from windows launcher. #8009

File IO tracking:

  • tracking r/w methods gives wrong picture of the environment
  • big part of IO is out of process (e.g the mvn/gradle process which might be even second level background daemons)
  • IO heavy tasks are well known (e.g code scanning) even without method level tracking
  • NVMe/SSDs are pretty good at concurrent IO today
    • scheduling is questionable on non-mechanical drives
    • most of this was written when hardware was more limited and RAM / caches were small
  • running NB without IO task scheduling works perfectly fine for me - I can't tell a difference

Tracking calls to Unsafe:

  • Unsafe is deprecated for removal, this problem will fix itself
  • switches exist, e.g: --sun-misc-unsafe-memory-access=deny
  • platform apps might be able to use JMS encapsulation to further limit reflective access

Tests:

  • some tests do use the SM to monitor certain APIs and will have to be updated at some point

-> I don't think we actually need the SM layer anymore. This gives us an opportunity to remove a layer without further complicating things with an agent requirement for everything (launcher, tests, archetypes etc).

The agent rewrites bytecode by hand(!) to avoid having to bundle ASM, this could be likely solved by shading ASM till JDK 23 and using the multi-release mechanism + classfile API for 24+ (now final) but this is all extra maintenance cost which we should try to avoid if possible.

Bytecode rewriting can cause followup issues e,g jumping out of methods during debugging sessions without seeing the reason for it in the source.

I do think the agent impl is actually pretty cool but I don't think everything what can be added, should be added if it is avoidable. (also please note that back then NB was still stuck on JDK 8)

this is just a draft - don't panic. This likely will need to be discussed on the mailing list anyway (thread).

But feel free to give the dev build it a try. (the windows launcher might still break things since it has to be build separately unfortunately)

@mbien mbien added do not merge Don't merge this PR, it is not ready or just demonstration purposes. ci:all-tests [ci] enable all tests ci:dev-build [ci] produce a dev-build zip artifact (7 days expiration, see link on workflow summary page) labels Oct 31, 2024
@mbien mbien added this to the NB25 milestone Oct 31, 2024
@lahodaj
Copy link
Contributor

lahodaj commented Oct 31, 2024

Not too much against SM removal.

A few assorted comments, just to list what I know of. None of these necessarily means we need to replace SM, but it is something we are loosing.

  • I know about 4 ways to trigger System.exit/Runtime.exit/Runtime.halt in current NetBeans: a NetBeans module, an annotation processor/javac Plugin running for an open project, a Jackpot rule and profiler query language. (I don't want to put too much details here, as they can be sensitive.)
  • for the clipboard, there's "Paste from History" (Ctrl-Shift-D), which I believe depends on the ability to listen on the clipboard, and the standard Swing clipboard is not firing events, so the history may be missing some entries. (Although I suspect there's a check and event on Window activated, so this mostly affects stuff copied inside the IDE.)
  • for I/O tracking, besides the runtime, there are quite a few tests that track I/O to verify the code is not touching "too many files".

@neilcsmith-net
Copy link
Member

neilcsmith-net commented Oct 31, 2024

Thanks @mbien Following the Slack conversation on this I've also been running NetBeans a lot with TopSecurityManager.disable=true and am mostly in favour of taking this way forward and removing a layer of complexity. Maybe call me a +0.8.

I know about 4 ways to trigger System.exit/Runtime.exit/Runtime.halt in current NetBeans ... (I don't want to put too much details here, as they can be sensitive.)

Sensitive in what way? There are numerous ways that code running in the IDE VM, or even in an external process, can cause the IDE to exit (not to mention any other havoc). I still agree with Alan Bateman's comment on the enhancement request for adding an API to intercept exit - https://bugs.openjdk.org/browse/JDK-8199704 - "There has been several APIs prototyped around this and the conclusion each time is that it probably isn't the right thing to do. If ant or some other task runner is running a task that is hostile to running embedded/in-process then it should be launched into their own process/VM."

IMO the benefits of intercepting System::exit are slight in comparison to the complexity introduced, were this the only reason for an agent ...

with webstart and SM removed, the clipboard swap trick will likely quit working too at some point

The agent approach doesn't use the clipboard swap trick to implement this AFAIK. 8114562

The clipboard aspect of this is actually the aspect that concerns me the most. At the same time, given all the clipboard issues on Windows, removing all hacks might at least prove it's not our fault! 😄

for the clipboard, there's "Paste from History" (Ctrl-Shift-D), which I believe depends on the ability to listen on the clipboard, and the standard Swing clipboard is not firing events,

With a disabled SM the paste from history is partly broken. It's possibly worth considering what the point of this feature is. If it's external and in-editor text, then that might be achievable without clipboard listening. There are of course now some events fired by the clipboard, but I assume not covering everything we want.

OTOH, with the custom clipboard disabled, certain pasting (I noticed file paths particularly) now functions better!

@neilcsmith-net
Copy link
Member

But feel free to give the dev build it a try. (the windows launcher might still break things since it has to be build separately unfortunately)

You can of course replace the launcher in the workflow dev build with the launcher built from this PR at https://github.com/apache/netbeans/actions/runs/11605258427

I wonder if we can find a way to do this automatically for dev builds that change the launcher code??? 🤔

@mbien
Copy link
Member Author

mbien commented Oct 31, 2024

I know about 4 ways to trigger System.exit/Runtime.exit/Runtime.halt in current NetBeans: a NetBeans module, an annotation processor/javac Plugin running for an open project, a Jackpot rule and profiler query language. (I don't want to put too much details here, as they can be sensitive.)

JFR will write events for both, halt and exit. the java.lang.Runtime logger unfortunately only logs exit. I asked on the JDK list a few weeks ago, there was also a proposal to log on halt too - but it was decided to not implement it there specifically due to the risk of the callback into application code (logger) which could fail for whatever reason.

While debugging, -Xlog:safepoint+stats=info could be also used to figure out the exit cause - however this isn't ideal for runtime. (JFR and the java.lang.Runtime is intended for runtime though, see note in javadoc)

IMO: an annotation processor calling halt, would be as broken as a annotation processor trying figure out your system password. Malicious or not - dependencies like that are filtered out by the java ecosystem, library supply chain reviews etc. You should not blindly use libraries you don't know - only protecting against exit/halt gives only a false sense of safety - a disappearing JVM is the smallest problem if you truly don't trust the code you execute.

for the clipboard, there's "Paste from History" (Ctrl-Shift-D), which I believe depends on the ability to listen on the clipboard, and the standard Swing clipboard is not firing events, so the history may be missing some entries. (Although I suspect there's a check and event on Window activated, so this mostly affects stuff copied inside the IDE.)

I feared the same. However, it seems to work fine with one slight change in behavior: Copy from the editor will only appear in the list after focus loss or after the next copy event (paste works correctly though), everywhere else it appears right away. So there seems to be a special case for copy from editor specifically - this might be also connected to the bug, see comment #7051 (comment)

@mbien
Copy link
Member Author

mbien commented Nov 17, 2024

update:

  • JDK merged the SM removal PR, it is integrated in 24-ea build 24
  • rebased this PR to latest master
  • enabled builds/tests on JDK 24-ea, however, zulu JDK is still at build 22 atm - needs to be restarted

@mbien
Copy link
Member Author

mbien commented Nov 29, 2024

@mbien
Copy link
Member Author

mbien commented Dec 18, 2024

temporarily merged #8037 into this PR so that we can test everything on JDK 24-ea (chicken-egg problem).

All tests green: https://github.com/apache/netbeans/actions/runs/12401186185

The only exception are the platform/masterfs tests which rely on the SM for read/write counting, those are kept on JDK 17 for now. Those tests already are very JDK version sensitive, remember having to patch them at least once f2f8d5a. Would be good to find other ways to tests this in future. Potentially via JFR IO events or other means.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ci:all-tests [ci] enable all tests ci:dev-build [ci] produce a dev-build zip artifact (7 days expiration, see link on workflow summary page) do not merge Don't merge this PR, it is not ready or just demonstration purposes.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants