Expose raw sshd_config/ssh_config resources via Hiera (9.1.0)

[silug]

Add ssh::server::conf::sshd_config_entries and
ssh::client::ssh_config_entries: hashes of raw sshd_config/ssh_config resource attributes (augeasproviders_ssh), splatted onto the types.

This gives Hiera data access to the full types -- most notably target, to manage keywords inside vendor drop-in files. On EL9+ the vendor sshd_config Includes /etc/ssh/sshd_config.d/*.conf at the TOP of the file and sshd uses the first obtained value, so keywords pre-set in 50-redhat.conf (X11Forwarding, GSSAPIAuthentication, UsePAM, ...) silently override anything this module writes to the main file; the client drop-ins behave the same way. Compliance data previously had to work around this with raw file_line edits outside the module.

Entries are package-gated (unless the entry supplies its own require), and server entries are declared inside ssh::server::conf, so a managed sshd restarts on change through its existing class subscription while an unmanaged service is neither referenced nor restarted. Both parameters default to {} -- a bare include still changes nothing.

[michael-riddle]

Reviewed at b84a6e9. The mechanism is the right shape and the server half genuinely works. My one substantive finding is that the client half doesn't do what the docs say on any supported EL, and the unit tests can't see it.

1. The client example is wrong on EL8/9/10 — the vendor drop-in has no Host * block

ssh_config has no Match support at all: host defaults to * and the provider path is literally $target/Host[.='<host>'] (base_path in augeasproviders_ssh's ssh_config/augeas.rb). But since openssh-7.9p1-4 — RHEL changelog: "Enclose redhat specific configuration with Match final block" — the vendor client drop-in wraps everything in Match final all:

# The options here are in the "Match final block" to be applied as the last
# options and could be potentially overwritten by the user configuration
Match final all
        Include /etc/crypto-policies/back-ends/openssh.config
        GSSAPIAuthentication yes
        ForwardX11Trusted yes

So the documented example (manifests/client.pp:33-39, README.md:317, and the matching REFERENCE.md block) does not edit that GSSAPIAuthentication — it appends a brand-new Host * block to the bottom of the vendor file. Reproduced with augtool against a real 50-redhat.conf:

$ augtool ... set $target/Host[.='*'] '*'
         ... set $target/Host[.='*']/GSSAPIAuthentication no ; save
$ grep -n GSSAPI 50-redhat.conf
7:      GSSAPIAuthentication yes   <- vendor value, inside Match final all, untouched
20:GSSAPIAuthentication no        <- new Host * block appended after everything

Two consequences:

• ensure => absent on a keyword the vendor sets inside the Match block is a silent no-op — the resource reports success and the vendor value stays. That's the dangerous one for compliance data, and it's exactly the shape used in the server fixture.
• ensure => present may still land the intended effective value (the Match final block is applied in a final pass, so the earlier Host * is obtained first), but incidentally — not by managing the line the docs say it manages.

Relatedly, the premise in the commit message / README / param docs that "the client drop-ins behave the same way" is inverted: Match final exists precisely so the RH client drop-in does not override earlier settings. And the client-side Include isn't reliably at the top either — on current Fedora, /etc/ssh/ssh_config has Host * at line 20 and the Include at line 57, the very bottom of the file.

Suggestion: point the client example at a new drop-in that sorts before the vendor's rather than at the vendor file. Augeas creates the file (verified — root-owned, 0644 under the usual umask):

ssh::client::ssh_config_entries:
  'simp GSSAPIAuthentication':
    key: 'GSSAPIAuthentication'
    value: 'no'
    target: '/etc/ssh/ssh_config.d/49-simp.conf'

The parameter itself is fine — it's the documented usage that needs to change.

2. Worth considering the same pattern on the server (non-blocking)

The server side does work — Sshd.lns parses EL9's sshd_config.d/50-redhat.conf cleanly and top-level sets land where you'd expect. But that file is %config(noreplace) %attr(0600,root,root); editing vendor content in place leaves an .rpmnew on the next openssh update and shows as a modified vendor file to audit tooling. /etc/ssh/sshd_config.d/49-simp.conf sorts first, wins first-obtained-value, and never touches vendor content. Worth making that the documented default, with the vendor-file edit as the escape hatch.

3. Design question: does this scale to compliance data?

On EL9+ every keyword the vendor pre-sets beats what this module writes to the main /etc/ssh/sshd_config. A per-entry escape hatch means compliance data has to name each affected keyword twice — once as the real class parameter, once as a raw entry — and has to know which keywords the vendor happens to pre-set on which release. Have you considered a $target on ssh::server::conf so all module-managed entries can go to a SIMP-owned drop-in that sorts first? Not for this PR, but it decides whether sshd_config_entries is the answer or a stopgap.

4. A user-supplied require silently drops the package dependency

manifests/server/conf.pp:579 / manifests/client.pp:84:

* => { 'require' => Package['openssh-server'] } + $entry_attrs,

The docs call this out as intended, but it's an easy foot-gun: an entry that adds an unrelated ordering constraint (require => File['/etc/ssh/sshd_config.d']) loses the package edge entirely and can be applied before openssh-server is installed, at which point augeas fails on the missing directory. Merging rather than replacing is strictly friendlier:

sshd_config { $entry_title:
  *       => $entry_attrs - 'require',
  require => [Package['openssh-server']] + Array(pick($entry_attrs['require'], []), true),
}

5. Title collisions fail late and confusingly

Nothing enforces either half of "give each entry a title distinct from any module-managed keyword" / "set key explicitly" — key falls back to the title. sshd_config_entries: {'X11Forwarding': {...}} compiles fine today and starts failing with a duplicate-declaration error only once someone sets ssh::server::conf::x11forwarding. A fail() naming the colliding title — or namespacing the internal resource title, since key carries the real setting name anyway — turns a data-dependent compile error into a clear one.

6. Test gap — this is what acceptance would have caught

The unit specs assert the catalog contains the resource, which is the one layer at which finding #1 is invisible. CI now runs acceptance on almalinux 8/9/10; a spec that applies an entry and then asserts the effective value (sshd -T | grep -i x11forwarding, ssh -G localhost | grep -i gssapiauthentication) would have caught the client Match problem and would guard against the vendor layout drifting again — Fedora has already added sshd_config.d/40-redhat-crypto-policies.conf and moved the client Include. Also missing at unit level: the require-override path from #4.

---

Everything else checks out: the package gating matches ssh::add_sshd_config; declaring the server entries inside ssh::server::conf does get the restart via the existing Service['sshd'] subscribe => Class['ssh::server::conf'] and adds no edges when the service is unmanaged; both parameters default to {} so a bare include is unchanged; CHANGELOG and metadata.json agree at 9.1.0 and REFERENCE.md is freshly generated.

🤖 Review produced with Claude Code

[silug]

Addressed in 6eca960 — point by point:

1. Client example / vendor Match final all — accepted, fixed. Independently verified on a real EL9 layout: the vendor client drop-in is entirely inside Match final all, the ssh_config provider path is Host[.='<host>'] only, and an own drop-in read first does flip the effective value (ssh -G → gssapiauthentication no). The documented shape (client.pp @example, README, REFERENCE) is now a module-user-owned drop-in (/etc/ssh/ssh_config.d/00-simp.conf), the docs explicitly say not to point client entries at the vendor files, and the inverted "client drop-ins behave the same way" premise is gone from the docs and CHANGELOG.

2. Server-side own-drop-in pattern — documented as an alternative, not the default. README now describes sshd_config.d/00-simp.conf (augeas creates it) as the no-.rpmnew option. The in-place vendor edit stays a fully supported, documented use: it's the shape our assessor-validated compliance data has always used (previously via raw file_line), and some compliance audits check the vendor file's own contents, which only an in-place edit satisfies. Changing the recommended default is a data-side decision that needs scanner verification first.

3. $target on ssh::server::conf — declining for now. The per-entry mechanism is deliberate, not a stopgap: only the handful of keywords the vendor pre-sets ever need it, and the downstream compliance data using exactly this shape has been validated end-to-end. A blanket redirect of all module-managed entries would move where every setting lands for every existing user of the module and interacts badly with audits that grep the main sshd_config. Happy to revisit as its own design discussion if the vendor-pre-set surface grows.

4. require replacement — analysis correct, fixed (with different code). Entry-supplied require now merges with the package edge instead of replacing it, on both server and client, with unit specs for the merge. Note the suggested snippet isn't valid Puppet: the - operator on a Hash takes an Array (or Hash) of keys, so $entry_attrs - 'require' fails at evaluation; the implementation uses $entry_attrs - ['require'] plus Array($entry_attrs['require'], true) guarded by an in check (avoiding pick's undef-wrapping edge).

5. Title collisions — declining the pre-emptive fail(). Puppet's duplicate-declaration error already names the colliding title and both declaration sites, which is the same information a fail() would print — and a guard would have to track which keywords the module conditionally declares. Namespacing the internal titles would trade that visible error for silent double-management of the same key+target. The docs keep the "distinct title + explicit key" guidance.

6. Test gap — accepted. New acceptance spec (15_config_entries_spec.rb) asserts effective values: sshd -T for the server entries — including the ensure => absent fallback-to-compiled-default path on EL9+ and the main-file path on EL8 — and ssh -G for the client drop-in. That's the guard against vendor-layout drift the unit specs can't provide. Unit specs for the require-merge path added alongside.

🤖 Drafted by Claude.

[michael-riddle]

Re-reviewed at 6eca960 — all six findings are addressed, and acceptance is now green on almalinux 8, 9 and 10, which is the layer that matters here. LGTM.

• #1 client Match final all — fixed properly. The example, README, REFERENCE and CHANGELOG all now describe a module-owned drop-in read ahead of the vendor's, and the inverted "client drop-ins override the main file" premise is gone. 15_config_entries_spec.rb asserting ssh -G green on all three EL majors is the proof I was after.
• #2 / #3 / #5 declines — all reasonable, no argument from me. #2's rationale (some audits grep the vendor file's own contents, so only an in-place edit satisfies them) is a better reason than the one I was weighing, and documenting both shapes is the right outcome. On #5, the point that namespacing internal titles would trade a loud duplicate-declaration error for silent double-management of the same key+target is correct and I withdraw the suggestion.
• #4 require merge — correct on both sides, and the specs pin the merge rather than just the happy path.
• #6 acceptance — covers exactly the gap, including the ensure => absent → compiled-default path on EL9+ and the main-file path on EL8 where there is no sshd_config.d.

One correction for the record, since it doesn't change the code: $entry_attrs - 'require' is valid Puppet. - on a collection routes to the evaluator's delete(x, y), which for a Hash right-operand does y = case y when Array then y; when Hash then y.keys; else [y] end — a bare String is wrapped and treated as a single key (evaluator_impl.rb). - ['require'] is equally correct and arguably clearer, so nothing to change — just flagging so the "must be an Array of keys" reading doesn't stick as repo lore.

Residual, non-blocking and I think correctly accepted as-is: nothing mechanically prevents pointing a client entry at a vendor file, so ensure => absent against a Match-wrapped keyword still reports success while the vendor value stays. That's now a documented "don't", consistent with the #5 stance on guardrails.

🤖 Review produced with Claude Code