From de43e1fb70ffefd8fe0427de80e4245601b663e1 Mon Sep 17 00:00:00 2001 From: Stephen Levine Date: Mon, 3 Mar 2025 16:24:04 -0500 Subject: [PATCH 01/12] Set umask 0022 for teleport-update --- tool/teleport-update/main.go | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/tool/teleport-update/main.go b/tool/teleport-update/main.go index f8a36e106ffbc..1069e82dcf7fd 100644 --- a/tool/teleport-update/main.go +++ b/tool/teleport-update/main.go @@ -60,6 +60,13 @@ const ( var plog = logutils.NewPackageLogger(teleport.ComponentKey, teleport.ComponentUpdater) +func init() { + // Set 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 before any goroutines that create files are started. + syscall.Umask(0022) +} + func main() { if code := Run(os.Args[1:]); code != 0 { os.Exit(code) From afe046ce103ce4fecee35b845ff1627858d168bd Mon Sep 17 00:00:00 2001 From: Stephen Levine Date: Mon, 3 Mar 2025 17:01:58 -0500 Subject: [PATCH 02/12] init -> main --- tool/teleport-update/main.go | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/tool/teleport-update/main.go b/tool/teleport-update/main.go index 1069e82dcf7fd..61552d540ac76 100644 --- a/tool/teleport-update/main.go +++ b/tool/teleport-update/main.go @@ -60,14 +60,11 @@ const ( var plog = logutils.NewPackageLogger(teleport.ComponentKey, teleport.ComponentUpdater) -func init() { +func main() { // Set 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 before any goroutines that create files are started. - syscall.Umask(0022) -} - -func main() { + syscall.Umask(0o022) if code := Run(os.Args[1:]); code != 0 { os.Exit(code) } From ffc692038873dc34862166eaeb9bb3b904f43768 Mon Sep 17 00:00:00 2001 From: Stephen Levine Date: Mon, 3 Mar 2025 17:23:32 -0500 Subject: [PATCH 03/12] refactor --- lib/autoupdate/agent/installer.go | 1 + lib/autoupdate/agent/updater.go | 16 ++++++++++++++++ tool/teleport-update/main.go | 7 +++---- 3 files changed, 20 insertions(+), 4 deletions(-) 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..7bf86c4df24ab 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" @@ -46,6 +47,9 @@ import ( const ( // BinaryName specifies the name of the updater binary. BinaryName = "teleport-update" + // requiredUmask must be set before this package can be used. + // Use syscall.Umask to set when no other goroutines are running. + requiredUmask = 0o022 ) const ( @@ -67,10 +71,22 @@ 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) { + old := syscall.Umask(requiredUmask) + if old != requiredUmask { + log.WarnContext(ctx, "Non-standard 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 // will not fill disk to within 10 MB of available capacity. +// SetRequiredUmask must be called before any methods are executed. func NewLocalUpdater(cfg LocalUpdaterConfig, ns *Namespace) (*Updater, error) { certPool, err := x509.SystemCertPool() if err != nil { diff --git a/tool/teleport-update/main.go b/tool/teleport-update/main.go index 61552d540ac76..fac38702352fc 100644 --- a/tool/teleport-update/main.go +++ b/tool/teleport-update/main.go @@ -61,10 +61,6 @@ const ( var plog = logutils.NewPackageLogger(teleport.ComponentKey, teleport.ComponentUpdater) func main() { - // Set 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 before any goroutines that create files are started. - syscall.Umask(0o022) if code := Run(os.Args[1:]); code != 0 { os.Exit(code) } @@ -188,6 +184,9 @@ func Run(args []string) int { return 1 } + // Set required umask for all commands, and warn loudly if it changes. + autoupdate.SetRequiredUmask(ctx, plog) + switch command { case enableCmd.FullCommand(): ccfg.Enabled = true From 317c5afc43cfb05a753ce13f405bdfe37ad2ba3a Mon Sep 17 00:00:00 2001 From: Stephen Levine Date: Mon, 3 Mar 2025 17:38:54 -0500 Subject: [PATCH 04/12] move const --- lib/autoupdate/agent/updater.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/lib/autoupdate/agent/updater.go b/lib/autoupdate/agent/updater.go index 7bf86c4df24ab..cd6ffe339c95b 100644 --- a/lib/autoupdate/agent/updater.go +++ b/lib/autoupdate/agent/updater.go @@ -47,9 +47,6 @@ import ( const ( // BinaryName specifies the name of the updater binary. BinaryName = "teleport-update" - // requiredUmask must be set before this package can be used. - // Use syscall.Umask to set when no other goroutines are running. - requiredUmask = 0o022 ) const ( @@ -61,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 From 77fa8a9a841b4783924fa19d4c469d539f486cd7 Mon Sep 17 00:00:00 2001 From: Stephen Levine Date: Mon, 3 Mar 2025 18:46:14 -0500 Subject: [PATCH 05/12] add flag --- lib/autoupdate/agent/updater.go | 11 ++++++++--- tool/teleport-update/main.go | 18 ++++++++++++++++-- 2 files changed, 24 insertions(+), 5 deletions(-) diff --git a/lib/autoupdate/agent/updater.go b/lib/autoupdate/agent/updater.go index cd6ffe339c95b..c3fb9f4a95ffb 100644 --- a/lib/autoupdate/agent/updater.go +++ b/lib/autoupdate/agent/updater.go @@ -73,13 +73,18 @@ const ( // SetRequiredUmask sets the umask to match the systemd umask that the teleport-update service will execute with. // This ensures consistent file permissions. +// If fail is true, SetRequiredUmask will fail if the overridden umask is more restrictive. // NOTE: This must be run in main.go before any goroutines that create files are started. -func SetRequiredUmask(ctx context.Context, log *slog.Logger) { +func SetRequiredUmask(ctx context.Context, log *slog.Logger, fail bool) error { old := syscall.Umask(requiredUmask) - if old != requiredUmask { - log.WarnContext(ctx, "Non-standard umask detected! Umask has been changed to 0022 for teleport-update and all child processes.") + if old&requiredUmask > 0 { + if fail { + return trace.Errorf("refusing to make umask less restrictive") + } + 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.") } + return nil } // NewLocalUpdater returns a new Updater that auto-updates local diff --git a/tool/teleport-update/main.go b/tool/teleport-update/main.go index fac38702352fc..ed5cec3bd3890 100644 --- a/tool/teleport-update/main.go +++ b/tool/teleport-update/main.go @@ -54,6 +54,8 @@ const ( updateGroupEnvVar = "TELEPORT_UPDATE_GROUP" // updateVersionEnvVar forces the version to specified value. updateVersionEnvVar = "TELEPORT_UPDATE_VERSION" + // ignoreUmaskEnvVar allows teleport-update to ignore a restrictive umask. + ignoreUmaskEnvVar = "TELEPORT_UPDATE_IGNORE_UMASK" // updateLockTimeout is the duration commands will wait for update to complete before failing. updateLockTimeout = 10 * time.Minute ) @@ -91,6 +93,7 @@ type cliConfig struct { func Run(args []string) int { var ccfg cliConfig + var ignoreUmask bool ctx := context.Background() ctx, _ = signal.NotifyContext(ctx, os.Interrupt, syscall.SIGTERM) @@ -106,6 +109,8 @@ func Run(args []string) int { Hidden().StringVar(&ccfg.InstallDir) app.Flag("insecure", "Insecure mode disables certificate verification. Do not use in production."). BoolVar(&ccfg.Insecure) + app.Flag("ignore-umask", "Allow teleport-update to override a restrictive umask with 0022."). + Envar(ignoreUmaskEnvVar).BoolVar(&ignoreUmask) app.HelpFlag.Short('h') @@ -184,8 +189,17 @@ func Run(args []string) int { return 1 } - // Set required umask for all commands, and warn loudly if it changes. - autoupdate.SetRequiredUmask(ctx, plog) + // Set required umask for most commands, and warn loudly if it changes. + switch command { + case statusCmd.FullCommand(), versionCmd.FullCommand(): + default: + err := autoupdate.SetRequiredUmask(ctx, plog, !ignoreUmask) + if err != nil { + plog.ErrorContext(ctx, "Restrictive umask detected. The teleport-update command requires umask 0022 or less.", "error", err) + plog.ErrorContext(ctx, "Pass --ignore-umask to allow teleport-update to ignore your umask.") + return 1 + } + } switch command { case enableCmd.FullCommand(): From efd4705a0610b07cd496bcd872a8e1addebfdbdd Mon Sep 17 00:00:00 2001 From: Stephen Levine Date: Mon, 3 Mar 2025 18:49:28 -0500 Subject: [PATCH 06/12] missed not --- lib/autoupdate/agent/updater.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/autoupdate/agent/updater.go b/lib/autoupdate/agent/updater.go index c3fb9f4a95ffb..64dc04e964e8a 100644 --- a/lib/autoupdate/agent/updater.go +++ b/lib/autoupdate/agent/updater.go @@ -77,7 +77,7 @@ const ( // NOTE: This must be run in main.go before any goroutines that create files are started. func SetRequiredUmask(ctx context.Context, log *slog.Logger, fail bool) error { old := syscall.Umask(requiredUmask) - if old&requiredUmask > 0 { + if old&^requiredUmask > 0 { if fail { return trace.Errorf("refusing to make umask less restrictive") } From aed8fcf3d4b8d3f6afe67dec53da3d612e14ff52 Mon Sep 17 00:00:00 2001 From: Stephen Levine Date: Mon, 3 Mar 2025 18:50:33 -0500 Subject: [PATCH 07/12] fix inequality --- lib/autoupdate/agent/updater.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/autoupdate/agent/updater.go b/lib/autoupdate/agent/updater.go index 64dc04e964e8a..fb3bfb652e692 100644 --- a/lib/autoupdate/agent/updater.go +++ b/lib/autoupdate/agent/updater.go @@ -77,7 +77,7 @@ const ( // NOTE: This must be run in main.go before any goroutines that create files are started. func SetRequiredUmask(ctx context.Context, log *slog.Logger, fail bool) error { old := syscall.Umask(requiredUmask) - if old&^requiredUmask > 0 { + if old&^requiredUmask != 0 { if fail { return trace.Errorf("refusing to make umask less restrictive") } From 9ea3fc26d797c4a71e2e884f0f255f9c53a51f9b Mon Sep 17 00:00:00 2001 From: Stephen Levine Date: Mon, 3 Mar 2025 19:19:42 -0500 Subject: [PATCH 08/12] remove flag --- lib/autoupdate/agent/updater.go | 7 +------ tool/teleport-update/main.go | 10 +--------- 2 files changed, 2 insertions(+), 15 deletions(-) diff --git a/lib/autoupdate/agent/updater.go b/lib/autoupdate/agent/updater.go index fb3bfb652e692..c9492d29b97a9 100644 --- a/lib/autoupdate/agent/updater.go +++ b/lib/autoupdate/agent/updater.go @@ -73,18 +73,13 @@ const ( // SetRequiredUmask sets the umask to match the systemd umask that the teleport-update service will execute with. // This ensures consistent file permissions. -// If fail is true, SetRequiredUmask will fail if the overridden umask is more restrictive. // NOTE: This must be run in main.go before any goroutines that create files are started. -func SetRequiredUmask(ctx context.Context, log *slog.Logger, fail bool) error { +func SetRequiredUmask(ctx context.Context, log *slog.Logger) { old := syscall.Umask(requiredUmask) if old&^requiredUmask != 0 { - if fail { - return trace.Errorf("refusing to make umask less restrictive") - } 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.") } - return nil } // NewLocalUpdater returns a new Updater that auto-updates local diff --git a/tool/teleport-update/main.go b/tool/teleport-update/main.go index ed5cec3bd3890..6907b459722b7 100644 --- a/tool/teleport-update/main.go +++ b/tool/teleport-update/main.go @@ -93,7 +93,6 @@ type cliConfig struct { func Run(args []string) int { var ccfg cliConfig - var ignoreUmask bool ctx := context.Background() ctx, _ = signal.NotifyContext(ctx, os.Interrupt, syscall.SIGTERM) @@ -109,8 +108,6 @@ func Run(args []string) int { Hidden().StringVar(&ccfg.InstallDir) app.Flag("insecure", "Insecure mode disables certificate verification. Do not use in production."). BoolVar(&ccfg.Insecure) - app.Flag("ignore-umask", "Allow teleport-update to override a restrictive umask with 0022."). - Envar(ignoreUmaskEnvVar).BoolVar(&ignoreUmask) app.HelpFlag.Short('h') @@ -193,12 +190,7 @@ func Run(args []string) int { switch command { case statusCmd.FullCommand(), versionCmd.FullCommand(): default: - err := autoupdate.SetRequiredUmask(ctx, plog, !ignoreUmask) - if err != nil { - plog.ErrorContext(ctx, "Restrictive umask detected. The teleport-update command requires umask 0022 or less.", "error", err) - plog.ErrorContext(ctx, "Pass --ignore-umask to allow teleport-update to ignore your umask.") - return 1 - } + autoupdate.SetRequiredUmask(ctx, plog) } switch command { From 7fde6deb20fae6fd816ae26a867c73fa975ef64f Mon Sep 17 00:00:00 2001 From: Stephen Levine Date: Mon, 3 Mar 2025 19:21:58 -0500 Subject: [PATCH 09/12] dead code --- tool/teleport-update/main.go | 2 -- 1 file changed, 2 deletions(-) diff --git a/tool/teleport-update/main.go b/tool/teleport-update/main.go index 6907b459722b7..d624602d6ac10 100644 --- a/tool/teleport-update/main.go +++ b/tool/teleport-update/main.go @@ -54,8 +54,6 @@ const ( updateGroupEnvVar = "TELEPORT_UPDATE_GROUP" // updateVersionEnvVar forces the version to specified value. updateVersionEnvVar = "TELEPORT_UPDATE_VERSION" - // ignoreUmaskEnvVar allows teleport-update to ignore a restrictive umask. - ignoreUmaskEnvVar = "TELEPORT_UPDATE_IGNORE_UMASK" // updateLockTimeout is the duration commands will wait for update to complete before failing. updateLockTimeout = 10 * time.Minute ) From 6b75b21f257d0ee12827054b3cd0c83559d213e1 Mon Sep 17 00:00:00 2001 From: Stephen Levine Date: Mon, 3 Mar 2025 19:24:45 -0500 Subject: [PATCH 10/12] docs --- lib/autoupdate/agent/updater.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/lib/autoupdate/agent/updater.go b/lib/autoupdate/agent/updater.go index c9492d29b97a9..73881a7dcebf4 100644 --- a/lib/autoupdate/agent/updater.go +++ b/lib/autoupdate/agent/updater.go @@ -86,7 +86,7 @@ func SetRequiredUmask(ctx context.Context, log *slog.Logger) { // installations of the Teleport agent. // The AutoUpdater uses an HTTP client with sane defaults for downloads, and // will not fill disk to within 10 MB of available capacity. -// SetRequiredUmask must be called before any methods are executed. +// SetRequiredUmask must be called before any methods are executed, except for Status. func NewLocalUpdater(cfg LocalUpdaterConfig, ns *Namespace) (*Updater, error) { certPool, err := x509.SystemCertPool() if err != nil { @@ -561,6 +561,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. From 619df9d0d8976b3c9767c323f77fb1a697f5b9f5 Mon Sep 17 00:00:00 2001 From: Stephen Levine Date: Mon, 3 Mar 2025 19:25:39 -0500 Subject: [PATCH 11/12] docs 2 --- lib/autoupdate/agent/updater.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/autoupdate/agent/updater.go b/lib/autoupdate/agent/updater.go index 73881a7dcebf4..757035426193b 100644 --- a/lib/autoupdate/agent/updater.go +++ b/lib/autoupdate/agent/updater.go @@ -86,7 +86,6 @@ func SetRequiredUmask(ctx context.Context, log *slog.Logger) { // installations of the Teleport agent. // The AutoUpdater uses an HTTP client with sane defaults for downloads, and // will not fill disk to within 10 MB of available capacity. -// SetRequiredUmask must be called before any methods are executed, except for Status. func NewLocalUpdater(cfg LocalUpdaterConfig, ns *Namespace) (*Updater, error) { certPool, err := x509.SystemCertPool() if err != nil { @@ -190,6 +189,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 From 68b224086583754277a4c6b6eb05762b546b3aa4 Mon Sep 17 00:00:00 2001 From: Stephen Levine Date: Tue, 4 Mar 2025 00:57:00 -0500 Subject: [PATCH 12/12] feedback --- lib/autoupdate/agent/updater.go | 5 +++- lib/autoupdate/agent/updater_test.go | 37 ++++++++++++++++++++++++++++ tool/teleport-update/main.go | 2 +- 3 files changed, 42 insertions(+), 2 deletions(-) diff --git a/lib/autoupdate/agent/updater.go b/lib/autoupdate/agent/updater.go index 757035426193b..fbffe7a1510a8 100644 --- a/lib/autoupdate/agent/updater.go +++ b/lib/autoupdate/agent/updater.go @@ -75,7 +75,10 @@ const ( // 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) { - old := syscall.Umask(requiredUmask) + 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.") 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 d624602d6ac10..58ccb6bdd0584 100644 --- a/tool/teleport-update/main.go +++ b/tool/teleport-update/main.go @@ -184,7 +184,7 @@ func Run(args []string) int { return 1 } - // Set required umask for most commands, and warn loudly if it changes. + // 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: