javakeystore: pre-deploy export snapshot + on-import-failure rollback + argv-password operator note

Closes Bundle 8 of the 2026-05-02 deployment-target coverage audit
(see cowork/deployment-target-audit-2026-05-02/RESULTS.md). Pre-fix,
DeployCertificate at javakeystore.go:172-272 ran an irreversible
keytool -delete against the existing alias, then keytool
-importkeystore. If the import failed after the delete succeeded,
the keystore was missing the alias entirely — previous cert gone,
new cert never landed. docs/deployment-atomicity.md L94 promised
"keytool snapshot; rollback via keytool -delete + re-import"; the
code didn't deliver. Separately, the operator-facing keystore
password is passed via -storepass argv (a standard keytool
limitation) which is visible to ps(1) for the duration of each
subprocess; this was undocumented as an operator-playbook caveat.

This commit:

1. Pre-delete snapshot. When os.Stat(KeystorePath) succeeds,
   snapshotKeystore runs keytool -exportkeystore to
   <BackupDir>/.certctl-bak.<unix-nanos>.p12 BEFORE the existing
   -delete step. Backup path persisted in a local variable for
   the rollback path; export-step failure aborts the deploy
   entirely (no mutation has happened yet — the keystore is
   untouched). Snapshot skipped on first-time deploys (no
   keystore file = nothing to roll back to). The "alias not
   present in pre-existing keystore" case is recognised via the
   well-known keytool error string and treated as a clean
   first-time-on-existing-keystore signal — the deploy proceeds
   without a backup, and rollback (if needed) becomes the
   no-backup branch.

2. On-import-failure rollback. When keytool -importkeystore
   returns error, rollbackImport(ctx, backupPath) runs:
   - keytool -delete -alias <Alias> ... (best-effort; the failed
     import may have created a partial alias entry).
   - keytool -importkeystore from the backup PKCS#12 to restore
     the previous state.
   On rollback success, the deploy returns wrapped error noting
   "rolled back from <backup_path>". On rollback failure,
   returns operator-actionable wrapped error containing both the
   import error AND the rollback error AND the backup path so
   the operator can manually keytool -importkeystore from the
   .p12 file to recover.

3. Backup retention. Successful deploys prune older
   .certctl-bak.*.p12 files beyond Config.BackupRetention.
   Sort by ModTime newest-first; keep most recent N. Defaults:
   BackupRetention=0  → keep most recent 3 (the default).
   BackupRetention=N  → keep most recent N.
   BackupRetention=-1 → opt out of pruning entirely (operators
                        that wire their own archival/rotation).
   Pruning runs in the success path AFTER the optional reload
   command so it doesn't interfere with deploy-time signals.
   ReadDir / Remove failures are non-fatal (debug log only) —
   the deploy already succeeded.

4. Config gains BackupRetention int and BackupDir string fields.
   BackupDir defaults to filepath.Dir(KeystorePath) so backups
   land on the same filesystem as the keystore (atomic-ish
   writes, disk-full failures fail fast at snapshot time).

5. Helper extraction. snapshotKeystore + rollbackImport +
   pruneBackups + backupDir are private methods on Connector.
   Constants backupFilePrefix=".certctl-bak." and
   backupFileSuffix=".p12" centralise the naming convention so
   the snapshot writer, the rollback reader, and the retention
   pruner all agree.

6. Operator-playbook section added to docs/connectors.md
   JavaKeystore section. Documents the standard keytool
   -storepass argv exposure: ps(1)-visible for the duration
   of each subprocess. Lists mitigations:
   - Restrict shell access to the agent host.
   - Linux user namespaces / AppArmor / SystemD ProtectProc=
     invisible to deny ps-visibility.
   - Single-purpose container for proper PID-namespace
     isolation.
   - Post-deploy keystore password rotation via reload_command
     for high-security environments.
   - BCFKS keystore type for FIPS environments (same argv
     caveat applies).
   Also documents an "Atomic rollback" subsection covering the
   snapshot/rollback flow, the new backup_retention /
   backup_dir Config fields, and the design choice to reuse
   the keystore password for the snapshot (rather than
   generating a separate transient password) — operator
   already trusts the connector with this secret, surface area
   doesn't grow, rollback's matching -srcstorepass stays
   simple.

Tests added to javakeystore_test.go (7 new tests, ~430 LOC):

