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

[Alan Snyder]

On Apr 21, 2021, at 11:40 AM, Alan Bateman  wrote:
> 
> Sure, if you are using native code then you have the full power of JVM TI and 
> JNI available. Project Panama is exploring how to restrict access to native 
> code, I think too early to say how this might extend to JNI.

I looked at some of the Panama documents and saw no hint that it might be 
extended to JNI. It seems to be positioned as an (partial) alternative to JNI.

What I do see is that native code has no direct access to the JDK via Panama, 
only the ability to invoke provided upcalls, which can only access objects and 
methods that the caller has access to. That is indeed much more restricted than 
JNI.

Even though it is “too early”, can you explain why you think Panama’s 
restrictions might apply to JNI?

As a library developer, I have often made use of JNI to work around limitations 
in current and older JDKs. I would hate to lose that ability.

   Alan

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

[Rafael Winterhalter]

On Fri, 16 Apr 2021 20:30:15 GMT, Rafael Winterhalter 
 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.

Setting '-javaagent' is mainly an operations problem. Many build tools do
not allow to declare a test dependency this way as the life cycles are not
laid out for it, the internal repository location might be machine
dependent, for example, and it's difficult to point to a specific folder
and file reliably. In this case, I'd say it would be easier to specify a
parameter in the sense of '-Djdk.attach.allowAttachSelf=true' as it is a
static value. This would however only work for build tools that initiate a
new VM for running tests which is not overly attractive for simple builds
due to the overhead. Of course, Maven, Gradle and similar tools could set
this parameter by default for their own JVM, that part is likely
overcomeable but it will certainly create confusion about how to run
libraries that are omnipresent today and make the JVM ecosystem less
approachable.

What bothers me more is the tooling perspective for non-self-attached
agents. For example, Aaeron offers a Java agent that adds plenty of debug
logging to relevant lines of code. This affects method size and so forth,
with Aaeron as a high-performance tool for banking and finance which is
written very consciously with regards to the JIT, adding it directly was
not feasible. Normally this logging is therefore thrown into a VM in
retrospect, once an application starts failing and is already taken off the
load balancer. For such a post-mortem, it would be rather annoying to
realize that a JVM cannot be attached to with full capabilities if you
forgot to set some flag. And often you did of course not consider the VM
instance to fail, sometimes it takes months to get a JVM into this buggy
state. This would be fairly inconvenient to face.

Therefore, I really hope that the dynamic attach from 'outside' the VM will
survive without imposing limits and that rather the self-attachment problem
will be targeted as such. When I mention a 'jdk.test' module in the Mockito
context, I am also rather hoping to improve performance compared to
convenience. The problem with '-Djdk.attach.allowAttachSelf=true' is that
you still need to start a new VM etc. Since Java 9, running single tests
with Mockito has for example become much slower compared to Java 8. Despite
the startup performance improvements in the JVM. If one could avoid the
location-bound '-javaagent:...', but get the Instrumentation instance
directly, I think this would render a real performance improvement in
actual execution scenarios.

Am Mi., 21. Apr. 2021 um 20:42 Uhr schrieb mlbridge[bot] <
***@***.***>:

> *Mailing list message from Alan Bateman ***@***.***> on
> core-libs-dev ***@***.***>:*
>
> On 20/04/2021 21:26, Rafael Winterhalter wrote:
>
> 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.
>
> Helping testing is good but a jdk.test module that hands out the
> Instrumentation object could be problematic. Is there a reason why the
> runner for Mockito and other mocking frameworks can't specify -javaagent
> when launching? I would expect at least some mocking scenarios to
> require load time transformations (to drop the final modifier from some
> API elements for example) so important to have the transformer set
> before classes are loaded.
>
> 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
> 

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

[Andrew Dinn]

On Fri, 16 Apr 2021 20:30:15 GMT, Rafael Winterhalter 
 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.

Sorry to be late to this party. I've been wanting to read this thread for a 
while but have bene too busy up to now. I have just a few comments

I too was party to the discussions about agent capabilities and recall well the 
decision to gradually impose restrictions, the first one being to control 
dynamic agent loading. I was happy to accept that general decision and the 
specific one to limit the opportunity for an agent self-hoisting into the 
current JVM. However, a key part of the plan to move forward on restrictions 
was to provide an override switch. I'd very much like to see that retained as 
an option. I know that in some uses self-hoisting is much preferable to having 
to install the agent on the command line and I'd expect he same to be true for 
any other capabilities for which restrictions were adopted.

