RFR: 8200559: Java agents doing instrumentation need a means to define auxiliary classes [v2]

Wed Apr 21 15:53:43 UTC 2021

I think you’re coming to this discussion with a very different perspective from us, which
makes understanding what is or isn’t on the table unclear, and also steers the ideas in
directions that are different from the one we’re going.

Our goal is not to maintain some status quo until such time we decide to restrict it. We’ve
started on a long process of strengthening the platform’s integrity, both to increase security
and to help with the ecosystem’s maintenance. Making some things that are possible today
less “convenient” is intentional. Locked doors are less convenient than unlocked ones, but 
sometimes you can only strive to increase convenience under the hard constraint that doors
must be locked. This is where we are: we’ve decided doors will be locked unless explicitly 
unlocked.

But this is a process. As long as there are gaps in the garden fence — Unsafe, self-attach, JNI — 
the locks can be circumvented. Rather than fixing all those loopholes at once, we’re doing it 
gradually one at a time. This does mean that a motivated developer can circumvent the locks up 
until the point the last loophole is closed, but the hope is that, knowing where we’re headed, library 
developers will gradually accept the reality of this direction (or not prepare their users for the coming
changes, keep using those gaps that are still open to them, and then surprise their users when the
last of them is closed).

Suggestions should, therefore, focus on ideas compatible with this vision. To be more constructive
and  less frustrating, the discussion should proceed under this assumption, even if it means 
accepting that some things will be less convenient than they are today.

For example, self-attach is not the only issue. Leaking powerful Instrumentation objects to libraries
circumvents encapsulation barriers without there being a key placed in the lock through the command
line. That this can be circumvented today is irrelevant, as these workarounds will be *gradually*
removed. I hope the operations people will be thrilled with the platform’s increased security and 
reduced maintenance pain when upgrading JDK versions, but either way, they should start 
preparing for the new reality sooner rather than later. Our goal, then, should be to make people’s life 
easier, but only under these constraints, that, at this point, should be taken as an axiom for the purpose 
of discussion.

— Ron

