Skip to content

Add PuTTY MSI install and per-user session logging scripts#78

Merged
Gumbees merged 3 commits into
mainfrom
enhancement/putty-install-and-session-logging
May 12, 2026
Merged

Add PuTTY MSI install and per-user session logging scripts#78
Gumbees merged 3 commits into
mainfrom
enhancement/putty-install-and-session-logging

Conversation

@mnelsondtc

Copy link
Copy Markdown
Contributor

Summary

Two new scripts under app-putty/ to standardize PuTTY deployment and engineer session capture:

  • putty-install.ps1 — silent MSI install of PuTTY 64-bit from the official the.earth.li "latest" symlink. Idempotent (skips if C:\Program Files\PuTTY\putty.exe is present unless $ForceReinstall=1). Captures the full MSI verbose log to $env:WINDIR\logs\putty-install-msi.log. Verifies the binary exists post-install before exiting 0. Falls back from BITS to Invoke-WebRequest if BITS is unavailable.

  • putty-configure-logging.ps1 — runs in user context (refuses to run as SYSTEM). Writes the five HKCU:\Software\SimonTatham\PuTTY\Sessions\Default%20Settings values that enable session logging:

    • LogType = 2 (all session output)
    • LogFileName = G:\Shared drives\Engineer Session Logs\<engineer>\&h-&Y&M&D-&T.log
    • LogFileClash = 1 (always append)
    • LogFlush = 1
    • LogHeader = 1

    Creates the engineer's folder under the log root if the drive is mounted; warns and continues otherwise so the registry config is in place when the drive comes online. RMM-overridable: $LogRoot, $EngineerName, $LogFilePattern, $LogType, $LogFileClash.