Although it is true -- as Ron said - -that configuring -javaagent on the 
command line is always /possible/ there are actually many scenarios for agent 
use where deployment of an agent after startup is pragmatically much more 
desirable. An obvious use is trouble shooting, where you only want an agent in 
place when something goes wrong. That turns out to be critical to solving some 
seriously difficult support cases. The interesting use cases also fall under 
testing where self-hoisting of a test agent by a test framework can result in 
an enormous simplification of test configuration. Usage of Byteman for testing 
went through the roof with this capability in place. Never underestimate the 
degree to which even the most minimal configuration complexity puts off 
Enterprise java developers when it is multiplied by the test suite size of a 
large project.

So, likewise with other restrictions on behaviour, I'm very happy to see them 
put in place for dynamically hoisted agents so long as there is still a command 
line override along the lines of the agent attach property that allows a 
dynamic agent to do all that a command line agent can.

One other thing I'd like to correct is a point made in the discussion about 
agent code residing in the system loader. It is true that the main agent class 
gets loaded by the system loader.  However, it is perfectly possible to ensure 
that all the rest of the agent code is loaded by the bootstrap loader. A Main 
class can add the agent jar to the bootstrap path and then load and use 
reflection to invoke an effective main routine on a bootstrap loaded SubMain 
class.

Byteman uses this trick on request in order to allow it to instrument bootstrap 
classes. Because all the Byteman classes except for the original Main shell 
class are loaded by the bootstrap loader Byteman can safely inject references 
to the Byteman rule engine and Byteman exception classes into bootstrap code.

-

PR: https://git.openjdk.java.net/jdk/pull/3546

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

[Alan Bateman]

On 20/04/2021 21:26, Rafael Winterhalter wrote:

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.
Helping testing is good but a jdk.test module that hands out the 
Instrumentation object could be problematic. Is there a reason why the 
runner for Mockito and other mocking frameworks can't specify -javaagent 
when launching? I would expect at least some mocking scenarios to 
require load time transformations (to drop the final modifier from some 
API elements for example) so important to have the transformer set 
before classes are loaded.



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 think it would really hard to put this genie back into a bottle. It's 
way more attractive to use that than the very agent oriented 
redefineModule and retransformClasses.





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.
Sure, if you are using native code then you have the full power of JVM 
TI and JNI available. Project Panama is exploring how to restrict access 
to native code, I think too early to say how this might extend to JNI.


-Alan

Re: 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.press...@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 

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

[Ron Pressler]

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  
> wrote:
> 
> On Fri, 16 Apr 2021 20:30:15 GMT, Rafael Winterhalter 
>  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 

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

[Rafael Winterhalter]

On Fri, 16 Apr 2021 20:30:15 GMT, Rafael Winterhalter 
 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 

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

[Alan Bateman]

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-.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.

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

[Rafael Winterhalter]

On Fri, 16 Apr 2021 20:30:15 GMT, Rafael Winterhalter 
 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

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.

Am Mo., 19. Apr. 2021 um 16:07 Uhr schrieb Peter Levart <
***@***.***>:

> an application or library can use the attach mechanism (directly or via
> the attach API in a child VM) to load an agent and leak the Instrumentation
> object. This is the genie that somehow needs to be put back in its bottle.
> One approach that I mentioned here to create is a less powerful
> Instrumentation object for untrusted agents. Trusted agents would continue
> to the full-power
>
> I hear Rafael that dynamic attach is important to support monitoring and
> instrumenting large numbers of JVMs with no preparations (i.e. without
> issueing special command-line options to enable it). As I understand,
> current attach mechanism is designed to allow a process running under the
> same UID as the JVM or under root to attach to the JVM.
>
> What if this dynamic attach mechanism was modified so that only a process
> running under root could dynamically attach to the JVM? Old behavior would
> be enabled by special command line option, so by default, dynamic attach
> would be limited to tools running under root. Rafael mentions discovery,
> monitoring and instrumenting large number of JVMs running on hosts, so if
> one such tool has to attach to different JVMs running under different UIDs,
> it has to run as root now anyway.
>
> With such default "secure" dynamic attach and when the JVM is not running
> as root (which is a recommended security practice anyway), a library in
> such JVM could not attach back to the same JVM even through spawning
> sub-processes.
>
> How to achieve such "secure" dynamic attach? One way that comes to mind is
> a modified handshake. Currently, I think at least on Linux, the tool that
> wishes to attach to the JVM searches for a special UNIX socket (
> $PWD/.java_pid, /tmp/.java_pid) and if not found, creates a
> special attach file ($PWD/.attach_pid, /tmp/.attach_pid) to
> signal the JVM to create a listening UNIX socket under mentioned special
> path, then it connects to the socket. The UNIX socket file has UID:GID set
> to effective UID:GID of the JVM process and permissions to 0600, so only a
> tool running under same UID or root can connect to such 

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

