Skip to content

feat: add background key rotation via LaunchAgent#7

Open
shawntz wants to merge 55 commits intostagingfrom
feat/add-headless-launchagent
Open

feat: add background key rotation via LaunchAgent#7
shawntz wants to merge 55 commits intostagingfrom
feat/add-headless-launchagent

Conversation

@shawntz
Copy link
Copy Markdown
Owner

@shawntz shawntz commented Dec 10, 2025

Description

Introduces a new LaunchAgent (com.shawnschwartz.cassh.rotate.plist) to run hourly background key rotation using a headless mode (--rotate-keys). Updates uninstall and postinstall scripts to handle the new agent, and refactors key title generation and GitHub key lookup for improved reliability.

Type of Change

  • Bug fix (non-breaking change that fixes an issue)
  • New feature (non-breaking change that adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Documentation update
  • Refactoring (no functional changes)

Related Issues

Closes #

How Has This Been Tested?

  • Unit tests
  • Integration tests
  • Manual testing

Test environment:

  • OS: macOS 26.2 Beta (25C5037j)
  • Go version: 1.25.5 darwin/arm64

Checklist

  • My code follows the project's style guidelines
  • I have performed a self-review of my code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • Any dependent changes have been merged and published

Security Considerations

  • This PR does not introduce any security concerns
  • I have considered the security implications and addressed them

Screenshots (if applicable)

Introduces a new LaunchAgent (com.shawnschwartz.cassh.rotate.plist) to run hourly background key rotation using a headless mode (--rotate-keys). Updates uninstall and postinstall scripts to handle the new agent, and refactors key title generation and GitHub key lookup for improved reliability.
@shawntz shawntz self-assigned this Dec 10, 2025
Copilot AI review requested due to automatic review settings December 10, 2025 20:23
@shawntz shawntz added the enhancement New feature or request label Dec 10, 2025
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR introduces automated background SSH key rotation for personal GitHub connections via a new LaunchAgent that runs hourly. The implementation adds a headless --rotate-keys mode to the cassh application, refactors key title generation to include hostnames for better identification, and updates the installation/uninstallation scripts to handle the new background rotation agent.

Key Changes

  • Adds hourly background key rotation via new LaunchAgent (com.shawnschwartz.cassh.rotate.plist) running in headless mode
  • Refactors SSH key title generation from cassh-{connID} to cassh-{connID}@{hostname} format for improved key identification
  • Updates GitHub CLI key listing parser to handle the newer output format with additional fields

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 7 comments.

File Description
packaging/macos/com.shawnschwartz.cassh.rotate.plist New LaunchAgent configuration for hourly background key rotation with low CPU priority
packaging/macos/scripts/postinstall Extended to install and load the rotation LaunchAgent alongside the main app agent
cmd/cassh-menubar/main.go Adds headless rotation mode, refactors key title generation functions, updates GitHub key lookup parsing, and implements runHeadlessKeyRotation function
Comments suppressed due to low confidence (1)

cmd/cassh-menubar/main.go:2283

  • The rotatePersonalGitHubSSH function attempts to delete the old key from GitHub using conn.GitHubKeyID, but if that deletion fails (and continues anyway per line 2265), followed by local key file deletion, there's a potential issue. If the new key generation or upload subsequently fails after the local files are deleted at lines 2270-2271, the user could end up with neither the old key files locally nor a working setup. Consider keeping the old key files until the new key is successfully uploaded to GitHub, or implementing a rollback mechanism.
	// 2. Delete local key files
	os.Remove(conn.SSHKeyPath)
	os.Remove(conn.SSHKeyPath + ".pub")

	// 3. Generate new key
	if err := generateSSHKeyForPersonal(conn); err != nil {
		return fmt.Errorf("key generation failed: %w", err)
	}

	// 4. Upload new key to GitHub
	keyTitle := getKeyTitle(conn.ID)
	keyID, err := uploadSSHKeyToGitHub(conn.SSHKeyPath, keyTitle)
	if err != nil {
		return fmt.Errorf("key upload failed: %w", err)
	}

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +2404 to +2416
// Save config if any keys were rotated
if rotatedCount > 0 {
if err := config.SaveUserConfig(userCfg); err != nil {
log.Printf("Failed to save config: %v", err)
os.Exit(1)
}
log.Printf("Rotated %d key(s)", rotatedCount)
} else {
log.Println("No keys needed rotation")
}

os.Exit(0)
}
Copy link

