fix: SSHConfig: atomically write ssh config #511
Conversation
johnstcn
force-pushed
the
cj/config-ssh/atomic-write
branch
from
9b53e6c to
2c491b7
Compare
johnstcn
force-pushed
the
cj/config-ssh/handle-malformed
branch
from
09777c5 to
eda19a2
Compare
johnstcn
force-pushed
the
cj/config-ssh/atomic-write
branch
from
2c491b7 to
5277656
Compare
src/sshConfig.ts
Outdated
| return this.fileSystem.writeFile(this.filePath, this.getRaw(), { | ||
| const randSuffix = Math.random().toString(36).substring(8) | ||
| const tempFilePath = `${this.filePath}.${randSuffix}` | ||
| await this.fileSystem.writeFile(tempFilePath, this.getRaw(), { |
There was a problem hiding this comment.
I am probably overthinking this, but if there is somehow a file with this suffix already, would we want to error instead of overwrite? We could use flag: "wx" on the writeFile call to error if the path already exists.
Vanishingly unlikely, but maybe there could be a freak collision where a user has a ~/.ssh/config.conf and the random suffix was also conf.
There was a problem hiding this comment.
- We could just write to
/tmp - We could instead name it something like
config.vscode-coder.${timestamp}.${randSuffix}to minimize the likelihood of collisions
Thoughts?
There was a problem hiding this comment.
I like 2 personally because I think it gives us a better chance to get the temp files for debugging, with /tmp they could be long gone.
But I could see an argument for /tmp to avoid "clutter".
There was a problem hiding this comment.
What about a ULID? It would be sufficiently random and you could sort them by time to determine write order when debugging.
There was a problem hiding this comment.
If we want to guarantee atomicity, same folder is best (/tmp for instance could be on another filesystem).
To hide clutter in the case of error, I'd suggest a . prefix on the filename, perhaps .tmp.[filename].[rand]. At this point guaranteeing uniqueness beyond this would seem like overkill.
I.e. my suggestion is just /home/user/.ssh/config -> /home/user/.ssh/.tmp.config.XXXXXX.
One use-case that won't work here is if /home/user/.ssh/config is bind-mounted. In that situation we either error out or try to write the file directly. Erroring is obviously safest so perhaps we go with that.
There was a problem hiding this comment.
What about a ULID?
I like the idea in theory, but it would be another import.
There was a problem hiding this comment.
Forgot to write down one more thing, we should also respect the existing file modes and permissions and make sure they're copied over.
There was a problem hiding this comment.
Done. I've also adjusted the tempfile naming convention to ~/.ssh/.config.vscode-coder-tmp.abc123 to make it abundantly clear that
- This refers to
~/.ssh/config - It was created by
vscode-coder
johnstcn
force-pushed
the
cj/config-ssh/atomic-write
branch
from
5277656 to
698b334
Compare
johnstcn
force-pushed
the
cj/config-ssh/atomic-write
branch
from
71e2751 to
8ac1e78
Compare
Relates to #504
To avoid race conditions, write to a tempfile and then rename.
Smoke-tested by:
Setting immutable flag on
/Users/cian/.ssh/config(sudo chflags uchg $PATH) and validating that the rename failed:Setting immutable flag on
/Users/cian/.sshand validating that writing the temp file failed:Once immutable flag removed (
sudo chflags nouchg $PATH), connecting to workspace is successful.