Refactor file writing out to decouple from Secrets #95
Conversation
| // 2. Nobody observes a partially-overwritten secret file. | ||
| // Since keysync is intended to write to tmpfs, this function doesn't do the necessary fsyncs if it | ||
| // were persisting content to disk. | ||
| // Write puts a Secret into OutputDir | ||
| func (out *OutputDir) Write(secret *Secret) (*secretState, error) { |
There was a problem hiding this comment.
This function stays the same (and all its tests still pass), just the core file logic is factored out into output.WriteFileAtomically
| @@ -0,0 +1,111 @@ | |||
| package output | |||
There was a problem hiding this comment.
All of the code in this file is extracted from write.go at the top level: WriteFileAtomically is a new function extracted from OutputDir.Write(Secret), and the rest is straight moved.
mcpherrinm
force-pushed
the
mmc-refactor
branch
from
e85be05
to
f6d86f5
Compare
This makes a new function which handles atomically writing files with a given mode and permissions. I plan to reuse this code in the future, so this is just the first part of seperation The package is called "output" because I plan to move the Output* code there too, but that requires a bit more refactoring to avoid circular dependencies. So that's another PR for later. Normalize uid and gid to int, instead of uint32, as int is what f.Chown takes.
mcpherrinm
force-pushed
the
mmc-refactor
branch
from
f6d86f5
to
6b9898f
Compare
| // 1. Nobody can open the file before we set owner/permissions properly | ||
| // 2. Nobody observes a partially-overwritten secret file. | ||
| // The returned FileInfo may not match the passed in one, especially if chownFiles is false. | ||
| func WriteFileAtomically(dir, filename string, chownFiles bool, fileInfo FileInfo, enforceFilesystem Filesystem, content []byte) (*FileInfo, error) { |
There was a problem hiding this comment.
This function takes a lot of arguments but they're all typed (except the first two) so I think it's okish
| // So both are safe to store as an int64. | ||
| // Linux Tmpfs = 0x01021994 | ||
| // Get these constants with `stat --file-system --format=%t` | ||
| type Filesystem int64 |
There was a problem hiding this comment.
nit: maybe call it FilesystemID?
There was a problem hiding this comment.
oh, i see this is just a copy. maybe a future improvement/tweak then
|
|
||
| // Rename is atomic, so nobody will observe a partially updated secret | ||
| err = os.Rename(fullPath+randSuffix, fullPath) | ||
| fileinfo, err := output.WriteFileAtomically(out.WriteDirectory, filename, out.ChownFiles, fileInfo, out.EnforceFilesystem, secret.Content) |
There was a problem hiding this comment.
this looks much nicer now! 👍
| @@ -10,16 +10,16 @@ import ( | |||
| // It is intended to be used with the implementation on Os. There's also one in mock.go that | |||
| // uses fixed data instead of operating-system sourced data. | |||
| type Lookup interface { | |||
| UID(username string) (uint32, error) | |||
There was a problem hiding this comment.
Not sure why we're changing it from uint32 to int. uid/gid are unsigned 32bit ints on POSIX systems, IIRC, but I don't know if it matters in reality.
There was a problem hiding this comment.
ultimately the point of these values are to call Chown, which takes int.
https://golang.org/pkg/os/#Chown
| return nil, fmt.Errorf("checking filesystem type: %v", err) | ||
| } | ||
| if !good { | ||
| return nil, fmt.Errorf("unexpected filesystem writing %s", filename) |
There was a problem hiding this comment.
nit: would logging the expected filesystem here leak important information? If not, might be useful for debugging.
There was a problem hiding this comment.
This is related to issue #54 where it's hard to understand/debug the filesystem checking. Added a note about this, there, for future improvement
This makes a new function which handles atomically writing files with a given
mode and permissions. I plan to reuse this code in the future, so this is just
the first part of seperation
The package is called "output" because I plan to move the Output* code there
too, but that requires a bit more refactoring to avoid circular dependencies.
So that's another PR for later.