Copilot AI Dec 10, 2025

Choose a reason for hiding this comment

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

The runHeadlessKeyRotation function modifies user configuration by calling config.SaveUserConfig without any locking mechanism. If the GUI app is running simultaneously and also modifying the configuration (e.g., during user-initiated changes), there could be a race condition leading to lost updates. Consider implementing file locking or using atomic file operations to ensure concurrent modifications don't corrupt the configuration.

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

@copilot open a new pull request to apply changes based on this feedback

Comment on lines +18 to +20
<!-- Also run shortly after load (30 seconds delay) -->
<key>RunAtLoad</key>
<true/>
Copy link

Copilot AI Dec 10, 2025

Choose a reason for hiding this comment

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

The combination of RunAtLoad set to true and StartInterval set to 3600 seconds could result in the rotation running immediately on system boot or user login (after a brief delay), and then again every hour. The comment on line 18 mentions "30 seconds delay" but RunAtLoad doesn't specify a delay - it runs as soon as the agent is loaded. If the intention is to wait 30 seconds after load, consider using StartCalendarInterval with a calculated time or removing RunAtLoad if you only want hourly execution.

Suggested change
<!-- Also run shortly after load (30 seconds delay) -->
<key>RunAtLoad</key>
<true/>
<!-- Only run every hour, no immediate execution on load -->

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

@claude please open a new pull request to apply changes and ensure this won't incidentally break other features in its current form

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

@copilot please open a new pull request to apply changes and ensure this won't incidentally break other features upstream its current form #7 (comment)

Comment on lines +23 to +24
<key>LaunchOnlyOnce</key>
<false/>
Copy link

Copilot AI Dec 10, 2025

Choose a reason for hiding this comment

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

The LaunchOnlyOnce key is set to false with the comment "Don't run if already running" which is contradictory. LaunchOnlyOnce set to false means launchd WILL launch the job multiple times if the interval is met, even if a previous instance is still running. If you want to prevent concurrent executions, consider removing this key (defaults to true for jobs with StartInterval), or implement a lock file mechanism in the runHeadlessKeyRotation function.

Suggested change
<key>LaunchOnlyOnce</key>
<false/>

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

@copilot open a new pull request to apply changes based on #7 (comment)

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

@copilot open a new pull request to apply changes based on #7 (comment)

Comment on lines +34 to +38
if [ -f "$ROTATE_AGENT_SRC" ]; then
cp "$ROTATE_AGENT_SRC" "$ROTATE_AGENT_DST"
chmod 644 "$ROTATE_AGENT_DST"
chown root:wheel "$ROTATE_AGENT_DST"
fi
Copy link

Copilot AI Dec 10, 2025

Choose a reason for hiding this comment

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

The script sets 'set -e' at line 5 but then uses '|| true' on many commands. While this is intentional for commands that may fail, the issue is that if ROTATE_AGENT_SRC doesn't exist (line 34 check passes but file is somehow unavailable between check and copy), the script would exit due to 'set -e'. Consider adding error handling or removing 'set -e' and explicitly checking return codes for critical operations only.

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

@copilot open a new pull request to apply changes based on this feedback

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

@copilot open a new pull request to apply changes based on this feedback

Comment on lines +2151 to +2155
// getLegacyKeyTitle returns the old key title format without hostname
// Used for backwards compatibility when looking up existing keys
func getLegacyKeyTitle(connID string) string {
return fmt.Sprintf("cassh-%s", connID)
}
Copy link

Copilot AI Dec 10, 2025

Choose a reason for hiding this comment

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

The getLegacyKeyTitle function is defined but never used in the codebase. If this is intended for backward compatibility with existing keys that were created with the old title format (without hostname), it should be called somewhere in the key rotation or lookup logic. Otherwise, when users upgrade, the system won't be able to find and delete their old keys from GitHub that were created with the legacy format. Consider either using this function in findGitHubKeyIDByTitle as a fallback, or removing it if not needed.

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

@copilot open a new pull request to apply changes based on this feedback

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

@copilot open a new pull request to apply changes based on this feedback

