diff --git a/internal/controller/api/node/process/process_get.go b/internal/controller/api/node/process/process_get.go index 19500288f..5d46041fb 100644 --- a/internal/controller/api/node/process/process_get.go +++ b/internal/controller/api/node/process/process_get.go @@ -41,6 +41,10 @@ func (s *Process) GetNodeProcessByPid( return gen.GetNodeProcessByPid400JSONResponse{Error: &errMsg}, nil } + if errMsg, ok := validatePid(request.Pid); !ok { + return gen.GetNodeProcessByPid400JSONResponse{Error: &errMsg}, nil + } + hostname := request.Hostname pid := request.Pid diff --git a/internal/controller/api/node/process/process_get_public_test.go b/internal/controller/api/node/process/process_get_public_test.go index c10ea9ac6..c500d3089 100644 --- a/internal/controller/api/node/process/process_get_public_test.go +++ b/internal/controller/api/node/process/process_get_public_test.go @@ -398,6 +398,28 @@ func (s *ProcessGetPublicTestSuite) TestGetNodeProcessByPidValidationHTTP() { s.Contains(rec.Body.String(), `"results"`) }, }, + { + name: "when the pid is zero returns 400", + path: "/api/node/server1/process/0", + setupJobMock: func() *jobmocks.MockJobClient { + return jobmocks.NewMockJobClient(s.mockCtrl) + }, + validateFunc: func(rec *httptest.ResponseRecorder) { + s.Equal(http.StatusBadRequest, rec.Code) + s.Contains(rec.Body.String(), "required") + }, + }, + { + name: "when the pid is minus one returns 400", + path: "/api/node/server1/process/-1", + setupJobMock: func() *jobmocks.MockJobClient { + return jobmocks.NewMockJobClient(s.mockCtrl) + }, + validateFunc: func(rec *httptest.ResponseRecorder) { + s.Equal(http.StatusBadRequest, rec.Code) + s.Contains(rec.Body.String(), "min") + }, + }, { name: "when target agent not found", path: "/api/node/nonexistent/process/1234", diff --git a/internal/controller/api/node/process/process_signal_post.go b/internal/controller/api/node/process/process_signal_post.go index 5631b5020..efeff425c 100644 --- a/internal/controller/api/node/process/process_signal_post.go +++ b/internal/controller/api/node/process/process_signal_post.go @@ -42,6 +42,10 @@ func (s *Process) PostNodeProcessSignal( return gen.PostNodeProcessSignal400JSONResponse{Error: &errMsg}, nil } + if errMsg, ok := validatePid(request.Pid); !ok { + return gen.PostNodeProcessSignal400JSONResponse{Error: &errMsg}, nil + } + if errMsg, ok := validation.Struct(request.Body); !ok { return gen.PostNodeProcessSignal400JSONResponse{Error: &errMsg}, nil } diff --git a/internal/controller/api/node/process/process_signal_post_public_test.go b/internal/controller/api/node/process/process_signal_post_public_test.go index 8d8a9abfb..1934e0ed7 100644 --- a/internal/controller/api/node/process/process_signal_post_public_test.go +++ b/internal/controller/api/node/process/process_signal_post_public_test.go @@ -483,6 +483,42 @@ func (s *ProcessSignalPublicTestSuite) TestPostNodeProcessSignalValidationHTTP() s.Contains(rec.Body.String(), "oneof") }, }, + { + name: "when the pid is zero returns 400", + path: "/api/node/server1/process/0/signal", + body: `{"signal":"KILL"}`, + setupJobMock: func() *jobmocks.MockJobClient { + return jobmocks.NewMockJobClient(s.mockCtrl) + }, + validateFunc: func(rec *httptest.ResponseRecorder) { + s.Equal(http.StatusBadRequest, rec.Code) + s.Contains(rec.Body.String(), "required") + }, + }, + { + name: "when the pid is minus one returns 400", + path: "/api/node/server1/process/-1/signal", + body: `{"signal":"KILL"}`, + setupJobMock: func() *jobmocks.MockJobClient { + return jobmocks.NewMockJobClient(s.mockCtrl) + }, + validateFunc: func(rec *httptest.ResponseRecorder) { + s.Equal(http.StatusBadRequest, rec.Code) + s.Contains(rec.Body.String(), "min") + }, + }, + { + name: "when the pid is a process group returns 400", + path: "/api/node/server1/process/-42/signal", + body: `{"signal":"KILL"}`, + setupJobMock: func() *jobmocks.MockJobClient { + return jobmocks.NewMockJobClient(s.mockCtrl) + }, + validateFunc: func(rec *httptest.ResponseRecorder) { + s.Equal(http.StatusBadRequest, rec.Code) + s.Contains(rec.Body.String(), "min") + }, + }, { name: "when target agent not found", path: "/api/node/nonexistent/process/1234/signal", diff --git a/internal/controller/api/node/process/validate.go b/internal/controller/api/node/process/validate.go index 36e9e562f..48011c2c2 100644 --- a/internal/controller/api/node/process/validate.go +++ b/internal/controller/api/node/process/validate.go @@ -20,7 +20,11 @@ package process -import "github.com/osapi-io/osapi/internal/validation" +import ( + "fmt" + + "github.com/osapi-io/osapi/internal/validation" +) // validateHostname validates a hostname path parameter using the shared // validator. Returns the error message and false if invalid. @@ -32,3 +36,30 @@ func validateHostname( ) (string, bool) { return validation.Var(hostname, "required,min=1,valid_target") } + +// validatePid validates a pid path parameter against the bound the +// specification declares for it. Same upstream limitation as +// validateHostname. +// +// min=1 is the rule that matters, not required. A pid reaches kill(2), where a +// value below 1 does not mean "that process": 0 is every process in the +// caller's process group, -1 is every process the caller may signal, and +// anything lower is a whole process group. required only rejects 0, so without +// min=1 a negative pid turns one signal into a fleet-wide one. +func validatePid( + pid int, +) (string, bool) { + errMsg, ok := validation.Var(pid, "required,min=1") + if ok { + return "", true + } + + // validation.Var has no field name to report and min carries no hint, so + // the bare message is "Field validation for '' failed on the 'min' tag", + // which tells a caller nothing. Name the value and the bound, and keep the + // tag's own message so a test can still assert which rule rejected it. + return fmt.Sprintf( + "pid %d must be greater than zero: %s", + pid, errMsg, + ), false +} diff --git a/internal/provider/node/process/debian.go b/internal/provider/node/process/debian.go index 19a46a642..34d437faf 100644 --- a/internal/provider/node/process/debian.go +++ b/internal/provider/node/process/debian.go @@ -33,6 +33,27 @@ import ( ) // allowedSignals maps signal names to their syscall equivalents. +// validatePid rejects a pid that kill(2) would not read as one process. +// +// kill(2) overloads the sign: 0 means every process in the caller's process +// group, -1 means every process the caller may signal, and anything below -1 +// means a whole process group. The agent usually runs as root, so a value +// below 1 turns one signal into a host-wide one. The API validates this too, +// and this is the copy that matters, because this is the frame that calls +// kill(2). +func validatePid( + pid int, +) error { + if pid < 1 { + return fmt.Errorf( + "process: pid %d must be greater than zero, a lower value signals a process group rather than a process", + pid, + ) + } + + return nil +} + var allowedSignals = map[string]syscall.Signal{ "TERM": syscall.SIGTERM, "KILL": syscall.SIGKILL, @@ -156,6 +177,10 @@ func (d *Debian) Get( _ context.Context, pid int, ) (*Info, error) { + if err := validatePid(pid); err != nil { + return nil, err + } + p, err := d.lister.NewProcess(int32(pid)) if err != nil { return nil, fmt.Errorf("process: get: %w", err) @@ -175,6 +200,10 @@ func (d *Debian) Signal( pid int, signal string, ) (*SignalResult, error) { + if err := validatePid(pid); err != nil { + return nil, err + } + sig, ok := allowedSignals[signal] if !ok { return nil, fmt.Errorf("process: signal: invalid signal %q", signal) diff --git a/internal/provider/node/process/debian_public_test.go b/internal/provider/node/process/debian_public_test.go index 789709a5d..3c440fb2f 100644 --- a/internal/provider/node/process/debian_public_test.go +++ b/internal/provider/node/process/debian_public_test.go @@ -245,6 +245,24 @@ func (suite *DebianPublicTestSuite) TestGet() { wantErrMsg string validateFunc func(result *process.Info) }{ + // setupMock sets no expectation, so the controller fails the test if + // NewProcess is called at all. kill(2) is not reached from Get, but the + // guard is here because the API and the provider validate the same + // bound, and a pid below 1 is not a process either way. + { + name: "when pid is zero it never reaches the lister", + pid: 0, + setupMock: func() {}, + wantErr: true, + wantErrMsg: "pid 0 must be greater than zero", + }, + { + name: "when pid is negative it never reaches the lister", + pid: -1, + setupMock: func() {}, + wantErr: true, + wantErrMsg: "pid -1 must be greater than zero", + }, { name: "when successful returns process info", pid: 42, @@ -394,6 +412,42 @@ func (suite *DebianPublicTestSuite) TestSignal() { wantErr: true, wantErrMsg: "process: signal: unexpected error", }, + // Each of these would reach kill(2) as a process-group signal rather + // than a process signal. setupMock sets no expectation, so the mock + // controller fails the test if Kill is called at all, which is the + // assertion that matters: the signal is never sent. + { + name: "when pid is zero it never reaches kill", + pid: 0, + signal: "KILL", + setupMock: func() {}, + wantErr: true, + wantErrMsg: "pid 0 must be greater than zero", + }, + { + name: "when pid is minus one it never reaches kill", + pid: -1, + signal: "KILL", + setupMock: func() {}, + wantErr: true, + wantErrMsg: "pid -1 must be greater than zero", + }, + { + name: "when pid is a process group it never reaches kill", + pid: -42, + signal: "TERM", + setupMock: func() {}, + wantErr: true, + wantErrMsg: "pid -42 must be greater than zero", + }, + { + name: "the pid is checked before the signal name", + pid: -1, + signal: "NOT-A-SIGNAL", + setupMock: func() {}, + wantErr: true, + wantErrMsg: "pid -1 must be greater than zero", + }, } for _, tc := range tests {