> On 20 Apr 2021, at 21:26, Rafael Winterhalter <winterhalter at openjdk.java.net> wrote:
> 
> On Fri, 16 Apr 2021 20:30:15 GMT, Rafael Winterhalter <winterhalter at openjdk.org> wrote:
> 
>>> To allow agents the definition of auxiliary classes, an API is needed to allow this. Currently, this is often achieved by using `sun.misc.Unsafe` or `jdk.internal.misc.Unsafe` ever since the `defineClass` method was removed from `sun.misc.Unsafe`.
>> 
>> Rafael Winterhalter has refreshed the contents of this pull request, and previous commits have been removed. The incremental views will show differences compared to the previous content of the PR. The pull request contains one new commit since the last revision:
>> 
>>  8200559: Java agents doing instrumentation need a means to define auxiliary classes
> 
> I fully understand your concerns about ByteBuddyAgent.install(). It is
> simply a convenience for something that can be meaningful in some contexts
> where I prefer offering a simple API. I use it mainly for two purposes:
> 
> a) For testing Java agents and integrations against Instrumentation within
> the current VM when tests are triggered by tools that do not support
> javaagents, also because builds do not bundle jars until after tests are
> executed.
> 
> b) For purposefully "hacky" test libraries like Mockito that need agent
> capabilities without this being meant to be used in production
> environments. I have earlier proposed to offer a "jdk.test" module that
> offers the  Instrumentation instance via a simple API similar to Byte
> Buddy's. The JVM would not load this module unless requested on the command
> line. Build tools like Maven's surefire or Gradle's testrunner could then
> standardize on loading this module as a convention to give access to this
> test module by default such that libraries like Mockito could continue to
> function out of the box without the libraries functioning on a standard VM
> without extra configuration. As far as I know, mainly test libraries need
> this API. This would also emphasise that Mockito and others are meant for
> testing and fewer people would abuse it for production applications. People
> would also have an explicit means of running a JVM for a production
> application or for executing a test.
> 
> As for adding the API, my thought is that if the Instrumentation API were
> to throw exceptions on some methods/arguments for dynamic agents in the
> future, for example for retransformClasses(Object.class), this breaking
> change would then simply extend to the proposed "defineClass" method. In
> this sense, the Instrumentation API already assumes full power, I find it
> not problematic to add the missing bit to this API even if it was
> restricted in the future in the same spirit as other methods of the API
> would be.
> 
> I mentioned JNI as it is a well-known approach to defining a class today,
> using a minimal native binding to an interface that directly calls down to
> JNI's:
> 
> jclass DefineClass(JNIEnv *env, const char *name, jobject loader, const
> jbyte *buf, jsize bufLen);
> 
> This interface can then simply be used to define any class just as I
> propse, even when not writing an agent or attaching. This method makes
> class definitions also already trivial for JVMTI agents compared to Java
> agents. Unless restricting JNI, the defineClass method is already a low
> hanging fruit, but at the cost of having to maintain a tiny bit of native
> code. I'd rather see this avoided and a standard API being offered to
> agents up to the time that Panama is in place and a JNI restriction is
> possibly also included. As a bonus: Once JNI is restricted, Byte Buddy's
> "install" would no longer work unless self-attachment (or JNI) is
> explicitly allowed. The emulation already requires to run native code while
> the Virtual Machine API explicitly checks for the process id of the current
> VM against the one that is targeted. With both disabled, self-attachment
> would no longer be practically be possible without needing to prune the
> capabilities of dynamic agents which is what I understand would be the
> desired effect.
> 
> From this viewpoint, I think that adding Instrumentation::defineClass
> method does no harm compared to the status quo. And on the upside, it gives
> agents an API to migrate to, avoiding the last need of using unsafe. To
> make the JVM a safe platform, binding native code would anyways need
> restriction and this would then also solve the problem of dynamic agents
> attaching from the same VM being used in libraries. This would in my eyes
> be the cleanest solution to the self-attachment problem without disturbing
> the existing landscape of dynamic agents. To run Mockito, one would then
> instead configure Maven surefire or Gradle to run the JVM with
> -Djdk.attach.allowAttachSelf=true. Ideally, some "jdk.test" module would be
> added at some point, to avoid the overhead of self-attachment, but I think
> this better fits into separate debate.
> 
> Am Di., 20. Apr. 2021 um 15:38 Uhr schrieb mlbridge[bot] <
> ***@***.***>:
> 
>> *Mailing list message from Alan Bateman ***@***.***> on
>> core-libs-dev ***@***.***>:*
>> 
>> On 19/04/2021 22:20, Rafael Winterhalter wrote:
>> 
>> :
>> At the moment, it is required for root to switch to the user that owns the
>> JVM process as the domain socket is only accessible to that user to avoid
>> that users without access to the JVM can inject themselves into a JVM. I am
>> not sure if operations teams would be thrilled to have a monitoring agent
>> required to run as root, even in these times of Kubernetes.
>> 
>> I mainly have two comments:
>> 
>> 1. The problem is the possibility of self-attach. I think this is the
>> problem to solve, a library getting agent privileges without being an
>> agent. I think this should be prevented while dynamic attach should
>> continue to be possible in today's format. It has proven to be so useful,
>> it would be a shame if the current tooling convenience would disappear from
>> the JVM. As it's my understanding, JNI is supposed to be restricted in the
>> future, in line with Panama. Without this restriction, JNI already allows
>> for random class definition, for example, which similarly to an agent
>> offers surpassing the majority of JVM restrictions. The second restriction
>> would be a control to restrict how a JVM process starts new processes. I
>> think both are reasonable restrictions for a library to face which require
>> explicit enabling. Especially with the security manager on it's way out,
>> certain capabilities should be rethought to begin with. If both are no
>> longer freely available, self-attachment is no longer possible anyways and
>> dynamic agents could retain their capabilities.
>> 
>> 2. The question of introducing an Instrumentation::defineClass method is
>> fully independent of that first question. If a dynamic agent was to be
>> restricted, the method could reject classloader/package combinations for
>> dynamically loaded agents the same way that
>> Instrumentation::retransformClasses would need to. At the same time,
>> introducing the method would allow agents to move to an official API with a
>> Java 17 baseline which will be the next long-standing base line. I fully
>> understand it needs a thorough discussion but it is a less complicated
>> problem then (1) and could therefore be decided prior to having found a
>> satisfactory solution for it.
>> 
>> I should have been clearer, it's the combination of the two that creates
>> the attractive nuisance. I don't think there are any objections to a
>> defineClass for agents specified on the command line with -javaagent.
>> However we have to be cautious about extending that capability to agents
>> that are loaded into a running VM with the attach mechanism.
>> 
>> ByteBuddy looks great for code generation and transforming classes but
>> ByteBuddyAgent makes me nervous. It looks like I can deploy
>> byte-buddy-agent-<version>.jar on my class path and invoke the public
>> static ByteBuddyAgent.install() method to get the Instrumentation object
>> for the current VM. That may be convenient for some but this is the
>> all-powerful Instrumentation object that shouldn't be leaked to library
>> or application code. Now combine this with the proposed defineClass and
>> it means that any code on the class path could inject a class into
>> java.lang or any run-time package without any agent voodoo or opt-in via
>> the command line. That would be difficult genie to re-bottle if it were
>> to get traction.
>> 
>> You mentioned restricting JNI in the future. I'm not aware of a definite
>> plan or time-frame. Project Panama is pioneering restricting access to
>> native operations as a bug or mis-use with the linker API can easily
>> crash the VM or breakage in other ways. Extending this to JNI would be a
>> logical next step but I could imagine it taking a long time and many
>> releases to get there.
>> 
>> As regards this PR then I would be happy to work with you on a revised
>> proposed that would limit it to agents specified with -javaagent. That
>> would not preclude extending the capability, maybe in a more restricted
>> form, to agents loaded into a running VM in the future.
>> 
>> -Alan.
>> 
>> —
>> You are receiving this because you were mentioned.
>> Reply to this email directly, view it on GitHub
>> <https://github.com/openjdk/jdk/pull/3546#issuecomment-823281169>, or
>> unsubscribe
>> <https://github.com/notifications/unsubscribe-auth/ABCIA4FE2B4DGBZS4QO6SM3TJV7T5ANCNFSM43BSDEGQ>
>> .
>> 
> 
> -------------
> 
> PR: https://git.openjdk.java.net/jdk/pull/3546