From b7058b29c3a159d8eb4ac8a1590d9085027e9c14 Mon Sep 17 00:00:00 2001
From: Stephen Levine <stephen.levine@goteleport.com>
Date: Tue, 4 Mar 2025 15:26:20 -0500
Subject: [PATCH] [v17] [teleport-update] Set umask 0022 for teleport-update to
 avoid errors on enable (#52755)

* Set umask 0022 for teleport-update

* init -> main

* refactor

* move const

* add flag

* missed not

* fix inequality

* remove flag

* dead code

* docs

* docs 2

* feedback
---
 lib/autoupdate/agent/installer.go    |  1 +
 lib/autoupdate/agent/updater.go      | 20 +++++++++++++++
 lib/autoupdate/agent/updater_test.go | 37 ++++++++++++++++++++++++++++
 tool/teleport-update/main.go         |  7 ++++++
 4 files changed, 65 insertions(+)

diff --git a/lib/autoupdate/agent/installer.go b/lib/autoupdate/agent/installer.go
index a3005ccd0406c..96b5d9c3fadd9 100644
--- a/lib/autoupdate/agent/installer.go
+++ b/lib/autoupdate/agent/installer.go
@@ -64,6 +64,7 @@ const (
 
 // LocalInstaller manages the creation and removal of installations
 // of Teleport.
+// SetRequiredUmask must be called before any methods are executed.
 type LocalInstaller struct {
 	// InstallDir contains each installation, named by version.
 	InstallDir string
diff --git a/lib/autoupdate/agent/updater.go b/lib/autoupdate/agent/updater.go
index f42579ea248a0..fbffe7a1510a8 100644
--- a/lib/autoupdate/agent/updater.go
+++ b/lib/autoupdate/agent/updater.go
@@ -30,6 +30,7 @@ import (
 	"path/filepath"
 	"runtime"
 	"slices"
+	"syscall"
 	"time"
 
 	"github.com/gravitational/trace"
@@ -57,6 +58,9 @@ const (
 	reservedFreeDisk = 10_000_000
 	// debugSocketFileName is the name of Teleport's debug socket in the data dir.
 	debugSocketFileName = "debug.sock" // 10 MB
+	// requiredUmask must be set before this package can be used.
+	// Use syscall.Umask to set when no other goroutines are running.
+	requiredUmask = 0o022
 )
 
 // Log keys
@@ -67,6 +71,20 @@ const (
 	errorKey  = "error"
 )
 
+// SetRequiredUmask sets the umask to match the systemd umask that the teleport-update service will execute with.
+// This ensures consistent file permissions.
+// NOTE: This must be run in main.go before any goroutines that create files are started.
+func SetRequiredUmask(ctx context.Context, log *slog.Logger) {
+	warnUmask(ctx, log, syscall.Umask(requiredUmask))
+}
+
+func warnUmask(ctx context.Context, log *slog.Logger, old int) {
+	if old&^requiredUmask != 0 {
+		log.WarnContext(ctx, "Restrictive umask detected. Umask has been changed to 0022 for teleport-update and all child processes.")
+		log.WarnContext(ctx, "All files created by teleport-update will have permissions set according to this umask.")
+	}
+}
+
 // NewLocalUpdater returns a new Updater that auto-updates local
 // installations of the Teleport agent.
 // The AutoUpdater uses an HTTP client with sane defaults for downloads, and
@@ -174,6 +192,7 @@ type LocalUpdaterConfig struct {
 }
 
 // Updater implements the agent-local logic for Teleport agent auto-updates.
+// SetRequiredUmask must be called before any methods are executed, except for Status.
 type Updater struct {
 	// Log contains a logger.
 	Log *slog.Logger
@@ -545,6 +564,7 @@ func isActiveOrEnabled(ctx context.Context, s Process) (bool, error) {
 
 // Status returns all available local and remote fields related to agent auto-updates.
 // Status is safe to run concurrently with other Updater commands.
+// Status does not write files, and therefore does not require SetRequiredUmask.
 func (u *Updater) Status(ctx context.Context) (Status, error) {
 	var out Status
 	// Read configuration from update.yaml.
diff --git a/lib/autoupdate/agent/updater_test.go b/lib/autoupdate/agent/updater_test.go
index cc789c20bca08..082b22e2ac162 100644
--- a/lib/autoupdate/agent/updater_test.go
+++ b/lib/autoupdate/agent/updater_test.go
@@ -19,9 +19,12 @@
 package agent
 
 import (
+	"bytes"
 	"context"
 	"encoding/json"
 	"errors"
+	"fmt"
+	"log/slog"
 	"net/http"
 	"net/http/httptest"
 	"os"
@@ -40,6 +43,40 @@ import (
 	"github.com/gravitational/teleport/lib/utils/testutils/golden"
 )
 
+func TestWarnUmask(t *testing.T) {
+	t.Parallel()
+
+	for _, tt := range []struct {
+		old  int
+		warn bool
+	}{
+		{old: 0o000, warn: false},
+		{old: 0o001, warn: true},
+		{old: 0o011, warn: true},
+		{old: 0o111, warn: true},
+		{old: 0o002, warn: false},
+		{old: 0o020, warn: false},
+		{old: 0o022, warn: false},
+		{old: 0o220, warn: true},
+		{old: 0o200, warn: true},
+		{old: 0o222, warn: true},
+		{old: 0o333, warn: true},
+		{old: 0o444, warn: true},
+		{old: 0o555, warn: true},
+		{old: 0o666, warn: true},
+		{old: 0o777, warn: true},
+	} {
+		t.Run(fmt.Sprintf("old umask %o", tt.old), func(t *testing.T) {
+			ctx := context.Background()
+			out := &bytes.Buffer{}
+			warnUmask(ctx, slog.New(slog.NewTextHandler(out,
+				&slog.HandlerOptions{ReplaceAttr: msgOnly},
+			)), tt.old)
+			assert.Equal(t, tt.warn, strings.Contains(out.String(), "detected"))
+		})
+	}
+}
+
 func TestUpdater_Disable(t *testing.T) {
 	t.Parallel()
 
diff --git a/tool/teleport-update/main.go b/tool/teleport-update/main.go
index f8a36e106ffbc..58ccb6bdd0584 100644
--- a/tool/teleport-update/main.go
+++ b/tool/teleport-update/main.go
@@ -184,6 +184,13 @@ func Run(args []string) int {
 		return 1
 	}
 
+	// Set required umask for commands that write files to system directories as root, and warn loudly if it changes.
+	switch command {
+	case statusCmd.FullCommand(), versionCmd.FullCommand():
+	default:
+		autoupdate.SetRequiredUmask(ctx, plog)
+	}
+
 	switch command {
 	case enableCmd.FullCommand():
 		ccfg.Enabled = true