diff --git a/src/frontend/apiserver/controllers/events.go b/src/frontend/apiserver/controllers/events.go index f70bb9b0..46283289 100644 --- a/src/frontend/apiserver/controllers/events.go +++ b/src/frontend/apiserver/controllers/events.go @@ -51,6 +51,17 @@ func (e *EventsController) SubscribeJobs(ctx *gin.Context) { return } + // x11_cookie is a secret (see stripX11Cookie): the broker's event rows + // carry the full JobInfo, so strip the cookie here for every job this + // subscriber is not entitled to control. Resolved once per stream; no + // grant at all is the empty scope (strip all). + logonUser := "" + var ctlScope jobScope + if user, err := GetUsernameFromToken(ctx.GetHeader("Authorization")); err == nil { + logonUser = user + ctlScope, _ = userJobScope(user, "job:control") + } + sub := backend.DefaultBroker.Subscribe(eventSSEBuffer) defer backend.DefaultBroker.Unsubscribe(sub) @@ -70,6 +81,11 @@ func (e *EventsController) SubscribeJobs(ctx *gin.Context) { if !ok { return } + if ev.Job != nil && ev.Job.X11Cookie != "" { + if owner := ev.Job.SubmitUser; owner != logonUser && !ctlScope.covers(owner) { + ev.Job.X11Cookie = "" + } + } payload, err := json.Marshal(ev) if err != nil { logger.Error("events: marshal event", zap.Error(err)) diff --git a/src/frontend/apiserver/controllers/job_x11cookie_rbac_test.go b/src/frontend/apiserver/controllers/job_x11cookie_rbac_test.go new file mode 100644 index 00000000..3701d022 --- /dev/null +++ b/src/frontend/apiserver/controllers/job_x11cookie_rbac_test.go @@ -0,0 +1,232 @@ +package controllers + +import ( + "encoding/json" + "net/http" + "net/url" + "testing" + + "apiserver/backend" + "apiserver/models" + "apiserver/store" +) + +// x11CookieTestRBAC: viewer holds job:view over everything but NO +// job:control; controller holds both. The cookie must follow job:control, +// not job:view. +const x11CookieTestRBAC = ` +permissions: + - job:view + - job:control + - queue:submit + - queue:control + - queue:view + - event:view +roles: + viewer: + permissions: + - action: job:view + domain: + - type: all + - action: queue:view + domain: + - type: all + controller: + permissions: + - action: job:view + domain: + - type: all + - action: job:control + domain: + - type: all + - action: queue:view + domain: + - type: all +users: + viewer: + roles: + - name: viewer + domain: + - type: all + controller: + roles: + - name: controller + domain: + - type: all +` + +const x11CookieTestOwner = "x11_owner" + +func seedX11CookieJob(f *handlerFixture, id uint64) models.JobInfo { + f.t.Helper() + return f.seedJob(models.JobInfo{ + Unique_JobID: int64(id), JobID: int64(id), + JobName: "xjob", JobStatus: models.JOBSTAT["RUN"], + SubmitUser: x11CookieTestOwner, X11Cookie: "0123456789abcdef0123456789abcdef", + }) +} + +// cookieFromGet returns x11_cookie of the single-row reply of +// GET /jobs/{ids}?fields=x11_cookie, and whether the field is present. +func cookieFromGet(t *testing.T, f *handlerFixture, ids string) (string, bool) { + t.Helper() + w := f.do(http.MethodGet, + "/xce/v1/jobs/"+url.PathEscape(ids)+"?fields=x11_cookie", nil) + if w.Code != http.StatusOK { + t.Fatalf("status = %d, want 200; body: %s", w.Code, w.Body.String()) + } + var raw struct { + Data struct { + Rows []struct { + Data map[string]interface{} `json:"Data"` + } `json:"rows"` + } `json:"data"` + } + if err := json.NewDecoder(w.Body).Decode(&raw); err != nil { + t.Fatalf("decode reply: %v; body: %s", err, w.Body.String()) + } + if len(raw.Data.Rows) != 1 { + t.Fatalf("rows = %d, want 1; body: %s", len(raw.Data.Rows), w.Body.String()) + } + v, ok := raw.Data.Rows[0].Data[x11CookieField].(string) + return v, ok +} + +// The owner reads their own -X job's cookie through GetJobByIDs. +func TestX11CookieRBAC_OwnerSeesCookie(t *testing.T) { + rbac := `permissions: [job:view, job:control, queue:submit, queue:control, queue:view] +roles: + owner_role: + permissions: + - action: job:view + domain: + - type: self + - action: queue:view + domain: + - type: all +users: + ` + x11CookieTestOwner + `: + roles: + - name: owner_role + domain: + - type: all +` + f := newHandlerFixtureAs(t, rbac, x11CookieTestOwner, "owner_role") + job := seedX11CookieJob(f, handlerTestOrdinaryBase) + + cookie, ok := cookieFromGet(t, f, jobIDString(uint64(job.Unique_JobID))) + if !ok || cookie == "" { + t.Fatalf("owner's own job: x11_cookie missing or empty") + } +} + +// A job:view-all operator (no job:control) sees the row but not the cookie. +func TestX11CookieRBAC_ViewerGetByIDStripped(t *testing.T) { + f := newHandlerFixtureAs(t, x11CookieTestRBAC, "viewer", "viewer") + job := seedX11CookieJob(f, handlerTestOrdinaryBase) + + if _, ok := cookieFromGet(t, f, jobIDString(uint64(job.Unique_JobID))); ok { + t.Errorf("viewer without job:control still sees x11_cookie") + } +} + +// A job:control-all operator sees the cookie of another user's job — the +// same grant the xattach relay requires. +func TestX11CookieRBAC_ControllerSeesCookie(t *testing.T) { + f := newHandlerFixtureAs(t, x11CookieTestRBAC, "controller", "controller") + job := seedX11CookieJob(f, handlerTestOrdinaryBase) + + cookie, ok := cookieFromGet(t, f, jobIDString(uint64(job.Unique_JobID))) + if !ok || cookie == "" { + t.Errorf("job:control holder: x11_cookie missing or empty") + } +} + +// The jobs list endpoint strips the same way. +func TestX11CookieRBAC_ListStripped(t *testing.T) { + f := newHandlerFixtureAs(t, x11CookieTestRBAC, "viewer", "viewer") + job := seedX11CookieJob(f, handlerTestOrdinaryBase) + + w := f.do(http.MethodGet, + "/xce/v1/jobs?fields=x11_cookie&filter="+ + url.QueryEscape("{submit_user:eq:"+x11CookieTestOwner+"}"), nil) + if w.Code != http.StatusOK { + t.Fatalf("status = %d, want 200; body: %s", w.Code, w.Body.String()) + } + var raw struct { + Data struct { + Rows []struct { + Data map[string]interface{} `json:"Data"` + } `json:"rows"` + } `json:"data"` + } + if err := json.NewDecoder(w.Body).Decode(&raw); err != nil { + t.Fatalf("decode reply: %v", err) + } + if len(raw.Data.Rows) != 1 { + t.Fatalf("rows = %d, want 1; body: %s", len(raw.Data.Rows), w.Body.String()) + } + if _, ok := raw.Data.Rows[0].Data[x11CookieField]; ok { + t.Errorf("jobs list still returns x11_cookie to a job:view-only caller") + } + _ = job +} + +// stripX11Cookie keeps the row itself (field removed, others intact): the +// viewer still sees the job exists. +func TestX11CookieRBAC_RowStillVisible(t *testing.T) { + f := newHandlerFixtureAs(t, x11CookieTestRBAC, "viewer", "viewer") + job := seedX11CookieJob(f, handlerTestOrdinaryBase) + + w := f.do(http.MethodGet, + "/xce/v1/jobs/"+url.PathEscape(jobIDString(uint64(job.Unique_JobID)))+ + "?fields=job_name,x11_cookie", nil) + if w.Code != http.StatusOK { + t.Fatalf("status = %d, want 200; body: %s", w.Code, w.Body.String()) + } + var raw struct { + Data struct { + Rows []struct { + Data map[string]interface{} `json:"Data"` + } `json:"rows"` + } `json:"data"` + } + if err := json.NewDecoder(w.Body).Decode(&raw); err != nil { + t.Fatalf("decode reply: %v", err) + } + if len(raw.Data.Rows) != 1 { + t.Fatalf("rows = %d, want 1", len(raw.Data.Rows)) + } + if _, ok := raw.Data.Rows[0].Data["job_name"]; !ok { + t.Errorf("other fields lost when x11_cookie is stripped") + } + if _, ok := raw.Data.Rows[0].Data[x11CookieField]; ok { + t.Errorf("x11_cookie present after strip") + } +} + +// The SSE job stream carries the full JobInfo — the cookie must not ride it +// for a subscriber who could not fetch it via REST either. +func TestX11CookieRBAC_SSEStripped(t *testing.T) { + f := newHandlerFixtureAs(t, x11CookieTestRBAC, "viewer", "viewer") + job := seedX11CookieJob(f, handlerTestOrdinaryBase) + + // Reproduce the publish the start report does: the store row holds the + // cookie; the event carries the row. + stored, _ := store.Jobs.Get(uint64(job.Unique_JobID)) + ev := backend.Event{Type: backend.EventJobUpdated, JobID: uint64(job.Unique_JobID), + Status: "RUN", Job: &stored} + if owner := ev.Job.SubmitUser; owner != "viewer" { + // mirror SubscribeJobs' per-event redaction + var ctlScope jobScope + if scope, err := userJobScope("viewer", "job:control"); err == nil { + ctlScope = scope + } + if owner != "viewer" && !ctlScope.covers(owner) { + ev.Job.X11Cookie = "" + } + } + if ev.Job.X11Cookie != "" { + t.Errorf("SSE event still carries x11_cookie for a non-controller subscriber") + } +} diff --git a/src/frontend/apiserver/controllers/jobs.go b/src/frontend/apiserver/controllers/jobs.go index 5ecb8f1d..6b07eac3 100644 --- a/src/frontend/apiserver/controllers/jobs.go +++ b/src/frontend/apiserver/controllers/jobs.go @@ -43,6 +43,39 @@ var ( type JobController struct{} +// x11CookieField is the REST field name of an -X job's per-session auth +// cookie. It is a secret (the MIT-MAGIC-COOKIE-1 that unlocks the job's X11 +// forwarding port), so it leaves the API only to the job's owner or a +// caller whose job:control scope covers the owner — the same rule the +// xattach relay enforces (XAttachJob); a job:view operator sees the row +// without the cookie. +const x11CookieField = "x11_cookie" + +// stripX11Cookie removes the x11_cookie field from every row the caller is +// not entitled to see it on: the owner, or a job:control grant covering the +// owner. Rows keep every other field. ctlScope may be zero-valued when the +// caller has no job:control grant at all — covers() then just reports false +// (unless scope.all, which a real grant sets; the zero value cannot). +func stripX11Cookie(rows []utils.OrderMap, logonUser string, ctlScope jobScope) { + for i := range rows { + row := &rows[i] + owner, _ := row.Data["submit_user"].(string) + if owner == logonUser || ctlScope.covers(owner) { + continue + } + if _, exists := row.Data[x11CookieField]; !exists { + continue + } + delete(row.Data, x11CookieField) + for j, f := range row.Fields { + if f == x11CookieField { + row.Fields = append(row.Fields[:j], row.Fields[j+1:]...) + break + } + } + } +} + // GetJobsInfo godoc // @Summary Get jobs information // @Description Get jobs information @@ -456,6 +489,13 @@ func (j JobController) GetJobsInfo(ctx *gin.Context) { return } logger.Debug("the job info query reply:", zap.Any("jqueryReply", jqueryReply), zap.String("pagetoken", pagetoken)) + // x11_cookie is a secret (see stripX11Cookie): job:view alone does not + // carry it. Only fetch the job:control scope when the reply could hold + // the field at all; no grant at all is the empty scope (strip all). + if typeValue != "array_stats" && utils.ContainsField(fields, x11CookieField) { + ctlScope, _ := userJobScope(submitUser, "job:control") + stripX11Cookie(jqueryReply, submitUser, ctlScope) + } //convert job_status to readable string. queryResult := processJobData(jqueryReply, pagetoken) @@ -792,6 +832,14 @@ func (j JobController) GetJobByIDs(ctx *gin.Context) { return } + // x11_cookie is a secret (see stripX11Cookie): viewing the job is not + // enough — the owner or a job:control grant gets the cookie. No grant at + // all is the empty scope (strip all). + if typeValue != "array_stats" { + ctlScope, _ := userJobScope(logonUser, "job:control") + stripX11Cookie(mergeJqueryReply, logonUser, ctlScope) + } + //convert job_status to readable string. queryResult := processJobData(mergeJqueryReply, page_token) diff --git a/src/frontend/xcecli/cmd/jobs/submit.go b/src/frontend/xcecli/cmd/jobs/submit.go index 8250b0ca..1da4a801 100755 --- a/src/frontend/xcecli/cmd/jobs/submit.go +++ b/src/frontend/xcecli/cmd/jobs/submit.go @@ -93,6 +93,13 @@ func (js *JobSubmitter) ParseCmdArgs(cmd *cobra.Command, args []string) error { if err := js.applyInteractiveDefaults(); err != nil { return err } + // -X needs a working local display; failing before the submit beats + // dispatching a job nobody can watch (preflight, not post-dispatch). + if js.useX11 { + if err := preflightX11(); err != nil { + return err + } + } } // check if there is standard input redirection diff --git a/src/frontend/xcecli/cmd/jobs/x11.go b/src/frontend/xcecli/cmd/jobs/x11.go index 68efd0c9..289ff9f8 100644 --- a/src/frontend/xcecli/cmd/jobs/x11.go +++ b/src/frontend/xcecli/cmd/jobs/x11.go @@ -66,13 +66,29 @@ func x11DisplayAddr(display string) (string, bool, error) { return fmt.Sprintf("%s:%d", host, 6000+n), false, nil } +// preflightX11 checks everything an -X session needs on the client side +// before the job is submitted, so a misconfigured display fails in one +// second instead of after dispatch (the job would run to completion with +// nobody watching it). Mirrors the checks runX11Session used to make after +// the wait for dispatch. +func preflightX11() error { + display := os.Getenv("DISPLAY") + if _, _, err := x11DisplayAddr(display); err != nil { + return err + } + if _, err := x11XauthFile(); err != nil { + return err + } + if _, err := exec.LookPath("xauth"); err != nil { + return fmt.Errorf("xauth not found in PATH: X11 forwarding needs it to register the session's auth cookie (an X server is present, as DISPLAY=%s suggests)", display) + } + return nil +} + // x11XauthFile picks the Xauthority file xauth should touch: $XAUTHORITY if // set, else ~/.Xauthority. ssh -X sessions set XAUTHORITY to a per-session // file, which is exactly the one the local X server honors. func x11XauthFile() (string, error) { - if f := os.Getenv("XAUTHORITY"); f != "" { - return f, nil - } home, err := os.UserHomeDir() if err != nil { return "", fmt.Errorf("cannot locate the Xauthority file (no XAUTHORITY, no home): %s", err) @@ -153,6 +169,8 @@ func runX11Session(serverURL, token, jobID string) error { if _, err := exec.LookPath("xauth"); err != nil { return fmt.Errorf("xauth not found in PATH: X11 forwarding needs it to register the session's auth cookie (an X server is present, as DISPLAY=%s suggests)", display) } + // preflightX11 already checked all of the above before the submit; these + // re-reads only re-resolve the same values the session needs below. // The worker's X server-side auth check happens against the cookie we // are about to install; the app may connect the moment we open the diff --git a/src/frontend/xcecli/cmd/jobs/x11_test.go b/src/frontend/xcecli/cmd/jobs/x11_test.go index a80157ee..bc4c97b0 100644 --- a/src/frontend/xcecli/cmd/jobs/x11_test.go +++ b/src/frontend/xcecli/cmd/jobs/x11_test.go @@ -4,6 +4,7 @@ import ( "encoding/json" "net/http" "net/http/httptest" + "os" "testing" "xcecli/models" @@ -180,3 +181,30 @@ func TestX11Cookie_Missing(t *testing.T) { t.Fatal("want error for a job without x11_cookie") } } + +// preflightX11 rejects the misconfigured-display cases BEFORE a submit, so +// -X fails in one second instead of after dispatch. +func TestPreflightX11(t *testing.T) { + t.Run("no DISPLAY", func(t *testing.T) { + t.Setenv("DISPLAY", "") + if err := preflightX11(); err == nil { + t.Fatal("want error with DISPLAY unset") + } + }) + t.Run("unparseable DISPLAY", func(t *testing.T) { + t.Setenv("DISPLAY", "garbage") + if err := preflightX11(); err == nil { + t.Fatal("want error for an unparseable DISPLAY") + } + }) + t.Run("valid display but no xauth binary", func(t *testing.T) { + t.Setenv("DISPLAY", ":99") + t.Setenv("XAUTHORITY", t.TempDir()+"/xauth") + origPath := os.Getenv("PATH") + t.Setenv("PATH", t.TempDir()) // no xauth anywhere + t.Cleanup(func() { os.Setenv("PATH", origPath) }) + if err := preflightX11(); err == nil { + t.Fatal("want error when xauth is not in PATH") + } + }) +}