[Alan Bateman]

On 19/04/2021 15:10, Peter Levart wrote:

:
I hear Rafael that dynamic attach is important to support monitoring and 
instrumenting large numbers of JVMs with no preparations (i.e. without issueing 
special command-line options to enable it). As I understand, current attach 
mechanism is designed to allow a process running under the same UID as the JVM 
or under root to attach to the JVM.

What if this dynamic attach mechanism was modified so that only a process 
running under root could dynamically attach to the JVM? Old behavior would be 
enabled by special command line option, so by default, dynamic attach would be 
limited to tools running under root. Rafael mentions discovery, monitoring and 
instrumenting large number of JVMs running on hosts, so if one such tool has to 
attach to different JVMs running under different UIDs, it has to run as root 
now anyway.

With such default "secure" dynamic attach and when the JVM is not running as 
root (which is a recommended security practice anyway), a library in such JVM could not 
attach back to the same JVM even through spawning sub-processes.

How to achieve such "secure" dynamic attach? One way that comes to mind 
is a modified handshake. Currently, I think at least on Linux, the tool that wishes to attach to the JVM searches for a special UNIX socket (`$PWD/.java_pid`, `/tmp/.java_pid`) and if not found, creates a special attach file (`$PWD/.attach_pid`, `/tmp/.attach_pid`) to 
signal the JVM to create a listening UNIX socket under mentioned special path, then it connects to the socket. The UNIX socket file has UID:GID set to effective UID:GID of the JVM process and permissions to 0600, so only a tool running under same UID or root can connect to such socket.


In modified handshake, JVM not running as root could not create a UNIX socket file with permissions to allow only root user to connect to it, but a tool running under root could create a listening UNIX socket with permission to allow JVM to connect to it in a way that the JVM connecting to 

such socket would know that the listening process is running as root. Simply by checking the owner of the 
listening UNIX socket file. Such socket file would have to have permission 0666 in order to allow JVMs 
running under any UID to connect to it, but otherwise be "hidden". This can be achieved by the 
tool creating a special directory and a UNIX socket in this directory, say: `/tmp/.attach_dir/`, The directory UID:GID would be 0:0 and have permission 0711. This means, 
any user could connect to the socket as long as it knows the ``, but no user but root 
can list the content of the directory to discover the name of the socket file. The last piece of the puzzle
   is how to signal to the JVM about the name of the socket file. Well, 
creating a file with the content holding the name of the socket file would be OK, as long as only target JVM could read it. File permissions could 
be set such that any process running under the same UID as the JVM could read the file. This would give a rouge library a chance to connect to the 
tool and pretend to be the monitoring JVM, but it could not connect to back to the JVM though...




One initial comment is that the attach mechanism is also used for the 
troubleshooting tools (jcmd, jstack, jmap, ...). This is how tools start 
the JMX agent in the target VM, or request a stack trace or heap dump, 
to name a few. It's just the loading of JVMTI agents that would need to 
be restricted (Java agents use the JPLIS JVMTI agent).


A second comment is that the attach mechanism on Linux and macOS is 
based on Unix domain sockets and the effective uid/gid of the peer is 
available. So if you are exploring here then you should be able to just 
use that rather than changing the handshake. The Windows implementation 
injects a stub into the target VM to queue the command so there isn't 
the equivalent.


-Alan

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

[Peter Levart]

On Mon, 19 Apr 2021 09:34:56 GMT, Alan Bateman  wrote:

> an application or library can use the attach mechanism (directly or via the 
> attach API in a child VM) to load an agent and leak the Instrumentation 
> object. This is the genie that somehow needs to be put back in its bottle. 
> One approach that I mentioned here to create is a less powerful 
> Instrumentation object for untrusted agents. Trusted agents would continue to 
> the full-power

I hear Rafael that dynamic attach is important to support monitoring and 
instrumenting large numbers of JVMs with no preparations (i.e. without issueing 
special command-line options to enable it). As I understand, current attach 
mechanism is designed to allow a process running under the same UID as the JVM 
or under root to attach to the JVM.

What if this dynamic attach mechanism was modified so that only a process 
running under root could dynamically attach to the JVM? Old behavior would be 
enabled by special command line option, so by default, dynamic attach would be 
limited to tools running under root. Rafael mentions discovery, monitoring and 
instrumenting large number of JVMs running on hosts, so if one such tool has to 
attach to different JVMs running under different UIDs, it has to run as root 
now anyway.

