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

[Rafael Winterhalter]

Hi Ron,

we certainly do come from different backgrounds and therefore perspectives,
I find this exchange of perspective to be the main advantage of this open
mailing list.

I do believe that I have an understanding of your angle, I tried to give my
feedback since before Java 9 introduced the module system and so long I
feel like it always helped to find a good compromise. In the end, if a
library like Mockito or the large set of agents would no longer work, this
would be significantly more impactful to the JVM ecosystem then any minor
API change that gets carefully reviewed for good reason. That's why I am
trying to preserve them. The lack of a class definition utility for Java
agents is, in my opinion, one of the last bits within "unsafe migration"
that has not been addressed. And it seems to me that this view is agreed
upon, Oracle already proposed an API to potentially tackle this issue, I
just disagree with the scope, given my extensive experience in agent
development, and try to revive the hibernated debate at the same time.

I am very much aware that this metaphoric fence between Java agents and
regular libraries pretending to be one is being built. I also think this is
necessary, it's a good step forward. And as a matter of fact, expecting
this is at the core of my argument. With the fence being completed at some
point, the Instrumentation instance would be shielded off from regular
libraries, therefore the restriction to emulate being an agent would imply
a restriction to define classes using this instance. At the same time, Java
agents could use a facility that they certainly require, I hopefully lined
out why this is needed. Today most Java agents are using Unsafe, but I
would hope that they could migrate to an official API for Java 17. This way
agents could disregard this fence and continue to function when building
the fence is completed, even if a user upgrades the JVM.

My second argument is that restricting dynamic agents would require to stub
some API of Instrumentation for some arguments already. I do not see any
(additional) harm in stubbing the defineClass API in a similar manner as it
would require stubbing retransformClasses. I think it would therefore be
best to keep this argument out of the consideration of what the best API
would be for "favored agents", where arguments in favor of my proposal were
made by not only me.

Therefore, I hope this API can be considered. Agent developers would
certainly want to migrate to stable, supported API. But as of today no such
API is offered. Since Java agents are often supplementary and need to
support unmaintained software, their baseline moves slower then that of
other Java code which is why I hoped that Java 17 could be the first
release to offer such API as it gets the LTS label attached.

Best regards, Rafael


Am Mi., 21. Apr. 2021 um 19:00 Uhr schrieb Ron Pressler <
ron.pressler at oracle.com>:

> 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
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.java.net/pipermail/serviceability-dev/attachments/20210421/130733b7/attachment-0001.htm>