Improve error handling in tbot start
#11756
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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.
|
I've made a dev build of this PR for testing purposes: https://get.gravitational.com/teleport-v10.0.0-dev.200-linux-amd64-bin.tar.gz |
|
@timothyb89 Don't forget to add test coverage. |
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.
r0mant
approved these changes
Apr 8, 2022
…d-improve-identity-loading-errors
zmb3
reviewed
Apr 13, 2022
tool/tbot/botfs/botfs_test.go
Outdated
| data, err := Read(path, test.mode) | ||
| require.NoError(t, err) | ||
|
|
||
| require.Zero(t, bytes.Compare(data, expectedData), "read bytes must be equal to those written") |
There was a problem hiding this comment.
Does require.Equal not work here? I would rather see an error message that shows the diff than an error message that says "got -1 want 0"
There was a problem hiding this comment.
That's fair, will change!
Co-authored-by: Zac Bergquist <[email protected]>
probakowski
approved these changes
Apr 14, 2022
timothyb89
deleted the
timothyb89/machineid-improve-identity-loading-errors
branch
timothyb89
added a commit
that referenced
this pull request
Apr 15, 2022
* 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]>
timothyb89
added a commit
that referenced
this pull request
Apr 20, 2022
* 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]> Co-authored-by: Zac Bergquist <[email protected]>
Closed
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This attempts to improve a number of error handling issues while loading the bot identity from storage in
tbot start:NotFound-type error.LoadIdentity()doesn't properly account for existing-but-empty identity files and happily tries to load empty identities fromtbot init. This isn't hugely harmful, but produces nonsensical error logs once (1) is fixed. We now check to make sure these files are non-empty and return an error if they are.symlinks: insecurebecause the open mode in the fallback case was wrong (O_WRONLYrather thanO_RDWRas it is for secure open). We now move that to a constant and share the opening mode between both implementations.NotFounderror.