feat: security hardening — TLS, token blacklist, field encryption, sandbox, secret rotation by intel352 · Pull Request #210 · GoCodeAlone/workflow

intel352 · 2026-02-28T09:48:17Z

Summary

Comprehensive security hardening across 5 areas:

TLS everywhere: Shared pkg/tlsutil package + TLS support on HTTP (autocert/manual/mTLS), Redis, Kafka (SASL+TLS), NATS, and Database transports
Token blacklist: auth.token-blacklist module (memory/redis) + step.token_revoke with JTI tracking in JWTAuthModule
Field-level data protection: pkg/fieldcrypt with AES-256-GCM, HKDF tenant-isolated KeyRing, versioned keys, security.field-protection module, step.field_reencrypt for key rotation
Docker sandbox hardening: seccomp profiles, cap-drop, read-only rootfs, no-new-privileges, PID limits, Wolfi base images, step.sandbox_exec with strict/standard/permissive profiles
Secret rotation: RotationProvider interface, Vault KV v2 implementation, step.secret_rotate

New modules

auth.token-blacklist
security.field-protection

New pipeline steps

step.token_revoke
step.field_reencrypt
step.sandbox_exec
step.secret_rotate

New packages

pkg/tlsutil — shared TLS config builder
pkg/fieldcrypt — field-level encryption, masking, key ring

Test plan

pkg/tlsutil — 10 tests (config building, cert loading, autocert)
module/tls_config_test.go — 8 tests (transport-specific TLS parsing)
Token blacklist — 14 tests (memory backend, revoke step, JWT integration)
pkg/fieldcrypt — 14 tests (encrypt/decrypt, masking, key ring, scanner)
Field protection module — 4 tests (module init, config, step)
Sandbox exec — 27 tests (security profiles, Docker config mapping)
Secret rotation — 15 tests (mock Vault HTTP server, step execution)
Full go test ./... — all passing

🤖 Generated with Claude Code

…on, sandbox, secret rotation Phase 1: TLS support for all transports - pkg/tlsutil: shared TLS config (manual, autocert, mTLS) - HTTP server: Let's Encrypt autocert + manual TLS + mTLS - Redis: TLS with client cert support - Kafka: SASL (PLAIN/SCRAM-SHA-256/512) + TLS - NATS: TLS via nats.Secure() - Database: explicit TLS fields (sslmode, ca_file) Phase 2: Token blacklist - auth.token-blacklist module (memory + redis backends) - step.token_revoke pipeline step - JTI generation + blacklist check in JWTAuthModule Phase 3: Field-level data protection - pkg/fieldcrypt: AES-256-GCM encryption, masking, HKDF key derivation - Tenant-isolated KeyRing with versioned keys - ProtectedFieldManager module (security.field-protection) - step.field_reencrypt for key rotation re-encryption - Backward compat: legacy enc:: prefix handled alongside new epf:v{n}: format Phase 4: Docker sandbox hardening - seccomp profiles, capability dropping, read-only rootfs, no-new-privileges - step.sandbox_exec with strict/standard/permissive security profiles - Default secure config with Wolfi base image (cgr.dev/chainguard/wolfi-base) Phase 5: Secret rotation - RotationProvider interface in secrets package - Vault provider Rotate() + GetPrevious() via versioned KV v2 - step.secret_rotate pipeline step Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

Copilot

Pull request overview

This PR introduces a broad security-hardening layer across the workflow engine, adding shared TLS configuration utilities, JWT revocation via token blacklisting, field-level encryption/masking with key rotation primitives, hardened Docker sandbox execution, and secret rotation support (Vault KV v2).

Changes:

Add shared TLS config builder (pkg/tlsutil) and wire TLS into HTTP server, Redis cache, Kafka broker (incl. SASL/SCRAM), NATS broker, and database DSN handling.
Add token revocation: auth.token-blacklist module, JWT jti issuance + blacklist enforcement, and step.token_revoke.
Add field-level protection (pkg/fieldcrypt, security.field-protection module, step.field_reencrypt), hardened sandbox execution (step.sandbox_exec + docker hardening), and Vault secret rotation primitives + step.secret_rotate.

Reviewed changes

Copilot reviewed 38 out of 39 changed files in this pull request and generated 8 comments.

Show a summary per file

