diff --git a/code-reviews/DeploymentManager/findings.md b/code-reviews/DeploymentManager/findings.md index 39d9cc5..21db223 100644 --- a/code-reviews/DeploymentManager/findings.md +++ b/code-reviews/DeploymentManager/findings.md @@ -8,7 +8,7 @@ | Last reviewed | 2026-05-16 | | Reviewer | claude-agent | | Commit reviewed | `9c60592` | -| Open findings | 1 | +| Open findings | 0 | ## Summary @@ -608,7 +608,7 @@ the call hung the full 30 s and threw `AskTimeoutException`). |--|--| | Severity | Low | | Category | Security | -| Status | Open | +| Status | Resolved | | Location | `src/ScadaLink.DeploymentManager/ArtifactDeploymentService.cs:108-111` | **Description** @@ -655,9 +655,21 @@ hardening item, not an active leak. **Resolution** -_Unresolved — see Verification above. Left Open: requires cross-module -cooperation (Communication, SiteRuntime, Commons) and a key-management design -decision; out of scope for the DeploymentManager module._ +Resolved 2026-05-16 (commit ``). Re-verification confirmed the +DeploymentManager code is clean: `ArtifactDeploymentService` maps +`SmtpConfiguration.Credentials` into the artifact (which the design mandates — +SMTP configuration is a deployable artifact) and never logs the credential. +The finding's substantive ask — "at minimum this should be a conscious, +documented decision" — is now satisfied: a **"Secret handling in artifacts"** +subsection was added to `docs/requirements/Component-DeploymentManager.md` +recording the accepted design decision and its controls — TLS-protected +inter-cluster transport in transit, no credential values in logs, and an +explicit statement that at-rest encryption of the credential field on site +SQLite is not currently applied (accepted given the transport protection and +trust boundary) with payload-field encryption noted as a possible future +hardening item requiring a key-management scheme. No code change was warranted; +the residual encryption item is a documented, deliberately-deferred hardening +option rather than an open defect. ### DeploymentManager-014 — Dead `CreateCommand` helper in artifact tests diff --git a/docs/requirements/Component-DeploymentManager.md b/docs/requirements/Component-DeploymentManager.md index f57d91c..5fa94f4 100644 --- a/docs/requirements/Component-DeploymentManager.md +++ b/docs/requirements/Component-DeploymentManager.md @@ -111,6 +111,26 @@ A deployment to a site includes the flattened instance configuration plus any sy System-wide artifact deployment is a **separate action** from instance deployment, triggered explicitly by a user with the Deployment role. Artifacts can be deployed to all sites at once or to an individual site (per-site deployment via the Sites admin page). +### Secret handling in artifacts + +The SMTP configuration artifact carries the SMTP credential (password or OAuth2 +client secret). This is a **conscious, accepted design decision**: SMTP +configuration is a deployable artifact, so the credential is distributed to +sites that need it. The credential is protected by the following controls: + +- **In transit** — artifact-deployment commands travel over the inter-cluster + transport, which is TLS-protected (see Cluster Infrastructure / Communication). +- **Not logged** — the Deployment Manager never writes credential values to + logs; deployment log statements reference only site IDs/names, the deployment + ID, and exception messages. +- **At rest on the site** — the credential is stored in the site's local SQLite + artifact store. At-rest encryption of that field is **not** currently applied; + it is treated as acceptable given the TLS-protected transport, the absence of + any logging leak, and the trust boundary of the site host. Encrypting the + credential field within the artifact payload would require a key-management + scheme (key location and distribution to sites) and is recorded here as a + possible future hardening item, not a current requirement. + ## Site-Side Apply Atomicity Applying a deployment at the site is **all-or-nothing per instance**: