-
Notifications
You must be signed in to change notification settings - Fork 16
Refactor file writing out to decouple from Secrets #95
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: would logging the expected filesystem here leak important information? If not, might be useful for debugging. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: maybe call it There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 | ||
} |
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 { | ||
UID(username string) (uint32, error) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not sure why we're changing it from There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ultimately the point of these values are to call Chown, which takes int. |
||
GID(groupname string) (uint32, error) | ||
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) (uint32, error) { | ||
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 uint32(id), nil | ||
return int(id), nil | ||
} | ||
|
||
func (o Os) GID(groupname string) (uint32, error) { | ||
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 uint32(id), nil | ||
return int(id), nil | ||
} |
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.