File	Description
`secrets/vault_provider.go`	Add Vault KV v2 secret rotation and previous-version retrieval helpers.
`secrets/secrets.go`	Introduce `RotationProvider` interface extending `Provider`.
`secrets/rotation_test.go`	Add tests for Vault rotation and previous-version semantics.
`sandbox/docker.go`	Add sandbox hardening fields and apply them to Docker host/container config; add hardened default config helper.
`sandbox/docker_test.go`	Add tests covering new Docker host config security fields and hardened defaults.
`plugins/secrets/plugin.go`	Register `step.secret_rotate` step type in the secrets plugin.
`plugins/pipelinesteps/plugin.go`	Register new pipeline steps: token revoke, field re-encrypt, sandbox exec.
`plugins/pipelinesteps/plugin_test.go`	Update expected step registry list for newly added step types.
`plugins/auth/plugin.go`	Register new module types and add wiring hook for JWT blacklist integration.
`plugins/auth/plugin_test.go`	Update manifest/module/wiring hook expectations to match new auth/security additions.
`pkg/tlsutil/tlsutil.go`	New shared TLS config loader for transport modules.
`pkg/tlsutil/tlsutil_test.go`	Tests for TLS config loading, cert/CA handling, and client auth parsing.
`pkg/fieldcrypt/fieldcrypt.go`	Define protected-field schema types (classification, log behavior, etc.).
`pkg/fieldcrypt/encrypt.go`	Implement AES-GCM encryption/decryption with versioned prefixes and legacy support.
`pkg/fieldcrypt/keyring.go`	Add HKDF-based, tenant-isolated, versioned keyring implementation.
`pkg/fieldcrypt/mask.go`	Add masking/redaction/hashing utilities for protected fields in logs.
`pkg/fieldcrypt/scanner.go`	Implement recursive scan/encrypt/decrypt/mask helpers over nested data structures.
`pkg/fieldcrypt/fieldcrypt_test.go`	Add tests for encryption/decryption, masking, scanning, and keyring behaviors.
`module/tls_config_test.go`	Add tests ensuring TLS config fields are present/usable across transports.
`module/cache_redis.go`	Add Redis TLS support via `tlsutil.LoadTLSConfig`.
`module/kafka_scram.go`	Add SCRAM client implementation to support SASL/SCRAM mechanisms.
`module/kafka_broker.go`	Add TLS + SASL configuration plumbing for Kafka connections.
`module/nats_broker.go`	Add TLS configuration support for NATS connection.
`module/database.go`	Add TLS config and DSN augmentation for Postgres-style URL DSNs.
`module/http_server.go`	Add manual TLS + autocert support for HTTP server module.
`module/jwt_auth.go`	Add `jti` issuance and blacklist check during Authenticate.
`module/auth_token_blacklist.go`	Add token blacklist module with memory/redis backends and cleanup loop.
`module/auth_token_blacklist_test.go`	Tests for token blacklist module behaviors and lifecycle.
`module/pipeline_step_token_revoke.go`	Add step to revoke JWTs by extracting `jti`/`exp` and adding to blacklist.
`module/pipeline_step_token_revoke_test.go`	Tests for token revoke step and JWT integration with blacklist.
`module/field_protection.go`	Add `security.field-protection` module providing a protected-field manager service.
`module/field_protection_test.go`	Tests for field protection module config, encrypt/decrypt, masking, and services.
`module/pipeline_step_field_reencrypt.go`	Add step to decrypt+re-encrypt data using the latest key version.
`module/pipeline_step_sandbox_exec.go`	Add hardened Docker sandbox execution step with security profiles and limits.
`module/pipeline_step_sandbox_exec_test.go`	Tests for sandbox exec step config parsing, defaults, and helpers.
`module/pipeline_step_secret_rotate.go`	Add step to rotate secrets via a `RotationProvider`.
`module/pipeline_step_secret_rotate_test.go`	Tests for secret rotation step validation and execution paths.
`go.mod`	Add indirect deps needed for SCRAM/SASL support.
`go.sum`	Update checksums for newly introduced dependencies.

Copilot · 2026-02-28T09:54:14Z

module/field_protection.go

+	// Resolve master key.
+	masterKeyStr, _ := cfg["master_key"].(string)
+	if masterKeyStr == "" {
+		masterKeyStr = os.Getenv("FIELD_ENCRYPTION_KEY")
+	}
+
+	var masterKey []byte
+	if masterKeyStr != "" {
+		masterKey = []byte(masterKeyStr)
+	} else {
+		log.Println("WARNING: field-protection module using zero key — set master_key or FIELD_ENCRYPTION_KEY")
+		masterKey = make([]byte, 32)
+	}


Field protection falls back to an all-zero master key when neither master_key nor FIELD_ENCRYPTION_KEY is set. This makes encryption deterministic/guessable and defeats the purpose of the module. Please fail fast (return an error) when the master key is missing/too short, and avoid printing secrets-related warnings via the global log package (prefer returning an error or using the app logger during Init).

Copilot · 2026-02-28T09:54:15Z

pkg/fieldcrypt/scanner.go

+		case []any:
+			for _, elem := range val {
+				if m, ok := elem.(map[string]any); ok {
+					if err := scanEncrypt(m, registry, keyFn, depth+1, maxDepth); err != nil {
+						return err
+					}
+				}
+			}


scan_arrays is parsed and stored on ProtectedFieldManager, but the scanner always traverses []any values. This makes the scan_arrays setting ineffective. Consider threading a scanArrays flag into ScanAndEncrypt/ScanAndDecrypt (and ScanAndMask if applicable) and skipping slice traversal when disabled.

Copilot · 2026-02-28T09:54:15Z

module/http_server.go

+	manualCfg := s.tlsCfg.Manual
+	manualCfg.Enabled = true
+
+	// Overlay mTLS settings from the top-level fields when set
+	if s.tlsCfg.ClientCAFile != "" {
+		manualCfg.CAFile = s.tlsCfg.ClientCAFile
+	}
+	if s.tlsCfg.ClientAuth != "" {
+		manualCfg.ClientAuth = s.tlsCfg.ClientAuth
+	}
+
+	tlsConfig, err := tlsutil.LoadTLSConfig(manualCfg)
+	if err != nil {
+		return fmt.Errorf("http server TLS config: %w", err)
+	}
+	s.server.TLSConfig = tlsConfig
+
 	go func() {
-		if err := s.server.ListenAndServe(); err != nil && !errors.Is(err, http.ErrServerClosed) {
-			s.logger.Error("HTTP server error", "error", err)
+		if err := s.server.ListenAndServeTLS(manualCfg.CertFile, manualCfg.KeyFile); err != nil && !errors.Is(err, http.ErrServerClosed) {
+			s.logger.Error("HTTPS server error", "error", err)


In manual TLS mode, Start returns nil even if no certificate/key are configured. tlsutil.LoadTLSConfig doesn’t error when CertFile/KeyFile are empty, and the goroutine running ListenAndServeTLS can fail immediately, leaving the engine thinking the server started successfully. Please validate that a cert/key (or TLSConfig.Certificates) is present for mode: manual and return an error before spawning the goroutine.

Copilot · 2026-02-28T09:54:15Z

module/http_server.go

+	go func() {
+		httpSrv := &http.Server{
+			Addr:              ":80",
+			Handler:           m.HTTPHandler(nil),
+			ReadHeaderTimeout: 10 * time.Second,
+		}
+		if err := httpSrv.ListenAndServe(); err != nil && !errors.Is(err, http.ErrServerClosed) {
+			s.logger.Error("autocert HTTP-01 listener error", "error", err)
+		}
+	}()
+
+	go func() {


startAutocert starts an additional HTTP-01 challenge server on ":80" but doesn’t keep a reference to it, so Stop() can’t shut it down. This can leak a listener and cause port conflicts on restart/tests. Consider storing the HTTP challenge *http.Server on StandardHTTPServer and shutting it down in Stop (or tie it to ctx cancellation).

Suggested change

go func() {

httpSrv := &http.Server{

Addr: ":80",

Handler: m.HTTPHandler(nil),

ReadHeaderTimeout: 10 * time.Second,

}

if err := httpSrv.ListenAndServe(); err != nil && !errors.Is(err, http.ErrServerClosed) {

s.logger.Error("autocert HTTP-01 listener error", "error", err)

}

}()

go func() {

httpSrv := &http.Server{

Addr: ":80",

Handler: m.HTTPHandler(nil),

ReadHeaderTimeout: 10 * time.Second,

}

// Start the ACME HTTP-01 challenge server

go func() {

if err := httpSrv.ListenAndServe(); err != nil && !errors.Is(err, http.ErrServerClosed) {

s.logger.Error("autocert HTTP-01 listener error", "error", err)

}

}()

// Shut down the ACME server when the context is canceled

go func() {

<-ctx.Done()

shutdownCtx, cancel := context.WithTimeout(context.Background(), 5*time.Second)

defer cancel()

if err := httpSrv.Shutdown(shutdownCtx); err != nil && !errors.Is(err, http.ErrServerClosed) {

s.logger.Error("error shutting down autocert HTTP-01 listener", "error", err)

}

}()

go func() {

Copilot · 2026-02-28T09:54:15Z

module/pipeline_step_secret_rotate.go

+	if _, err := rp.Rotate(ctx, s.key); err != nil {
+		return nil, fmt.Errorf("secret_rotate step %q: rotate failed: %w", s.name, err)
+	}
+
+	return &StepResult{Output: map[string]any{
+		"rotated":  true,
+		"key":      s.key,
+		"provider": s.provider,
+	}}, nil


SecretRotateStep’s comment says it “returns the new value”, but Execute discards the value returned by RotationProvider.Rotate and does not include it in step output. Also notify_module is parsed and stored but never used. Either return the new value (and implement notification) or update the step contract/config to match the actual behavior to avoid misleading pipeline authors.

Copilot · 2026-02-28T09:54:16Z

module/pipeline_step_sandbox_exec.go

+			// allowed — step may be used without a command for future use
+		default:
+			return nil, fmt.Errorf("sandbox_exec step %q: 'command' must be a list of strings", name)
+		}


The factory allows command to be omitted (case nil), but Execute calls DockerSandbox.Exec which hard-errors when len(cmd)==0. If command-less steps aren’t supported yet, make command required at factory time; otherwise, handle the no-command case explicitly in Execute (e.g., return a clear error/output).

Suggested change

// allowed — step may be used without a command for future use

default:

return nil, fmt.Errorf("sandbox_exec step %q: 'command' must be a list of strings", name)

}

return nil, fmt.Errorf("sandbox_exec step %q: 'command' is required and must be a non-empty list of strings", name)

default:

return nil, fmt.Errorf("sandbox_exec step %q: 'command' must be a list of strings", name)

}

if len(step.command) == 0 {

return nil, fmt.Errorf("sandbox_exec step %q: 'command' must be a non-empty list of strings", name)

}

Copilot · 2026-02-28T09:54:16Z

module/pipeline_step_sandbox_exec.go

+		if cpu, ok := cfg["cpu_limit"].(float64); ok {
+			step.cpuLimit = cpu
+		}


cpu_limit parsing only accepts float64. YAML unmarshalling into map[string]any often produces int for whole numbers, so configs like cpu_limit: 1 will be ignored. Consider accepting int, int64, and float64 (and possibly string) to make this robust.

Copilot · 2026-02-28T09:54:16Z

module/auth_token_blacklist.go

+func (m *TokenBlacklistModule) Add(jti string, expiresAt time.Time) {
+	if m.backend == "redis" && m.redisClient != nil {
+		ttl := time.Until(expiresAt)
+		if ttl <= 0 {
+			return // already expired, nothing to blacklist
+		}
+		_ = m.redisClient.Set(context.Background(), m.redisKey(jti), "1", ttl).Err()
+		return
+	}


Redis backend operations ignore errors (e.g., Set in Add). If Redis is unavailable, tokens may not actually be blacklisted and there’s no signal to the operator. At minimum, log errors via the module logger; ideally consider whether the interface should allow propagating errors for security-critical revocation paths.

- Fix nilerr: use separate parseErr variable in token revoke step - Fix staticcheck: remove redundant embedded field selector in SCRAM client - Remove unused alwaysErrorApp type and fmt import in test - Update example/go.sum with xdg-go/scram dependency Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

- Require master_key (error instead of zero-key fallback) - Handle error in field-protection module factory - Add field-protection-wiring hook: connects ProtectedFieldManager to KafkaBroker - KafkaBroker.SetFieldProtection() for field-level encrypt/decrypt in JSON payloads - Add Registry.Len() method - Add TestFieldProtectionRequiresMasterKey test - Update wiring hook count in auth plugin tests Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

…fecycle Replace stub CopyIn/CopyOut methods with real implementations that use the Docker API. Added CreateContainer() to create a container and store its ID, and RemoveContainer() to clean up. CopyIn delegates to the existing copyToContainer helper; CopyOut uses client.CopyFromContainer. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

Copilot

Pull request overview

Copilot reviewed 39 out of 41 changed files in this pull request and generated 8 comments.

Copilot · 2026-02-28T10:27:50Z

module/http_server.go

+	// ACME HTTP-01 challenge listener on :80
+	go func() {
+		httpSrv := &http.Server{
+			Addr:              ":80",
+			Handler:           m.HTTPHandler(nil),
+			ReadHeaderTimeout: 10 * time.Second,
+		}
+		if err := httpSrv.ListenAndServe(); err != nil && !errors.Is(err, http.ErrServerClosed) {
+			s.logger.Error("autocert HTTP-01 listener error", "error", err)
+		}
+	}()


In autocert mode, the HTTP-01 challenge listener (:80) server is created inside a goroutine and is not stored or shut down in Stop(). This can leak a listener and prevent clean restarts/shutdowns. Consider keeping a reference to the challenge *http.Server and shutting it down when the module stops / ctx is canceled.

Copilot · 2026-02-28T10:27:50Z

module/kafka_broker.go

+							payload = out
+						}
+					}


On consume, field-level decryption failures are silently ignored (decErr != nil results in leaving the payload encrypted with no log/metric). This makes debugging and incident response hard and can break downstream handlers unexpectedly. Consider logging and choosing an explicit failure policy (e.g., drop/mark message, or pass through only when decryption succeeds).

Suggested change

payload = out

}

}

