From 5607dfd90949a72784fd87603d3515a6e1f3fb56 Mon Sep 17 00:00:00 2001 From: Noah Stride Date: Mon, 16 Oct 2023 11:43:35 +0100 Subject: [PATCH 1/3] Improve warning/err message for secure symlinks --- lib/tbot/botfs/fs_linux.go | 20 +++++++------------- 1 file changed, 7 insertions(+), 13 deletions(-) diff --git a/lib/tbot/botfs/fs_linux.go b/lib/tbot/botfs/fs_linux.go index b3d38f3f8d91e..d336618290252 100644 --- a/lib/tbot/botfs/fs_linux.go +++ b/lib/tbot/botfs/fs_linux.go @@ -90,9 +90,7 @@ func openSymlinksMode(path string, mode OpenMode, symlinksMode SymlinksMode) (*o case SymlinksSecure: file, err = openSecure(path, mode) if err == unix.ENOSYS { - return nil, trace.Errorf("openSecure(%q) failed due to missing "+ - "syscall; `symlinks: insecure` may be required for this "+ - "system", path) + return trace.Errorf("openSecure(%q) failed due to missing syscall; configure `symlinks: insecure` for %q", path, path) } else if err != nil { return nil, trace.Wrap(err) } @@ -100,10 +98,7 @@ func openSymlinksMode(path string, mode OpenMode, symlinksMode SymlinksMode) (*o file, err = openSecure(path, mode) if err == unix.ENOSYS { missingSyscallWarning.Do(func() { - log.Warnf("Failed to write to %q securely due to missing "+ - "syscall; falling back to regular file write. Set "+ - "`symlinks: insecure` on this destination to disable this "+ - "warning.", path) + log.Warnf("Failed to open %q securely due to missing syscall; falling back to regular file handling. Configure `symlinks: insecure` for %q to disable this warning.", path, path) }) file, err = openStandard(path, mode) @@ -169,9 +164,7 @@ func Create(path string, isDir bool, symlinksMode SymlinksMode) error { case SymlinksSecure: if err := createSecure(path, isDir); err != nil { if err == unix.ENOSYS { - return trace.Errorf("createSecure(%q) failed due to missing "+ - "syscall; `symlinks: insecure` may be required for this "+ - "system", path) + return trace.Errorf("createSecure(%q) failed due to missing syscall; configure `symlinks: insecure` for %q", path, path) } return trace.Wrap(err) @@ -191,9 +184,7 @@ func Create(path string, isDir bool, symlinksMode SymlinksMode) error { // It's a bit gross to stuff this sync.Once into a global, but // hopefully that's forgivable since it just manages a log message. missingSyscallWarning.Do(func() { - log.Warnf("Failed to create %q securely due to missing syscall; "+ - "falling back to regular file creation. Set `symlinks: "+ - "insecure` on this destination to disable this warning.", path) + log.Warnf("Failed to create %q securely due to missing syscall; falling back to regular file handling. Configure `symlinks: insecure` for %q to disable this warning.", path, path) }) return trace.Wrap(createStandard(path, isDir)) @@ -440,6 +431,9 @@ func HasACLSupport() (bool, error) { // HasSecureWriteSupport determines if `CreateSecure()` should be supported // on this OS / kernel version. Note that it just checks the kernel version, // so this should be treated as a fallible hint. +// +// We've encountered this being incorrect in environments where access to the +// kernel is hampered e.g. seccomp/apparmor/container runtimes. func HasSecureWriteSupport() (bool, error) { minKernel := semver.New(Openat2MinKernel) version, err := utils.KernelVersion() From 5cb22a8c06587ab9bd9a2914305d43ee3f7fbe11 Mon Sep 17 00:00:00 2001 From: Noah Stride Date: Mon, 16 Oct 2023 11:45:58 +0100 Subject: [PATCH 2/3] Further tweak message --- lib/tbot/botfs/fs_linux.go | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/lib/tbot/botfs/fs_linux.go b/lib/tbot/botfs/fs_linux.go index d336618290252..33181089491d3 100644 --- a/lib/tbot/botfs/fs_linux.go +++ b/lib/tbot/botfs/fs_linux.go @@ -90,7 +90,7 @@ func openSymlinksMode(path string, mode OpenMode, symlinksMode SymlinksMode) (*o case SymlinksSecure: file, err = openSecure(path, mode) if err == unix.ENOSYS { - return trace.Errorf("openSecure(%q) failed due to missing syscall; configure `symlinks: insecure` for %q", path, path) + return trace.Errorf("openSecure failed due to missing syscall; configure `symlinks: insecure` for %q", path) } else if err != nil { return nil, trace.Wrap(err) } @@ -98,7 +98,7 @@ func openSymlinksMode(path string, mode OpenMode, symlinksMode SymlinksMode) (*o file, err = openSecure(path, mode) if err == unix.ENOSYS { missingSyscallWarning.Do(func() { - log.Warnf("Failed to open %q securely due to missing syscall; falling back to regular file handling. Configure `symlinks: insecure` for %q to disable this warning.", path, path) + log.Warnf("Failed to open file securely due to missing syscall; falling back to regular file handling. Configure `symlinks: insecure` for %q to disable this warning.", path) }) file, err = openStandard(path, mode) @@ -164,7 +164,7 @@ func Create(path string, isDir bool, symlinksMode SymlinksMode) error { case SymlinksSecure: if err := createSecure(path, isDir); err != nil { if err == unix.ENOSYS { - return trace.Errorf("createSecure(%q) failed due to missing syscall; configure `symlinks: insecure` for %q", path, path) + return trace.Errorf("createSecure failed due to missing syscall; configure `symlinks: insecure` for %q", path) } return trace.Wrap(err) @@ -184,7 +184,7 @@ func Create(path string, isDir bool, symlinksMode SymlinksMode) error { // It's a bit gross to stuff this sync.Once into a global, but // hopefully that's forgivable since it just manages a log message. missingSyscallWarning.Do(func() { - log.Warnf("Failed to create %q securely due to missing syscall; falling back to regular file handling. Configure `symlinks: insecure` for %q to disable this warning.", path, path) + log.Warnf("Failed to create file securely due to missing syscall; falling back to regular file handling. Configure `symlinks: insecure` for %q to disable this warning.", path) }) return trace.Wrap(createStandard(path, isDir)) From b40fa1206399538de4d6a7fd7c77b33bf3dbf1e5 Mon Sep 17 00:00:00 2001 From: Noah Stride Date: Mon, 16 Oct 2023 12:04:33 +0100 Subject: [PATCH 3/3] Add missing nil return --- lib/tbot/botfs/fs_linux.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/tbot/botfs/fs_linux.go b/lib/tbot/botfs/fs_linux.go index 33181089491d3..1d9502edfb934 100644 --- a/lib/tbot/botfs/fs_linux.go +++ b/lib/tbot/botfs/fs_linux.go @@ -90,7 +90,7 @@ func openSymlinksMode(path string, mode OpenMode, symlinksMode SymlinksMode) (*o case SymlinksSecure: file, err = openSecure(path, mode) if err == unix.ENOSYS { - return trace.Errorf("openSecure failed due to missing syscall; configure `symlinks: insecure` for %q", path) + return nil, trace.Errorf("openSecure failed due to missing syscall; configure `symlinks: insecure` for %q", path) } else if err != nil { return nil, trace.Wrap(err) }