- TestJKS_Snapshot_RunsBefore_Delete: mock executor records call
  order; asserts -exportkeystore is call[0], -delete is call[1],
  -importkeystore is call[2]. The snapshot MUST run before the
  delete — otherwise the delete destroys the very state the
  snapshot is meant to capture.
- TestJKS_Snapshot_FirstTimeDeploy_NoExport: no keystore file
  pre-created; asserts exactly 1 keytool call (-importkeystore
  only), no -exportkeystore.
- TestJKS_ImportFails_RollsBack: happy rollback path with one
  same-Subject backup. Asserts rollback re-import references the
  same backup path the snapshot wrote (verified via arg
  comparison between call[0] and call[4]).
- TestJKS_ImportFails_RollbackAlsoFails_OperatorActionable:
  wrapped-error escalation with backup path in the error
  message.
- TestJKS_BackupRetention_PrunesOldBackups: 5 pre-existing
  staggered-ModTime backups + 1 deploy-created → retention=3 →
  exactly 3 newest survive (deploy-created + 2 newest
  pre-existing); 3 oldest pre-existing pruned.
- TestJKS_BackupRetention_Zero_DefaultsTo3: BackupRetention=0
  must default to 3 (not "keep none").
- TestJKS_BackupRetention_Negative_OptsOut: BackupRetention=-1
  pre-existing 5 + deploy 1 = 6 total, all 6 remain.
- TestJKS_Snapshot_AliasNotInKeystore_ProceedsCleanly: keystore
  exists but alias missing; -exportkeystore returns "alias does
  not exist" → snapshot helper recognises this signal and
  returns ("", nil) so the deploy proceeds cleanly.

mockExecutor extended with optional `onCall` hook so the
retention-pruning tests can simulate keytool -exportkeystore's
file-write side effect (via the simulateExportSideEffect helper
that parses -destkeystore from args and writes a placeholder
.p12 file). Existing tests that don't set onCall behave
identically to before — backward compatible.

docs/deployment-atomicity.md L94 unchanged from today's text —
Bundle 1 doc-realignment hasn't shipped, so the "keytool snapshot;
rollback via keytool -delete + re-import" line was never softened.
Post-Bundle-8 the claim is honest (was aspirational pre-fix).

Verified locally (sandbox lacks staticcheck install due to disk
pressure; CI runs the full lint gate):
- gofmt -l ./internal/connector/target/javakeystore/ clean
- go vet ./internal/connector/target/javakeystore/ clean
- go build ./cmd/agent/... clean
- go test -race -count=1 ./internal/connector/target/javakeystore/
  green (16 tests total: 9 pre-existing + 7 new)