payload = out

} else {

h.broker.logger.Error("Error marshaling Kafka message after field-level decryption", "topic", msg.Topic, "error", err)

}

} else {

h.broker.logger.Error("Error decrypting Kafka message fields", "topic", msg.Topic, "error", decErr)

}

} else {

h.broker.logger.Error("Error unmarshaling Kafka message for field-level decryption", "topic", msg.Topic, "error", err)

Copilot · 2026-02-28T10:27:50Z

plugins/auth/plugin.go

 					{Name: "user-management", Role: "provider", Priority: 10},
 				},
-				WiringHooks: []string{"auth-provider-wiring", "oauth2-jwt-wiring"},
+				WiringHooks: []string{"auth-provider-wiring", "oauth2-jwt-wiring", "token-blacklist-wiring"},
 			},


Plugin manifest WiringHooks list is missing "field-protection-wiring" even though WiringHooks() actually provides it and the plugin tests expect 4 hooks. Add the hook name to Manifest.WiringHooks so plugin metadata matches runtime behavior.

Copilot · 2026-02-28T10:27:51Z

module/pipeline_step_sandbox_exec.go

+			// allowed — step may be used without a command for future use
+		default:
+			return nil, fmt.Errorf("sandbox_exec step %q: 'command' must be a list of strings", name)
+		}
+


The factory explicitly allows 'command' to be omitted (cfg["command"] == nil), but Execute always calls DockerSandbox.Exec which hard-fails when len(cmd)==0. Either make 'command' required at factory time, or add a clear Execute-time validation/error (and update tests/docs accordingly).

Suggested change

// allowed — step may be used without a command for future use

default:

return nil, fmt.Errorf("sandbox_exec step %q: 'command' must be a list of strings", name)

}

// handled by validation below

default:

return nil, fmt.Errorf("sandbox_exec step %q: 'command' must be a list of strings", name)

}

if len(step.command) == 0 {

return nil, fmt.Errorf("sandbox_exec step %q: 'command' is required and must contain at least one string", name)

}

Copilot · 2026-02-28T10:27:51Z

module/pipeline_step_secret_rotate.go

+// SecretRotateStep rotates a secret in a RotationProvider and returns the new value.
+type SecretRotateStep struct {
+	name         string
+	provider     string // service name of the secrets RotationProvider module
+	key          string // the secret key to rotate
+	notifyModule string // optional module name to notify of rotation
+	app          modular.Application
+}


SecretRotateStep's docstring says it "returns the new value", and the config accepts notify_module, but Execute discards the value returned by RotationProvider.Rotate and notifyModule is never used. Either update the comment/config to reflect intended behavior, or include the rotated value (and/or notification) behind an explicit, security-conscious option.

Copilot · 2026-02-28T10:27:51Z

