From bff527622d597048c401d42da6cded255f01a48e Mon Sep 17 00:00:00 2001 From: Andrew Lytvynov Date: Fri, 19 Apr 2024 13:37:21 -0700 Subject: [PATCH] ipn/ipnlocal,clientupdate: disallow auto-updates in containers (#11814) Containers are typically immutable and should be updated as a whole (and not individual packages within). Deny enablement of auto-updates in containers. Also, add the missing check in EditPrefs in LocalAPI, to catch cases like tailnet default auto-updates getting enabled for nodes that don't support it. Updates #11544 Signed-off-by: Andrew Lytvynov --- clientupdate/clientupdate.go | 28 ++++++++++++++------------- ipn/ipnlocal/local.go | 10 ++++++++++ ipn/ipnlocal/local_test.go | 37 ++++++++++++++++++++++++++++++++++++ 3 files changed, 62 insertions(+), 13 deletions(-) diff --git a/clientupdate/clientupdate.go b/clientupdate/clientupdate.go index 2bfb0f294..f3b8dca7c 100644 --- a/clientupdate/clientupdate.go +++ b/clientupdate/clientupdate.go @@ -29,6 +29,7 @@ "github.com/google/uuid" "tailscale.com/clientupdate/distsign" + "tailscale.com/hostinfo" "tailscale.com/types/logger" "tailscale.com/util/cmpver" "tailscale.com/util/winutil" @@ -162,9 +163,10 @@ func NewUpdater(args Arguments) (*Updater, error) { type updateFunction func() error func (up *Updater) getUpdateFunction() (fn updateFunction, canAutoUpdate bool) { + canAutoUpdate = !hostinfo.New().Container.EqualBool(true) // EqualBool(false) would return false if the value is not set. switch runtime.GOOS { case "windows": - return up.updateWindows, true + return up.updateWindows, canAutoUpdate case "linux": switch distro.Get() { case distro.NixOS: @@ -178,20 +180,20 @@ func (up *Updater) getUpdateFunction() (fn updateFunction, canAutoUpdate bool) { // auto-update mechanism. return up.updateSynology, false case distro.Debian: // includes Ubuntu - return up.updateDebLike, true + return up.updateDebLike, canAutoUpdate case distro.Arch: if up.archPackageInstalled() { // Arch update func just prints a message about how to update, // it doesn't support auto-updates. return up.updateArchLike, false } - return up.updateLinuxBinary, true + return up.updateLinuxBinary, canAutoUpdate case distro.Alpine: - return up.updateAlpineLike, true + return up.updateAlpineLike, canAutoUpdate case distro.Unraid: - return up.updateUnraid, true + return up.updateUnraid, canAutoUpdate case distro.QNAP: - return up.updateQNAP, true + return up.updateQNAP, canAutoUpdate } switch { case haveExecutable("pacman"): @@ -200,21 +202,21 @@ func (up *Updater) getUpdateFunction() (fn updateFunction, canAutoUpdate bool) { // it doesn't support auto-updates. return up.updateArchLike, false } - return up.updateLinuxBinary, true + return up.updateLinuxBinary, canAutoUpdate case haveExecutable("apt-get"): // TODO(awly): add support for "apt" // The distro.Debian switch case above should catch most apt-based // systems, but add this fallback just in case. - return up.updateDebLike, true + return up.updateDebLike, canAutoUpdate case haveExecutable("dnf"): - return up.updateFedoraLike("dnf"), true + return up.updateFedoraLike("dnf"), canAutoUpdate case haveExecutable("yum"): - return up.updateFedoraLike("yum"), true + return up.updateFedoraLike("yum"), canAutoUpdate case haveExecutable("apk"): - return up.updateAlpineLike, true + return up.updateAlpineLike, canAutoUpdate } // If nothing matched, fall back to tarball updates. if up.Update == nil { - return up.updateLinuxBinary, true + return up.updateLinuxBinary, canAutoUpdate } case "darwin": switch { @@ -230,7 +232,7 @@ func (up *Updater) getUpdateFunction() (fn updateFunction, canAutoUpdate bool) { return nil, false } case "freebsd": - return up.updateFreeBSD, true + return up.updateFreeBSD, canAutoUpdate } return nil, false } diff --git a/ipn/ipnlocal/local.go b/ipn/ipnlocal/local.go index d783fa72f..4c57502aa 100644 --- a/ipn/ipnlocal/local.go +++ b/ipn/ipnlocal/local.go @@ -2974,6 +2974,9 @@ func (b *LocalBackend) checkPrefsLocked(p *ipn.Prefs) error { if err := b.checkFunnelEnabledLocked(p); err != nil { errs = append(errs, err) } + if err := b.checkAutoUpdatePrefsLocked(p); err != nil { + errs = append(errs, err) + } return multierr.New(errs...) } @@ -3064,6 +3067,13 @@ func (b *LocalBackend) checkFunnelEnabledLocked(p *ipn.Prefs) error { return nil } +func (b *LocalBackend) checkAutoUpdatePrefsLocked(p *ipn.Prefs) error { + if p.AutoUpdate.Apply.EqualBool(true) && !clientupdate.CanAutoUpdate() { + return errors.New("Auto-updates are not supported on this platform.") + } + return nil +} + // SetUseExitNodeEnabled turns on or off the most recently selected exit node. // // On success, it returns the resulting prefs (or current prefs, in the case of no change). diff --git a/ipn/ipnlocal/local_test.go b/ipn/ipnlocal/local_test.go index 65fd981a9..c8c658f9d 100644 --- a/ipn/ipnlocal/local_test.go +++ b/ipn/ipnlocal/local_test.go @@ -25,6 +25,7 @@ "golang.org/x/net/dns/dnsmessage" "tailscale.com/appc" "tailscale.com/appc/appctest" + "tailscale.com/clientupdate" "tailscale.com/control/controlclient" "tailscale.com/drive" "tailscale.com/drive/driveimpl" @@ -3402,3 +3403,39 @@ func TestMinLatencyDERPregion(t *testing.T) { }) } } + +func TestEnableAutoUpdates(t *testing.T) { + lb := newTestLocalBackend(t) + + _, err := lb.EditPrefs(&ipn.MaskedPrefs{ + AutoUpdateSet: ipn.AutoUpdatePrefsMask{ + ApplySet: true, + }, + Prefs: ipn.Prefs{ + AutoUpdate: ipn.AutoUpdatePrefs{ + Apply: opt.NewBool(true), + }, + }, + }) + // Enabling may fail, depending on which environment we are running this + // test in. + wantErr := !clientupdate.CanAutoUpdate() + gotErr := err != nil + if gotErr != wantErr { + t.Fatalf("enabling auto-updates: got error: %v (%v); want error: %v", gotErr, err, wantErr) + } + + // Disabling should always succeed. + if _, err := lb.EditPrefs(&ipn.MaskedPrefs{ + AutoUpdateSet: ipn.AutoUpdatePrefsMask{ + ApplySet: true, + }, + Prefs: ipn.Prefs{ + AutoUpdate: ipn.AutoUpdatePrefs{ + Apply: opt.NewBool(false), + }, + }, + }); err != nil { + t.Fatalf("disabling auto-updates: got error: %v", err) + } +}