Audit reference: cowork/deployment-target-audit-2026-05-02/RESULTS.md
Bundle 8.
This commit is contained in:
shankar0123
2026-05-02 19:01:06 +00:00
parent 1dd1dd4e0a
commit 87e0009d97
3 changed files with 887 additions and 2 deletions
+33
View File
@@ -1159,6 +1159,8 @@ The Java Keystore connector deploys certificates to JKS or PKCS#12 keystores via
| `reload_command` | string | | Optional command to run after keystore update |
| `create_keystore` | boolean | `true` | Create keystore if it doesn't exist |
| `keytool_path` | string | `"keytool"` | Override keytool binary path |
| `backup_retention` | int | `3` | Number of `.certctl-bak.<unix-nanos>.p12` snapshot files to keep after a successful deploy. `0` means use the default of 3; `-1` opts out of pruning entirely. |
| `backup_dir` | string | `dirname(keystore_path)` | Override directory where rollback snapshots are written and pruned from. Defaults to the keystore's own directory so snapshots land on the same filesystem. |
**Security:**
- Reload commands validated against shell injection via `validation.ValidateShellCommand()`
@@ -1166,6 +1168,37 @@ The Java Keystore connector deploys certificates to JKS or PKCS#12 keystores via
- Path traversal prevention on keystore path
- Transient PKCS#12 temp file cleaned up after import (even on error)
**Atomic rollback (Bundle 8 of the 2026-05-02 deployment-target audit):**
The deploy flow is **snapshot → delete → import → reload**. Before the irreversible `keytool -delete` step (which removes the existing alias from the keystore), the connector runs `keytool -exportkeystore` to write a sibling `.certctl-bak.<unix-nanos>.p12` file containing the prior alias. If the subsequent `keytool -importkeystore` fails for any reason, the rollback path runs `keytool -delete` (best-effort cleanup of any partial alias the failed import created) followed by `keytool -importkeystore` from the snapshot PFX, restoring the keystore to its pre-deploy state. If both the import AND the rollback fail, the connector returns an operator-actionable wrapped error containing both error strings AND the snapshot path so the operator can manually `keytool -importkeystore` from the `.p12` file to recover.
Successful deploys prune older `.certctl-bak.*.p12` files beyond the configured `backup_retention` count; pruning sorts by file ModTime and removes the oldest entries first. Operators that wire their own archival/rotation logic can opt out via `backup_retention: -1`.
First-time deploys (no keystore file exists at the configured path) skip the snapshot phase entirely — there's nothing to roll back to. The same is true for "alias-not-present-in-existing-keystore" deploys: `keytool -exportkeystore` returns "alias does not exist" which the connector recognises as a normal first-time-on-existing-keystore signal, not an outage.
### Operator playbook: keytool argv password exposure
Java's `keytool` accepts the keystore password via the `-storepass` argv flag — there is no stdin or file-based password mode in OpenJDK keytool. While the keytool subprocess is running, the password is visible in `ps(1)` output to any user on the same host who can read `/proc/<pid>/cmdline`. This is a **standard keytool limitation, not a certctl-specific issue**, but operators in regulated environments should know about it before deploying certctl on shared hosts.
**What this means in practice:**
- The password is visible for the duration of each keytool invocation (typically <1s on modern hardware; the connector runs 2-4 keytool calls per deploy: snapshot, optional pre-import delete, import, optional rollback).
- A local user with shell access on the agent host who polls `ps -ef` aggressively can capture the password.
- The exposure is local to the agent host; remote attackers without shell access cannot see it.
- The same applies to the snapshot's transient `-deststorepass` (which mirrors the operator's keystore password by design — see "Why the snapshot reuses the keystore password" below).
**Mitigations** (layer one or more depending on threat model):
- **Restrict shell access to the agent host.** Only the certctl agent's service account should have a login shell. Other admins SSH to a bastion that doesn't host the agent.
- **Use Linux user namespaces or AppArmor** to deny `ps`-visibility into the keytool subprocess for non-root users. SystemD's `ProtectKernelTunables=yes` + `ProtectProc=invisible` (kernel 5.8+) hides `/proc/<pid>` from non-owner users.
- **Run the certctl agent in a single-purpose container** so only the agent's processes are visible to anyone who execs into the container. The host's `ps` doesn't see container internals if proper PID-namespace isolation is configured.
- **Rotate the keystore password post-deployment.** For high-security environments where the brief exposure is unacceptable, the rotation can itself be automated via a post-deploy hook running `keytool -storepasswd`. The certctl `reload_command` is the natural place for this; just be aware the new password must be propagated to whatever service reads the keystore (Tomcat's `server.xml`, Kafka's `kafka.properties`, etc.).
- **For FIPS environments**, use the `BCFKS` (BouncyCastle FIPS) keystore type which supports stronger password-derivation. Same argv-exposure caveat applies; the keystore-format change doesn't affect how keytool receives the password.
For a fundamentally different password-handling model, switch to a non-Java target (e.g. PEM-on-disk via the SSH connector + a JCA-shim like `tomcat-native` reading PEMs directly) or a PKCS#11 keystore (where the password is supplied to the cryptoki library, not via argv).
**Why the snapshot reuses the keystore password.** The snapshot's `keytool -exportkeystore` writes a PKCS#12 file under a `-deststorepass`. The connector reuses the operator's `keystore_password` for this rather than generating a separate transient password. Two reasons: (a) the operator already trusts the connector with this secret, so the surface area doesn't grow; (b) the rollback's matching `keytool -importkeystore` needs to know the password too, and threading a second random password through the in-memory state machine adds complexity (and another argv-exposure window) for no security gain. If you rotate the keystore password between deploys, the rollback may fail to read the snapshot — keep stale `.certctl-bak.*.p12` files on disk until the rotation completes, and clean them up manually if rotation invalidates them.
Location: `internal/connector/target/javakeystore/javakeystore.go`
### Kubernetes Secrets