module/field_protection.go

+// EncryptMap encrypts protected fields in the data map in-place.
+func (m *ProtectedFieldManager) EncryptMap(ctx context.Context, tenantID string, data map[string]any) error {
+	tid := m.resolveTenant(tenantID)
+	return fieldcrypt.ScanAndEncrypt(data, m.Registry, func() ([]byte, int, error) {
+		return m.KeyRing.CurrentKey(ctx, tid)
+	}, m.ScanDepth)
+}
+
+// DecryptMap decrypts protected fields in the data map in-place.
+// Version 0 is the legacy enc:: format and uses the raw master key.
+func (m *ProtectedFieldManager) DecryptMap(ctx context.Context, tenantID string, data map[string]any) error {
+	tid := m.resolveTenant(tenantID)
+	return fieldcrypt.ScanAndDecrypt(data, m.Registry, func(version int) ([]byte, error) {
+		if version == 0 {
+			// Legacy enc:: values were encrypted with sha256(masterKey).
+			// encrypt.go's decryptLegacy calls keyFn(0) expecting the raw key
+			// bytes, then SHA256-hashes them before use.
+			return m.rawMasterKey, nil
+		}
+		return m.KeyRing.KeyByVersion(ctx, tid, version)
+	}, m.ScanDepth)
+}


ScanArrays is parsed from config and stored on ProtectedFieldManager, but EncryptMap/DecryptMap always call fieldcrypt.ScanAndEncrypt/ScanAndDecrypt which currently recurse into []any unconditionally. As a result, scan_arrays has no effect; either plumb ScanArrays into the scanner logic or remove the option to avoid misleading configuration.

Copilot · 2026-02-28T10:27:51Z

module/kafka_broker.go