Test plan

  • putty-install.ps1 ran elevated, installed PuTTY 0.83 to C:\Program Files\PuTTY\, exited 0
  • putty-install.ps1 second run detected existing install and skipped (idempotent path)
  • putty-configure-logging.ps1 ran in user context, wrote all 5 registry values with correct types (DWord/REG_SZ), read-back verification passed
  • Test override ($LogRoot = 'C:\') confirmed working — folder C:\<username>\ created, registry values pointed at test path
  • Diffed against a .reg export of PuTTY's own manual Save of Default Settings — script writes byte-identical values to the same key with the same types
  • Final manual verification: closed PuTTY, re-opened, navigated to Session -> Logging, confirmed correct values displayed in the GUI

Notes for reviewers

  • PuTTY's hostname token is documented as &H (uppercase) in the manual, but the requested filename pattern uses &h (lowercase). PuTTY 0.83 appears to accept both; if a deployed engineer sees literal &h- in filenames, switch the pattern in the script to &H-&Y&M&D-&T.log.
  • The script is hard-coded to default $LogRoot to the Google Drive shared-drive path. RMM deployments that need a different root (e.g., a client environment where the engineer's logs go elsewhere) can set $LogRoot as an RMM variable.

- app-putty/putty-install.ps1: silent MSI install of PuTTY 64-bit
  from the.earth.li, idempotent (skips if installed unless
  $ForceReinstall=1), MSI install log captured to
  $env:WINDIR\logs\putty-install-msi.log.

- app-putty/putty-configure-logging.ps1: runs in user context,
  writes HKCU Default Settings so PuTTY logs every session to
  G:\Shared drives\Engineer Session Logs\<engineer>\&h-&Y&M&D-&T.log.
  Refuses to run as SYSTEM. Supports \$LogRoot / \$EngineerName /
  \$LogFilePattern / \$LogType / \$LogFileClash overrides for testing
  and special cases.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
@mnelsondtc mnelsondtc requested a review from Gumbees as a code owner May 12, 2026 13:46

@claude claude Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Code review skipped — your organization's overage spend limit has been reached.

Code review is billed via overage credits. To resume reviews, an organization admin can raise the monthly limit at claude.ai/admin-settings/claude-code.

Once credits are available, push a new commit or reopen this pull request to trigger a review.

@Gumbees Gumbees left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review: PR #78 .. PuTTY MSI install + per-user session logging

Author: mnelsondtc → main, +271/-0, two new files in app-putty/

What it does: silently installs PuTTY 64-bit MSI on the endpoint, and writes HKCU registry values so each engineer's PuTTY sessions auto-log to G:\Shared drives\Engineer Session Logs\<engineer>\<host>-<date>-<time>.log.


Blockers (must fix before merge)

1. $RMMScriptPath null-check is inverted .. both files

putty-install.ps1:23 and putty-configure-logging.ps1:35:

if ($null -eq $RMMScriptPath) { $LogPath = "$RMMScriptPath\logs\..." }

When $RMMScriptPath is null, this builds \logs\foo.log with an empty prefix. The branches are backwards from intent. Swap the condition.

2. RMM variables not read via $env: .. both files

Per repo CLAUDE.md, RMM passes variables as environment vars, so bare $RMM, $RMMScriptPath, $Description, $EngineerName, $LogRoot, $LogType, $InstallerUrl, $ForceReinstall all resolve to $null in true RMM mode. Use $env:RMM, $env:RMMScriptPath, etc.

3. No integrity check on the MSI download

putty-install.ps1:73-82 pulls the MSI from the.earth.li over HTTPS but skips SHA-256 / Authenticode entirely. Compromised mirror or mis-issued cert = SYSTEM-level RCE on every endpoint. PuTTY publishes sha512sums (GPG-signed). At minimum verify with:

$sig = Get-AuthenticodeSignature $installer
if ($sig.Status -ne 'Valid' -or $sig.SignerCertificate.Subject -notmatch 'Simon Tatham') { throw }

4. Installer URL points at "latest"

putty-install.ps1:62 .. silent upstream version bumps push to SYSTEM on every endpoint with no gate. Pin a known-good version or route through your approved-package list.


Real gaps (please address)

5. Per-user enrollment story is missing

The PR description doesn't say HOW each engineer's HKCU gets these values. Manual RMM "Run As: Logged-On User" once per engineer doesn't scale and isn't enforceable. Logon script, scheduled task, or GPO Preferences? If this is a compliance control, the enrollment path needs to be in the PR.

6. Trivially bypassable

Engineer opens regedit, clears HKCU\...\Default%20Settings\LogFileName, done. Per-session "Logging > None" also overrides defaults. If this is for compliance/auditability, this isn't a control .. it's a default. Acceptable if framed that way; not acceptable if framed as enforcement.

7. Compliance question on the log destination

Logs go to G:\Shared drives\Engineer Session Logs (Google Drive). Is that BAA-covered Workspace? PuTTY session logs WILL contain PHI-adjacent content the moment an engineer ssh's into anything touching dental data (hostnames, paths, command output). Confirm BAA + per-engineer ACL on the shared drive before merge.

8. LogType=2 captures everything typed

Including passwords pasted at prompts and full banners. Consider LogType=1 (printable) + LogOmitPasswords=1 + LogOmitData=1. If you keep LogType=2, document the risk in the script header.

9. ForceReinstall type coercion

putty-install.ps1:67 .. RMM passes strings, so "1" -ne 1 is $true under strict mode. Use [int]$env:ForceReinstall -eq 1 or string-compare against "1".

10. Installer not validated before msiexec

If Invoke-WebRequest silently writes a 0-byte file or an HTML error page, Test-Path still passes. Check (Get-Item $installer).Length + extension before launching the MSI.


Nits

  • Typo VARIALBES in both file headers (carried from template, but new files .. fix it).
  • Doc drift: PR description claims $LogFileClash and $LogFilePattern are overridable; code only honors $LogType, $LogRoot, $EngineerName. Either wire them up or update the doc.
  • [string]$LogType vs [int]$LogType cast is inconsistent between files.
  • $ENV:WINDIR (uppercase) vs $env:WINDIR (lowercase) .. repo uses lowercase elsewhere.

Style notes (informational)

  • Both files match the script-template-powershell.ps1 shape and app-putty/ is correctly kebab-case.
  • No need for a shared lib here; repo has no lib-bootstrap pattern in main yet (PR #68 is on development). Self-contained is correct.
  • No CmdletBinding/Param() block .. consistent with the template, but worth a one-line comment that everything routes via $env:.

Recommendation

Request changes. The inverted null-check in both files is a real bug, the missing $env: prefixes break RMM mode, and the MSI hash/signature verification gap is non-negotiable for a SYSTEM-context installer pushed fleet-wide. The enrollment and compliance questions (5, 6, 7) are conversation-shape rather than line-edit, but they should land before this merges into main.

🤖 Reviewed by pia + 3-agent team (diff/intent, security/correctness, style/conventions). Local review, not posted by an automated reviewer .. nate's call on whether to post as-is.

Back-ports the same hardening applied to the RDS logon report script
(commits 3207aee and bfad74b) to both PuTTY scripts:

- Auto-detect -NonInteractive on the PowerShell command line and force
  RMM mode so Read-Host can never hang the run (NinjaOne launches with
  -NonInteractive by default).
- Correct the inverted RMMScriptPath fallback so $LogPath always resolves
  to a real folder. The buggy template branch landed transcripts at
  \logs\... which killed RMM runs silently.
- Emit context (description, computer, user, PS version, bitness, RMM)
  to stdout BEFORE Start-Transcript so the RMM stdout buffer always has
  something useful even if the transcript fails.
- Pre-create the transcript directory and wrap Start-Transcript in
  try/catch with a $transcriptStarted flag so a transcript failure can
  no longer kill the script.
- Guard every Stop-Transcript on success and error exit paths with
  if ($transcriptStarted) { ... } so we don't throw when the transcript
  was skipped.

putty-configure-logging.ps1 specifically uses LOCALAPPDATA\DTC\logs as
its transcript path (vs WINDIR\logs for the install script) because it
runs in user context without admin rights.

@Gumbees Gumbees left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review: PR #78 .. PuTTY MSI install + per-user session logging
Author: mnelsondtc → main, +342/-0, two new files in app-putty/

What it does: silently installs PuTTY 64-bit MSI on the endpoint, then writes HKCU registry values under PuTTY's Default%20Settings so each engineer's sessions auto-log to G:\Shared drives\Engineer Session Logs\<engineer>\<host>-<date>-<time>.log.

Blockers (must fix before merge)

  1. RMM variables not read via $env: .. both files
    putty-install.ps1:27 if ($RMM -ne 1) {
    putty-configure-logging.ps1:32 if ($RMM -ne 1) {
    NinjaRMM injects Script Variables as environment vars, so bare $RMM, $Description, $InstallerUrl, $ForceReinstall, $EngineerName, $LogRoot, $LogType all resolve to $null in true RMM mode and fall through to Read-Host .. which hangs the runner with no stdin. The canonical pattern lives at app-google-chrome/chrome-remote-desktop-detect-system.ps1 ($env:RMM -eq "1") and the CLAUDE.md "Input Handling" section is explicit.

  2. Integer compare against an env-var string
    Same files: $RMM -ne 1 should be $env:RMM -ne "1" once #1 is fixed. Env vars are strings .. comparing "1" -ne 1 is $true under strict mode. CLAUDE.md calls this out by name. Same gotcha on $ForceReinstall -ne 1 (putty-install.ps1:67) and the [int]$LogType cast inconsistency between the two files.

  3. msiexec exit code captured but never inspected
    putty-install.ps1: $proc = Start-Process ... -Wait -PassThru then Write-Host "msiexec exit code: $($proc.ExitCode)". The exit code is logged and dropped. Falls through to a Test-Path $expectedExe check, so a 1603 with a stale putty.exe on disk fakes [SUCCESS] and exits 0. Require $proc.ExitCode -eq 0 (or 3010 reboot-required) before the success branch.

  4. No download validation before handing the MSI to SYSTEM-context msiexec
    putty-install.ps1:73-82 pulls the MSI over HTTPS and only Test-Paths the resulting file. No size check, no Authenticode verification, no SHA-512 against PuTTY's published sums. A captive-portal HTML 200 written to putty-64bit-installer.msi would get handed to msiexec.exe running as SYSTEM. At minimum:

    $sig = Get-AuthenticodeSignature $installer
    if ($sig.Status -ne 'Valid' -or $sig.SignerCertificate.Subject -notmatch 'Simon Tatham') { throw }

    Reject zero-byte or sub-1MB files. Pairs with #5.

  5. Installer URL pinned at "latest"
    putty-install.ps1:62: https://the.earth.li/~sgtatham/putty/latest/w64/putty-64bit-installer.msi. Silent upstream bumps push to every SYSTEM-context endpoint with no review gate. Pin a known-good version (e.g. /0.83/w64/...) and the matching expected hash. Keep $env:InstallerUrl as the emergency-rollout override.

  6. LogType=2 writes pasted passwords to a Google Shared Drive
    putty-configure-logging.ps1:144 writes LogType=2 (all session content) to HKCU with no LogOmitPasswords=1 companion. Per PuTTY's manual, LogType=2 records the password-prompt bytes for telnet/rlogin AND any in-band credentials typed inside an established SSH session (sudo, su, scp prompts). With logs landing on G:\Shared drives\Engineer Session Logs, every engineer's typed credentials become a durable shared artifact. Add 'LogOmitPasswords' = 1 (DWord) at minimum; consider LogType=1 (printable) if the goal is command auditing rather than full session replay.

Real gaps (please address)

  1. Per-user enrollment story is absent from the PR
    The configure script writes HKCU and correctly refuses SYSTEM context (putty-configure-logging.ps1:88-93). But the PR description never says HOW each engineer's hive gets this. NinjaOne defaults to SYSTEM; "Run As: Logged-On User" is per-script per-endpoint. No logon script, no GPO Preferences ref, no scheduled task, no Intune policy doc. If this is the engineer-session-logging audit control, the enrollment path (Ninja policy + condition, AD logon script, or Intune ESP) needs to ship with this PR. Without it this merges and enrolls zero engineers.

  2. Compliance question on the log destination .. CISO sign-off needed
    Logs land on G:\Shared drives\Engineer Session Logs (Google Drive). LogType=2 plus the actual day-to-day shape of DTC work (engineers SSH'ing into dental practice systems and DoD/CMMC contractor systems) means these logs WILL contain PHI-adjacent and CUI-adjacent content: hostnames, file paths, command output, banners, the occasional inline patient identifier visible in a mysql tail. Three load-bearing questions for Mike Schepers:

    • Is this Workspace tenant BAA-covered for HIPAA?
    • Is it CMMC-aligned for the DoD client subset, or are those engineers excluded from this control?
    • Are per-engineer folders ACL'd so engineer A can't read engineer B's logs? Shared drives default to drive-wide membership; subfolder ACLs need explicit setup.
  3. Trivially bypassable as written
    Engineer opens regedit, clears HKCU\...\Default%20Settings\LogFileName, done. Per-session "Logging > None" in the PuTTY UI also overrides defaults. Plus they can just use a different SSH client. If framed as "a sane default that lowers logging friction," fine .. if framed as "a compliance control," this isn't a control, it's a default. Pick a frame and document it. Pairs with #7.

  4. Empty-string vs null on RMM-injected vars
    putty-install.ps1: if ($null -eq $Description) { $Description = ... } .. RMM unset fields commonly arrive as "" not $null, so the null branch falls through and $Description stays empty. Use [string]::IsNullOrWhiteSpace($Description), which the logging script already does correctly for $EngineerName / $LogRoot.

  5. BITS fallback path leaks the transcript on IWR throw
    putty-install.ps1: Start-BitsTransfer is in a try/catch, but the Invoke-WebRequest call inside the catch block is bare. If IWR throws (DNS, TLS, 404), the exception propagates out of the script .. Stop-Transcript never fires, the transcript file locks. Same shape on the Set-ItemProperty loop in putty-configure-logging.ps1 (a registry-write failure orphans the transcript). Wrap each in its own try/catch with explicit exit 1, or put the main body in try { ... } finally { Stop-Transcript }.

  6. No log audit / review loop
    No retention policy stated, no spot-check process, no alert when an engineer's folder hasn't received a log in N days (= bypass signal). A logging control without a review loop is shelfware. Out of scope for this PR but worth flagging while the framing in #7/#9 gets settled.

  7. Doc drift: PR body claims $LogFilePattern and $LogFileClash are RMM-overridable
    Code reality (putty-configure-logging.ps1:120): $logFileName is hard-coded as Join-Path $engineerFolder '&h-&Y&M&D-&T.log'. No $LogFilePattern variable exists. LogFileClash = 1 is hard-coded at line 144 in the $values hashtable. Either wire them or strike those bullets from the PR description.

Nits

  • VARIALBES typo on line 1 of both files (carried from an older template version .. canonical template now says VARIABLES). New files, fix it.
  • User-context log path $env:LOCALAPPDATA\DTC\logs vs CLAUDE.md's canonical $env:LOCALAPPDATA\dtc-logs. Pick one and conform.
  • Casing drift: putty-install.ps1 mixes $ENV:WINDIR (uppercase) with $env:TEMP, $env:COMPUTERNAME (lowercase) in the same file. Repo norm is lowercase $env:.
  • Stop-Transcript repeated at every exit instead of a single try { ... } finally { Stop-Transcript }. Three exit paths per script, three duplicate calls. The finally pattern would also close the exception-path gaps in #11.
  • LogFileClash = 1 (always append) means a single per-engineer-per-day-per-host log file grows across sessions. With LogType=2 and #6 unresolved, that compounds the exposure. Filename already includes &T so same-second collisions are the only risk .. fine as a nit.

Style notes (informational)

  • Folder placement and file naming both correct: app-putty/ matches the category-vendor convention; kebab-case filenames.
  • PR body Summary / Test plan / Notes structure is exemplary .. real test results with checkmarks, the &h vs &H token caveat documented, idempotency called out. Best PR-body shape on this repo recently. Carry forward.
  • Operational shape is solid where it lands: SYSTEM-context refusal in the configure script, transcript-wrapped main body, idempotency check + $ForceReinstall override, post-install Test-Path verify, BITS-first with IWR fallback. The gap is policy + compliance + enrollment, not code mechanics.
  • Self-contained (no shared lib) is correct for this repo at current scale .. matches recent doctrine (closed PRs #74/#75/#76 walked away from the fat-script build system).

Recommendation
Request changes. Blockers 1-5 are real correctness and supply-chain issues for a SYSTEM-context fleet installer; #6 is a compliance fail-condition once you account for the Google Drive destination. Real gaps 7 and 8 (enrollment + BAA/CMMC fitness) are conversation-shape rather than line edits, but they belong in the PR before merge so the audit trail shows the right stakeholders (Mike Schepers, in particular) signed off. Everything in Nits is fix-on-the-way.

🤖 Reviewed by apis + 3-agent team (diff/intent, security/correctness, style/conventions). Corroborates Pia's earlier CHANGES_REQUESTED review with overlap on most blockers, plus additional findings (msiexec exit code drop, empty-string vs null on RMM vars, BITS/IWR exception leak, audit-loop gap).

…t framing

Picks up the changes-requested items from Pia (commit 46ac1f3) and Apis
(commit 185c7fc) review rounds. Item-by-item:

Blockers (both files):
- All RMM inputs now read via $env:* per repo CLAUDE.md convention.
  Bare $RMM/$Description/$RMMScriptPath/$InstallerUrl/$ForceReinstall/
  $EngineerName/$LogRoot/$LogType/$LogFilePattern/$LogFileClash would
  resolve to $null under NinjaRMM's env-var injection model and fall
  through to Read-Host (hang).
- String-compare env-derived integers ($env:RMM -eq "1",
  $env:ForceReinstall -eq "1") instead of `-eq 1` against bare vars.
- Use [string]::IsNullOrWhiteSpace() for empty-string vs null on RMM
  inputs (RMM unset fields often arrive as "" not $null).

putty-install.ps1 specific:
- Pin the MSI URL to PuTTY 0.83 (https://the.earth.li/.../0.83/.../
  putty-64bit-0.83-installer.msi); $env:InstallerUrl is the
  emergency-rollout override.
- Reject downloads < 1MB before handing to msiexec (captive-portal HTML,
  zero-byte, etc.).
- Get-AuthenticodeSignature gate: status must be Valid AND signer
  subject must match 'Simon Tatham'. Reject anything else even with a
  valid chain.
- Check msiexec exit code: 0 or 3010 (reboot required) only.
  Previously the exit code was logged and dropped, so a 1603 with a
  stale putty.exe on disk faked [SUCCESS].

putty-configure-logging.ps1 specific:
- LogType default 2 -> 1 (printable). LogType=2 records raw input
  bytes including pasted credentials going over the wire to in-session
  sudo/su/scp prompts; printable mode captures the displayed terminal
  output without those.
- Write SSHLogOmitPasswords=1 and SSHLogOmitData=1 explicitly as
  defense-in-depth defaults.
- $LogFilePattern and $LogFileClash are now actually env-var
  overridable (the PR body claimed they were; the previous code didn't
  honor either).
- Log path moved from LOCALAPPDATA\DTC\logs to canonical
  LOCALAPPDATA\dtc-logs per repo norm.
- Script header reframed: this is a SANE DEFAULT, not an enforcement
  control. Engineers can override per-session via PuTTY's Logging panel.
  Audit/retention/ACL is handled outside this script.

Both files:
- Main body now wrapped in try/catch/finally so any unhandled throw
  still runs Stop-Transcript and returns a clean exit code. Removes the
  duplicate guarded Stop-Transcript at every exit path.
- VARIALBES typo -> VARIABLES in header comments.
- Casing drift fixed: $env:WINDIR uniformly lowercase.

Enrollment path (review item B): delivered via NinjaRMM "Run As:
Logged-On User" -- no script change needed. Compliance/BAA framing
(item C) is being handled outside the repo with Mike Schepers.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
@mnelsondtc

Copy link
Copy Markdown
Contributor Author

Pushed 13821b2 addressing the review. Item-by-item map against pia + apis:

Blockers / line-edit findings — all addressed

# Reviewer item Resolution
1 Inverted $null -eq $RMMScriptPath Fixed in earlier 185c7fc. Now if ($env:RMMScriptPath).
2 Bare $RMM etc. vs $env: All RMM inputs migrated to $env:*. String-compared against "1".
3 No MSI integrity check Get-AuthenticodeSignature gate added — status must be Valid AND signer subject must match Simon Tatham.
4 URL pinned at /latest/ Pinned to /0.83/w64/putty-64bit-0.83-installer.msi (HEAD-checked: 200, 3.8MB, application/x-msi). $env:InstallerUrl kept as emergency override.
A LogType=2 writes pasted credentials to Google Drive Default lowered to LogType=1 (printable). SSHLogOmitPasswords=1 and SSHLogOmitData=1 written explicitly.
9 ForceReinstall string vs int $env:ForceReinstall -eq "1".
10 Installer not validated before msiexec Reject files < 1MB. Authenticode check before msiexec.
msiexec exit code dropped exit code must be 0 or 3010 enforced; throw otherwise.
Empty-string vs null on RMM vars [string]::IsNullOrWhiteSpace() used throughout.
BITS/IWR exception leak / transcript orphan Main body now in single try { } catch { } finally { Stop-Transcript }. Removes the guarded-per-exit pattern entirely.
Doc drift on $LogFilePattern/$LogFileClash Both now actually env-var overridable. Header docs match code.
VARIALBES typo Fixed in both files.
$ENV: vs $env: casing Lowercase throughout.
LOCALAPPDATA\DTC\logs vs canonical Moved to LOCALAPPDATA\dtc-logs.
[int]$LogType cast inconsistency Cast uniformly.

Real gaps — addressed via framing + outside-script work

  • B (enrollment): delivered via NinjaRMM "Run As: Logged-On User" — no script change needed.
  • C (BAA / CMMC / per-engineer ACLs): being handled outside this PR with Mike Schepers. Not a code item.
  • D (control vs default): script header now explicitly frames this as a sane default, not an enforcement control. Engineers can override per-session in PuTTY's Logging panel or clear HKCU values directly. Documented.
  • E (audit/review loop): out of scope per apis; separate ticket if/when the framing changes.

Tested

  • Both scripts parse-check clean (no syntax errors).
  • putty-configure-logging.ps1 ran end-to-end with simulated NinjaRMM env vars ($env:RMM="1", $env:LogRoot="C:\", etc.) — all values flowed through env vars correctly, registry verifies, exit 0.
  • HEAD-checked the new pinned MSI URL.
  • Live install retest deferred to once this lands on a clean VM (don't want to re-pull MSI on this workstation without value).

Ready for re-review.

@Gumbees Gumbees left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-review: PR #78 .. PuTTY MSI install + per-user session logging
HEAD: 13821b2f .. mnelsondtc, +409/-0, two files in app-putty/ ("Address PR review: env-var inputs, MSI safety, LogType=1, sane-defaul…")

What changed: Mike picked up essentially every code-level blocker and gap from the prior two reviews (Pia's CHANGES_REQUESTED + apis's COMMENTED follow-up) and worked through them in one focused pass. MSI safety hardening, full $env:* sweep, password-omit defaults, error-handling cleanup, header documentation .. all landed. What's left is PR-body and process artifacts, not code.

Resolved since the prior reviews

  1. RMM variables now read via $env: everywhere .. putty-install.ps1 and putty-configure-logging.ps1 both consistent across $env:RMM, $env:Description, $env:InstallerUrl, $env:ForceReinstall, $env:EngineerName, $env:LogRoot, $env:LogType, $env:LogFilePattern, $env:LogFileClash.

  2. Integer compare against env-var string fixed .. $env:RMM -eq "1" (string), $null-safe via [string]::IsNullOrWhiteSpace() for the optional fields. No more $null -eq fall-throughs on empty-string RMM payloads.

  3. msiexec exit code now inspected .. putty-install.ps1:170-173 throws on anything other than 0 or 3010 (reboot-required). The 1603-with-stale-binary fake-success path is closed.

  4. MSI download validation before SYSTEM exec .. size check (>= 1MB), Authenticode Status -eq 'Valid', signer subject -notmatch 'Simon Tatham' rejected. No SHA-512 pin, but signer-subject gate plus the pinned-version URL is equivalent strength for this threat model.

  5. Installer URL pinned .. 0.83/w64/putty-64bit-0.83-installer.msi, with $env:InstallerUrl as the emergency-override. Silent upstream pushes to fleet closed.

  6. LogType=2LogType=1 default + password-omit DWords .. putty-configure-logging.ps1 writes SSHLogOmitPasswords=1 AND SSHLogOmitData=1 regardless of LogType. Mike used PuTTY's canonical setting names (SSHLogOmit*), which is the correct shape. Belt + suspenders.

  7. Stop-Transcript collapsed into a single try { ... } finally { Stop-Transcript } with a $transcriptStarted guard. Replaces the three-inline-copies pattern from the prior review. BITS/IWR exception path now wrapped, registry-write loop now inside the outer try.

  8. VARIALBES typo fixed to VARIABLES on line 1 of both files.

  9. User-context log path canonicalized to $env:LOCALAPPDATA\dtc-logs. Casing drift on $ENV:WINDIR$env:WINDIR cleaned up.

  10. HKCU-only advisory framing made explicit in the script header (putty-configure-logging.ps1:7-12) .. "sane default, not enforcement control." Picks a frame and documents it.

  11. _LogFilePattern and _LogFileClash actually wired this time .. not just claimed.

  12. New positive: MSI cleaned up in finally (putty-install.ps1:189-191) closes a lingering-temp-file vector that wasn't called out previously.

Still open

  1. PR body doc drift in reverse .. Summary still says LogType = 2 (all session output) but code now defaults to LogType = 1 (printable) for the credential-paste safety reason Mike documented in-script. One-line fix in the description. Body should also mention SSHLogOmitPasswords=1 / SSHLogOmitData=1 are now defaults, since they're the load-bearing safety belt.

  2. Enrollment artifact still not linked in the PR body. The commit message asserts NinjaRMM "Run As: Logged-On User" is the enrollment path, which is fine .. but for the audit trail, link the actual Ninja policy / endpoint condition that puts that into effect. Without the artifact, "we said we'd enroll" sits on Mike's word alone .. and confirms what's deployable from the PR is enroll: zero engineers until that policy lands.

  3. BAA / CMMC fitness on the Google Drive destination deferred to Schepers with no signoff link / ticket gate on the PR. Get a "approved, see ticket #N" or "moved destination to BAA-covered store" .. either lands the question, neither lets it sit. The sane default frame is sympathetic to deferring this, but PHI / CUI flowing into the logs the moment an engineer SSHs into a customer system means the deferral has a clock on it.

Nits

  • return from inside the try block on the idempotent-skip path in putty-install.ps1:107. finally still fires so the transcript closes, but a break out of a labelled block reads cleaner. Cosmetic.
  • PR body should mention SSHLogOmitPasswords / SSHLogOmitData are written as DWords regardless of LogType .. that's the actual safety belt and the next reviewer / operator should see it without having to grep the script.

Style notes (informational)

  • Full $env:* sweep + try/catch/finally + Stop-Transcript-in-finally pattern is the canonical shape now. app-google-chrome/chrome-remote-desktop-detect-system.ps1 does the same. PR #78 is now itself a good reference for the next contributor who copies a sibling script.
  • New env vars are documented in the file headers in the aligned-comment style. Matches the template.
  • Write-Host usage stayed .. matches the repo idiom (siblings do the same). Not worth churning on for this PR.

Recommendation

APPROVE pending three small follow-ups, none of them code:

  1. Update the PR-body Summary so LogType and SSHLogOmit* defaults match the code.
  2. Link the Ninja "Run As: Logged-On User" policy artifact (URL or Ninja policy ID).
  3. Reply to the BAA / CMMC compliance question with either a Schepers signoff link or a re-scoped destination .. either is fine, neither is "later."

Code-level blockers are gone. The remaining items live in the PR description and the audit trail. Once the body matches the code, this is mergeable.

🤖 Reviewed by apis + 3-agent team (diff/intent, security/correctness, style/conventions). Independent corroboration: security slice clears all 10 prior blockers; style slice clears 6 of 7 (the seventh is the body doc drift); intent slice flags the three remaining process items above.

@Gumbees Gumbees merged commit fe1e703 into main May 12, 2026
@Gumbees Gumbees deleted the enhancement/putty-install-and-session-logging branch May 12, 2026 18:59
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.

2 participants