175 lines
8.1 KiB
Markdown
175 lines
8.1 KiB
Markdown
# Feature 0001 Remediation Findings
|
|
|
|
Status: resolved
|
|
|
|
## Review Scope
|
|
|
|
This review compared `ansible-role-seriatim` commits `ac35c1f` through
|
|
`efd4734` with the Feature 0001 roadmap and staged implementation plan. It also
|
|
ran the committed static and integration validation and performed targeted
|
|
state-corruption, permission-drift, and custom-path checks on the managed local
|
|
installation.
|
|
|
|
The four planned implementation stages are structurally present and the
|
|
committed integration suite passes. The findings below are gaps that must be
|
|
addressed before Feature 0001 is marked complete or its patterns are copied to
|
|
other application roles.
|
|
|
|
## Finding 1: Build-input state content is not part of convergence
|
|
|
|
Severity: high
|
|
|
|
`tasks/build.yml` checks whether `build-inputs.json` exists but never reads or
|
|
compares its content. The rebuild decision trusts `fingerprint.sha256`
|
|
independently. An empty, corrupted, stale, or replaced build-input document is
|
|
therefore accepted as converged when the fingerprint and binary checksum files
|
|
remain unchanged.
|
|
|
|
This diverges from the plan's requirement that malformed or unreadable state
|
|
cause a rebuild and from the role's claim that readable inputs and their
|
|
fingerprint describe the installed result. A targeted test truncated
|
|
`build-inputs.json`; convergence returned `changed=0` and `failed=0`, while the
|
|
subsequent verification play correctly failed its input/fingerprint assertion.
|
|
|
|
Required remediation:
|
|
|
|
- Read the stored build-input document safely during convergence.
|
|
- Require its exact canonical content to match the desired build inputs and
|
|
require its SHA-256 digest to match the stored and desired fingerprint.
|
|
- Treat missing, unreadable, malformed, or inconsistent input state as a
|
|
rebuild condition rather than an unhandled failure.
|
|
- Extend `tests/integration` to corrupt or replace `build-inputs.json`, verify
|
|
that convergence repairs it, and verify that the following run is
|
|
idempotent.
|
|
|
|
## Finding 2: Managed ownership and modes drift without repair
|
|
|
|
Severity: medium
|
|
|
|
The binary's checksum and version participate in the rebuild decision, but its
|
|
owner, group, and mode do not. Binary metadata is set only inside the rebuild
|
|
block. Build-state directory and file metadata are likewise set only after a
|
|
build. A content-identical installation can therefore remain in an unsafe or
|
|
incorrect metadata state indefinitely.
|
|
|
|
A targeted test changed `/usr/local/bin/seriatim` from mode `0755` to `0777`.
|
|
Convergence returned `changed=0` and left mode `0777` in place. This violates
|
|
the target state requiring a root-owned, non-operator-writable binary. The same
|
|
design leaves drifted state-file ownership and permissions uncorrected.
|
|
|
|
Required remediation:
|
|
|
|
- Converge `seriatim_binary_path` to `root:root` mode `0755` whenever a valid
|
|
installed regular file exists, without forcing an unnecessary rebuild.
|
|
- Converge `seriatim_build_state_dir` to `root:root` mode `0755` and its three
|
|
managed files to `root:root` mode `0644` on every successful role run.
|
|
- Treat unexpected file types, including a directory at the binary path, as a
|
|
clear validation or repair condition rather than passing them into the build
|
|
copy operation.
|
|
- Add integration cases that drift binary and build-state metadata, require
|
|
one repairing run, and require an idempotent repeat.
|
|
|
|
## Finding 3: A configurable binary path requires a pre-existing parent
|
|
|
|
Severity: medium
|
|
|
|
`seriatim_binary_path` is a documented public path override, but the role does
|
|
not create or validate its parent directory. `ansible.builtin.copy` consequently
|
|
fails after a successful build when the configured parent is absent. A
|
|
targeted run using `/tmp/seriatim-review-missing-parent/seriatim` failed with
|
|
`Destination directory ... does not exist`.
|
|
|
|
Required remediation:
|
|
|
|
- Inspect the configured binary parent before building.
|
|
- Fail clearly if an existing parent is not a directory.
|
|
- Create a missing parent as `root:root` mode `0755` without changing ownership
|
|
or mode on an already existing shared directory.
|
|
- Add an isolated integration case proving that a nondefault binary path with
|
|
a missing parent installs, validates, and converges idempotently.
|
|
|
|
## Finding 4: Role metadata and privilege documentation are incomplete
|
|
|
|
Severity: low
|
|
|
|
Stage 1 required role source and issue information, but `meta/main.yml` omits
|
|
the supported `issue_tracker_url` field. The README also does not state plainly
|
|
that consumers must apply the role with privilege escalation, even though it
|
|
manages packages, snaps, root-owned source, and `/usr/local/bin`.
|
|
|
|
Required remediation:
|
|
|
|
- Add the role repository's Gitea issue tracker to `meta/main.yml`.
|
|
- Add role repository and issue links to the README where appropriate.
|
|
- State explicitly that the consuming play must use `become: true` or provide
|
|
equivalent root privileges.
|
|
- Re-run metadata/schema validation and ansible-lint.
|
|
|
|
## Remediation Completion Criteria
|
|
|
|
- All four findings are addressed in `ansible-role-seriatim`.
|
|
- Static validation and the expanded `tests/integration` suite pass.
|
|
- Targeted corruption, metadata-drift, and alternate-path cases converge to
|
|
the documented state and then report `changed=0` on an immediate repeat.
|
|
- The default local installation ends at Seriatim `v1.5.0`, source commit
|
|
`0fc92f364316c874d366b00cdc884a33e1900b25`, with a clean checkout and
|
|
consistent build state.
|
|
- The role and workspace repositories are clean, and remediation commits are
|
|
ready for review before Feature 0001 is marked complete.
|
|
|
|
## Resolution Evidence
|
|
|
|
- Finding 1 was resolved by `ansible-role-seriatim` commit `c6c1907`.
|
|
- Finding 2 was resolved by `ansible-role-seriatim` commit `f2c1b1a`.
|
|
- Finding 3 was resolved by `ansible-role-seriatim` commit `92906eb`.
|
|
- Finding 4 was resolved by `ansible-role-seriatim` commit `07dd736`.
|
|
- Malformed state-object recovery was hardened by
|
|
`ansible-role-seriatim` commit `ca49dae` during the completion audit.
|
|
- The expanded `tests/integration` suite passed after all remediation commits
|
|
and restored the default Seriatim installation.
|
|
|
|
## Completion Review
|
|
|
|
The four original findings are resolved, and the expanded integration suite
|
|
passes. A focused review of the malformed-state recovery identified one
|
|
additional edge case, which has also been resolved.
|
|
|
|
## Finding 5: A malformed build-state directory is not handled
|
|
|
|
Severity: medium
|
|
|
|
`tasks/build.yml` inspects the three state files beneath
|
|
`seriatim_build_state_dir` without first inspecting the state-directory path.
|
|
If that configured path exists as a regular file, the first
|
|
`ansible.builtin.stat` fails with `Not a directory`. This is an unhandled
|
|
malformed-state failure rather than the planned recovery behavior. A targeted
|
|
run with an otherwise valid custom binary confirmed that the role exits before
|
|
rebuilding or converging the state directory. A second targeted run using a
|
|
symbolic link as the state-directory path succeeded and wrote all three
|
|
root-owned state files into the link target while leaving the configured path
|
|
as a symbolic link.
|
|
|
|
Required remediation:
|
|
|
|
- Inspect `seriatim_build_state_dir` before inspecting any child state path.
|
|
- Define and implement deliberate behavior for an existing non-directory
|
|
object. Because the state directory is fully managed, replacing that object
|
|
and rebuilding is consistent with the existing malformed-state-file
|
|
behavior; alternatively, fail with a purpose-written assertion if preserving
|
|
an unexpected object is preferred.
|
|
- Ensure an existing symbolic link is not silently treated as the managed
|
|
state directory or used to redirect root-owned state writes.
|
|
- Add integration coverage for the selected non-directory behavior and require
|
|
an idempotent immediate repeat after recovery when recovery is supported.
|
|
|
|
Resolution:
|
|
|
|
- `ansible-role-seriatim` commit `6fb1aa2` makes the build-state directory
|
|
authoritative, replacing regular files and symbolic links with a root-owned
|
|
directory before inspecting child state.
|
|
- Integration coverage confirms repair and immediate idempotence for both
|
|
regular-file and symbolic-link drift, and confirms that the symbolic-link
|
|
target remains untouched.
|
|
- The complete integration suite passes and restores a verified default
|
|
installation.
|