With such default "secure" dynamic attach and when the JVM is not running as 
root (which is a recommended security practice anyway), a library in such JVM 
could not attach back to the same JVM even through spawning sub-processes.

How to achieve such "secure" dynamic attach? One way that comes to mind is a 
modified handshake. Currently, I think at least on Linux, the tool that wishes 
to attach to the JVM searches for a special UNIX socket 
(`$PWD/.java_pid`, `/tmp/.java_pid`) and if not found, creates a 
special attach file (`$PWD/.attach_pid`, `/tmp/.attach_pid`) to 
signal the JVM to create a listening UNIX socket under mentioned special path, 
then it connects to the socket. The UNIX socket file has UID:GID set to 
effective UID:GID of the JVM process and permissions to 0600, so only a tool 
running under same UID or root can connect to such socket.

In modified handshake, JVM not running as root could not create a UNIX socket 
file with permissions to allow only root user to connect to it, but a tool 
running under root could create a listening UNIX socket with permission to 
allow JVM to connect to it in a way that the JVM connecting to such socket 
would know that the listening process is running as root. Simply by checking 
the owner of the listening UNIX socket file. Such socket file would have to 
have permission 0666 in order to allow JVMs running under any UID to connect to 
it, but otherwise be "hidden". This can be achieved by the tool creating a 
special directory and a UNIX socket in this directory, say: 
`/tmp/.attach_dir/`, The directory UID:GID would be 
0:0 and have permission 0711. This means, any user could connect to the socket 
as long as it knows the ``, but no user but root can list the 
content of the directory to discover the name of the socket file. The last 
piece of the puzzle
  is how to signal to the JVM about the name of the socket file. Well, creating 
a file with the content holding the name of the socket file would be OK, as 
long as only target JVM could read it. File permissions could be set such that 
any process running under the same UID as the JVM could read the file. This 
would give a rouge library a chance to connect to the tool and pretend to be 
the monitoring JVM, but it could not connect to back to the JVM though...

WDYT?

-

PR: https://git.openjdk.java.net/jdk/pull/3546

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

[Alan Bateman]

On Mon, 19 Apr 2021 09:11:34 GMT, Peter Levart  wrote:

> I think it would be easy to limit the use of this Instrumentation method to 
> the agent code as agent classes are loaded by the bootstrap classloader. 
> Simply make the method implementation caller-sensitive and check the caller 
> is from bootstrap class loader. This way Instrumentation instance escaped to 
> application code would not give that code any ability to define arbitrary 
> classes.

The agent JAR file is added to application class path and is loaded using the 
system class loader. So almost always the defining loader will be the 
application class loader.

In general it's a hard problem to try to balance the integrity and security of 
the platform with the needs of agents that do arbitrary injection and 
instrumentation. Specifying an agent on the command line with -javaagent is the 
opt-in to trust that agent and a defineClass that allows arbitrary injection is 
plausible for that deployment. As Rafael's mentioned in one of the messages, 
there is enough power in the existing Instrumentation API to do that in a round 
about way already.

We don't have anything equivalent for agents that are loaded by tools into a 
target VM. I added the attach mechanism and the dynamic agent loading back in 
JDK 6 and regret not putting more restrictions around this. As it stands, it is 
open to mis-use in that an application or library can use the attach mechanism 
(directly or via the attach API in a child VM) to load an agent and leak the 
Instrumentation object. This is the genie that somehow needs to be put back in 
its bottle. One approach that I mentioned here to create is a less powerful 
Instrumentation object for untrusted agents. Trusted agents would continue to 
the full-power Instrumentation object.  A less powerful Instrumentation object 
would not be able to redefine java.base or other core modules and would not be 
able to retransform classes in those modules. The option on the table during 
JDK 9 was to disable dynamic loading of agents by default but that was kicked 
down the road. I don't particularly like that option and I th
 ink we can do better.

-

PR: https://git.openjdk.java.net/jdk/pull/3546

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

[Peter Levart]

On Fri, 16 Apr 2021 20:30:15 GMT, Rafael Winterhalter 
 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

