Skip to content

Add serialization of writes to known_hosts file.#16057

Merged
AntonAM merged 1 commit into
masterfrom
anton/15298-tsh-known-hosts-writing
Sep 7, 2022
Merged

Add serialization of writes to known_hosts file.#16057
AntonAM merged 1 commit into
masterfrom
anton/15298-tsh-known-hosts-writing

Conversation

@AntonAM
Copy link
Copy Markdown
Contributor

@AntonAM AntonAM commented Sep 1, 2022

We had a situation when multiple instances of tsh tried to write to known_hosts file and eventually corrupting it.

To fix it we're trying to serlialize writes to the file, by first locking on it. Locking was chosen over lock files because it seems like a cleaner approach and we don't need to handle situation with leftover lock files in case something unexpected happens. With locks if process dies file is freed automatically.
I also added locking to the reads to account for potential situation when one instance of tsh partly written content to the file and another intance tried to read and parse it. Not sure, how probable this potential situation is, but since the cost of locking is so low in our use case, I think it makes sense to add it anyway.

Fixes #15298

@AntonAM AntonAM requested review from r0mant and tigrato September 1, 2022 17:22
@AntonAM AntonAM force-pushed the anton/15298-tsh-known-hosts-writing branch 5 times, most recently from 96fb5e3 to 91c9e36 Compare September 2, 2022 16:48
@AntonAM AntonAM marked this pull request as ready for review September 2, 2022 17:55
Copy link
Copy Markdown
Contributor

@greedy52 greedy52 left a comment

Choose a reason for hiding this comment

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

The file lock is cool. I think it can be applied to #15577 as well eventually.

Comment thread lib/client/keystore.go
Comment thread lib/client/keystore.go
Comment thread lib/client/keystore.go
Comment thread lib/client/keystore.go
Comment thread lib/client/keystore.go
@github-actions github-actions Bot removed the request for review from r0mant September 2, 2022 22:28
@AntonAM AntonAM force-pushed the anton/15298-tsh-known-hosts-writing branch from 91c9e36 to 6361812 Compare September 6, 2022 22:41
@AntonAM AntonAM added backport/branch/v9 tsh tsh - Teleport's command line tool for logging into nodes running Teleport. labels Sep 6, 2022
@AntonAM AntonAM force-pushed the anton/15298-tsh-known-hosts-writing branch from 6361812 to 284e7b8 Compare September 7, 2022 03:57
@AntonAM AntonAM merged commit 135735e into master Sep 7, 2022
@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Sep 7, 2022

@AntonAM See the table below for backport results.

Branch Result
branch/v10 Failed
branch/v9 Failed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

tsh tsh - Teleport's command line tool for logging into nodes running Teleport.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

tsh should not allow 3rd party tool inspects kubeconfig and executes tsh kube credentials

4 participants