Improve error handling in tbot start (#11756)

* Improve error handling in `tbot start`

This attempts to improve a number of error handling issues while
loading the bot identity from storage in `tbot start`:
1. Identity loading errors are silently ignored and the bot
   always attempts to generate a new identity from token. This isn't
   always correct and is impossible to debug as the true error is
   never logged. We now debug log these errors.
2. `LoadIdentity()` doesn't properly account for existing-but-empty
   identity files and happily tries to load empty identities from
   `tbot init`. This isn't hugely harmful, but produces nonsensical
   error logs once #1 is fixed.

* Use `O_RDWR` instead of `O_WRONLY` in `botfs.openStandard()`

This behaves the same as the fs_linux secure implementation in
all cases, and moves the open mode to a shared constant for good
measure.

* Add a small unit test for symlinks mode read/write.

* Fail on non-NotFound errors while reading an Identity.

* Add small unit test for empty identities.

* Remove outdated TODO comment

* Apply suggestions from code review

Co-authored-by: Zac Bergquist <[email protected]>

* Address review feedback

Co-authored-by: Zac Bergquist <[email protected]>

tool/tbot/botfs/botfs.go

@@ -76,6 +76,10 @@ const (
 	// Directories need the execute bit set for most operations on their
 	// contents to succeed.
 	DefaultDirMode fs.FileMode = 0700
+
+	// OpenMode is the mode with which files should be opened for reading and
+	// writing.
+	OpenMode int = os.O_CREATE | os.O_RDWR
 )
 
 // ACLOptions contains parameters needed to configure ACLs
@@ -88,9 +92,10 @@ type ACLOptions struct {
 	ReaderUser *user.User
 }
 
-// openStandard attempts to open the given path for writing with O_CREATE set.
+// openStandard attempts to open the given path for reading and writing with
+// O_CREATE set.
 func openStandard(path string) (*os.File, error) {
-	file, err := os.OpenFile(path, os.O_CREATE|os.O_WRONLY, DefaultMode)
+	file, err := os.OpenFile(path, OpenMode, DefaultMode)
 	if err != nil {
 		return nil, trace.ConvertSystemError(err)
 	}

tool/tbot/botfs/botfs_test.go

@@ -0,0 +1,56 @@
+/*
+Copyright 2022 Gravitational, Inc.
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package botfs
+
+import (
+	"bytes"
+	"path/filepath"
+	"testing"
+
+	"github.com/stretchr/testify/require"
+)
+
+// TestReadWrite attempts to test read/write against all possible symlink
+// modes.
+func TestReadWrite(t *testing.T) {
+	dir := t.TempDir()
+
+	secureWriteExpected, err := HasSecureWriteSupport()
+	require.NoError(t, err)
+
+	expectedData := []byte{1, 2, 3, 4}
+
+	for _, mode := range []SymlinksMode{SymlinksInsecure, SymlinksTrySecure, SymlinksSecure} {
+		if mode == SymlinksSecure && !secureWriteExpected {
+			t.Logf("skipping secure read/write test due to lack of platform support")
+			continue
+		}
+
+		path := filepath.Join(dir, string(mode))
+
+		err := Create(path, false, mode)
+		require.NoError(t, err)
+
+		err = Write(path, expectedData, mode)
+		require.NoError(t, err)
+
+		data, err := Read(path, mode)
+		require.NoError(t, err)
+
+		require.Equal(t, 0, bytes.Compare(data, expectedData), "read bytes must be equal to those written")
+	}
+}

tool/tbot/botfs/fs_linux.go

@@ -63,7 +63,7 @@ func openSecure(path string) (*os.File, error) {
 		// Equivalent to 0600. Unfortunately it's not worth reusing our
 		// default file mode constant here.
 		Mode:    unix.O_RDONLY | unix.S_IRUSR | unix.S_IWUSR,
-		Flags:   unix.O_RDWR | unix.O_CREAT,
+		Flags:   uint64(OpenMode),
 		Resolve: unix.RESOLVE_NO_SYMLINKS,
 	}
 

tool/tbot/identity/artifact.go

@@ -28,6 +28,7 @@ type Artifact struct {
 	Kind      ArtifactKind
 	ToBytes   func(*Identity) []byte
 	FromBytes func(*proto.Certs, *LoadIdentityParams, []byte)
+	Optional  bool
 }
 
 // Matches returns true if this artifact's Kind matches any one of the given
@@ -136,6 +137,7 @@ var artifacts = []Artifact{
 		FromBytes: func(c *proto.Certs, p *LoadIdentityParams, b []byte) {
 			// nothing to do
 		},
+		Optional: true,
 	},
 }
 

tool/tbot/identity/identity.go

@@ -432,6 +432,14 @@ func LoadIdentity(d destination.Destination, kinds ...ArtifactKind) (*Identity,
 			return nil, trace.WrapWithMessage(err, "could not read artifact %q from destination %s", artifact.Key, d)
 		}
 
+		// We generally expect artifacts to exist beforehand regardless of
+		// whether or not they've actually been written to (due to `tbot init`
+		// or our lightweight init during `tbot start`). If required artifacts
+		// are empty, this identity can't be loaded.
+		if !artifact.Optional && len(data) == 0 {
+			return nil, trace.NotFound("artifact %q is unexpectedly empty in destination %s", artifact.Key, d)
+		}
+
 		artifact.FromBytes(&certs, &params, data)
 	}
 

tool/tbot/identity_test.go

@@ -0,0 +1,42 @@
+/*
+Copyright 2022 Gravitational, Inc.
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+// Note: this lives in main to avoid import cycles since this depends on the
+// config/identity/destinations packages.
+
+package main
+
+import (
+	"testing"
+
+	"github.com/gravitational/teleport/tool/tbot/config"
+	"github.com/gravitational/teleport/tool/tbot/identity"
+	"github.com/gravitational/trace"
+	"github.com/stretchr/testify/require"
+)
+
+func TestLoadEmptyIdentity(t *testing.T) {
+	dir := t.TempDir()
+	dest := config.DestinationDirectory{
+		Path: dir,
+	}
+	require.NoError(t, dest.CheckAndSetDefaults())
+
+	_, err := identity.LoadIdentity(&dest, identity.BotKinds()...)
+	require.Error(t, err)
+
+	require.True(t, trace.IsNotFound(err))
+}

tool/tbot/main.go

@@ -208,11 +208,14 @@ func onStart(botConfig *config.BotConfig) error {
 		if ident != nil {
 			// If ident is set here, we detected a token change above.
 			log.Warnf("Detected a token change, will attempt to fetch a new identity.")
+		} else if trace.IsNotFound(err) {
+			// This is _probably_ a fresh start, so we'll log the true error
+			// and try to fetch a fresh identity.
+			log.Debugf("Identity %s is not found or empty and could not be loaded, will start from scratch: %+v", dest, err)
+		} else {
+			return trace.Wrap(err)
 		}
 
-		// TODO: validate that errors from LoadIdentity are sanely typed; we
-		// actually only want to ignore NotFound errors
-
 		// Verify we can write to the destination.
 		if err := identity.VerifyWrite(dest); err != nil {
 			return trace.Wrap(err, "Could not write to destination %s, aborting.", dest)