fix(security): x11_cookie field-level RBAC; xcecli -X preflight before submit
The per-job MIT-MAGIC-COOKIE-1 that unlocks an -X job's X11 forwarding port rode every job reply that asked for it: any job:view caller could read another user's cookie and connect straight to the worker's forwarding port (6100-6199), bypassing the xattach relay's ownership check. The SSE job stream carried it too, since its events embed the full JobInfo row. - apiserver: stripX11Cookie drops the cookie from GetJobsInfo, GetJobByIDs and the SSE stream for every job the caller does not own and holds no job:control grant over — the same rule XAttachJob enforces. The row itself stays visible; only the secret goes. - xcecli: preflightX11 checks DISPLAY, the Xauthority file and the xauth binary before the job is submitted, so a misconfigured display fails in one second instead of dispatching a job nobody can watch. Verified end-to-end on t1: owner reads the cookie, a group mate with job:view(self_ug)+job:control(self) gets the row with the cookie stripped, an unrelated user is refused at the row level; controllers tests cover the matrix plus the SSE path. Co-Authored-By: Claude Code <noreply@anthropic.com>
This commit is contained in:
parent
f995123e6c
commit
f94cc08680
6 changed files with 352 additions and 3 deletions
|
|
@ -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))
|
||||
|
|
|
|||
232
src/frontend/apiserver/controllers/job_x11cookie_rbac_test.go
Normal file
232
src/frontend/apiserver/controllers/job_x11cookie_rbac_test.go
Normal file
|
|
@ -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")
|
||||
}
|
||||
}
|
||||
|
|
@ -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)
|
||||
|
||||
|
|
|
|||
|
|
@ -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
|
||||
|
|
|
|||
|
|
@ -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
|
||||
|
|
|
|||
|
|
@ -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")
|
||||
}
|
||||
})
|
||||
}
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue