8327114: Attach in Linux may have wrong behaviour when pid == ns_pid (Kubernetes debug container) #19055
Conversation
…(Kubernetes debug container)
|
👋 Welcome back slovdahl! A progress list of the required criteria for merging this PR into |
|
@slovdahl This change now passes all automated pre-integration checks. ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details. After integration, the commit message for the final commit will be: You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed. At the time when this comment was updated there had been 6 new commits pushed to the
Please see this link for an up-to-date comparison between the source branch of this pull request and the As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@kevinjwalls, @jerboaa) but any other Committer may sponsor as well. ➡️ To flag this PR as ready for integration with the above commit message, type |
|
@slovdahl The following labels will be automatically applied to this pull request:
When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing lists. If you would like to change these labels, use the /label pull request command. |
openjdk
bot
added
serviceability
Webrevs
|
|
This is a first stab at fixing the regression introduced in #17628. There has been a bit of discussion in https://mail.openjdk.org/pipermail/serviceability-dev/2024-April/055317.html and https://mail.openjdk.org/pipermail/serviceability-dev/2024-May/055364.html about exactly how to solve it, but this first attempt requires quite few changes at least. |
|
Ran the following tests locally: And also manually tested it under various conditions. Basic environment information: Reproducer.java used for the target process: import java.io.IOException;
import java.net.InetSocketAddress;
import java.net.ServerSocket;
public class Reproducer {
public static void main(String[] args) throws InterruptedException, IOException {
int port;
if (args.length > 0) {
port = Integer.parseInt(args[0]);
}
else {
port = 81;
}
System.out.println("Hello, World!");
try (var server = new ServerSocket()) {
server.bind(new InetSocketAddress("localhost", port));
System.out.println("Bound to port " + port);
while (true) {
Thread.sleep(1_000L);
}
}
}
}systemd unit used for testing the fix for https://bugs.openjdk.org/browse/JDK-8226919: Fails with vanilla OpenJDK 17: Works when attaching as root with vanilla OpenJDK 17: Still works without root with a JDK built from this PR (fixed in https://bugs.openjdk.org/browse/JDK-8226919): Attaching to a JVM inside a Docker container from the host works as before with vanilla OpenJDK 17 and the JDK built from this PR (always requires root): Attaching to the same Docker container JVM from a sidecar container mounted into the same process namespace:
|
|
Mailing list message from Laurence Cable on serviceability-dev: using pid to namespace comparison is IMO inappropriate/misleading what Rgds - Larry On 5/2/24 3:22 AM, Sebastian L?vdahl wrote: |
|
Mailing list message from Laurence Cable on serviceability-dev: diff --git ?import static java.nio.charset.StandardCharsets.UTF_8; @@ -47,7 +48,21 @@ public class VirtualMachineImpl extends ???? private String findTargetProcessTmpDirectory(int pid, int ns_pid) -??????????? root = procRootDirectory + "/" + tmpdir; ???? /* |
|
Thanks for the patch @larry-cable, much appreciated! I really like this idea. I tried it out a bit locally. These cases seem to work:
Unfortunately, attaching to a JVM process running as the same user in the same PID and mount namespace but one that has elevated privileges no longer works (the case that JDK-8226919 fixed). That case ends up failing like this with the patch: I think it boils down to the same reason as why the fix for JDK-8226919 was needed in the first place - a non-root user cannot read the symlinks in FWIW, my IntelliJ also highlighted the fact that the suggested patch contains unreachable code. The I'm not sure if we can do any better than always falling back to |
@slovdahl - In that test case (target JVM process has more privileges), where is the attach file created? Does jcmd end up writing it to |
|
Mailing list message from Laurence Cable on serviceability-dev: I'll ponder ... have a good weekend! regardless I think the added check for mnt ns comparison "adds value" by Rgds - Larry On 5/3/24 9:45 AM, Sebastian L?vdahl wrote: |
Yes, the attach file ends up in
Yep, agreed. |
|
I pushed an updated attempt at this now with d3e26a0. Local testing and Still eager to see what you come up with @larry-cable. |
|
On 5/6/24 10:35 AM, Sebastian Lövdahl wrote:
I pushed an updated attempt at this now with d3e26a0
<https://urldefense.com/v3/__https://github.com/openjdk/jdk/commit/d3e26a0c444e06ba9757fd528d72d83f56cd098b__;!!ACWV5N9M2RV99hQ!M_KzQgiC8WkHTfJnvTw6zsw7l0RACDgJU3ShDs0R1dAWE_IxEinuX1_Uqom0UPt96Bl6iEqHc-yUUmz5svPHZrq9ZQ$>.
Local testing and |make test
TEST="jtreg:test/hotspot/jtreg/containers"| + |make test
TEST="jtreg:test/hotspot/jtreg/serviceability"| indicate that all the
known use-cases work.
Still eager to see what you come up with @larry-cable
<https://urldefense.com/v3/__https://github.com/larry-cable__;!!ACWV5N9M2RV99hQ!M_KzQgiC8WkHTfJnvTw6zsw7l0RACDgJU3ShDs0R1dAWE_IxEinuX1_Uqom0UPt96Bl6iEqHc-yUUmz5svNMPdGFLg$>.
|createAttachFile| could still be improved for example. And I would
also be interested in looking into writing some test for the elevated
privileges case.
—
Reply to this email directly, view it on GitHub
<https://urldefense.com/v3/__https://github.com/openjdk/jdk/pull/19055*issuecomment-2096564990__;Iw!!ACWV5N9M2RV99hQ!M_KzQgiC8WkHTfJnvTw6zsw7l0RACDgJU3ShDs0R1dAWE_IxEinuX1_Uqom0UPt96Bl6iEqHc-yUUmz5svNUwHWtZA$>,
or unsubscribe
<https://urldefense.com/v3/__https://github.com/notifications/unsubscribe-auth/ANTA67SGOTAXCKY2TO2OBDDZA65N5AVCNFSM6AAAAABHDNNTT6VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDAOJWGU3DIOJZGA__;!!ACWV5N9M2RV99hQ!M_KzQgiC8WkHTfJnvTw6zsw7l0RACDgJU3ShDs0R1dAWE_IxEinuX1_Uqom0UPt96Bl6iEqHc-yUUmz5svO2ymONGw$>.
You are receiving this because you were mentioned.Message ID:
***@***.***>
/*
diff --git
a/src/jdk.attach/linux/classes/sun/tools/attach/VirtualMachineImpl.java
b/src/jdk.attach/linux/classes/sun/tools/attach/VirtualMachineImpl.java
index 81d4fd259ed..c148dbd61b7 100644
--- a/src/jdk.attach/linux/classes/sun/tools/attach/VirtualMachineImpl.java
+++ b/src/jdk.attach/linux/classes/sun/tools/attach/VirtualMachineImpl.java
@@ -34,6 +34,7 @@
import java.nio.file.Path;
import java.nio.file.Paths;
import java.nio.file.Files;
+import java.util.Optional;
import static java.nio.charset.StandardCharsets.UTF_8;
@@ -46,8 +47,28 @@ public class VirtualMachineImpl extends
HotSpotVirtualMachine {
// location is the same for all processes, otherwise the tools
// will not be able to find all Hotspot processes.
// Any changes to this needs to be synchronized with HotSpot.
- private static final String tmpdir = "/tmp";
+ private static final Path TMPDIR = Path.of("/tmp");
+
+ private static final Path PROC = Path.of("/proc");
+ private static final Path NS_MNT = Path.of("ns/mnt");
+ private static final Path SELF = PROC.resolve("self");
+ private static final Path TMP = Path.of("tmp");
+
+ private static final Optional<Path> SELF_MNT_NS;
+
+ static {
+ Path mountns = null;
+ try {
+ mountns = Files.readSymbolicLink(SELF.resolve(NS_MNT));
+ } catch (IOException _) {
+ // do nothing
+ } finally {
+ SELF_MNT_NS = Optional.ofNullable(mountns);
+ }
+ }
+
String socket_path;
+
/**
* Attaches to the target VM
*/
@@ -235,24 +256,36 @@ private File createAttachFile(int pid, int ns_pid)
throws IOException {
}
private String findTargetProcessTmpDirectory(int pid, int ns_pid)
throws IOException {
- String root;
- if (pid != ns_pid) {
+ final var procPid = PROC.resolve(Integer.toString(pid));
+
+ Optional<Path> tgtMountNS = Optional.empty();
+
+ try {
+ tgtMountNS =
Optional.ofNullable(Files.readSymbolicLink(procPid.resolve(NS_MNT))); //
attempt to read the tgt's mnt ns id...
+ } catch (IOException _) { // if we fail to read the tgt's mnt
ns id then we either dont have access or it no longer exists!
+ if (!Files.exists(procPid)) throw new
IOException(String.format("unable to attach, %s non-existent! process:
%d terminated", procPid, pid));
+
+ // ok so if we get here we have failed to read the tgt's
mnt ns, but the tgt process still exists ... we do not have privs to
read its procfs
+ }
+
+ final boolean sameMountNS = SELF_MNT_NS.isPresent() &&
SELF_MNT_NS.equals(tgtMountNS); // will be false if we did not read the
tgt's mnt ns
+
+ if (!sameMountNS || pid != ns_pid) {
// A process may not exist in the same mount namespace as
the caller, e.g.
// if we are trying to attach to a JVM process inside a
container.
// Instead, attach relative to the target root filesystem
as exposed by
// procfs regardless of namespaces.
- String procRootDirectory = "/proc/" + pid + "/root";
- if (!Files.isReadable(Path.of(procRootDirectory))) {
- throw new IOException(
- String.format("Unable to access root directory
%s " +
- "of target process %d", procRootDirectory, pid));
+ final var procRootDirectory = procPid.resolve("root");
+
+ if (Files.isReadable(procRootDirectory))
+ return procRootDirectory.resolve(TMP).toString();
+ else if (Files.exists(procPid)) { // process is still
alive... so its a permissions issue... fallback but this may also fail
+ return TMPDIR.toString();
+ } else {
+ throw new IOException(String.format("Unable to access
root directory %s " + "of target process %d",
procRootDirectory.toString(), pid));
}
-
- root = procRootDirectory + "/" + tmpdir;
- } else {
- root = tmpdir;
- }
- return root;
+ } else
+ return TMPDIR.toString(); // we are in the same mnt ns
}
/*
…--------------ZiY5EhYyDoaC0Fy7Oxy0DC0T
Content-Type: text/html; charset=UTF-8
Content-Transfer-Encoding: 8bit
<!DOCTYPE html><html><head>
<meta http-equiv="Content-Type" content="text/html; charset=utf-8">
</head>
<body>
<br>
<br>
<div class="moz-cite-prefix">On 5/6/24 10:35 AM, Sebastian Lövdahl
wrote:<br>
</div>
<blockquote type="cite" ***@***.***">
<p dir="auto">I pushed an updated attempt at this now with <a class="commit-link" data-hovercard-type="commit" data-hovercard-url="https://github.com/openjdk/jdk/commit/d3e26a0c444e06ba9757fd528d72d83f56cd098b/hovercard" href="https://urldefense.com/v3/__https://github.com/openjdk/jdk/commit/d3e26a0c444e06ba9757fd528d72d83f56cd098b__;!!ACWV5N9M2RV99hQ!M_KzQgiC8WkHTfJnvTw6zsw7l0RACDgJU3ShDs0R1dAWE_IxEinuX1_Uqom0UPt96Bl6iEqHc-yUUmz5svPHZrq9ZQ$" moz-do-not-send="true"><tt>d3e26a0</tt></a>. Local testing and
<code class="notranslate">make test
TEST="jtreg:test/hotspot/jtreg/containers"</code> + <code class="notranslate">make test
TEST="jtreg:test/hotspot/jtreg/serviceability"</code> indicate
that all the known use-cases work.</p>
<p dir="auto">Still eager to see what you come up with <a class="user-mention notranslate" data-hovercard-type="user" data-hovercard-url="/users/larry-cable/hovercard" data-octo-click="hovercard-link-click" data-octo-dimensions="link_type:self" href="https://urldefense.com/v3/__https://github.com/larry-cable__;!!ACWV5N9M2RV99hQ!M_KzQgiC8WkHTfJnvTw6zsw7l0RACDgJU3ShDs0R1dAWE_IxEinuX1_Uqom0UPt96Bl6iEqHc-yUUmz5svNMPdGFLg$" ***@***.***</a>. <code class="notranslate">createAttachFile</code> could still be
improved for example. And I would also be interested in looking
into writing some test for the elevated privileges case.</p>
<p>—<br>
Reply to this email directly, <a href="https://urldefense.com/v3/__https://github.com/openjdk/jdk/pull/19055*issuecomment-2096564990__;Iw!!ACWV5N9M2RV99hQ!M_KzQgiC8WkHTfJnvTw6zsw7l0RACDgJU3ShDs0R1dAWE_IxEinuX1_Uqom0UPt96Bl6iEqHc-yUUmz5svNUwHWtZA$" moz-do-not-send="true">view it on GitHub</a>, or <a href="https://urldefense.com/v3/__https://github.com/notifications/unsubscribe-auth/ANTA67SGOTAXCKY2TO2OBDDZA65N5AVCNFSM6AAAAABHDNNTT6VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDAOJWGU3DIOJZGA__;!!ACWV5N9M2RV99hQ!M_KzQgiC8WkHTfJnvTw6zsw7l0RACDgJU3ShDs0R1dAWE_IxEinuX1_Uqom0UPt96Bl6iEqHc-yUUmz5svO2ymONGw$" moz-do-not-send="true">unsubscribe</a>.<br>
You are receiving this because you were mentioned.<img alt="" moz-do-not-send="true" width="1" height="1"><span>Message ID:
<span><openjdk/jdk/pull/19055/c2096564990</span><span>@</span><span>github</span><span>.</span><span>com></span></span></p>
</blockquote>
<br>
<br>
/*<br>
diff --git
a/src/jdk.attach/linux/classes/sun/tools/attach/VirtualMachineImpl.java
b/src/jdk.attach/linux/classes/sun/tools/attach/VirtualMachineImpl.java<br>
index 81d4fd259ed..c148dbd61b7 100644<br>
---
a/src/jdk.attach/linux/classes/sun/tools/attach/VirtualMachineImpl.java<br>
+++
b/src/jdk.attach/linux/classes/sun/tools/attach/VirtualMachineImpl.java<br>
@@ -34,6 +34,7 @@<br>
import java.nio.file.Path;<br>
import java.nio.file.Paths;<br>
import java.nio.file.Files;<br>
+import java.util.Optional;<br>
<br>
import static java.nio.charset.StandardCharsets.UTF_8;<br>
<br>
@@ -46,8 +47,28 @@ public class VirtualMachineImpl extends
HotSpotVirtualMachine {<br>
// location is the same for all processes, otherwise the tools<br>
// will not be able to find all Hotspot processes.<br>
// Any changes to this needs to be synchronized with HotSpot.<br>
- private static final String tmpdir = "/tmp";<br>
+ private static final Path TMPDIR = Path.of("/tmp");<br>
+<br>
+ private static final Path PROC = Path.of("/proc");<br>
+ private static final Path NS_MNT = Path.of("ns/mnt");<br>
+ private static final Path SELF = PROC.resolve("self");<br>
+ private static final Path TMP = Path.of("tmp");<br>
+<br>
+ private static final Optional<Path> SELF_MNT_NS;<br>
+<br>
+ static {<br>
+ Path mountns = null;<br>
+ try {<br>
+ mountns = Files.readSymbolicLink(SELF.resolve(NS_MNT));<br>
+ } catch (IOException _) {<br>
+ // do nothing<br>
+ } finally {<br>
+ SELF_MNT_NS = Optional.ofNullable(mountns);<br>
+ }<br>
+ }<br>
+<br>
String socket_path;<br>
+<br>
/**<br>
* Attaches to the target VM<br>
*/<br>
@@ -235,24 +256,36 @@ private File createAttachFile(int pid, int
ns_pid) throws IOException {<br>
}<br>
<br>
private String findTargetProcessTmpDirectory(int pid, int
ns_pid) throws IOException {<br>
- String root;<br>
- if (pid != ns_pid) {<br>
+ final var procPid = PROC.resolve(Integer.toString(pid));<br>
+<br>
+ Optional<Path> tgtMountNS = Optional.empty();<br>
+<br>
+ try {<br>
+ tgtMountNS =
Optional.ofNullable(Files.readSymbolicLink(procPid.resolve(NS_MNT)));
// attempt to read the tgt's mnt ns id...<br>
+ } catch (IOException _) { // if we fail to read the tgt's
mnt ns id then we either dont have access or it no longer exists!<br>
+ if (!Files.exists(procPid)) throw new
IOException(String.format("unable to attach, %s non-existent!
process: %d terminated", procPid, pid));<br>
+<br>
+ // ok so if we get here we have failed to read the
tgt's mnt ns, but the tgt process still exists ... we do not have
privs to read its procfs<br>
+ }<br>
+<br>
+ final boolean sameMountNS = SELF_MNT_NS.isPresent()
&& SELF_MNT_NS.equals(tgtMountNS); // will be false if we
did not read the tgt's mnt ns<br>
+<br>
+ if (!sameMountNS || pid != ns_pid) {<br>
// A process may not exist in the same mount namespace
as the caller, e.g.<br>
// if we are trying to attach to a JVM process inside a
container.<br>
// Instead, attach relative to the target root
filesystem as exposed by<br>
// procfs regardless of namespaces.<br>
- String procRootDirectory = "/proc/" + pid + "/root";<br>
- if (!Files.isReadable(Path.of(procRootDirectory))) {<br>
- throw new IOException(<br>
- String.format("Unable to access root
directory %s " +<br>
- "of target process %d",
procRootDirectory, pid));<br>
+ final var procRootDirectory = procPid.resolve("root");<br>
+<br>
+ if (Files.isReadable(procRootDirectory))<br>
+ return procRootDirectory.resolve(TMP).toString();<br>
+ else if (Files.exists(procPid)) { // process is still
alive... so its a permissions issue... fallback but this may also
fail<br>
+ return TMPDIR.toString();<br>
+ } else {<br>
+ throw new IOException(String.format("Unable to
access root directory %s " + "of target process %d",
procRootDirectory.toString(), pid));<br>
}<br>
-<br>
- root = procRootDirectory + "/" + tmpdir;<br>
- } else {<br>
- root = tmpdir;<br>
- }<br>
- return root;<br>
+ } else<br>
+ return TMPDIR.toString(); // we are in the same mnt ns<br>
}<br>
<br>
/*<br>
<br>
</body>
</html>
--------------ZiY5EhYyDoaC0Fy7Oxy0DC0T--
|
|
Mailing list message from Laurence Cable on serviceability-dev: just a thought ... what if the code were to recurse "up" the process once an ancestor with a readable /proc/.../root/tmp was located, that of course the recursion would stop at "1" ... thoughts? - Larry On 5/6/24 10:37 AM, Sebastian L?vdahl wrote: |
|
Mailing list message from Laurence Cable on serviceability-dev: On 5/3/24 10:43 AM, jdoylei wrote:
note that the use of 'cwd' is a historical artifact, while the |
|
Mailing list message from Laurence Cable on serviceability-dev: I did some thinking on this issue over the weekend and came up with an basically, the idea is to recurse "up"the process hierarchy from the In this case, the attach code walks "up" the process hierarchy looking since the JVM does not manipulate its pid nor mnt ns'es or modify it's should the jcmd find either an ancestor in the same mnt ns (/tmp) or a this approach "increases the odds" that the jcmd will successfully needless to say this is "experimental" and needs proper stress testing Rgds - Larry -------------- next part -------------- import static java.nio.charset.StandardCharsets.UTF_8; @@ -46,13 +47,45 @@ public class VirtualMachineImpl extends HotSpotVirtualMachine { @@ -63,12 +96,12 @@ public class VirtualMachineImpl extends HotSpotVirtualMachine { // Try to resolve to the "inner most" pid namespace // Find the socket file. If not found then we attempt to start the // Return the socket file for the given process. // On Linux a simple handshake is used to start the attach mechanism - private String findTargetProcessTmpDirectory(int pid, int ns_pid) throws IOException { - root = procRootDirectory + "/" + tmpdir; /* // Return the inner most namespaced PID if there is one, |
|
Thanks for all the deep thinking you're doing here @larry-cable, appreciated. And sorry for the delay in my response, I'll try to get more time devoted to this during the coming week.
I haven't fully digested the patches you have provided yet, but one question this far. In these cases, is it not a requirement that One thing I would like to eventually achieve is to have JDK-8226919 and this fix backported to the current LTS releases. Would it make more sense to fix JDK-8327114 with as few changes as possible, and use that for backports, and do more extensive improvements in a separate follow-up that don't need backporting? |
Or could it work in case the container is also run as a non- |
|
I tested your patch @larry-cable, and, as far as I can tell, everything works. I ran through all the cases in #19055 (comment) once more to verify this. Thanks! Feel free to take a look. |
|
Hi Sebastian!
On 5/21/24 9:50 AM, Sebastian Lövdahl wrote:
In these cases, is it not a requirement that jcmd is run as root?
So even if the target process is run with elevated privileges,
attaching would always work.
the constraint (as I understand it) is based upon the filesystem access
to /proc/<attachee>/root/tmp, where the createAttachFile fails... if the
"attacher" (jcmd) has access, if it has the
appropriate +og r/w access then it will be successful.
the "root" requirement comes from the default behavior of the container
mgmt (docker) running containers as 'root'.
if you employ the --user option to 'force' the container to adopt a
non-root identity jcmd will succeed if issued from the same
user&group... because it has r/w access to the /proc/<attachee>/root/tmp
note: if the container is in a distinct uid ns (from the attacher) I
think the current checks performed by
*os::Posix::matches_effective_uid_and_gid_or_root* will complete since
the comparison is based on the uid values returned by the O.S
(independent of the fact that the uid's may exist in distinct uid ns'es!)
Or is there some way to attach from host to container with a
non-root user that I'm missing?
Or could it work in case the container is also run as a non-|root| user?
the use case I was addressing with my proposal is when the jcmd
"container" (as a sidecar) is in the same pid ns as the target container
but in a different mnt ns (I believe this is the regression use case) in
that case falling back
to /tmp will not work and since the attacher and the attachee do not
share a fs...
if the target JVM has elevated privs a (sidecar) attacher cannot use the
target's /proc/<attachee>/root/... hence my experiment to recurse "up"
the attachee's pid ns to look for a an un-privileged ancestor, which does
share the same mnt ns as the attachee, so the attacher can use the
/proc/<ancestor>/root/tmp to attach to the target... all things being
equal...
Rgds
- Larry
… —
Reply to this email directly, view it on GitHub
<https://urldefense.com/v3/__https://github.com/openjdk/jdk/pull/19055*issuecomment-2123042958__;Iw!!ACWV5N9M2RV99hQ!NbYaR1TB-qDG-x11SE_XRwyQgFwVKEPiL9gmrrFqxio1p2TAsJmsyBKhvICqMROIcMGpC8U7jfjenhr5WbKzJlBhjQ$>,
or unsubscribe
<https://urldefense.com/v3/__https://github.com/notifications/unsubscribe-auth/ANTA67XPE34N46VDFUEMSHDZDN3NHAVCNFSM6AAAAABHDNNTT6VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCMRTGA2DEOJVHA__;!!ACWV5N9M2RV99hQ!NbYaR1TB-qDG-x11SE_XRwyQgFwVKEPiL9gmrrFqxio1p2TAsJmsyBKhvICqMROIcMGpC8U7jfjenhr5WbLIEsaUnQ$>.
You are receiving this because you were mentioned.Message ID:
***@***.***>
--------------XQdcZo9hO6wbcp2fGsjP1B9A
Content-Type: text/html; charset=UTF-8
Content-Transfer-Encoding: 8bit
<!DOCTYPE html><html><head>
<meta http-equiv="Content-Type" content="text/html; charset=utf-8">
</head>
<body>
Hi Sebastian!<br>
<br>
<div class="moz-cite-prefix">On 5/21/24 9:50 AM, Sebastian Lövdahl
wrote:<br>
</div>
<blockquote type="cite" ***@***.***">
<blockquote>
<p dir="auto">In these cases, is it not a requirement that jcmd
is run as root? So even if the target process is run with
elevated privileges, attaching would always work. </p>
</blockquote>
</blockquote>
<br>
the constraint (as I understand it) is based upon the filesystem
access to /proc/<attachee>/root/tmp, where the
createAttachFile fails... if the "attacher" (jcmd) has access, if it
has the<br>
appropriate +og r/w access then it will be successful.<br>
<br>
the "root" requirement comes from the default behavior of the
container mgmt (docker) running containers as 'root'.<br>
<br>
if you employ the --user option to 'force' the container to adopt a
non-root identity jcmd will succeed if issued from the same
user&group... because it has r/w access to the
/proc/<attachee>/root/tmp<br>
<br>
note: if the container is in a distinct uid ns (from the attacher) I
think the current checks performed by <font face="Courier New, Courier, monospace"><b>os::Posix::matches_effective_uid_and_gid_or_root</b></font>
will complete since the comparison is based on the uid values
returned by the O.S (independent of the fact that the uid's may
exist in distinct uid ns'es!)<br>
<br>
<blockquote type="cite" ***@***.***">
<blockquote>
<p dir="auto">Or is there some way to attach from host to
container with a non-root user that I'm missing?</p>
</blockquote>
<p dir="auto">Or could it work in case the container is also run
as a non-<code class="notranslate">root</code> user?</p>
</blockquote>
<br>
the use case I was addressing with my proposal is when the jcmd
"container" (as a sidecar) is in the same pid ns as the target
container but in a different mnt ns (I believe this is the
regression use case) in that case falling back<br>
to /tmp will not work and since the attacher and the attachee do not
share a fs...<br>
<br>
if the target JVM has elevated privs a (sidecar) attacher cannot use
the target's /proc/<attachee>/root/... hence my experiment to
recurse "up" the attachee's pid ns to look for a an un-privileged
ancestor, which does<br>
share the same mnt ns as the attachee, so the attacher can use the
/proc/<ancestor>/root/tmp to attach to the target... all
things being equal...<br>
<br>
Rgds<br>
<br>
- Larry<br>
<br>
<blockquote type="cite" ***@***.***">
<p style="font-size:small;-webkit-text-size-adjust:none;color:#666;">—<br>
Reply to this email directly, <a href="https://urldefense.com/v3/__https://github.com/openjdk/jdk/pull/19055*issuecomment-2123042958__;Iw!!ACWV5N9M2RV99hQ!NbYaR1TB-qDG-x11SE_XRwyQgFwVKEPiL9gmrrFqxio1p2TAsJmsyBKhvICqMROIcMGpC8U7jfjenhr5WbKzJlBhjQ$" moz-do-not-send="true">view it on GitHub</a>, or <a href="https://urldefense.com/v3/__https://github.com/notifications/unsubscribe-auth/ANTA67XPE34N46VDFUEMSHDZDN3NHAVCNFSM6AAAAABHDNNTT6VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCMRTGA2DEOJVHA__;!!ACWV5N9M2RV99hQ!NbYaR1TB-qDG-x11SE_XRwyQgFwVKEPiL9gmrrFqxio1p2TAsJmsyBKhvICqMROIcMGpC8U7jfjenhr5WbLIEsaUnQ$" moz-do-not-send="true">unsubscribe</a>.<br>
You are receiving this because you were mentioned.<img src="https://github.com/notifications/beacon/ANTA67Q4FA2GDVB3NAFU5I3ZDN3NHA5CNFSM6AAAAABHDNNTT6WGG33NNVSW45C7OR4XAZNMJFZXG5LFINXW23LFNZ2KUY3PNVWWK3TUL5UWJTT6RMII4.gif" alt="" moz-do-not-send="true" width="1" height="1"><span style="color: transparent; font-size: 0; display: none; visibility: hidden; overflow: hidden; opacity: 0; width: 0; height: 0; max-width: 0; max-height: 0; mso-hide: all">Message
ID: <span><openjdk/jdk/pull/19055/c2123042958</span><span>@</span><span>github</span><span>.</span><span>com></span></span></p>
<script type="application/ld+json">[
{
***@***.***": "http://schema.org",
***@***.***": "EmailMessage",
"potentialAction": {
***@***.***": "ViewAction",
"target": "#19055 (comment)",
"url": "#19055 (comment)",
"name": "View Pull Request"
},
"description": "View this Pull Request on GitHub",
"publisher": {
***@***.***": "Organization",
"name": "GitHub",
"url": "https://github.com"
}
}
]</script>
</blockquote>
<br>
</body>
</html>
--------------XQdcZo9hO6wbcp2fGsjP1B9A--
|
|
Thanks for the explanation @larry-cable, that makes sense. By chance, did you already test the Docker |
|
I set up rootless Docker in a VM by following https://docs.docker.com/engine/security/rootless. Started a container running as my user: Using the Ubuntu OpenJDK 17 package: Using mainline JDK without the changes in this PR: Using JDK built from this PR: Using a sidecar container mounted into the same PID namespace with Eclipse Temurin 17: Using a sidecar container mounted into the same PID namespace with mainline JDK (expected to fail): Using a sidecar container mounted into the same PID namespace with JDK built from this PR: Starting the target container with elevated privileges: Attaching from a sidecar container with a JDK built from this PR: |
|
/contributor add @larry-cable |
|
@slovdahl Syntax:
User names can only be used for users in the census associated with this repository. For other contributors you need to supply the full name and email address. |
|
/contributor add Larry Cable larry.cable@oracle.com |
|
@slovdahl |
|
/integrate |
|
@slovdahl This pull request has not yet been marked as ready for integration. |
|
Ah, maybe @kevinjwalls needs to approve the PR again before it can be integrated? IIRC the only change since that approval was these comment changes: #19055 (comment) |
Correct. Any changes have to be re-approved nowadays, it's a fairly recent development. |
|
Once again, thanks everyone involved in getting this fix in a great shape! /integrate |
|
/sponsor |
|
Going to push as commit 52ba728.
Your commit was automatically rebased without conflicts. |
openjdk
bot
removed
ready
|
We are seeing a number of test failures after this was integrated. Failing tests:
I will file bugs, but the permission test fails because the new code throws a SecurityException when the SecurityManager is enabled: |
What's the failure?
This is handled here: https://git.openjdk.org/jdk/pull/21269 |
|
Sorry for this. I'm happy to look into the |
|
Is this something that should have failed in GHA checks too? |
No. Only tier1 tests run in GHA. |
|
I believe we need to wrap the readlink() in an AccessController.doPrivileged() block ... something like this:
|
Progress
Issue
Reviewers
Contributors
<larry.cable@oracle.com>Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/19055/head:pull/19055$ git checkout pull/19055Update a local copy of the PR:
$ git checkout pull/19055$ git pull https://git.openjdk.org/jdk.git pull/19055/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 19055View PR using the GUI difftool:
$ git pr show -t 19055Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/19055.diff
Webrev
Link to Webrev Comment