Comment on lines 2201 to 2208
for _, line := range lines {
if strings.Contains(line, title) {
fields := strings.Fields(line)
if len(fields) >= 4 {
// Key ID is typically the last field
return fields[len(fields)-1]
if len(fields) >= 5 {
// Key ID is the second-to-last field (last is "authentication" or "signing")
return fields[len(fields)-2]
}
}
Copy link

Copilot AI Dec 10, 2025

Choose a reason for hiding this comment

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

The findGitHubKeyIDByTitle function uses a simple string contains check which could match incorrect keys. For example, if the hostname contains another connection ID, or if there are multiple keys with similar titles. Consider using a more precise matching strategy, such as checking if the line starts with the exact title followed by whitespace, or using strings.HasPrefix after trimming whitespace.

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

@copilot open a new pull request to apply changes based on this feedback

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

@copilot open a new pull request to apply changes based on this feedback

Comment on lines +36 to +38
<string>/tmp/cassh-rotate.log</string>
<key>StandardErrorPath</key>
<string>/tmp/cassh-rotate.log</string>
Copy link

Copilot AI Dec 10, 2025

Choose a reason for hiding this comment

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

Logging to /tmp is problematic because /tmp is typically cleared on reboot on macOS, and may have permission issues. Consider using a more appropriate location such as ~/Library/Logs/cassh/ or using the system logging facilities (which would appear in Console.app). The LaunchAgent runs per-user, so a user-specific log location would be more appropriate.

Suggested change
<string>/tmp/cassh-rotate.log</string>
<key>StandardErrorPath</key>
<string>/tmp/cassh-rotate.log</string>
<string>$HOME/Library/Logs/cassh/cassh-rotate.log</string>
<key>StandardErrorPath</key>
<string>$HOME/Library/Logs/cassh/cassh-rotate.log</string>

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

@copilot open a new pull request to apply changes based on this discussion

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

@copilot open a new pull request to apply changes based on this discussion

@shawntz shawntz changed the base branch from main to staging December 10, 2025 21:01
@shawntz
Copy link
Copy Markdown
Owner Author

shawntz commented Dec 13, 2025

@copilot open a new pull request to apply changes based on the comments in this thread

Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

// Example: "cassh-personal-123 ssh-ed25519 AAAA... 2025-12-09T02:43:40Z 137889594 authentication"
lines := strings.Split(string(output), "\n")
for _, line := range lines {
if strings.Contains(line, title) {
Copy link

Copilot AI Dec 13, 2025

Choose a reason for hiding this comment

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

Using strings.Contains for title matching can lead to false positives. For example, if searching for "cassh-personal-123", it would match "cassh-personal-1234" as well. Consider using exact matching by checking if the title field equals the search term, or at minimum use strings.HasPrefix on the trimmed fields[0].

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

@copilot open a new pull request to apply changes based on this feedback

func getLegacyKeyTitle(connID string) string {
return fmt.Sprintf("cassh-%s", connID)
}

Copy link

Copilot AI Dec 13, 2025

Choose a reason for hiding this comment

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

The getLegacyKeyTitle function is defined but never used. For keys created before this change (using the old title format "cassh-{connID}"), the rotation logic in rotatePersonalGitHubSSH will fail to delete the old key from GitHub if conn.GitHubKeyID is empty. Consider adding a fallback in rotatePersonalGitHubSSH to look up the key by legacy title when GitHubKeyID is empty, or remove this function if it's not needed.

Suggested change
// rotatePersonalGitHubSSH rotates the personal GitHub SSH key, deleting the old key if present
func rotatePersonalGitHubSSH(conn *config.Connection, keyPath string) error {
// Delete old key if we have the key ID
keyID := conn.GitHubKeyID
if keyID == "" {
// Try to find by current title
keyID = findGitHubKeyIDByTitle(getKeyTitle(conn.ID))
if keyID == "" {
// Try to find by legacy title
keyID = findGitHubKeyIDByTitle(getLegacyKeyTitle(conn.ID))
}
}
if keyID != "" {
err := deleteGitHubKeyByID(keyID)
if err != nil {
log.Printf("Failed to delete old GitHub SSH key (ID: %s): %v", keyID, err)
} else {
log.Printf("Deleted old GitHub SSH key (ID: %s)", keyID)
}
}
// Upload new key
title := getKeyTitle(conn.ID)
newKeyID, err := uploadSSHKeyToGitHub(keyPath, title)
if err != nil {
return fmt.Errorf("failed to upload new SSH key: %w", err)
}
conn.GitHubKeyID = newKeyID
return nil
}

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

@copilot please open a new pull request to apply changes based on this feedback

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Copilot AI review requested due to automatic review settings December 13, 2025 04:55
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

// Example: "cassh-personal-123 ssh-ed25519 AAAA... 2025-12-09T02:43:40Z 137889594 authentication"
lines := strings.Split(string(output), "\n")
for _, line := range lines {
if strings.Contains(line, title) {
Copy link

Copilot AI Dec 13, 2025

Choose a reason for hiding this comment

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

Using strings.Contains(line, title) to match key titles is fragile and can produce false positives. For example, if a user has keys titled "cassh-personal" and "cassh-personal-123", searching for "cassh-personal" would match both. This could lead to deleting or misidentifying the wrong key. Consider using exact matching by checking if the first field equals the title, or using a more robust parsing approach that validates the title is a complete token match.

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

@copilot open a new pull request to apply changes based on this feedback

Comment on lines +49 to +61
# Unload old LaunchAgents if exist (ignore errors)
if ! sudo -u "$CONSOLE_USER" launchctl bootout "gui/$CONSOLE_UID/$LABEL" 2>/dev/null; then
sudo -u "$CONSOLE_USER" launchctl unload "$LAUNCH_AGENT_DST" 2>/dev/null || true
fi
if ! launchctl bootout "gui/$CONSOLE_UID/$LABEL" 2>/dev/null; then
launchctl unload "$LAUNCH_AGENT_DST" 2>/dev/null || true
fi
if ! sudo -u "$CONSOLE_USER" launchctl bootout "gui/$CONSOLE_UID/$ROTATE_LABEL" 2>/dev/null; then
sudo -u "$CONSOLE_USER" launchctl unload "$ROTATE_AGENT_DST" 2>/dev/null || true
fi
if ! launchctl bootout "gui/$CONSOLE_UID/$ROTATE_LABEL" 2>/dev/null; then
launchctl unload "$ROTATE_AGENT_DST" 2>/dev/null || true
fi
Copy link

Copilot AI Dec 13, 2025

Choose a reason for hiding this comment

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

The LaunchAgent unloading logic is duplicated for both agents with very similar code blocks. This creates maintainability issues as any changes to the unloading logic need to be repeated in multiple places. Consider extracting this into a shell function that takes the label and destination path as parameters to reduce code duplication and make the script easier to maintain.

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

@copilot open a new pull request to apply changes based on this feedback

@shawntz
Copy link
Copy Markdown
Owner Author

shawntz commented Dec 13, 2025

@copilot open a new pull request to apply changes based on the comments in this thread

Copy link
Copy Markdown
Contributor

Copilot AI commented Dec 13, 2025

@shawntz I've opened a new pull request, #16, to work on those changes. Once the pull request is ready, I'll request review from you.

Copy link
Copy Markdown
Contributor

Copilot AI commented Dec 13, 2025

@shawntz I've opened a new pull request, #17, to work on those changes. Once the pull request is ready, I'll request review from you.

Copy link
Copy Markdown
Contributor

Copilot AI commented Dec 13, 2025

@shawntz I've opened a new pull request, #18, to work on those changes. Once the pull request is ready, I'll request review from you.

Copy link
Copy Markdown
Contributor

Copilot AI commented Dec 13, 2025

@shawntz I've opened a new pull request, #19, to work on those changes. Once the pull request is ready, I'll request review from you.

Copy link
Copy Markdown
Contributor

Copilot AI commented Dec 13, 2025

@shawntz I've opened a new pull request, #20, to work on those changes. Once the pull request is ready, I'll request review from you.

Copilot AI review requested due to automatic review settings December 19, 2025 09:57
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 7 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.


<!-- Run every hour (no immediate execution on load) -->
<key>StartInterval</key>
<integer>3600</integer>
Copy link

Copilot AI Dec 19, 2025

Choose a reason for hiding this comment

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

The comment states "no immediate execution on load" but StartInterval alone doesn't guarantee this behavior. By default, without an explicit RunAtLoad key set to false, launchd may run the job immediately when loaded. If you want to ensure the job only runs on the interval schedule and not immediately on load, add "<key>RunAtLoad</key><false/>" after the StartInterval key. Otherwise, update the comment to reflect the actual behavior.

Suggested change
<integer>3600</integer>
<integer>3600</integer>
<key>RunAtLoad</key>
<false/>

Copilot uses AI. Check for mistakes.
Comment on lines +2168 to +2172
// getLegacyKeyTitle returns the old key title format without hostname
// Used for backwards compatibility when looking up existing keys
func getLegacyKeyTitle(connID string) string {
return fmt.Sprintf("cassh-%s", connID)
}
Copy link

Copilot AI Dec 19, 2025

Choose a reason for hiding this comment

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

The getLegacyKeyTitle function is defined but never used. Since the key title format has changed from "cassh-{connID}" to "cassh-{connID}@{hostname}", existing keys with the old format will not be found by findGitHubKeyIDByTitle when rotating keys. This means the old key will not be deleted from GitHub during rotation, leaving orphaned keys. Consider using getLegacyKeyTitle as a fallback in rotatePersonalGitHubSSH when conn.GitHubKeyID is empty, to find and delete keys created with the old format.

Copilot uses AI. Check for mistakes.
// Example: "cassh-personal-123 ssh-ed25519 AAAA... 2025-12-09T02:43:40Z 137889594 authentication"
lines := strings.Split(string(output), "\n")
for _, line := range lines {
if strings.Contains(line, title) {
Copy link

Copilot AI Dec 19, 2025

Choose a reason for hiding this comment

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

Using strings.Contains for key title matching could match the wrong key. For example, if you have keys titled "cassh-personal@host" and "cassh-personal-backup@host", searching for "cassh-personal@host" would match both. Consider using exact matching by checking if fields[0] equals the title, or use a more precise matching strategy like checking if the line starts with the title followed by whitespace.

Copilot uses AI. Check for mistakes.
Comment on lines +31 to +34
<key>StandardOutPath</key>
<string>/tmp/cassh-rotate.log</string>
<key>StandardErrorPath</key>
<string>/tmp/cassh-rotate.log</string>
Copy link

Copilot AI Dec 19, 2025

Choose a reason for hiding this comment

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

Logging to /tmp/cassh-rotate.log without rotation or size limits could lead to disk space issues over time. Since this LaunchAgent runs hourly, the log file will grow continuously. Consider either: (1) using a system logging mechanism like os_log on macOS, (2) implementing log rotation, or (3) documenting that users should periodically clean this file. Note that /tmp is cleared on reboot on macOS, but the file could still grow large between reboots.

Copilot uses AI. Check for mistakes.
Comment on lines +18 to +21
<!-- Don't run if already running -->
<key>LaunchOnlyOnce</key>
<false/>

Copy link

Copilot AI Dec 19, 2025

Choose a reason for hiding this comment

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

LaunchOnlyOnce is not a valid launchd plist key. The comment suggests the intention is to prevent overlapping executions, but this key will be ignored by launchd. If the goal is to prevent multiple simultaneous executions, that's already the default behavior for LaunchAgents with StartInterval. Consider removing this invalid key entirely, or if you need more control over concurrent executions, investigate using other mechanisms like a lock file in the script itself.

Suggested change
<!-- Don't run if already running -->
<key>LaunchOnlyOnce</key>
<false/>

Copilot uses AI. Check for mistakes.
log.Println("No keys needed rotation")
}

os.Exit(0)
Copy link

Copilot AI Dec 19, 2025

Choose a reason for hiding this comment

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

The function exits with code 0 even when some key rotations fail. If rotation fails for one or more connections (line 2412 continues on error), but succeeds for others, the function still exits with 0 indicating success. This could mask partial failures in monitoring systems. Consider tracking failed rotations and exiting with a non-zero code if any rotations failed, or provide more nuanced exit codes (e.g., 0 for full success, 1 for partial failure, 2 for complete failure).

Copilot uses AI. Check for mistakes.
Comment on lines +30 to +34
<!-- Logging -->
<key>StandardOutPath</key>
<string>/tmp/cassh-rotate.log</string>
<key>StandardErrorPath</key>
<string>/tmp/cassh-rotate.log</string>
Copy link

Copilot AI Dec 19, 2025

Choose a reason for hiding this comment

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

Logging to /tmp without secure file creation could expose sensitive information. The /tmp directory is world-readable by default on macOS, and log files created there may contain SSH key paths, GitHub usernames, connection names, or other potentially sensitive information. Consider logging to a user-specific location like ~/Library/Logs/cassh/ with appropriate permissions (0600), or use the macOS unified logging system which handles security automatically.

Suggested change
<!-- Logging -->
<key>StandardOutPath</key>
<string>/tmp/cassh-rotate.log</string>
<key>StandardErrorPath</key>
<string>/tmp/cassh-rotate.log</string>
<!-- Logging is handled by macOS unified logging; no explicit StandardOutPath/StandardErrorPath -->

Copilot uses AI. Check for mistakes.
@shawntz
Copy link
Copy Markdown
Owner Author

shawntz commented Jan 1, 2026

fizzy

shawntz and others added 4 commits January 11, 2026 00:54
…8b-bc6f-e8daec5b1c46

Extract duplicated LaunchAgent unloading logic into reusable function
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
…06-8f8f-411296b257da

Fix SSH key title matching to prevent false positives
Copilot AI review requested due to automatic review settings January 11, 2026 08:55
shawntz and others added 17 commits January 11, 2026 00:55
…69-b8f3-42e94d5195ca

Add legacy key fallback for personal GitHub SSH key rotation
…c3-be7a-0585366d19df

Fix SSH key title matching to prevent false positives
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
…4a-ac67-14800ba1b2d5

Fix LaunchAgent log location for key rotation background task
Fix findGitHubKeyIDByTitle to use exact field matching
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Use getLegacyKeyTitle for backward-compatible key lookup
Fix postinstall script error handling for LaunchAgent operations
Fix LaunchOnlyOnce contradiction in key rotation LaunchAgent
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 7 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +30 to +34
<!-- Logging -->
<key>StandardOutPath</key>
<string>~/Library/Logs/cassh/cassh-rotate.log</string>
<key>StandardErrorPath</key>
<string>~/Library/Logs/cassh/cassh-rotate.log</string>
Copy link

Copilot AI Jan 11, 2026

Choose a reason for hiding this comment

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

The logging paths use world-readable /tmp directory which could expose sensitive information. SSH key rotation logs may contain usernames, connection names, or error messages with sensitive details. Consider using a user-specific log directory like ~/Library/Logs/cassh-rotate.log or /var/log/cassh-rotate.log with appropriate permissions.

Copilot uses AI. Check for mistakes.
Comment on lines +2167 to +2172
// getLegacyKeyTitle returns the old key title format without hostname
// Used for backwards compatibility when looking up existing keys
func getLegacyKeyTitle(connID string) string {
return fmt.Sprintf("cassh-%s", connID)
}

Copy link

Copilot AI Jan 11, 2026

Choose a reason for hiding this comment

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

The getLegacyKeyTitle function is defined but never used in the codebase. If backward compatibility with old key titles is needed during rotation (to find and delete old keys created before this PR), this function should be called in the rotatePersonalGitHubSSH or deleteSSHKeyFromGitHub functions. Otherwise, this function should be removed as dead code.

Suggested change
// getLegacyKeyTitle returns the old key title format without hostname
// Used for backwards compatibility when looking up existing keys
func getLegacyKeyTitle(connID string) string {
return fmt.Sprintf("cassh-%s", connID)
}

Copilot uses AI. Check for mistakes.
}
if keyID != "" {
if err := deleteSSHKeyFromGitHub(keyID); err != nil {
log.Printf("Warning: failed to delete old key (ID: %s): %v", keyID, err)
Copy link

Copilot AI Jan 11, 2026

Choose a reason for hiding this comment

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

The key rotation logic changes the key title format from 'cassh-{connID}' to 'cassh-{connID}@{hostname}', but there's no migration path for existing keys. When rotatePersonalGitHubSSH runs, it attempts to delete the old key using conn.GitHubKeyID, but if that fails or is empty, old keys with the legacy title format will remain orphaned on GitHub. Consider attempting to find and delete legacy-format keys by calling findGitHubKeyIDByTitle with both the new and legacy title formats.

Suggested change
log.Printf("Warning: failed to delete old key (ID: %s): %v", keyID, err)
log.Printf("Warning: failed to delete old key (ID: %s): %v", keyID, err)
// Attempt migration cleanup: look for keys by both new and legacy titles
if conn.ID != "" {
currentTitle := getKeyTitle(conn.ID)
if currentTitle != "" {
if altID := findGitHubKeyIDByTitle(currentTitle); altID != "" && altID != keyID {
if err := deleteSSHKeyFromGitHub(altID); err != nil {
log.Printf("Warning: failed to delete key found by current title (ID: %s): %v", altID, err)
} else {
log.Printf("Deleted GitHub SSH key found by current title (ID: %s)", altID)
}
}
}
legacyTitle := getLegacyKeyTitle(conn.ID)
if legacyTitle != "" {
if altID := findGitHubKeyIDByTitle(legacyTitle); altID != "" && altID != keyID {
if err := deleteSSHKeyFromGitHub(altID); err != nil {
log.Printf("Warning: failed to delete key found by legacy title (ID: %s): %v", altID, err)
} else {
log.Printf("Deleted GitHub SSH key found by legacy title (ID: %s)", altID)
}
}
}
}

Copilot uses AI. Check for mistakes.
<!-- Run every hour (no immediate execution on load) -->
<key>StartInterval</key>
<integer>3600</integer>

Copy link

Copilot AI Jan 11, 2026

Choose a reason for hiding this comment

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

The comment on line 14 states 'Run every hour (no immediate execution on load)', but StartInterval actually triggers immediately on load and then every hour after that according to macOS LaunchAgent documentation. If you truly want to prevent immediate execution on load, you should use StartCalendarInterval instead of StartInterval, or add RunAtLoad set to false explicitly.

Suggested change
<!-- Do not run immediately when the job is loaded -->
<key>RunAtLoad</key>
<false/>

Copilot uses AI. Check for mistakes.
Comment on lines +2233 to 2246
fields := strings.Fields(line)
if len(fields) >= 5 {
keyTitle := fields[0]
keyID := fields[len(fields)-2] // Key ID is the second-to-last field
// Check for exact match with new title format
if keyTitle == title {
return keyID
}
// Check for exact match with legacy title format (if applicable)
if legacyTitle != "" && keyTitle == legacyTitle {
log.Printf("Found key with legacy title format: %s", legacyTitle)
return keyID
}
}
Copy link

Copilot AI Jan 11, 2026

Choose a reason for hiding this comment

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

The key title parsing logic assumes exactly 6 fields in the output, with the title being everything before the last 5 fields. However, if the gh CLI output format changes in future versions, or if there are edge cases with fewer fields, this could fail silently. Consider adding validation or logging when the field count is unexpected, and handling the case where len(fields) is between 1 and 5 more explicitly.

Suggested change
fields := strings.Fields(line)
if len(fields) >= 5 {
keyTitle := fields[0]
keyID := fields[len(fields)-2] // Key ID is the second-to-last field
// Check for exact match with new title format
if keyTitle == title {
return keyID
}
// Check for exact match with legacy title format (if applicable)
if legacyTitle != "" && keyTitle == legacyTitle {
log.Printf("Found key with legacy title format: %s", legacyTitle)
return keyID
}
}
line = strings.TrimSpace(line)
if line == "" {
continue
}
fields := strings.Fields(line)
// We expect at least 6 fields:
// TITLE (may contain spaces) + TYPE + KEY + ADDED + KEY_ID + KEY_TYPE
if len(fields) < 2 {
log.Printf("Unexpected ssh-key list line (too few fields): %q", line)
continue
}
if len(fields) < 6 {
log.Printf("Unexpected ssh-key list field count (%d) for line: %q", len(fields), line)
continue
}
// Title is everything before the last 5 fields
titleFieldCount := len(fields) - 5
keyTitle := strings.Join(fields[:titleFieldCount], " ")
keyID := fields[len(fields)-2] // Key ID is the second-to-last field
// Check for exact match with new title format
if keyTitle == title {
return keyID
}
// Check for exact match with legacy title format (if applicable)
if legacyTitle != "" && keyTitle == legacyTitle {
log.Printf("Found key with legacy title format: %s", legacyTitle)
return keyID
}

Copilot uses AI. Check for mistakes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants