diff --git a/internal/agent/profiler/jvm/async_profiler.go b/internal/agent/profiler/jvm/async_profiler.go index afd4daa..9114946 100644 --- a/internal/agent/profiler/jvm/async_profiler.go +++ b/internal/agent/profiler/jvm/async_profiler.go @@ -63,6 +63,7 @@ type AsyncProfilerManager interface { linkTmpDirToTargetTmpDir(string) error copyProfilerToTmpDir() error selectProfilerLibrary(string) error + chownProfilerToTarget(string) error invoke(*job.ProfilingJob, string) (error, time.Duration) cleanUp(*job.ProfilingJob, string) } @@ -83,14 +84,25 @@ func NewAsyncProfiler(commander executil.Commander, publisher publish.Publisher) } func (j *AsyncProfiler) SetUp(job *job.ProfilingJob) error { - targetFs, err := util.ContainerFileSystem(job.ContainerRuntime, job.ContainerID, job.ContainerRuntimePath) - if err != nil { - return err + // PIDs first: everything below is staged through the target's own mount + // namespace, which we can only reach via one of its PIDs. + if stringUtils.IsNotBlank(job.PID) { + j.targetPIDs = []string{job.PID} + } else { + pids, err := util.GetCandidatePIDs(job) + if err != nil { + return err + } + log.DebugLogLn(fmt.Sprintf("The PIDs to be profiled: %s", pids)) + j.targetPIDs = pids } + + // Every PID of a container shares its mount namespace, so any of them + // resolves the same filesystem. + targetFs := util.TargetRootFS(j.targetPIDs[0]) log.DebugLogLn(fmt.Sprintf("The target filesystem is: %s", targetFs)) - err = j.removeTmpDir() - if err != nil { + if err := j.removeTmpDir(); err != nil { return err } @@ -98,27 +110,21 @@ func (j *AsyncProfiler) SetUp(job *job.ProfilingJob) error { // remove previous files from a previous profiling file.RemoveAll(targetTmpDir, config.ProfilingPrefix+string(job.OutputType)) - err = j.linkTmpDirToTargetTmpDir(targetTmpDir) - if err != nil { + if err := j.linkTmpDirToTargetTmpDir(targetTmpDir); err != nil { return err } - if stringUtils.IsNotBlank(job.PID) { - j.targetPIDs = []string{job.PID} - } else { - pids, err := util.GetCandidatePIDs(job) - if err != nil { - return err - } - log.DebugLogLn(fmt.Sprintf("The PIDs to be profiled: %s", pids)) - j.targetPIDs = pids + if err := j.copyProfilerToTmpDir(); err != nil { + return err } - if err := j.copyProfilerToTmpDir(); err != nil { + if err := j.selectProfilerLibrary(targetFs); err != nil { return err } - return j.selectProfilerLibrary(targetFs) + // The JVM dlopens the library and writes the profile itself, as whatever + // user it runs as — root-owned staging is unreadable/unwritable for it. + return j.chownProfilerToTarget(j.targetPIDs[0]) } // targetUsesMusl reports whether the target container's root filesystem is @@ -151,6 +157,23 @@ func (j *asyncProfilerManager) copyProfilerToTmpDir() error { return cmd.Run() } +// chownProfilerToTarget hands the staged directory to the user the target runs +// as. We stage as root; the JVM then has to read libasyncProfiler.so and write +// its own output file into that directory, and most hardened images do not run +// as root. +func (j *asyncProfilerManager) chownProfilerToTarget(pid string) error { + uid, gid, err := util.TargetCredentials(pid) + if err != nil { + return err + } + if uid == "0" && gid == "0" { + return nil + } + log.DebugLogLn(fmt.Sprintf("Handing the staged profiler to %s:%s", uid, gid)) + cmd := j.commander.Command("chown", "-R", uid+":"+gid, asyncProfilerDir) + return cmd.Run() +} + // selectProfilerLibrary points libasyncProfiler.so at the build matching the // target's libc. The library is dlopen'd by the target JVM rather than by us, // so a mismatch fails the attach with "libc.musl-x86_64.so.1: cannot open diff --git a/internal/agent/profiler/jvm/async_profiler_fake.go b/internal/agent/profiler/jvm/async_profiler_fake.go index 73ef552..08aa198 100644 --- a/internal/agent/profiler/jvm/async_profiler_fake.go +++ b/internal/agent/profiler/jvm/async_profiler_fake.go @@ -128,6 +128,19 @@ func (f *fakeAsyncProfilerManager) selectProfilerLibrary(s string) error { return err } +func (f *fakeAsyncProfilerManager) chownProfilerToTarget(s string) error { + var err error + f.fakeMethods["chownProfilerToTarget"].invokes++ + if f.fakeMethods["chownProfilerToTarget"].fakeReturnValues != nil && len(f.fakeMethods["chownProfilerToTarget"].fakeReturnValues) > 0 { + f.fakeMethods["chownProfilerToTarget"].indexExecution++ + arg0 := f.fakeMethods["chownProfilerToTarget"].fakeReturnValues[f.fakeMethods["chownProfilerToTarget"].indexExecution-1].([]interface{})[0] + if arg0 != nil { + err = arg0.(error) + } + } + return err +} + func (f *fakeAsyncProfilerManager) cleanUp(profilingJob *job.ProfilingJob, s string) { f.fakeMethods["cleanUp"].invokes++ if f.fakeMethods["cleanUp"].fakeReturnValues != nil && len(f.fakeMethods["cleanUp"].fakeReturnValues) > 0 { diff --git a/internal/agent/profiler/jvm/async_profiler_test.go b/internal/agent/profiler/jvm/async_profiler_test.go index 33cc3cc..215fac8 100644 --- a/internal/agent/profiler/jvm/async_profiler_test.go +++ b/internal/agent/profiler/jvm/async_profiler_test.go @@ -43,6 +43,7 @@ func TestAsyncProfiler_SetUp(t *testing.T) { asyncProfilerManager.On("linkTmpDirToTargetTmpDir").Return(nil) asyncProfilerManager.On("copyProfilerToTmpDir").Return(nil) asyncProfilerManager.On("selectProfilerLibrary").Return(nil) + asyncProfilerManager.On("chownProfilerToTarget").Return(nil) return fields{ AsyncProfiler: &AsyncProfiler{ @@ -75,6 +76,7 @@ func TestAsyncProfiler_SetUp(t *testing.T) { asyncProfilerManager.On("linkTmpDirToTargetTmpDir").Return(nil) asyncProfilerManager.On("copyProfilerToTmpDir").Return(nil) asyncProfilerManager.On("selectProfilerLibrary").Return(nil) + asyncProfilerManager.On("chownProfilerToTarget").Return(nil) return fields{ AsyncProfiler: &AsyncProfiler{ @@ -101,13 +103,14 @@ func TestAsyncProfiler_SetUp(t *testing.T) { }, }, { - name: "should fail when getting target filesystem fail", + name: "should fail when the container runtime is unknown", given: func() (fields, args) { asyncProfilerManager := newFakeAsyncProfilerManager() asyncProfilerManager.On("removeTmpDir").Return(nil) asyncProfilerManager.On("linkTmpDirToTargetTmpDir").Return(nil) asyncProfilerManager.On("copyProfilerToTmpDir").Return(nil) asyncProfilerManager.On("selectProfilerLibrary").Return(nil) + asyncProfilerManager.On("chownProfilerToTarget").Return(nil) return fields{ AsyncProfiler: &AsyncProfiler{ @@ -118,7 +121,6 @@ func TestAsyncProfiler_SetUp(t *testing.T) { Duration: 0, ContainerRuntime: "other", ContainerID: "ContainerID", - PID: "PID_ContainerID", }, } }, @@ -197,6 +199,8 @@ func TestAsyncProfiler_SetUp(t *testing.T) { asyncProfilerManager := newFakeAsyncProfilerManager() asyncProfilerManager.On("removeTmpDir").Return(nil) asyncProfilerManager.On("linkTmpDirToTargetTmpDir").Return(nil) + // PIDs are resolved first now — the target's mount namespace is + // reached through one of them, so nothing can be staged before. return fields{ AsyncProfiler: &AsyncProfiler{ @@ -215,8 +219,8 @@ func TestAsyncProfiler_SetUp(t *testing.T) { }, then: func(t *testing.T, err error, fields fields) { assert.NotNil(t, err) - assert.Equal(t, 1, fields.AsyncProfiler.AsyncProfilerManager.(FakeAsyncProfilerManager).On("removeTmpDir").InvokedTimes()) - assert.Equal(t, 1, fields.AsyncProfiler.AsyncProfilerManager.(FakeAsyncProfilerManager).On("linkTmpDirToTargetTmpDir").InvokedTimes()) + assert.Equal(t, 0, fields.AsyncProfiler.AsyncProfilerManager.(FakeAsyncProfilerManager).On("removeTmpDir").InvokedTimes()) + assert.Equal(t, 0, fields.AsyncProfiler.AsyncProfilerManager.(FakeAsyncProfilerManager).On("linkTmpDirToTargetTmpDir").InvokedTimes()) assert.Equal(t, 0, fields.AsyncProfiler.AsyncProfilerManager.(FakeAsyncProfilerManager).On("copyProfilerToTmpDir").InvokedTimes()) }, }, @@ -495,6 +499,54 @@ func Test_asyncProfilerManager_selectProfilerLibrary(t *testing.T) { }) } +// Test_asyncProfilerManager_chownProfilerToTarget — we stage as root, but the +// JVM reads the library and writes its own output into that directory as +// whatever user it runs as, which in hardened images is not root. +func Test_asyncProfilerManager_chownProfilerToTarget(t *testing.T) { + t.Run("a root target needs no chown", func(t *testing.T) { + requireProcFS(t) + if os.Getuid() != 0 { + t.Skip("this test reads our own /proc entry, so it only says root when we are") + } + commander := executil.NewFakeCommander() + commander.On("Command").Return(exec.Command("false")) + a := NewAsyncProfiler(commander, publish.NewFakePublisher()) + + assert.Nil(t, a.chownProfilerToTarget("self")) + assert.Equal(t, 0, commander.On("Command").InvokedTimes()) + }) + + t.Run("a non-root target is handed the staged directory", func(t *testing.T) { + requireProcFS(t) + if os.Getuid() == 0 { + t.Skip("running as root, so our own /proc entry cannot stand in for a non-root target") + } + commander := executil.NewFakeCommander() + commander.On("Command").Return(exec.Command("true")) + a := NewAsyncProfiler(commander, publish.NewFakePublisher()) + + assert.Nil(t, a.chownProfilerToTarget("self")) + assert.Equal(t, 1, commander.On("Command").InvokedTimes()) + }) + + t.Run("an unreadable pid is an error, not a silent skip", func(t *testing.T) { + commander := executil.NewFakeCommander() + commander.On("Command").Return(exec.Command("true")) + a := NewAsyncProfiler(commander, publish.NewFakePublisher()) + + assert.NotNil(t, a.chownProfilerToTarget("not-a-pid")) + }) +} + +// requireProcFS skips on platforms without /proc — the credentials come from +// /proc//status, which only exists on the Linux boxes this runs on. +func requireProcFS(t *testing.T) { + t.Helper() + if _, err := os.Stat("/proc/self/status"); err != nil { + t.Skip("no procfs on this platform") + } +} + func Test_asyncProfilerManager_invoke(t *testing.T) { type fields struct { AsyncProfiler *AsyncProfiler diff --git a/internal/agent/profiler/jvm/jcmd.go b/internal/agent/profiler/jvm/jcmd.go index c6f354a..2b374df 100644 --- a/internal/agent/profiler/jvm/jcmd.go +++ b/internal/agent/profiler/jvm/jcmd.go @@ -96,26 +96,8 @@ func NewJcmdProfiler(commander executil.Commander, publisher publish.Publisher) } func (j *JcmdProfiler) SetUp(job *job.ProfilingJob) error { - targetFs, err := util.ContainerFileSystem(job.ContainerRuntime, job.ContainerID, job.ContainerRuntimePath) - if err != nil { - return err - } - log.DebugLogLn(fmt.Sprintf("The target filesystem is: %s", targetFs)) - - err = j.removeTmpDir() - if err != nil { - return err - } - - targetTmpDir := filepath.Join(targetFs, "tmp") - // remove previous files from a previous profiling - file.RemoveAll(targetTmpDir, config.ProfilingPrefix+string(job.OutputType)) - - err = j.linkTmpDirToTargetTmpDir(targetTmpDir) - if err != nil { - return err - } - + // PIDs first: the tmp dir below is the target's own, reachable only + // through one of its PIDs. if stringUtils.IsNotBlank(job.PID) { j.targetPIDs = []string{job.PID} recordingPIDs = make(chan string, 1) @@ -129,6 +111,24 @@ func (j *JcmdProfiler) SetUp(job *job.ProfilingJob) error { recordingPIDs = make(chan string, len(pids)) } + // The JVM writes heap dumps and JFR recordings itself, to a path in its own + // mount namespace — so we have to agree with it on what /tmp is. Every PID + // of a container shares that namespace. + targetFs := util.TargetRootFS(j.targetPIDs[0]) + log.DebugLogLn(fmt.Sprintf("The target filesystem is: %s", targetFs)) + + if err := j.removeTmpDir(); err != nil { + return err + } + + targetTmpDir := filepath.Join(targetFs, "tmp") + // remove previous files from a previous profiling + file.RemoveAll(targetTmpDir, config.ProfilingPrefix+string(job.OutputType)) + + if err := j.linkTmpDirToTargetTmpDir(targetTmpDir); err != nil { + return err + } + return j.copyJfrSettingsToTmpDir() } diff --git a/internal/agent/profiler/jvm/jcmd_test.go b/internal/agent/profiler/jvm/jcmd_test.go index a32b2fb..8ca06b7 100644 --- a/internal/agent/profiler/jvm/jcmd_test.go +++ b/internal/agent/profiler/jvm/jcmd_test.go @@ -210,8 +210,10 @@ func TestJcmdProfiler_SetUp(t *testing.T) { }, then: func(t *testing.T, err error, fields fields) { assert.NotNil(t, err) - assert.Equal(t, 1, fields.JcmdProfiler.JcmdManager.(FakeJcmdManager).On("removeTmpDir").InvokedTimes()) - assert.Equal(t, 1, fields.JcmdProfiler.JcmdManager.(FakeJcmdManager).On("linkTmpDirToTargetTmpDir").InvokedTimes()) + // PIDs are resolved first now — the target's /tmp is reached + // through one of them, so nothing is staged before that. + assert.Equal(t, 0, fields.JcmdProfiler.JcmdManager.(FakeJcmdManager).On("removeTmpDir").InvokedTimes()) + assert.Equal(t, 0, fields.JcmdProfiler.JcmdManager.(FakeJcmdManager).On("linkTmpDirToTargetTmpDir").InvokedTimes()) assert.Equal(t, 0, fields.JcmdProfiler.JcmdManager.(FakeJcmdManager).On("copyJfrSettingsToTmpDir").InvokedTimes()) }, }, diff --git a/internal/agent/profiler/node_dummy.go b/internal/agent/profiler/node_dummy.go index b3f73c0..13efedc 100644 --- a/internal/agent/profiler/node_dummy.go +++ b/internal/agent/profiler/node_dummy.go @@ -45,10 +45,16 @@ func NewNodeDummyProfiler(publisher publish.Publisher) *NodeDummyProfiler { } func (n *NodeDummyProfiler) SetUp(job *job.ProfilingJob) error { - targetFs, err := util.ContainerFileSystem(job.ContainerRuntime, job.ContainerID, job.ContainerRuntimePath) + rootPID, err := util.GetRootPID(job) if err != nil { return err } + + // The heapsnapshot is written by the Node process into its working + // directory, so we have to read it back from the target's own mount + // namespace — the runtime's overlay path misses it whenever that + // directory is a volume. + targetFs := util.TargetRootFS(rootPID) log.DebugLogLn(fmt.Sprintf("The target filesystem is: %s", targetFs)) cwd, err := util.GetCWD(job) diff --git a/internal/agent/profiler/node_dummy_test.go b/internal/agent/profiler/node_dummy_test.go index 2239e8a..fc0ca42 100644 --- a/internal/agent/profiler/node_dummy_test.go +++ b/internal/agent/profiler/node_dummy_test.go @@ -53,11 +53,13 @@ func TestNodeDummyProfiler_SetUp(t *testing.T) { }, then: func(t *testing.T, err error, fields fields) { assert.Nil(t, err) - assert.Equal(t, "/root/fs/ContainerID/cwd", fields.NodeDummyProfiler.cwd) + // The heapsnapshot is read back through the target's own mount + // namespace, not the runtime's overlay directory. + assert.Equal(t, "/proc/PID_ContainerID/root/cwd", fields.NodeDummyProfiler.cwd) }, }, { - name: "should fail when get root file system fail", + name: "should fail when the container PID is not found", given: func() (fields, args) { return fields{ NodeDummyProfiler: &NodeDummyProfiler{ @@ -66,7 +68,7 @@ func TestNodeDummyProfiler_SetUp(t *testing.T) { }, args{ job: &job.ProfilingJob{ Duration: 0, - ContainerRuntime: api.FakeContainerWithRootFileSystemLocationResultError, + ContainerRuntime: api.FakeContainerWithPIDResultError, ContainerID: "ContainerID", }, } diff --git a/internal/agent/util/container.go b/internal/agent/util/container.go index 3b36ecc..926a727 100644 --- a/internal/agent/util/container.go +++ b/internal/agent/util/container.go @@ -3,6 +3,8 @@ package util import ( "bytes" "fmt" + "os" + "path/filepath" "regexp" "strings" "time" @@ -60,7 +62,55 @@ var runtime = func(r api.ContainerRuntime) (Container, error) { var commander = exec.NewCommander() -// ContainerFileSystem returns the root path of the container filesystem +// TargetRootFS returns the path through which we can reach the target's +// filesystem *as the target itself sees it*: /proc//root resolves the +// process's whole mount namespace, volumes included. +// +// Prefer this over ContainerFileSystem for anything the target has to read or +// write. The two agree only when nothing is mounted over the container's root; +// a pod with readOnlyRootFilesystem: true and an emptyDir at /tmp is the common +// counter-example, and there a file written through the runtime's overlay +// directory never becomes visible inside the container. +func TargetRootFS(pid string) string { + return filepath.Join("/proc", pid, "root") +} + +// TargetCredentials returns the uid and gid the target process runs as, from +// /proc//status. Files we stage for it to read — and the directory it +// writes its own output into — have to belong to it: the debugger pod is root, +// the target frequently is not. +func TargetCredentials(pid string) (uid string, gid string, err error) { + status, err := os.ReadFile(filepath.Join("/proc", pid, "status")) //nolint:gosec // pid comes from the container runtime + if err != nil { + return "", "", err + } + for _, line := range strings.Split(string(status), "\n") { + // "Uid:\t\t\t\t" — the real id is the one + // the process runs as, which is what file ownership has to match. + fields := strings.Fields(line) + if len(fields) < 2 { + continue + } + switch fields[0] { + case "Uid:": + uid = fields[1] + case "Gid:": + gid = fields[1] + } + } + if uid == "" || gid == "" { + return "", "", errors.Errorf("could not read uid/gid of PID %s from /proc", pid) + } + return uid, gid, nil +} + +// ContainerFileSystem returns the container runtime's own root path for the +// container — the overlay directory. +// +// NOTE: this is NOT what the container sees. Any path mounted over +// (an emptyDir at /tmp, a PVC, a projected secret) resolves differently inside +// the container's mount namespace, so reads and writes through here are +// invisible to the process. Use TargetRootFS for anything the target touches. func ContainerFileSystem(r api.ContainerRuntime, containerID string, containerRuntimePath string) (string, error) { if r == "" || containerID == "" { return "", errors.New(ContainerRuntimeAndContainerIdMandatoryText) diff --git a/internal/agent/util/target_test.go b/internal/agent/util/target_test.go new file mode 100644 index 0000000..ed04380 --- /dev/null +++ b/internal/agent/util/target_test.go @@ -0,0 +1,39 @@ +package util + +import ( + "os" + "path/filepath" + "strconv" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// TestTargetRootFS locks the path we reach the target through. The runtime's +// overlay directory is NOT equivalent: anything mounted over the container's +// root (an emptyDir at /tmp is the common one) resolves differently inside the +// container, and a file staged through the overlay is invisible to it. +func TestTargetRootFS(t *testing.T) { + assert.Equal(t, "/proc/1234/root", TargetRootFS("1234")) + assert.Equal(t, "/proc/1234/root/tmp", filepath.Join(TargetRootFS("1234"), "tmp")) +} + +// TestTargetCredentials — we stage as root, but the target reads the library +// and writes its own output, so ownership has to follow the target. +func TestTargetCredentials(t *testing.T) { + t.Run("reads our own process", func(t *testing.T) { + if _, err := os.Stat("/proc/self/status"); err != nil { + t.Skip("no procfs on this platform") + } + uid, gid, err := TargetCredentials("self") + require.NoError(t, err) + assert.Equal(t, strconv.Itoa(os.Getuid()), uid) + assert.Equal(t, strconv.Itoa(os.Getgid()), gid) + }) + + t.Run("unknown pid", func(t *testing.T) { + _, _, err := TargetCredentials("not-a-pid") + assert.Error(t, err) + }) +}