+	// Field-level encryption: encrypt individual protected fields in JSON payloads.
+	if fieldProt != nil {
+		var data map[string]any
+		if err := json.Unmarshal(payload, &data); err == nil {
+			if encErr := fieldProt.EncryptMap(context.Background(), "", data); encErr != nil {
+				return fmt.Errorf("failed to field-encrypt kafka message for topic %q: %w", topic, encErr)
+			}


Kafka field-level encryption/decryption always passes an empty tenantID (""), which means enabling tenant isolation in ProtectedFieldManager will still route everything through the default tenant. If tenant isolation is a supported mode, this should derive tenantID from message metadata/config, or explicitly document that Kafka integration is single-tenant only.

Copilot · 2026-02-28T10:27:51Z

secrets/vault_provider.go

+// Rotate generates a new random 32-byte hex-encoded secret and stores it at the given key,
+// creating a new version in Vault KV v2. It returns the newly generated value.
+func (p *VaultProvider) Rotate(ctx context.Context, key string) (string, error) {
+	if key == "" {
+		return "", ErrInvalidKey
+	}
+
+	raw := make([]byte, 32)
+	if _, err := rand.Read(raw); err != nil {
+		return "", fmt.Errorf("secrets: failed to generate random secret: %w", err)
+	}
+	newValue := hex.EncodeToString(raw)
+
+	path, _ := parseVaultKey(key)
+	kv := p.client.KVv2(p.config.MountPath)
+	if _, err := kv.Put(ctx, path, map[string]interface{}{
+		"value": newValue,
+	}); err != nil {


Rotate() accepts keys in the same "path#field" format as Get(), but it ignores the field component (parseVaultKey(key) assigns it to _), always writing to {"value": ...} at the base path. That makes "#field" rotations no-ops/surprising for callers. Either reject keys containing "#" for Rotate (and document it) or support rotating the specified field.

Return parseErr when JWT parsing fails and fmt.Errorf for invalid claims type, instead of swallowing the error. Fixes nilerr lint violation. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

The manifest WiringHooks list was missing the field-protection-wiring entry, and TestModuleFactories needed FIELD_ENCRYPTION_KEY set for the security.field-protection factory to succeed. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

Copilot

Pull request overview

Copilot reviewed 39 out of 41 changed files in this pull request and generated 11 comments.

Copilot · 2026-02-28T10:46:17Z

module/pipeline_step_sandbox_exec.go

+		}
+
+		if cpu, ok := cfg["cpu_limit"].(float64); ok {
+			step.cpuLimit = cpu


cpu_limit parsing only accepts float64. YAML unmarshalling into map[string]any often produces int for whole numbers (e.g., cpu_limit: 1), which would be silently ignored. Consider accepting int/int64 as well and converting to float64 so configs behave as expected.

Suggested change

step.cpuLimit = cpu

step.cpuLimit = cpu

} else if cpuInt, ok := cfg["cpu_limit"].(int); ok {

step.cpuLimit = float64(cpuInt)

} else if cpuInt64, ok := cfg["cpu_limit"].(int64); ok {

step.cpuLimit = float64(cpuInt64)

Copilot · 2026-02-28T10:46:17Z

sandbox/docker.go

+// CopyOut copies a file out of the active container. Returns a ReadCloser with the file contents.
+// Call CreateContainer first to set the active container ID.
+func (s *DockerSandbox) CopyOut(ctx context.Context, srcPath string) (io.ReadCloser, error) {


CopyOut returns the raw stream from CopyFromContainer, which is a tar archive rather than direct file contents. Either clarify this in the comment/API, or untar internally and return the extracted file contents.

Copilot · 2026-02-28T10:46:17Z

module/field_protection.go

+func (m *ProtectedFieldManager) EncryptMap(ctx context.Context, tenantID string, data map[string]any) error {
+	tid := m.resolveTenant(tenantID)
+	return fieldcrypt.ScanAndEncrypt(data, m.Registry, func() ([]byte, int, error) {
+		return m.KeyRing.CurrentKey(ctx, tid)
+	}, m.ScanDepth)


ScanArrays is part of ProtectedFieldManager config, but EncryptMap/DecryptMap always call the fieldcrypt scanners without any way to disable array traversal. As a result, scan_arrays=false is ignored. Wire ScanArrays into the scanning implementation (or remove the option).

Copilot · 2026-02-28T10:46:18Z

module/http_server.go

+	default:
+		// Plain HTTP
+		go func() {
+			if err := s.server.ListenAndServe(); err != nil && !errors.Is(err, http.ErrServerClosed) {
+				s.logger.Error("HTTP server error", "error", err)


The default branch serves plain HTTP for any unrecognized tlsCfg.Mode. That means a typo/misconfig can silently downgrade to HTTP, which is risky for a security-hardening change. Consider explicitly handling intended values (e.g., "disabled"/empty) and returning an error for unknown modes.

Copilot · 2026-02-28T10:46:18Z

module/http_server.go

+			Addr:              ":80",
+			Handler:           m.HTTPHandler(nil),
+			ReadHeaderTimeout: 10 * time.Second,
+		}


The autocert HTTP-01 listener server is created as a local variable (httpSrv) and never stored on StandardHTTPServer, so Stop() can’t shut it down. This can leak the :80 listener and block restarts. Keep a reference and shut it down in Stop (or tie it to ctx cancellation).

Suggested change

}

}

// Shut down the HTTP-01 listener when the context is cancelled.

go func() {

<-ctx.Done()

shutdownCtx, cancel := context.WithTimeout(context.Background(), 5*time.Second)

defer cancel()

if err := httpSrv.Shutdown(shutdownCtx); err != nil && !errors.Is(err, http.ErrServerClosed) {

s.logger.Error("autocert HTTP-01 listener shutdown error", "error", err)

}

}()

Copilot · 2026-02-28T10:46:18Z

module/pipeline_step_secret_rotate.go

+type SecretRotateStep struct {
+	name         string
+	provider     string // service name of the secrets RotationProvider module
+	key          string // the secret key to rotate
+	notifyModule string // optional module name to notify of rotation
+	app          modular.Application


The step struct accepts notify_module but it’s never used anywhere in Execute. Either implement the notification behavior (or remove the option) so configs don’t contain silently ignored fields.

Copilot · 2026-02-28T10:46:18Z

module/pipeline_step_secret_rotate.go

+	if _, err := rp.Rotate(ctx, s.key); err != nil {
+		return nil, fmt.Errorf("secret_rotate step %q: rotate failed: %w", s.name, err)
+	}
+
+	return &StepResult{Output: map[string]any{


Rotate returns the new secret value, but Execute discards it and the step output does not include it. Either include the new value in output (ideally behind an explicit opt-in to avoid leaking secrets) or update the doc comment to avoid claiming it returns the new value.

Copilot · 2026-02-28T10:46:19Z

module/kafka_broker.go

+// SetTLSConfig sets the TLS and SASL configuration for the Kafka broker.
+// SetFieldProtection sets the field-level encryption manager for this broker.
+// When set, individual protected fields are encrypted/decrypted in JSON payloads
+// before the legacy whole-message encryptor runs.
+func (b *KafkaBroker) SetFieldProtection(mgr *ProtectedFieldManager) {


This doc comment mentions SetTLSConfig but is attached to SetFieldProtection, and it doesn’t document the actual method. Consider splitting/rewording so SetFieldProtection and SetTLSConfig each have accurate godoc comments.

Copilot · 2026-02-28T10:46:19Z

plugins/auth/plugin.go

+			mod, err := module.NewFieldProtectionModule(name, cfg)
+			if err != nil {
+				log.Printf("ERROR: field-protection module %q: %v", name, err)
+				return nil
+			}


This factory logs the constructor error and returns nil. BuildFromConfig treats a nil module as a hard error, so the user will lose the real cause (they’ll only see “factory ... returned nil”). Prefer returning a non-nil module that carries the init error (e.g., stored initErr returned from Init/Start) and use the engine/app logger instead of log.Printf.

Copilot · 2026-02-28T10:46:19Z

module/http_server.go

 	go func() {
-		if err := s.server.ListenAndServe(); err != nil && !errors.Is(err, http.ErrServerClosed) {
-			s.logger.Error("HTTP server error", "error", err)
+		if err := s.server.ListenAndServeTLS(manualCfg.CertFile, manualCfg.KeyFile); err != nil && !errors.Is(err, http.ErrServerClosed) {
+			s.logger.Error("HTTPS server error", "error", err)


ListenAndServeTLS is called with manualCfg.CertFile/manualCfg.KeyFile, but there’s no validation that those are set (LoadTLSConfig does not require them). Add a fast-fail check for non-empty cert/key paths (and optionally readability) when Mode=="manual".

Copilot AI review requested due to automatic review settings February 28, 2026 09:48

Copilot started reviewing on behalf of intel352 February 28, 2026 09:48 View session

Copilot AI reviewed Feb 28, 2026

View reviewed changes

intel352 and others added 2 commits February 28, 2026 05:10

Copilot AI review requested due to automatic review settings February 28, 2026 10:22

Copilot started reviewing on behalf of intel352 February 28, 2026 10:22 View session

Copilot AI reviewed Feb 28, 2026

View reviewed changes

intel352 and others added 2 commits February 28, 2026 05:31

fix: return errors from token revoke step for invalid tokens

ba249f7

Return parseErr when JWT parsing fails and fmt.Errorf for invalid claims type, instead of swallowing the error. Fixes nilerr lint violation. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

Copilot AI review requested due to automatic review settings February 28, 2026 10:40

Copilot started reviewing on behalf of intel352 February 28, 2026 10:41 View session

Copilot AI reviewed Feb 28, 2026

View reviewed changes

intel352 merged commit f678468 into main Feb 28, 2026
18 checks passed

intel352 deleted the feat/security-hardening branch February 28, 2026 10:49

-			step.cpuLimit = cpu
+			step.cpuLimit = cpu
+		} else if cpuInt, ok := cfg["cpu_limit"].(int); ok {
+			step.cpuLimit = float64(cpuInt)
+		} else if cpuInt64, ok := cfg["cpu_limit"].(int64); ok {
+			step.cpuLimit = float64(cpuInt64)

-		}
+		}
+		// Shut down the HTTP-01 listener when the context is cancelled.
+		go func() {
+			<-ctx.Done()
+			shutdownCtx, cancel := context.WithTimeout(context.Background(), 5*time.Second)
+			defer cancel()
+			if err := httpSrv.Shutdown(shutdownCtx); err != nil && !errors.Is(err, http.ErrServerClosed) {
+				s.logger.Error("autocert HTTP-01 listener shutdown error", "error", err)
+			}
+		}()

Conversation

intel352 commented Feb 28, 2026

Summary

New modules

New pipeline steps

New packages

Test plan

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Feb 28, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 28, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 28, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 28, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 28, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 28, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 28, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 28, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Feb 28, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 28, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 28, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 28, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 28, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 28, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 28, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 28, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Feb 28, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 28, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 28, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 28, 2026

Choose a reason for hiding this comment