This repository has been archived by the owner on Nov 22, 2023. It is now read-only.
Refactor file writing out to decouple from Secrets #95
There are no files selected for viewing
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 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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,112 @@ | ||
| package output | ||
|
|
||
| import ( | ||
| "crypto/rand" | ||
| "encoding/hex" | ||
| "fmt" | ||
| "os" | ||
| "path/filepath" | ||
| "syscall" | ||
| ) | ||
|
|
||
| // FileInfo returns the filesystem properties atomicWrite wrote | ||
| type FileInfo struct { | ||
| Mode os.FileMode | ||
| UID int | ||
| GID int | ||
| } | ||
|
|
||
| // GetFileInfo from an open file | ||
| func GetFileInfo(file *os.File) (*FileInfo, error) { | ||
| stat, err := file.Stat() | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to stat after writing: %v", err) | ||
| } | ||
| filemode := stat.Mode() | ||
| uid := int(stat.Sys().(*syscall.Stat_t).Uid) | ||
| gid := int(stat.Sys().(*syscall.Stat_t).Gid) | ||
|
|
||
| return &FileInfo{filemode, uid, gid}, nil | ||
| } | ||
|
|
||
| // WriteFileAtomically creates a temporary file, sets perms, writes content, and renames it to filename | ||
| // This sequence ensures the following: | ||
| // 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 |
||
| if err := os.MkdirAll(dir, 0775); err != nil { | ||
| return nil, fmt.Errorf("making client directory '%s': %v", dir, err) | ||
| } | ||
|
|
||
| // We can't use ioutil.TempFile because we want to open 0000. | ||
| buf := make([]byte, 32) | ||
| _, err := rand.Read(buf) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| randSuffix := hex.EncodeToString(buf) | ||
| fullPath := filepath.Join(dir, filename) | ||
| f, err := os.OpenFile(fullPath+randSuffix, os.O_RDWR|os.O_CREATE|os.O_EXCL, 0000) | ||
| // Try to remove the file, in event we early-return with an error. | ||
| defer os.Remove(fullPath + randSuffix) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| if chownFiles { | ||
| err = f.Chown(fileInfo.UID, fileInfo.GID) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| } | ||
|
|
||
| // Always Chmod after the Chown, so we don't expose secret with the wrong owner. | ||
| err = f.Chmod(fileInfo.Mode) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| if enforceFilesystem != 0 { | ||
| good, err := isFilesystem(f, enforceFilesystem) | ||
| if err != nil { | ||
| 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 |
||
| } | ||
| } | ||
| _, err = f.Write(content) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed writing filesystem content: %v", err) | ||
| } | ||
|
|
||
| fileinfo, err := GetFileInfo(f) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to get file mode back from file: %v", err) | ||
| } | ||
|
|
||
| // While this is intended for use with tmpfs, you could write secrets to disk. | ||
| // We ignore any errors from syncing, as it's not strictly required. | ||
| _ = f.Sync() | ||
|
|
||
| // Rename is atomic, so nobody will observe a partially updated secret | ||
| err = os.Rename(fullPath+randSuffix, fullPath) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| return fileinfo, nil | ||
| } | ||
|
|
||
| // The Filesystem identification. On Mac, this is uint32, and int64 on linux | ||
| // 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 There was a problem hiding this comment. oh, i see this is just a copy. maybe a future improvement/tweak then |
||
|
|
||
| func isFilesystem(file *os.File, fs Filesystem) (bool, error) { | ||
| var statfs syscall.Statfs_t | ||
| err := syscall.Fstatfs(int(file.Fd()), &statfs) | ||
| return Filesystem(statfs.Type) == fs, err | ||
| } | ||
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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 { | ||
|
There was a problem hiding this comment. Not sure why we're changing it from There was a problem hiding this comment. ultimately the point of these values are to call Chown, which takes int. |
||
| UID(username string) (int, error) | ||
| GID(groupname string) (int, error) | ||
| } | ||
|
|
||
| // Os implements Lookup using the os/user standard library package | ||
| type Os struct{} | ||
|
|
||
| var _ Lookup = Os{} | ||
|
|
||
| func (o Os) UID(username string) (int, error) { | ||
| u, err := user.Lookup(username) | ||
| if err != nil { | ||
| return 0, fmt.Errorf("error resolving uid for %s: %v", username, err) | ||
|
|
@@ -28,10 +28,10 @@ func (o Os) UID(username string) (uint32, error) { | |
| if err != nil { | ||
| return 0, fmt.Errorf("error parsing uid %s for %s: %v", u.Uid, username, err) | ||
| } | ||
| return int(id), nil | ||
| } | ||
|
|
||
| func (o Os) GID(groupname string) (int, error) { | ||
| group, err := user.LookupGroup(groupname) | ||
| if err != nil { | ||
| return 0, fmt.Errorf("error resolving gid for %s: %v", group, err) | ||
|
|
@@ -40,5 +40,5 @@ func (o Os) GID(groupname string) (uint32, error) { | |
| if err != nil { | ||
| return 0, fmt.Errorf("error parsing gid %s for %s: %v", group.Gid, groupname, err) | ||
| } | ||
| return int(id), nil | ||
| } | ||
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 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 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 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 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
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.