Skip to content

Fix/admin managed apache paths - #5530

Draft
tpurschke wants to merge 4 commits into
CactuseSecurity:developfrom
tpurschke:fix/admin-managed-apache-paths
Draft

tpurschke wants to merge 4 commits into
CactuseSecurity:developfrom
tpurschke:fix/admin-managed-apache-paths

Conversation

@tpurschke

@tpurschke tpurschke commented Oct 19, 2026 •

Copy link
Copy Markdown
Contributor
  • Administrator-managed Apache certificate is now applied: with internalca_issue_apache_certificate: false, the pair named in fwo-install-settings.yml (internalca_apache_certificate / internalca_apache_private_key) is now used on every FWO vhost. Before, ownership detection overwrote it with the installed vhost paths on the run that switched away from the FWO-issued certificate (include_vars is outranked by set_fact).
  • New select-operator-apache-identity.yml: records the configured pair before anything rewrites the paths. Only a path that differs from the FWO location counts as a configured pair. Naming the FWO location, or no pair at all, still keeps whatever the installed vhost serves.
  • New internalca_fwo_apache_certificate / internalca_fwo_apache_private_key: the FWO issue location, kept separate from the operator-facing variables so the two can be compared.
  • Intermediates on fresh installs: a pair configured this way is served with internalca_peer_ca_intermediate_certificates on new installations too. The upgrade path takes the chain only from that setting, not from the vhost being replaced.
  • Preflight checks: the run stops early if the configured certificate or key file is missing. A symlink target is followed, so a dangling certbot live/ link is caught.
  • Chain validation for Apache: every retained or administrator-managed Apache certificate must now chain to the FWO trust bundle, including the separately served intermediates. validate-retained-chain.yml gains optional retained_chain_intermediates and retained_chain_consequence inputs. A missing intermediate or the wrong root (e.g. a Let's Encrypt ECDSA leaf under ISRG Root YE while only X1 is configured) now stops the run before the database is upgraded, instead of failing later in the integration tests.
  • Error message: the trust-bundle read failure now includes the reason the file could not be read.
  • Integration tests:
    • New fixture cases for recognising a configured pair. Covered: a pair named with issuance switched off, the FWO location named with issuance switched off, and a pair named while FWO issues the certificate itself.
    • New detect-pinned-variables.yml detects inputs that the settings file pins as extra variables, which happens when the installer is launched via run-playbook-with-sudo.sh. Fixture cases that would need to override such inputs are skipped with a message instead of failing; this fixes the integration test on hosts like fwodemo.
  • Docs: documentation/certificates.md gains a section on configuring an administrator-managed certificate (Let's Encrypt/certbot example, multi-root bundle, renewal and reload). The settings template and the revision history (9.7.7) are updated.

@tpurschke tpurschke self-assigned this Oct 19, 2026
@tpurschke

Copy link
Copy Markdown
Contributor Author

FWO PR Review — #5530 Fix/admin managed apache paths

Round 1 of max. 5 · Reviewer: Claude Opus 5.5 (claude-opus-5-5[1m], 1M context), run via Claude Code with the fwo-review-pr skill; reasoning level: session default (the effort setting cannot be read from inside the session)

Findings

# Criticality Confidence Status Subject
F1 🟠 medium 🟢 high 🔴 new Operator-configured Apache pair skips key, passphrase and endpoint-name checks the docs promise
F4 🟠 medium 🟢 high 🔴 new No fixture for the new retained_chain_intermediates path or for detection of the operator pair
F2 🟠 medium 🟡 medium 🔴 new Installed SSLCertificateChainFile is dropped even when the named pair is the one already served
F3 🟡 low 🟢 high 🔴 new Chain failure calls a newly configured pair "the customer managed certificate kept on" the host

Recommendations

Fix in this PR (introduced by this diff, small, and within the PR's purpose):

  • F1: either run the key-match, unencrypted-key and endpoint-name checks for internalca_operator_apache_identity on both install modes, or narrow the claim in documentation/certificates.md to what is actually enforced.
  • F2: only discard the installed chain file when the operator pair differs from the pair the installed vhost names.
  • F3: reword the failure message when the certificate was configured, not retained.
  • F4: add a validate-retained-chain.yml case with a separate intermediates file (accepted with it, refused without it).

Create follow-up issue:

  • F4 (rest): "tests-integration: cover detect-existing-certificates with an operator-configured Apache pair".

Details

F1 — Operator-configured Apache pair skips key, passphrase and endpoint-name checks the docs promise (medium, confidence high)

documentation/certificates.md:207-214 says that a pair named in the settings file "gets the same checks as a retained certificate: both files must exist, the root must be configured, and the certificate must cover every FWO endpoint name". The code enforces only part of this:

  • New installation: detect-existing-certificates.yml runs only when installation_mode == 'upgrade' (roles/internalCA/tasks/main.yml:107-109). The endpoint-name check (validate-retained-certificate.yml) and the explicit-root guard therefore never run for an operator pair on a fresh install. The chain check in main.yml:119-146 does run, so the root is covered indirectly, but the host names are not. A certificate that does not cover api_uri's host passes the whole internalCA play. This is the late "Hostname mismatch" failure that validate-retained-certificate.yml was written to prevent.
  • Upgrade, pair not preserved: the name check and the root guard are gated on internalca_preserve_apache_certificate (detect-existing-certificates.yml:206-248). That flag is false when the key does not match the certificate or cannot be parsed. Both guards that would catch those cases are skipped whenever internalca_issue_apache_certificate is false (detect-existing-certificates.yml:111-124 and :144-166). use-installed-apache-identity.yml then still points every vhost at the pair.
  • Encrypted key: the passphrase preflight in roles/common/tasks/apache/validate-unencrypted-tls-key.yml:16-25 only looks at the key the installed vhost names and at the FWO default. On the run that switches to an operator pair, the new key is in neither place.

In all three cases the run passes the any-errors-fatal internalCA play, the database play migrates the schema, and Apache then fails its restart in roles/api / roles/middleware / roles/ui. The new existence assert in main.yml:95-105 stops exactly this "fails after the vhosts were rewritten" pattern for a missing file, but not for a mismatched or encrypted key or a wrong name. Suggested fix: for internalca_operator_apache_identity, read the certificate and key info and run validate-retained-certificate.yml plus a fingerprint comparison and an openssl pkey -passin pass: check, independent of installation_mode and of internalca_preserve_apache_certificate. The alternative is to reduce the documentation to what is actually enforced.

F4 — No fixture for the new retained_chain_intermediates path or for detection of the operator pair (medium, confidence high)

The new fixtures in test-certificate-ownership.yml only test select-operator-apache-identity.yml, i.e. which settings count as an operator pair. Two parts of the change have no tests:

  • validate-retained-chain.yml:53-62 now builds argv as a Jinja list with an optional second -untrusted file. test-ldap-client-tls.yml already drives this file with generated certificates, but always without intermediates. One case with the leaf plus a separate intermediates file (expected: passes) and one without the file (expected: fails with the anchor verdict) would cover the new branch and retained_chain_consequence.
  • No test covers detect-existing-certificates.yml:30-55 with the operator branch. Here the inspected certificate is the operator pair, the installed chain file is blanked, and Retain / use-installed-apache-identity.yml keep the operator paths. This is the actual fix described in the revision history.

Note that the fixture block is skipped whenever the settings pin any of the Apache inputs, which is the case on a Let's Encrypt host like fwodemo. The installations this PR targets will therefore never run these cases. That is unavoidable, but it makes CI coverage the only coverage.

F2 — Installed SSLCertificateChainFile is dropped even when the named pair is the one already served (medium, confidence medium)

detect-existing-certificates.yml:48-55 sets internalca_existing_peer_ca_intermediate_certificates to '' whenever internalca_operator_apache_identity is true. The reason given is that "the installed SSLCertificateChainFile belongs to the certificate being replaced". That only holds when the configured pair differs from the pair the installed vhost names. The new template text (fwo-install-settings.template.yml:74-78) tells administrators to name their pair explicitly. An installation that already serves its own pair, with the chain only in the vhost's SSLCertificateChainFile (the input the Retain step at :191-196 explicitly falls back to), now loses that chain as soon as the administrator follows this advice. The new chain check stops the upgrade early with a usable remedy, so nothing breaks silently. But an upgrade that worked before now stops, for a pair that has not changed. Suggested fix: blank the installed chain only when internalca_operator_apache_certificate differs from the SSLCertificateFile parsed from the vhost.

F3 — Chain failure calls a newly configured pair "the customer managed certificate kept on" the host (low, confidence high)

main.yml:119-146 now also sends an operator-configured pair, including on a new installation, through validate-retained-chain.yml. Its failure message (validate-retained-chain.yml:162-167) still reads "The customer managed Apache certificate kept on <host>". On a fresh install, or on the run that switches to the new pair, nothing is being kept. The verdict and remedy are still correct, so this is wording only. A retained_chain_subject_phrase-style input, or neutral wording ("The Apache certificate configured on …"), would fix it.

Checks with no findings

  • Security pass: no TLS verification was weakened; the new chain check tightens it. openssl is called with an argv list (no shell) on root-configured paths. No secrets are added to source, logs or messages; the stat/assert tasks print paths only. No auth, Hasura, tenant, injection or HTML-output surface is touched.
  • Upgrade safety: no schema or migration change. The new chain gate stops early on purpose; the internalCA play is any_errors_fatal and runs before the database play (site.yml:108-122). This matches the revision-history entry. internalca_apache_certificate keeps its effective default through the new internalca_fwo_apache_* variables.
  • FWO-specific checks: no UI, localization, help or Python importer code changed. No C# changed, so the inline-array and CODING_GUIDELINES.md checks do not apply. whats_new_facts is not required for an installer fix. revision-history.md is updated under 9.7.7, which matches product_version.
  • Mechanics: all changed YAML files parse. CI passed, including both test installs and the integration tests.

Review metadata

  • Depth: deep (installer, TLS trust and upgrade path), performed inline against the skill's correctness and security checklists. No built-in review command was used.
  • Delegation: none; all scans and judgements ran on the primary model.
  • Inline comments: none were posted, because the repository's review authority covers one top-level comment only. Details are therefore in this comment.
  • Token cost: about 110k new tokens (input + output + cache creation), with about 1.8M cache-read tokens counted separately and excluded. The ceiling is 1M for deep. Measured from the session transcript on disk (Measuring it, method 2), deduplicated by message id. No sub-agents were used.
  • Coverage: all 13 changed files were read in full, together with the direct context (detect-existing-certificates.yml, validate-retained-chain.yml, use-installed-apache-identity.yml, validate-apache-intermediates.yml, reconcile-maintenance-certificate.yml, roles/common Apache preflights, site.yml, the Apache vhost templates).

@sonarqubecloud

Copy link
Copy Markdown

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant