Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Machine ID: Handle Kernel Version check failing more gracefully #34780

Merged
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
3 changes: 1 addition & 2 deletions lib/tbot/botfs/botfs_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -29,8 +29,7 @@ import (
func TestReadWrite(t *testing.T) {
dir := t.TempDir()

secureWriteExpected, err := HasSecureWriteSupport()
require.NoError(t, err)
secureWriteExpected := HasSecureWriteSupport()

expectedData := []byte{1, 2, 3, 4}

Expand Down
13 changes: 7 additions & 6 deletions lib/tbot/botfs/fs_linux.go
Original file line number Diff line number Diff line change
Expand Up @@ -422,10 +422,10 @@ func ConfigureACL(path string, owner *user.User, opts *ACLOptions) error {
}

// HasACLSupport determines if this binary / system supports ACLs.
func HasACLSupport() (bool, error) {
func HasACLSupport() bool {
// We just assume Linux _can_ support ACLs here, and will test for support
// at runtime.
return true, nil
return true
}

// HasSecureWriteSupport determines if `CreateSecure()` should be supported
Expand All @@ -434,15 +434,16 @@ func HasACLSupport() (bool, error) {
//
// 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) {
func HasSecureWriteSupport() bool {
minKernel := semver.New(Openat2MinKernel)
version, err := utils.KernelVersion()
if err != nil {
return false, trace.Wrap(err)
log.WithError(err).Info("Failed to determine kernel version. It will be assumed secure write support is not available.")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we only ever call this once? It's gonna be annoying if we end up accidentally calling this in a loop.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup - this is only called on initialisation at the mo.

return false
}
if version.LessThan(*minKernel) {
return false, nil
return false
}

return true, nil
return true
}
8 changes: 4 additions & 4 deletions lib/tbot/botfs/fs_other.go
Original file line number Diff line number Diff line change
Expand Up @@ -112,13 +112,13 @@ func ConfigureACL(path string, owner *user.User, opts *ACLOptions) error {

// HasACLSupport determines if this binary / system supports ACLs. This
// catch-all implementation just returns false.
func HasACLSupport() (bool, error) {
return false, nil
func HasACLSupport() bool {
return false
}

// HasSecureWriteSupport determines if `CreateSecure()` should be supported
// on this OS / kernel version. This is only supported on Linux, so this
// catch-all implementation just returns false.
func HasSecureWriteSupport() (bool, error) {
return false, nil
func HasSecureWriteSupport() bool {
return false
}
20 changes: 3 additions & 17 deletions lib/tbot/config/destination_directory.go
Original file line number Diff line number Diff line change
Expand Up @@ -69,16 +69,6 @@ func (dd *DestinationDirectory) CheckAndSetDefaults() error {
return trace.BadParameter("destination path must not be empty")
}

secureSupported, err := botfs.HasSecureWriteSupport()
if err != nil {
return trace.Wrap(err)
}

aclsSupported, err := botfs.HasACLSupport()
if err != nil {
return trace.Wrap(err)
}

switch dd.Symlinks {
case "":
// We default to SymlinksTrySecure. It's become apparent that the
Expand All @@ -89,13 +79,14 @@ func (dd *DestinationDirectory) CheckAndSetDefaults() error {
case botfs.SymlinksInsecure, botfs.SymlinksTrySecure:
// valid
case botfs.SymlinksSecure:
if !secureSupported {
if !botfs.HasSecureWriteSupport() {
return trace.BadParameter("symlink mode %q not supported on this system", dd.Symlinks)
}
default:
return trace.BadParameter("invalid symlinks mode: %q", dd.Symlinks)
}

aclsSupported := botfs.HasACLSupport()
switch dd.ACLs {
case "":
if aclsSupported {
Expand Down Expand Up @@ -162,11 +153,6 @@ func (dd *DestinationDirectory) Init(_ context.Context, subdirs []string) error
}

func (dd *DestinationDirectory) Verify(keys []string) error {
aclsSupported, err := botfs.HasACLSupport()
if err != nil {
return trace.Wrap(err)
}

currentUser, err := user.Current()
if err != nil {
return trace.Wrap(err)
Expand All @@ -189,7 +175,7 @@ func (dd *DestinationDirectory) Verify(keys []string) error {
// Make sure it's worth warning about ACLs for this Destination. If ACLs
// are disabled, unsupported, or the Destination is owned by the bot
// (implying the user is not trying to use ACLs), just bail.
if dd.ACLs == botfs.ACLOff || !aclsSupported || ownedByBot {
if dd.ACLs == botfs.ACLOff || !botfs.HasACLSupport() || ownedByBot {
return nil
}

Expand Down