Combined outcome of two SAST-driven hardening passes against v0.16.1 plus
a parallel-code-review enhancement plan that audited the first pass. Net
result: ~30 deepsec findings closed, six stdlib CVEs out of the binary, ten
new regression-guard tests pinning fixes that had been log-scrape-only,
and a maintainability counterweight pass that undid four micro-extractions
the lint chase created. No schema changes — existing MachineClass values
from v0.16.1 reconcile byte-identically.
Security
- GHA tag-name RCE on runner. Release workflow no longer interpolates
${{ github.ref }}into a shell command before signature verification. Attacker-controlled tag names can no longer execute on the runner. - ISO TOFU bypass on property-read error.
stepUploadISO's cache-hit path now treats a TOFU metadata read failure as cache-miss + redownload rather than silently accepting cached bytes. Regression test pins the new branch. - Probe Bearer-leak via redirect. New shared
internal/truenasrpcpackage stripsAuthorizationon 3xx that crosses host. Probe and production share the same redirect policy now; previously only production stripped. isLocalOmniEndpointuserinfo + subdomain bypass. Host-equality tightened: rejectsuser:pass@host,evil.localhost,localhost.evil.compatterns that previously short-circuited the loopback check on hostile DNS.- /healthz amplification. Reworked to a background-refresh model with a single in-flight check and rate-limited response. /healthz no longer fans out a TrueNAS RPC per request.
- ZFS passphrase trust-boundary doc.
SECURITY.mdnow spells out that the provider does not handle dataset passphrases; operators must use the TrueNAS keystore. docs.yamlworkflow least-privilege. Workflow-levelpermissions:block split into per-job blocks so thebuildjob only getscontents: readand onlydeploygetspages: write+id-token: write. A future workflow change touching thebuildsteps can no longer accidentally write Pages content.- Probe TLS verification on by default.
scripts/verify-api-key-rolesno longer hardcodesInsecureSkipVerify: true. Verification is enabled unless the operator opts out withTRUENAS_INSECURE_SKIP_VERIFYusing the samestrconv.ParseBoolparser the production provider has always used ("1","true","T", etc.). Both the WebSocket dialer and the/_uploadHTTP client pinMinVersion: TLS 1.2. ProbeapiKeyis wrapped inSecretStringand zeroed after auth so a post-auth core dump cannot exfiltrate the key. - Provider chart drops automounted SA token. Helm chart sets
automountServiceAccountToken: falseon the pod spec. The provider talks Omni gRPC + TrueNAS WebSocket and never reads the Kubernetes API — the default-mounted token had no legitimate consumer and only widened blast radius if the container were compromised.
Build
- Go toolchain 1.26.2 → 1.26.3. Closes six stdlib CVEs reachable from the binary:
GO-2026-4918(HTTP/2 SETTINGS_MAX_FRAME_SIZE infinite loop on outboundhttp.Client.Do— reachable from ISO upload path),GO-2026-4980(html/template escaper bypass),GO-2026-4977(mail.ParseAddressquadratic concat), plus three others.golang.org/x/{crypto,net,sys,text}indirects refreshed;netjumps v0.52.0 → v0.53.0.
Reliability
- Deprovision survives ctx cancellation without losing traces.
internal/provisioner/deprovision.gozvol-cleanup and force-stop paths usecontext.WithoutCancel(ctx)instead ofcontext.Background(). Cleanup still completes on shutdown but trace IDs and observability metadata propagate from the parent context. Same pattern applied to the autoscaler singleton-lease release. - Autoscaler
lease.Releaserespects a 5 s timeout. Matches the provisioner-side release path; pod shutdown can no longer hang on a slow Omni RPC. Trace IDs still propagate viacontext.WithoutCancel(baseCtx). fetchOwnedVM(nil, nil)sentinel replaced witherrVMNotFound. Returning(*VM, error)where(nil, nil)means "treat as success" was a foot-gun; callers now distinguish viaerrors.Is(err, errVMNotFound).- Test context leaks closed.
deprovision_test.goandinternal/autoscaler/server_test.godefer cancel()(ort.Cleanup(cancel)) immediately aftercontext.WithCancel, so a test that returns early no longer strands the cancel func until GC.
Observability
ISOHashMismatchesgains adetection_pathlabel so "factory rotated under the same URL" stays separate from "bytes tampered at rest" on the dashboard.- New TOFU-marker counters + alert rules.
truenas_iso_poison_marker_write_failed_total,truenas_iso_poison_marker_retries_total,truenas_iso_tofu_metadata_write_failed_total, plus three matching Prometheus alert rules.MANUAL CLEANUP REQUIREDno longer relies on log scraping. SingletonLeaseHeldgauge wired on the autoscaler. Previously only the provisioner side emitted it; autoscaler lease state is now visible on Grafana dashboards viatruenas_singleton_lease_held{scope="autoscaler-<cluster>"}.provider_idattribute on graceful-shutdown counters.GracefulShutdownSuccess/GracefulShutdownTimeoutcarryprovider_idso fleet dashboards can segment per provider.cleanupOrphanVMs::maybeDeleteOrphanVMrecordsspan.RecordErroron StopVM failure — was previously only emitted on DeleteVM failure, so half-completed orphan teardown was invisible in trace UI.setIfPoisonabletimer hygiene. Retries viatime.NewTimer+Reset(no leaked timers on cold ctx-cancel) and returns the persisted-bool so the outer error mentions when the marker write itself failed.
Hardening
internal/truenasrpcshared package closes the probe-vs-production drift flagged during review — the probe was missing the strict DNS allow-list and now inherits the production transport contract.verifyCachedISOextracted fromstepUploadISO. Cache-hit path drops from 80 lines of nesting to a 12-line dispatch, sheds a redundantfilesystem.statRPC per hit, and adds a TOCTOU re-stat immediately before CDROM attach so replication firing mid-provision cannot slip tampered bytes through. Legacy cache entries re-record metadata on observation so the next hit upgrades to the full TOFU triple without forcing a redownload.- /healthz checker rework. Background refresher with functional options (closes a test-only field-poke seam), every check wrapped in an OTel span,
truenas_healthcheck_errors_totalincrements on every failed check including WS-level and context-deadline failures (previously only inside specific checker subpaths, leaving those failure modes invisible to the existing alert). autoscalerlease + capacity gate API shape.acquireAutoscalerLeasereturns(release func(), error)with a sentinelerrAutoscalerLeaseShutdownDuringAcquireinstead of a tri-state return that the caller had to re-decode.buildAutoscalerCapacityGatereturns aCapacityGateBundlewith an unconditionalClose()(no-op when gate is disabled) so the caller candefer bundle.Close()without a nil-check dance.isoCacheRefstruct + constructor.newISOCacheRef(dataset, path, imageID)builds the canonical struct with all three TOFU property names derived fromimageID.verifyCachedISO,recordTOFUProperty,setIfPoisonable, andhandleISOHashMismatcheach take 4 args instead of 7–9. A future rename of a property prefix touches one line instead of twelve.validateDatasetPrefixSegmentsshared. BothValidate(root) andvalidateOneAdditionalDisk(per-disk) route through a single segment-splitter, parametrized on whether empty segments are rejected. The pre-existing rule-of-three drift (root rejected empty segments, per-disk silently skipped) is now explicit.pool.dataset.queryliteral de-duplication. ExtractedmethodPoolDatasetQuery+errPoolDatasetQueryFmtconstants; replaced 7 + 5 inline usages.- Helm chart ephemeral-storage. Provider values.yaml declares
requests.ephemeral-storage: 64Mi+limits.ephemeral-storage: 256Mi. Kubelet can size the pod's writable layer + log buffers explicitly instead of falling back to BestEffort eviction. Dockerfilebase image. Dropped the redundant:nonroottag from the FROM line — the digest already pins the image bytes immutably. Same digest, same uid override (65534).
Regression-guard tests
TestNodeGroupIncreaseSize_Gate{DeniedHard,Errored,SoftWarn,Allowed}_*— pin each capacity-gate Outcome arm to its gRPC status code + metric. Future edits that swapResourceExhausted↔Unavailable, drop arecordScaleUpResultincrement, or break soft-warn fall-through fail CI immediately.TestCleanupVM_DeleteRunsEvenAfterParentCancel— pins thecontext.WithoutCancelcontract by assertingDeleteVMis called on a non-cancelled context after the parent ctx is cancelled mid-deprovision. A revert tocleanupCtx := ctxwould silently break this and pass the existing "exits quickly" test.TestUploadFile_DataReaderErrorPropagates— pins that a failing source reader surfaces throughwriteUploadMultipart'spw.CloseWithErrorpath within a bounded deadline, rather than hanging on the pipe.TestMaybeDeleteOrphanVM_LegacyVMNoRequestID_Skipped+TestMaybeDeleteOrphanVM_RequestIDNotInLiveSet_Deleted— regression-pin the v0.15.3 mass-delete fix (VMs with no parseable request-id MUST NOT be deleted) and the counter-positive (real orphans MUST be deleted) so the v0.15.0 incident pattern can't recur.- Capacity-gate + lease helper smoke tests —
TestBuildAutoscalerCapacityGate_HostUnset_ReturnsNilQuery,TestAcquireAutoscalerLease_Disabled_ReturnsNoopRelease,TestAcquireProviderLease_Disabled_ReturnsNilRelease,TestErrAutoscalerLeaseShutdownDuringAcquire_IsDistinguishable— pin the disabled-branch and sentinel-identity contracts the runAutoscaler caller relies on. internal/client.NewMockClientCtx— new ctx-aware mock transport that exposesctx.Err()to test handlers. Required for theWithoutCancelcontract pin; reusable for future tests that need to inspect cancellation behavior at the call site.
Maintainability
- Large-function refactors.
stepCreateVM(provisioner core, previously cog-complexity 129) split intoattachAdditionalDisks,attachPrimaryNIC,attachAdditionalNICs,applyNICConfigPatches, andapplyAdvertisedSubnetsConfigPatch.stepUploadISO(cog 56) factored intodownloadOrReuseISO,ensureISODatasets,downloadAndUploadISO, andhandleISOHashMismatch.ensureZvol(cog 30) extracts the encrypted-existing recovery path intoensureEncryptedZvol.Data.Validate(cog 81) split into per-domain helpers.run()inmain.go(cog 45) extractsbuildLoggerandacquireProviderLease. ProvisionercleanupVM(cog 23) extractsfetchOwnedVMandgracefullyStopVM. No behavior change in any. - Maintainability counterweight. Four micro-extractions that the lint chase introduced were reverted after review:
recordGracefulOutcome(4-line two-arm metric — back inline with the addedprovider_idlabel),attachOneAdditionalNIC(split one for-loop iteration in half — back inline),runShutdownFuncs(one-call helper wrapped in a one-call closure — back inline), anddialCleartextFallback(conflated TLS-error formatting with cleartext-redial — split honestly into the redial +warnCleartextFallback). singleton.Clockinterface name preserved with a//nolint:reviveand rationale comment — matches theuber-go/clock/benbjohnson/clockGo-ecosystem convention; renaming to "Nower" would cascade ~40 callsites for a stylistic lint with no value to readers.data.gomap size hints.seenVolumeNamesandseenpre-sized to known upper bounds.autoscaler/metrics.go::containssimplified from a hand-rolled case-insensitive byte loop tostrings.Contains(ToLower(...), ToLower(...)). Allocates two strings per call; the comment now explicitly flags this as cold-path-only.- Shell scripts.
test-longhorn.sh'slog()/fail()switched from((PASS++))(which returns exit status 1 the first time the counter is zero, masking helper failure as success) toPASS=$((PASS+1)); return 0.install-longhorn.sh/annotate-machineclass-autoscale.shlogger helpers assign$1to alocal msg.
Operator note
The probe TRUENAS_INSECURE_SKIP_VERIFY env var now uses the same strconv.ParseBool parser the production provider has always used. Operators whose .env had =true previously saw the probe verify TLS while production silently skipped (false sense of safety); both now honor the setting consistently. The production provider's behavior has not changed — only the probe was brought into alignment. No action required unless your .env deliberately relied on the old parser difference.
Full Changelog: v0.16.2-rc.3...v0.16.2