From 88ee7851c44e07b3f266d7bb8ee652b4eede60a5 Mon Sep 17 00:00:00 2001 From: Fabian Beyer Date: Sat, 8 Aug 2026 18:59:30 +0200 Subject: [PATCH] fix(up): don't pass OLM secret via argv to the detached subprocess In detached mode the parent re-spawned itself with --id/--secret on the command line of the elevated subprocess, leaving the device secret visible to every local process for the daemon's entire lifetime (/proc//cmdline is world-readable), plus transiently in the sudo sh -c wrapper argv. When the credentials come from the account store, stop passing them via argv entirely: the subprocess already resolves the invoking user's config dir via SUDO_USER and reads the store for the session token, so it now reads the OLM credentials from the same place. Explicitly passed --id/--secret flags keep working unchanged. The subprocess also skips EnsureOlmCredentials (the parent just ran it and saved the result), which avoids duplicate API calls and prevents the root subprocess from ever writing the user-owned account store. Co-Authored-By: Claude Fable 5 --- cmd/up/client/client.go | 70 +++++++++++++++++++++++++++-------------- 1 file changed, 46 insertions(+), 24 deletions(-) diff --git a/cmd/up/client/client.go b/cmd/up/client/client.go index 09b3db2..bc7e8e9 100644 --- a/cmd/up/client/client.go +++ b/cmd/up/client/client.go @@ -198,6 +198,12 @@ func clientUpMain(cmd *cobra.Command, opts *ClientUpCmdOpts, extraArgs []string) credentialsFromKeyring := olmID == "" && olmSecret == "" + // Detect whether we are the elevated subprocess spawned by a parent + // `pangolin up` (detached mode). We use an environment variable rather + // than checking if running as root, because the user might run + // "sudo pangolin up" directly and still expect the TUI. + isSubprocess := os.Getenv("PANGOLIN_SUBPROCESS") == "1" + // Determine endpoint early var endpoint string if opts.Endpoint != "" { @@ -249,29 +255,43 @@ func clientUpMain(cmd *cobra.Command, opts *ClientUpCmdOpts, extraArgs []string) return err } - // Ensure OLM credentials exist and are valid - newCredsGenerated, err := utils.EnsureOlmCredentials(apiClient, activeAccount) - if err != nil { - if errors.Is(err, utils.ErrSudoRequired) { - logger.Error("%v", err) - } else { - logger.Error("Failed to ensure OLM credentials: %v", err) - } - return err - } - - if newCredsGenerated { - // fmt.Println("New creds generated saving them") - // Update the account in the store since ActiveAccount() returns a copy - if err := accountStore.UpdateActiveAccount(activeAccount); err != nil { - logger.Error("Failed to update account in store: %v", err) + if isSubprocess { + // The parent process already ensured the credentials exist and + // saved them to the account store; read them back from there + // instead of receiving them via argv, which would expose the + // secret in the process list (/proc//cmdline is + // world-readable). Skipping EnsureOlmCredentials here also + // avoids the elevated subprocess writing the account store. + if activeAccount.OlmCredentials == nil { + err := errors.New("no OLM credentials found in account store") + logger.Error("Error: %v", err) return err } - err := accountStore.Save() + } else { + // Ensure OLM credentials exist and are valid + newCredsGenerated, err := utils.EnsureOlmCredentials(apiClient, activeAccount) if err != nil { - logger.Error("Failed to save accounts to store: %v", err) + if errors.Is(err, utils.ErrSudoRequired) { + logger.Error("%v", err) + } else { + logger.Error("Failed to ensure OLM credentials: %v", err) + } return err } + + if newCredsGenerated { + // fmt.Println("New creds generated saving them") + // Update the account in the store since ActiveAccount() returns a copy + if err := accountStore.UpdateActiveAccount(activeAccount); err != nil { + logger.Error("Failed to update account in store: %v", err) + return err + } + err := accountStore.Save() + if err != nil { + logger.Error("Failed to save accounts to store: %v", err) + return err + } + } } olmID = activeAccount.OlmCredentials.ID @@ -303,9 +323,6 @@ func clientUpMain(cmd *cobra.Command, opts *ClientUpCmdOpts, extraArgs []string) // Handle detached mode - subprocess self without --attach flag // Skip detached mode if we're a subprocess spawned by the parent process - // We use an environment variable to detect this, rather than checking if running as root, - // because the user might run "sudo pangolin up" directly and still expect the TUI - isSubprocess := os.Getenv("PANGOLIN_SUBPROCESS") == "1" if !opts.Attached && !isSubprocess { executable, err := os.Executable() if err != nil { @@ -323,9 +340,14 @@ func clientUpMain(cmd *cobra.Command, opts *ClientUpCmdOpts, extraArgs []string) } // Add all flags that were set (except --attach) - // OLM credentials are always included (from flags, config, or newly created) - cmdArgs = append(cmdArgs, "--id", olmID) - cmdArgs = append(cmdArgs, "--secret", olmSecret) + // OLM credentials are passed via argv only when the user supplied + // them explicitly. When they come from the account store, the + // subprocess reads them from the store itself so the secret never + // appears in the process list (/proc//cmdline is world-readable). + if !credentialsFromKeyring { + cmdArgs = append(cmdArgs, "--id", olmID) + cmdArgs = append(cmdArgs, "--secret", olmSecret) + } // Always pass endpoint to subprocess (required, subprocess won't have user's config) // Get endpoint from flag or hostname config (same logic as attached mode)