From 6f7e262f3f665ab5b007805a2e707adf8d7b4cd9 Mon Sep 17 00:00:00 2001 From: Jan Eitzinger Date: Thu, 4 Jun 2026 18:33:30 +0200 Subject: [PATCH] Fix issues after security audit Entire-Checkpoint: bc18358a9343 --- CLAUDE.md | 18 +++++++++++++++++ cmd/cc-backend/server.go | 10 +++++++++- internal/api/api_test.go | 1 + internal/api/nats.go | 5 ++++- internal/api/nats_test.go | 1 + internal/auth/auth.go | 32 +++++++++++++++--------------- internal/auth/jwtCookieSession.go | 33 ++++++++++++++++++------------- 7 files changed, 68 insertions(+), 32 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index 061d056b..fe6a7aab 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -314,6 +314,24 @@ job,function=stop_job event="{\"jobId\":123,\"cluster\":\"test\",\"startTime\":1 - Messages are logged; no responses are sent back to publishers - If NATS client is unavailable, API subscriptions are skipped (logged as warning) +### Security Considerations + +**The NATS API has no application-layer authentication or authorization.** Unlike +the REST endpoints (which require a JWT with `RoleAPI`), the subscribers process +any message delivered on the configured subjects. Anyone with publish rights to +those subjects on the broker can: + +- Insert arbitrary jobs (potentially attributed to other users) +- Mark running jobs as stopped, triggering archive/finalization +- Overwrite node state and health metadata for any cluster + +Operators MUST restrict publish ACLs at the NATS broker (per-account or +per-subject permissions) so that only trusted producers — e.g. the scheduler +integration on a known host or service account — can publish to the configured +`subject-job-event` and `subject-node-state` subjects. A shared, unrestricted +NATS broker is not a safe deployment topology for this API. A startup warning +is logged when these subscriptions are enabled. + ## Development Guidelines ### Performance diff --git a/cmd/cc-backend/server.go b/cmd/cc-backend/server.go index 85503535..33504988 100644 --- a/cmd/cc-backend/server.go +++ b/cmd/cc-backend/server.go @@ -129,11 +129,19 @@ func (s *Server) init() error { s.router.Use(middleware.Compress(5)) s.router.Use(middleware.Recoverer) s.router.Use(cors.Handler(cors.Options{ - AllowCredentials: true, + AllowCredentials: false, AllowedHeaders: []string{"X-Requested-With", "Content-Type", "Authorization", "Origin"}, AllowedMethods: []string{"GET", "POST", "HEAD", "OPTIONS"}, AllowedOrigins: []string{"*"}, })) + s.router.Use(func(next http.Handler) http.Handler { + return http.HandlerFunc(func(rw http.ResponseWriter, r *http.Request) { + if r.TLS != nil || r.Header.Get("X-Forwarded-Proto") == "https" { + rw.Header().Set("Strict-Transport-Security", "max-age=31536000; includeSubDomains") + } + next.ServeHTTP(rw, r) + }) + }) s.restAPIHandle = api.New() diff --git a/internal/api/api_test.go b/internal/api/api_test.go index a8aef889..26ee456a 100644 --- a/internal/api/api_test.go +++ b/internal/api/api_test.go @@ -170,6 +170,7 @@ func setup(t *testing.T) *api.RestAPI { archiver.Start(repository.GetJobRepository(), context.Background()) + t.Setenv("SESSION_KEY", "AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA=") if cfg := ccconf.GetPackageConfig("auth"); cfg != nil { auth.Init(&cfg) } else { diff --git a/internal/api/nats.go b/internal/api/nats.go index db33c0e2..0c967b83 100644 --- a/internal/api/nats.go +++ b/internal/api/nats.go @@ -151,7 +151,10 @@ func (api *NatsAPI) StartSubscriptions() error { return err } - cclog.Info("NATS API subscriptions started") + cclog.Warnf("NATS API subscriptions started on subjects %q and %q — these are UNAUTHENTICATED: "+ + "anyone with publish rights on the broker can start/stop jobs and update node state. "+ + "Restrict publish ACLs on the NATS broker to trusted producers only.", + s.SubjectJobEvent, s.SubjectNodeState) } return nil } diff --git a/internal/api/nats_test.go b/internal/api/nats_test.go index b1d2a624..cc0bdf90 100644 --- a/internal/api/nats_test.go +++ b/internal/api/nats_test.go @@ -156,6 +156,7 @@ func setupNatsTest(t *testing.T) *NatsAPI { archiver.Start(repository.GetJobRepository(), context.Background()) + t.Setenv("SESSION_KEY", "AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA=") if cfg := ccconf.GetPackageConfig("auth"); cfg != nil { auth.Init(&cfg) } else { diff --git a/internal/auth/auth.go b/internal/auth/auth.go index d1c004bd..8d8bc222 100644 --- a/internal/auth/auth.go +++ b/internal/auth/auth.go @@ -9,7 +9,6 @@ package auth import ( "bytes" "context" - "crypto/rand" "database/sql" "encoding/base64" "encoding/json" @@ -17,6 +16,7 @@ import ( "fmt" "net" "net/http" + "net/url" "os" "sync" "time" @@ -187,19 +187,15 @@ func Init(authCfg *json.RawMessage) { sessKey := os.Getenv("SESSION_KEY") if sessKey == "" { - cclog.Warn("environment variable 'SESSION_KEY' not set (will use non-persistent random key)") - bytes := make([]byte, 32) - if _, err := rand.Read(bytes); err != nil { - cclog.Fatal("Error while initializing authentication -> failed to generate random bytes for session key") - } - authInstance.sessionStore = sessions.NewCookieStore(bytes) - } else { - bytes, err := base64.StdEncoding.DecodeString(sessKey) - if err != nil { - cclog.Fatal("Error while initializing authentication -> decoding session key failed") - } - authInstance.sessionStore = sessions.NewCookieStore(bytes) + cclog.Fatal("environment variable 'SESSION_KEY' not set: refusing to start with an ephemeral session key. " + + "Set SESSION_KEY in .env (base64-encoded 32 random bytes); a random key would invalidate all sessions on every restart " + + "and prevent sessions from validating across replicas.") } + keyBytes, err := base64.StdEncoding.DecodeString(sessKey) + if err != nil { + cclog.Fatal("Error while initializing authentication -> decoding session key failed") + } + authInstance.sessionStore = sessions.NewCookieStore(keyBytes) if d, err := time.ParseDuration(config.Keys.SessionMaxAge); err == nil { authInstance.SessionMaxAge = d @@ -325,6 +321,7 @@ func (auth *Authentication) SaveSession(rw http.ResponseWriter, r *http.Request, session.Options.Secure = false } session.Options.SameSite = http.SameSiteLaxMode + session.Options.HttpOnly = true session.Values["username"] = user.Username session.Values["projects"] = user.Projects session.Values["roles"] = user.Roles @@ -388,9 +385,12 @@ func (auth *Authentication) Login( cclog.Infof("login successfull: user: %#v (roles: %v, projects: %v)", user.Username, user.Roles, user.Projects) ctx := context.WithValue(r.Context(), repository.ContextUserKey, user) - if r.FormValue("redirect") != "" { - http.RedirectHandler(r.FormValue("redirect"), http.StatusFound).ServeHTTP(rw, r.WithContext(ctx)) - return + if redirect := r.FormValue("redirect"); redirect != "" { + if u, perr := url.Parse(redirect); perr == nil && u.Scheme == "" && u.Host == "" { + http.RedirectHandler(redirect, http.StatusFound).ServeHTTP(rw, r.WithContext(ctx)) + return + } + cclog.Warnf("login redirect rejected (not a relative path): %q", redirect) } http.RedirectHandler("/", http.StatusFound).ServeHTTP(rw, r.WithContext(ctx)) diff --git a/internal/auth/jwtCookieSession.go b/internal/auth/jwtCookieSession.go index 4c4bbeb6..f16dbe5e 100644 --- a/internal/auth/jwtCookieSession.go +++ b/internal/auth/jwtCookieSession.go @@ -9,6 +9,7 @@ import ( "crypto/ed25519" "encoding/base64" "errors" + "fmt" "net/http" "os" @@ -119,22 +120,26 @@ func (ja *JWTCookieSessionAuthenticator) Login( rawtoken = jwtCookie.Value } - token, err := jwt.Parse(rawtoken, func(t *jwt.Token) (any, error) { - if t.Method != jwt.SigningMethodEdDSA { - return nil, errors.New("only Ed25519/EdDSA supported") - } + parser := jwt.NewParser(jwt.WithValidMethods([]string{jwt.SigningMethodEdDSA.Alg()})) - unvalidatedIssuer, success := t.Claims.(jwt.MapClaims)["iss"].(string) - if success && unvalidatedIssuer == jc.TrustedIssuer { - // The (unvalidated) issuer seems to be the expected one, - // use public cross login key from config - return ja.publicKeyCrossLogin, nil - } + unverified, _, perr := parser.ParseUnverified(rawtoken, jwt.MapClaims{}) + if perr != nil { + cclog.Warn("JWT cookie session: error while parsing token") + return nil, perr + } + issuer, _ := unverified.Claims.(jwt.MapClaims)["iss"].(string) - // No cross login key configured or issuer not expected - // Try own key - return ja.publicKey, nil - }) + var key any + switch issuer { + case jc.TrustedIssuer: + key = ja.publicKeyCrossLogin + case "": + key = ja.publicKey + default: + return nil, fmt.Errorf("untrusted JWT issuer: %q", issuer) + } + + token, err := parser.Parse(rawtoken, func(*jwt.Token) (any, error) { return key, nil }) if err != nil { cclog.Warn("JWT cookie session: error while parsing token") return nil, err