fix(auth): address review feedback on machine account key file

- Enforce 0700 perms on existing machine-accounts directory, not just
  on first creation
- Use os.CreateTemp for atomic writes so concurrent logins for the
  same machine account cannot race on the same .tmp filename
- Remove the on-disk PEM if the keyring write fails during login, so
  failed logins don't leave crypto material behind
- Also remove the PEM from disk in the logout "user not found but
  stray state" branch — this is the exact cleanup path users will
  hit after a failed #146 login
- Use WrapUserErrorWithHint for the token refresh error paths to match
  the surrounding style; acknowledge in the hint that the original
  credentials file may no longer be available

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

Author: scotwells

internal/authutil/machine_account_keyfile.go

@@ -3,6 +3,7 @@ package authutil
 import (
 	"crypto/sha256"
 	"encoding/hex"
+	"errors"
 	"fmt"
 	"os"
 	"path/filepath"
@@ -43,20 +44,54 @@ func WriteMachineAccountKeyFile(userKey, pemKey string) (string, error) {
 		return "", fmt.Errorf("failed to create machine account key directory %s: %w", dir, err)
 	}
 
+	// Tighten perms on the directory even when it already existed with looser ones.
+	if err := os.Chmod(dir, 0700); err != nil && !errors.Is(err, os.ErrNotExist) {
+		return "", fmt.Errorf("failed to set permissions on machine account key directory %s: %w", dir, err)
+	}
+
 	destPath, err := MachineAccountKeyFilePath(userKey)
 	if err != nil {
 		return "", err
 	}
 
-	tmpPath := destPath + ".tmp"
-	if err := os.WriteFile(tmpPath, []byte(pemKey), 0600); err != nil {
-		return "", fmt.Errorf("failed to write machine account key to %s: %w", tmpPath, err)
+	// Use a unique temp filename so concurrent logins for the same account
+	// do not race on the same .tmp path and corrupt each other's writes.
+	tmpFile, err := os.CreateTemp(dir, filepath.Base(destPath)+".tmp.*")
+	if err != nil {
+		return "", fmt.Errorf("failed to create temp file for machine account key in %s: %w", dir, err)
+	}
+	tmpName := tmpFile.Name()
+
+	// Ensure the temp file is removed on any error path. After a successful
+	// Rename the file no longer exists at tmpName, so Remove is a no-op.
+	var writeErr error
+	defer func() {
+		if writeErr != nil {
+			_ = os.Remove(tmpName)
+		}
+	}()
+
+	// Be explicit about mode even though CreateTemp already uses 0600.
+	if writeErr = tmpFile.Chmod(0600); writeErr != nil {
+		_ = tmpFile.Close()
+		writeErr = fmt.Errorf("failed to set permissions on temp key file %s: %w", tmpName, writeErr)
+		return "", writeErr
+	}
+
+	if _, writeErr = tmpFile.Write([]byte(pemKey)); writeErr != nil {
+		_ = tmpFile.Close()
+		writeErr = fmt.Errorf("failed to write machine account key to %s: %w", tmpName, writeErr)
+		return "", writeErr
+	}
+
+	if writeErr = tmpFile.Close(); writeErr != nil {
+		writeErr = fmt.Errorf("failed to close temp key file %s: %w", tmpName, writeErr)
+		return "", writeErr
 	}
 
-	if err := os.Rename(tmpPath, destPath); err != nil {
-		// Best-effort cleanup of the temp file on rename failure.
-		_ = os.Remove(tmpPath)
-		return "", fmt.Errorf("failed to move machine account key to %s: %w", destPath, err)
+	if writeErr = os.Rename(tmpName, destPath); writeErr != nil {
+		writeErr = fmt.Errorf("failed to move machine account key to %s: %w", destPath, writeErr)
+		return "", writeErr
 	}
 
 	return destPath, nil

internal/authutil/machineaccount.go

@@ -203,11 +203,19 @@ func (m *machineAccountTokenSource) Token() (*oauth2.Token, error) {
 		var readErr error
 		pemKey, readErr = ReadMachineAccountKeyFile(ma.PrivateKeyPath)
 		if readErr != nil {
-			return nil, fmt.Errorf("failed to read machine account private key from %s: %w (try logging in again with 'datumctl auth login --credentials')", ma.PrivateKeyPath, readErr)
+			return nil, customerrors.WrapUserErrorWithHint(
+				"failed to read machine account private key from "+ma.PrivateKeyPath,
+				"re-run 'datumctl auth login --credentials <file>'; you may need to download a new machine account credentials file from the Datum portal if the original is no longer available",
+				readErr,
+			)
 		}
 	}
 	if pemKey == "" {
-		return nil, fmt.Errorf("machine account session is missing its private key; log in again with 'datumctl auth login --credentials'")
+		return nil, customerrors.WrapUserErrorWithHint(
+			"machine account session is missing its private key",
+			"re-run 'datumctl auth login --credentials <file>'; you may need to download a new machine account credentials file from the Datum portal if the original is no longer available",
+			nil,
+		)
 	}
 
 	signedJWT, err := MintJWT(ma.ClientID, ma.PrivateKeyID, pemKey, ma.TokenURI)

internal/cmd/auth/logout.go

@@ -91,6 +91,11 @@ func logoutSingleUser(userKeyToLogout string) error {
 		if err := keyring.Delete(authutil.ServiceName, userKeyToLogout); err != nil && !errors.Is(err, keyring.ErrNotFound) {
 			fmt.Printf("Warning: attempt to delete potential stray key for %s failed: %v\n", userKeyToLogout, err)
 		}
+		// Also remove any stray on-disk PEM file. This is the exact cleanup path
+		// users hit after a failed login left crypto material behind (issue #146).
+		if removeErr := authutil.RemoveMachineAccountKeyFile(userKeyToLogout); removeErr != nil {
+			fmt.Printf("Warning: failed to remove machine account key file for '%s': %v\n", userKeyToLogout, removeErr)
+		}
 		return nil
 	}
 

internal/cmd/auth/machine_account_login.go

@@ -160,6 +160,11 @@ func runMachineAccountLogin(ctx context.Context, credentialsPath, hostname, apiH
 	}
 
 	if err := keyring.Set(authutil.ServiceName, userKey, string(credsJSON)); err != nil {
+		// The PEM key was written to disk but the keyring write failed. Remove the
+		// key file as best-effort cleanup so we don't leave crypto material behind.
+		if cleanupErr := authutil.RemoveMachineAccountKeyFile(userKey); cleanupErr != nil {
+			fmt.Printf("Warning: failed to remove machine account key file after keyring error for %s: %v\n", userKey, cleanupErr)
+		}
 		return fmt.Errorf("failed to store credentials in keyring for %s: %w", userKey, err)
 	}