> This won't work as agents are loaded by the system class loader, not the boot 
> loader. Peter Levart ***@***.***> schrieb am Mo., 19. Apr. 2021, 11:02:
> […](#)
> From: Alan Bateman JDK-8200559 is about defining auxiliary classes in the 
> same runtime package at load-time whereas I think the proposal in this PR is 
> adding an unrestricted defineClass to the Instrumentation class. I think this 
> will require a lot of discussion as there are significant issues and concerns 
> here. An unrestricted defineClass might be okay for tool/java agents when 
> started from the command line with -javaagent but only if the Instrumentation 
> object is never ever leaked to library or application code. It could 
> potentially be part of a large effort to reduce the capabilities of agents 
> loaded via the attach mechanism. More generally I think we need clearer 
> separation of the requirements of tool agents from the requirement of 
> framework/libraries that want to inject proxy and other classes at runtime. I 
> think it would be easy to limit the use of this Instrumentation method to the 
> agent code as agent classes are loaded by the bootstrap classloader. Simply 
> make the method imp
 lementation caller-sensitive and check the caller is from bootstrap class 
loader. This way Instrumentation instance escaped to application code would not 
give that code any ability to define arbitrary classes. Good enough? Peter 
Separately, the proposal in JEP 410 is to terminally deprecate 
ProtectionDomain. — You are receiving this because you were mentioned. Reply to 
this email directly, view it on GitHub <[#3546 
(comment)](https://github.com/openjdk/jdk/pull/3546#issuecomment-822302347)>, 
or unsubscribe 

 .

Ah sorry, I didn't know that. So what about the following: agent is able to 
define (or load) a class from bootstrap loader. So it would be able to 
instantiate an intermediary class, loaded by bootstrap loader which would serve 
as an intermediary to call into this limited Instrumentation API point...
Or better: add a Lookup parameter to the Instrumentation method. The 
implementation would check that the Lookup is an unrestricted Lookup from a 
class loaded by bootstrap loader. Agent would just have to take the pain of 
obtaining such Lookup instance...

-

PR: https://git.openjdk.java.net/jdk/pull/3546

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

[Peter Levart]

On Fri, 16 Apr 2021 20:30:15 GMT, Rafael Winterhalter 
 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

From: Alan Bateman
> JDK-8200559 is about defining auxiliary classes in the same runtime package 
> at load-time whereas I think the proposal in this PR is adding an 
> unrestricted defineClass to the Instrumentation class. I think this will 
> require a lot of discussion as there are significant issues and concerns 
> here. An unrestricted defineClass might be okay for tool/java agents when 
> started from the command line with -javaagent but only if the Instrumentation 
> object is never ever leaked to library or application code. It could 
> potentially be part of a large effort to reduce the capabilities of agents 
> loaded via the attach mechanism. More generally I think we need clearer 
> separation of the requirements of tool agents from the requirement of 
> framework/libraries that want to inject proxy and other classes at runtime.

I think it would be easy to limit the use of this Instrumentation method to the 
agent code as agent classes are loaded by the bootstrap classloader. Simply 
make the method implementation caller-sensitive and check the caller is from 
bootstrap class loader. This way Instrumentation instance escaped to 
application code would not give that code any ability to define arbitrary 
classes.

Good enough?

Peter

> 
> Separately, the proposal in JEP 410 is to terminally deprecate 
> ProtectionDomain.

-

PR: https://git.openjdk.java.net/jdk/pull/3546

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

[Rafael Winterhalter]

On Fri, 16 Apr 2021 20:30:15 GMT, Rafael Winterhalter 
 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

This won't work as agents are loaded by the system class loader, not the
boot loader.

Peter Levart ***@***.***> schrieb am Mo., 19. Apr. 2021,
11:02:

> From: Alan Bateman
>
> JDK-8200559 is about defining auxiliary classes in the same runtime
> package at load-time whereas I think the proposal in this PR is adding an
> unrestricted defineClass to the Instrumentation class. I think this will
> require a lot of discussion as there are significant issues and concerns
> here. An unrestricted defineClass might be okay for tool/java agents when
> started from the command line with -javaagent but only if the
> Instrumentation object is never ever leaked to library or application code.
> It could potentially be part of a large effort to reduce the capabilities
> of agents loaded via the attach mechanism. More generally I think we need
> clearer separation of the requirements of tool agents from the requirement
> of framework/libraries that want to inject proxy and other classes at
> runtime.
>
> I think it would be easy to limit the use of this Instrumentation method
> to the agent code as agent classes are loaded by the bootstrap classloader.
> Simply make the method implementation caller-sensitive and check the caller
> is from bootstrap class loader. This way Instrumentation instance escaped
> to application code would not give that code any ability to define
> arbitrary classes.
>
> Good enough?
>
> Peter
>
> Separately, the proposal in JEP 410 is to terminally deprecate
> ProtectionDomain.
>
> —
> You are receiving this because you were mentioned.
> Reply to this email directly, view it on GitHub
> , or
> unsubscribe
> 
> .
>

-

PR: https://git.openjdk.java.net/jdk/pull/3546