Skip to content

Commit cff2ef5

Browse files
onutconutc
committed
fix(provisioning): tighten service principal policy
1 parent dc41888 commit cff2ef5

8 files changed

Lines changed: 204 additions & 20 deletions

File tree

‎api/main.go‎

Lines changed: 10 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -330,6 +330,9 @@ func (s *server) createSpritz(c echo.Context) error {
330330
if err != nil {
331331
return writeError(c, http.StatusBadRequest, err.Error())
332332
}
333+
if principal.isService() && len(userConfigKeys) > 0 {
334+
return writeError(c, http.StatusBadRequest, "userConfig is not allowed for service principals")
335+
}
333336
var normalizedUserConfig json.RawMessage
334337
if len(userConfigKeys) > 0 {
335338
normalized, err := normalizeUserConfig(s.userConfigPolicy, userConfigKeys, userConfigPayload)
@@ -343,6 +346,12 @@ func (s *server) createSpritz(c echo.Context) error {
343346
}
344347
normalizedUserConfig = encodedUserConfig
345348
applyUserConfig(&body.Spec, userConfigKeys, userConfigPayload)
349+
if _, ok := userConfigKeys["image"]; ok {
350+
requestedImage = strings.TrimSpace(body.Spec.Image) != ""
351+
}
352+
if _, ok := userConfigKeys["repo"]; ok {
353+
requestedRepo = body.Spec.Repo != nil || len(body.Spec.Repos) > 0
354+
}
346355
}
347356

348357
if body.Spec.Image == "" {
@@ -403,7 +412,7 @@ func (s *server) createSpritz(c echo.Context) error {
403412
if !nameProvided {
404413
fingerprintName = ""
405414
}
406-
fingerprint, err := s.validateProvisionerCreate(c.Request().Context(), principal, namespace, &body, normalizedUserConfig, requestedImage, requestedRepo, requestedNamespace, fingerprintName)
415+
fingerprint, err := s.validateProvisionerCreate(c.Request().Context(), principal, namespace, &body, normalizedUserConfig, userConfigKeys, requestedImage, requestedRepo, requestedNamespace, fingerprintName)
407416
if err != nil {
408417
if errors.Is(err, errForbidden) {
409418
return writeError(c, http.StatusForbidden, "forbidden")

‎api/main_create_owner_test.go‎

Lines changed: 64 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -470,3 +470,67 @@ func TestCreateSpritzRetriesGeneratedServiceNameAfterAlreadyExists(t *testing.T)
470470
t.Fatalf("expected second generated name after race, got %#v", name)
471471
}
472472
}
473+
474+
func TestCreateSpritzRejectsProvisionerLowLevelSpecFields(t *testing.T) {
475+
s := newCreateSpritzTestServer(t)
476+
configureProvisionerTestServer(s)
477+
e := echo.New()
478+
secured := e.Group("", s.authMiddleware())
479+
secured.POST("/api/spritzes", s.createSpritz)
480+
481+
body := []byte(`{
482+
"presetId":"openclaw",
483+
"ownerId":"user-123",
484+
"idempotencyKey":"discord-low-level",
485+
"spec":{
486+
"env":[{"name":"SHOULD_NOT_PASS","value":"1"}]
487+
}
488+
}`)
489+
req := httptest.NewRequest(http.MethodPost, "/api/spritzes", bytes.NewReader(body))
490+
req.Header.Set(echo.HeaderContentType, echo.MIMEApplicationJSON)
491+
req.Header.Set("X-Spritz-User-Id", "zenobot")
492+
req.Header.Set("X-Spritz-Principal-Type", "service")
493+
req.Header.Set("X-Spritz-Principal-Scopes", "spritz.instances.create,spritz.instances.assign_owner")
494+
rec := httptest.NewRecorder()
495+
496+
e.ServeHTTP(rec, req)
497+
498+
if rec.Code != http.StatusBadRequest {
499+
t.Fatalf("expected status 400, got %d: %s", rec.Code, rec.Body.String())
500+
}
501+
if !strings.Contains(rec.Body.String(), "spec.env is not allowed") {
502+
t.Fatalf("expected low-level spec validation error, got %s", rec.Body.String())
503+
}
504+
}
505+
506+
func TestCreateSpritzRejectsProvisionerUserConfigOverrides(t *testing.T) {
507+
s := newCreateSpritzTestServer(t)
508+
configureProvisionerTestServer(s)
509+
e := echo.New()
510+
secured := e.Group("", s.authMiddleware())
511+
secured.POST("/api/spritzes", s.createSpritz)
512+
513+
body := []byte(`{
514+
"presetId":"openclaw",
515+
"ownerId":"user-123",
516+
"idempotencyKey":"discord-user-config",
517+
"userConfig":{
518+
"repo":{"url":"https://example.com/private.git"}
519+
}
520+
}`)
521+
req := httptest.NewRequest(http.MethodPost, "/api/spritzes", bytes.NewReader(body))
522+
req.Header.Set(echo.HeaderContentType, echo.MIMEApplicationJSON)
523+
req.Header.Set("X-Spritz-User-Id", "zenobot")
524+
req.Header.Set("X-Spritz-Principal-Type", "service")
525+
req.Header.Set("X-Spritz-Principal-Scopes", "spritz.instances.create,spritz.instances.assign_owner")
526+
rec := httptest.NewRecorder()
527+
528+
e.ServeHTTP(rec, req)
529+
530+
if rec.Code != http.StatusBadRequest {
531+
t.Fatalf("expected status 400, got %d: %s", rec.Code, rec.Body.String())
532+
}
533+
if !strings.Contains(rec.Body.String(), "userConfig is not allowed") {
534+
t.Fatalf("expected service userConfig validation error, got %s", rec.Body.String())
535+
}
536+
}

‎api/provisioning.go‎

Lines changed: 59 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@ import (
66
"encoding/json"
77
"fmt"
88
"os"
9+
"reflect"
910
"sort"
1011
"strings"
1112
"time"
@@ -148,6 +149,58 @@ func normalizeCreateOwner(body *createRequest, principal principal, authEnabled
148149
return owner, nil
149150
}
150151

152+
func validateProvisionerRequestSurface(body *createRequest, userConfigKeys map[string]json.RawMessage) error {
153+
if body == nil {
154+
return nil
155+
}
156+
if len(userConfigKeys) > 0 {
157+
return fmt.Errorf("userConfig is not allowed for service principals")
158+
}
159+
if body.Spec.Owner.Team != "" {
160+
return fmt.Errorf("spec.owner.team is not allowed")
161+
}
162+
if len(body.Labels) > 0 {
163+
return fmt.Errorf("labels are not allowed for service principals")
164+
}
165+
if len(body.Annotations) > 0 {
166+
return fmt.Errorf("annotations are not allowed for service principals")
167+
}
168+
if len(body.Spec.Labels) > 0 {
169+
return fmt.Errorf("spec.labels are not allowed")
170+
}
171+
if len(body.Spec.Annotations) > 0 {
172+
return fmt.Errorf("spec.annotations are not allowed")
173+
}
174+
if len(body.Spec.Env) > 0 {
175+
return fmt.Errorf("spec.env is not allowed")
176+
}
177+
if len(body.Spec.Repos) > 0 {
178+
return fmt.Errorf("spec.repos is not allowed")
179+
}
180+
if body.Spec.Repo != nil && body.Spec.Repo.Auth != nil {
181+
return fmt.Errorf("spec.repo.auth is not allowed")
182+
}
183+
if len(body.Spec.SharedMounts) > 0 {
184+
return fmt.Errorf("spec.sharedMounts is not allowed")
185+
}
186+
if !reflect.DeepEqual(body.Spec.Resources, corev1.ResourceRequirements{}) {
187+
return fmt.Errorf("spec.resources is not allowed")
188+
}
189+
if body.Spec.Features != nil {
190+
return fmt.Errorf("spec.features is not allowed")
191+
}
192+
if body.Spec.SSH != nil {
193+
return fmt.Errorf("spec.ssh is not allowed")
194+
}
195+
if len(body.Spec.Ports) > 0 {
196+
return fmt.Errorf("spec.ports is not allowed")
197+
}
198+
if body.Spec.Ingress != nil {
199+
return fmt.Errorf("spec.ingress is not allowed")
200+
}
201+
return nil
202+
}
203+
151204
func provisionerSource(body *createRequest) string {
152205
source := strings.TrimSpace(body.Source)
153206
if source == "" {
@@ -182,7 +235,7 @@ func (p provisionerPolicy) validatePreset(presetID string) error {
182235
return fmt.Errorf("preset is not allowed: %s", presetID)
183236
}
184237

185-
func (s *server) validateProvisionerCreate(ctx context.Context, principal principal, namespace string, body *createRequest, userConfig json.RawMessage, requestedImage, requestedRepo, requestedNamespace bool, nameForFingerprint string) (string, error) {
238+
func (s *server) validateProvisionerCreate(ctx context.Context, principal principal, namespace string, body *createRequest, userConfig json.RawMessage, userConfigKeys map[string]json.RawMessage, requestedImage, requestedRepo, requestedNamespace bool, nameForFingerprint string) (string, error) {
186239
if !principalCanUseProvisionerFlow(principal) {
187240
return "", errForbidden
188241
}
@@ -192,6 +245,11 @@ func (s *server) validateProvisionerCreate(ctx context.Context, principal princi
192245
if err := authorizeServiceAction(principal, scopeInstancesAssignOwner, true); err != nil {
193246
return "", err
194247
}
248+
if principal.isService() {
249+
if err := validateProvisionerRequestSurface(body, userConfigKeys); err != nil {
250+
return "", err
251+
}
252+
}
195253
if requestedNamespace && !s.provisioners.allowNamespaceOverride {
196254
return "", fmt.Errorf("namespace override is not allowed")
197255
}

‎api/terminal.go‎

Lines changed: 42 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -27,11 +27,12 @@ import (
2727
)
2828

2929
type terminalConfig struct {
30-
enabled bool
31-
containerName string
32-
command []string
33-
allowedOrigins map[string]struct{}
34-
sessionMode terminalSessionMode
30+
enabled bool
31+
containerName string
32+
command []string
33+
allowedOrigins map[string]struct{}
34+
sessionMode terminalSessionMode
35+
activityDebounce time.Duration
3536
}
3637

3738
type terminalSessionMode string
@@ -43,11 +44,12 @@ const (
4344

4445
func newTerminalConfig() terminalConfig {
4546
return terminalConfig{
46-
enabled: parseBoolEnv("SPRITZ_TERMINAL_ENABLED", true),
47-
containerName: envOrDefault("SPRITZ_TERMINAL_CONTAINER", "spritz"),
48-
command: splitCommand(envOrDefault("SPRITZ_TERMINAL_COMMAND", "bash -l")),
49-
allowedOrigins: splitSet(os.Getenv("SPRITZ_TERMINAL_ORIGINS")),
50-
sessionMode: parseTerminalSessionMode(os.Getenv("SPRITZ_TERMINAL_SESSION_MODE")),
47+
enabled: parseBoolEnv("SPRITZ_TERMINAL_ENABLED", true),
48+
containerName: envOrDefault("SPRITZ_TERMINAL_CONTAINER", "spritz"),
49+
command: splitCommand(envOrDefault("SPRITZ_TERMINAL_COMMAND", "bash -l")),
50+
allowedOrigins: splitSet(os.Getenv("SPRITZ_TERMINAL_ORIGINS")),
51+
sessionMode: parseTerminalSessionMode(os.Getenv("SPRITZ_TERMINAL_SESSION_MODE")),
52+
activityDebounce: parseDurationEnv("SPRITZ_TERMINAL_ACTIVITY_DEBOUNCE", 5*time.Second),
5153
}
5254
}
5355

@@ -233,16 +235,17 @@ func (s *server) streamTerminal(ctx context.Context, namespace, name string, pod
233235
stdinReader, stdinWriter := io.Pipe()
234236
sizeQueue := newTerminalSizeQueue()
235237
wsWriter := &terminalWSWriter{conn: conn}
238+
reportActivity := debounceTerminalActivity(s.terminal.activityDebounce, func() {
239+
refreshCtx, cancel := context.WithTimeout(context.Background(), 2*time.Second)
240+
defer cancel()
241+
if err := s.markSpritzActivity(refreshCtx, namespace, name, time.Now()); err != nil {
242+
log.Printf("spritz terminal: failed to refresh activity name=%s namespace=%s pod=%s err=%v", name, namespace, pod.Name, err)
243+
}
244+
})
236245

237246
readErr := make(chan error, 1)
238247
go func() {
239-
readErr <- readTerminalInput(ctx, conn, stdinWriter, sizeQueue, func() {
240-
refreshCtx, cancel := context.WithTimeout(context.Background(), 2*time.Second)
241-
defer cancel()
242-
if err := s.markSpritzActivity(refreshCtx, namespace, name, time.Now()); err != nil {
243-
log.Printf("spritz terminal: failed to refresh activity name=%s namespace=%s pod=%s err=%v", name, namespace, pod.Name, err)
244-
}
245-
})
248+
readErr <- readTerminalInput(ctx, conn, stdinWriter, sizeQueue, reportActivity)
246249
}()
247250

248251
streamErr := executor.StreamWithContext(ctx, remotecommand.StreamOptions{
@@ -303,6 +306,28 @@ func readTerminalInput(ctx context.Context, conn *websocket.Conn, stdin *io.Pipe
303306
}
304307
}
305308

309+
func debounceTerminalActivity(interval time.Duration, report func()) func() {
310+
if report == nil {
311+
return func() {}
312+
}
313+
if interval <= 0 {
314+
return report
315+
}
316+
var mu sync.Mutex
317+
var last time.Time
318+
return func() {
319+
now := time.Now()
320+
mu.Lock()
321+
if !last.IsZero() && now.Sub(last) < interval {
322+
mu.Unlock()
323+
return
324+
}
325+
last = now
326+
mu.Unlock()
327+
report()
328+
}
329+
}
330+
306331
func handleResize(payload []byte, sizeQueue *terminalSizeQueue) bool {
307332
var msg resizeMessage
308333
if err := json.Unmarshal(payload, &msg); err != nil {

‎api/terminal_test.go‎

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -85,3 +85,23 @@ func TestReadTerminalInputInvokesActivityCallbackOnInput(t *testing.T) {
8585
t.Fatal("timed out waiting for terminal reader to exit")
8686
}
8787
}
88+
89+
func TestDebounceTerminalActivityCoalescesRapidInput(t *testing.T) {
90+
var callbacks atomic.Int32
91+
report := debounceTerminalActivity(50*time.Millisecond, func() {
92+
callbacks.Add(1)
93+
})
94+
95+
report()
96+
report()
97+
report()
98+
if callbacks.Load() != 1 {
99+
t.Fatalf("expected rapid calls to coalesce into one activity write, got %d", callbacks.Load())
100+
}
101+
102+
time.Sleep(60 * time.Millisecond)
103+
report()
104+
if callbacks.Load() != 2 {
105+
t.Fatalf("expected a second activity write after debounce window, got %d", callbacks.Load())
106+
}
107+
}

‎helm/spritz/templates/api-deployment.yaml‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -209,6 +209,10 @@ spec:
209209
- name: SPRITZ_TERMINAL_ORIGINS
210210
value: {{ join "," .Values.api.terminal.origins | quote }}
211211
{{- end }}
212+
{{- if .Values.api.terminal.activityDebounce }}
213+
- name: SPRITZ_TERMINAL_ACTIVITY_DEBOUNCE
214+
value: {{ .Values.api.terminal.activityDebounce | quote }}
215+
{{- end }}
212216
{{- end }}
213217
- name: SPRITZ_ACP_ENABLED
214218
value: {{ .Values.acp.enabled | quote }}

‎helm/spritz/values.yaml‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -162,7 +162,7 @@ api:
162162
- scope
163163
- scopes
164164
- scp
165-
defaultType: human
165+
defaultType: service
166166
provisioners:
167167
defaultPresetId: ""
168168
allowedPresetIds: []
@@ -196,6 +196,7 @@ api:
196196
container: spritz
197197
command: "bash -l"
198198
origins: []
199+
activityDebounce: 5s
199200
acp:
200201
origins: []
201202
sshGateway:

‎scripts/verify-helm.sh‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -85,8 +85,11 @@ expect_contains "${acp_network_policy_render}" "name: spritz-acp" "ACP network p
8585
expect_contains "${default_render}" 'resources: ["spritzes/status", "spritzconversations/status"]' "status RBAC for spritz conversations"
8686
expect_contains "${default_render}" "name: SPRITZ_AUTH_HEADER_TYPE" "principal type auth header wiring"
8787
expect_contains "${default_render}" "name: SPRITZ_AUTH_BEARER_SCOPES_PATHS" "bearer scope path wiring"
88+
expect_contains "${default_render}" "name: SPRITZ_AUTH_BEARER_DEFAULT_TYPE" "bearer default type wiring"
89+
expect_contains "${default_render}" "value: \"service\"" "service default bearer principal type in chart render"
8890
expect_contains "${default_render}" "name: SPRITZ_PROVISIONER_DEFAULT_IDLE_TTL" "default provisioner idle ttl wiring"
8991
expect_contains "${default_render}" "name: SPRITZ_PROVISIONER_DEFAULT_TTL" "default provisioner ttl wiring"
92+
expect_contains "${default_render}" "name: SPRITZ_TERMINAL_ACTIVITY_DEBOUNCE" "terminal activity debounce wiring"
9093
expect_contains "${default_render}" 'resources: ["configmaps"]' "configmap RBAC for idempotency reservations"
9194

9295
expect_failure \

0 commit comments

Comments
 (0)