From f6d86f583b2513d1f3ccf89889bd675d41368e55 Mon Sep 17 00:00:00 2001 From: Matthew McPherrin Date: Thu, 22 Aug 2019 15:38:52 -0700 Subject: [PATCH] Refactor file writing out to decouple from Secrets 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. --- config.go | 42 +++++++++-------- output/write.go | 111 ++++++++++++++++++++++++++++++++++++++++++++ syncer.go | 4 +- write.go | 119 ++++++------------------------------------------ 4 files changed, 150 insertions(+), 126 deletions(-) create mode 100644 output/write.go diff --git a/config.go b/config.go index 3659bb7..27d3144 100644 --- a/config.go +++ b/config.go @@ -22,31 +22,33 @@ import ( "strings" "time" + "github.com/square/keysync/output" + yaml "gopkg.in/yaml.v2" ) // Config is the main yaml configuration file passed to the keysync binary type Config struct { - ClientsDir string `yaml:"client_directory"` // A directory of configuration files - SecretsDir string `yaml:"secrets_directory"` // The directory secrets will be written to - CaFile string `yaml:"ca_file"` // The CA to trust (PEM) for Keywhiz communication - YamlExt string `yaml:"yaml_ext"` // The filename extension of the yaml config files - PollInterval string `yaml:"poll_interval"` // If specified, poll at the given interval, otherwise, exit after syncing - ClientTimeout string `yaml:"client_timeout"` // If specified, timeout client connections after specified duration, otherwise use default. - MinBackoff string `yaml:"min_backoff"` // If specified, wait time before first retry, otherwise, use default. - MaxBackoff string `yaml:"max_backoff"` // If specified, max wait time before retries, otherwise, use default. - MaxRetries uint16 `yaml:"max_retries"` // If specified, retry each HTTP call after non-200 response - Server string `yaml:"server"` // The server to connect to (host:port) - Debug bool `yaml:"debug"` // Enable debugging output - DefaultUser string `yaml:"default_user"` // Default user to own files - DefaultGroup string `yaml:"default_group"` // Default group to own files - APIPort uint16 `yaml:"api_port"` // Port for API to listen on - SentryDSN string `yaml:"sentry_dsn"` // Sentry DSN - SentryCaFile string `yaml:"sentry_ca_file"` // The CA to trust (PEM) for Sentry communication - FsType Filesystem `yaml:"filesystem_type"` // Enforce writing this type of filesystem. Use value from statfs. - ChownFiles bool `yaml:"chown_files"` // Do we chown files? Set to false when running without CAP_CHOWN. - MetricsPrefix string `yaml:"metrics_prefix"` // Prefix metric names with this - Monitor MonitorConfig `yaml:"monitor"` // Config for monitoring/alerts + ClientsDir string `yaml:"client_directory"` // A directory of configuration files + SecretsDir string `yaml:"secrets_directory"` // The directory secrets will be written to + CaFile string `yaml:"ca_file"` // The CA to trust (PEM) for Keywhiz communication + YamlExt string `yaml:"yaml_ext"` // The filename extension of the yaml config files + PollInterval string `yaml:"poll_interval"` // If specified, poll at the given interval, otherwise, exit after syncing + ClientTimeout string `yaml:"client_timeout"` // If specified, timeout client connections after specified duration, otherwise use default. + MinBackoff string `yaml:"min_backoff"` // If specified, wait time before first retry, otherwise, use default. + MaxBackoff string `yaml:"max_backoff"` // If specified, max wait time before retries, otherwise, use default. + MaxRetries uint16 `yaml:"max_retries"` // If specified, retry each HTTP call after non-200 response + Server string `yaml:"server"` // The server to connect to (host:port) + Debug bool `yaml:"debug"` // Enable debugging output + DefaultUser string `yaml:"default_user"` // Default user to own files + DefaultGroup string `yaml:"default_group"` // Default group to own files + APIPort uint16 `yaml:"api_port"` // Port for API to listen on + SentryDSN string `yaml:"sentry_dsn"` // Sentry DSN + SentryCaFile string `yaml:"sentry_ca_file"` // The CA to trust (PEM) for Sentry communication + FsType output.Filesystem `yaml:"filesystem_type"` // Enforce writing this type of filesystem. Use value from statfs. + ChownFiles bool `yaml:"chown_files"` // Do we chown files? Set to false when running without CAP_CHOWN. + MetricsPrefix string `yaml:"metrics_prefix"` // Prefix metric names with this + Monitor MonitorConfig `yaml:"monitor"` // Config for monitoring/alerts } // The MonitorConfig has extra settings for monitoring/alerts. diff --git a/output/write.go b/output/write.go new file mode 100644 index 0000000..c5cffa2 --- /dev/null +++ b/output/write.go @@ -0,0 +1,111 @@ +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 uint32 + GID uint32 +} + +// 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 := stat.Sys().(*syscall.Stat_t).Uid + gid := 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. +func WriteFileAtomically(dir, filename string, mode os.FileMode, chownFiles bool, uid, gid int, enforceFilesystem Filesystem, content []byte) (*FileInfo, error) { + 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(uid, 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(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) + } + } + _, 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 + +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 +} diff --git a/syncer.go b/syncer.go index ea49252..2d72ce0 100644 --- a/syncer.go +++ b/syncer.go @@ -24,6 +24,8 @@ import ( "time" "unsafe" + "github.com/square/keysync/output" + "github.com/sirupsen/logrus" "github.com/square/go-sq-metrics" ) @@ -39,7 +41,7 @@ type secretState struct { // Checksum is the server's identifier for the contents of the hash (it's an HMAC) Checksum string // We store the mode we wrote to the filesystem - FileInfo + output.FileInfo // Owner, Group, and Mode come from the Keywhiz server Owner string Group string diff --git a/write.go b/write.go index 290de0f..19b1b5f 100644 --- a/write.go +++ b/write.go @@ -16,18 +16,15 @@ package keysync import ( "bytes" - "crypto/rand" "crypto/sha256" - "encoding/hex" "fmt" "io/ioutil" "os" "path/filepath" - "syscall" + "github.com/square/keysync/output" "github.com/square/keysync/ownership" - pkgerr "github.com/pkg/errors" "github.com/sirupsen/logrus" ) @@ -106,8 +103,8 @@ func (c OutputDirCollection) Cleanup(known map[string]struct{}, logger *logrus.E type OutputDir struct { WriteDirectory string DefaultOwnership ownership.Ownership - EnforceFilesystem Filesystem // What filesystem type do we expect to write to? - ChownFiles bool // Do we chown the file? (Needs root or CAP_CHOWN). + EnforceFilesystem output.Filesystem // What filesystem type do we expect to write to? + ChownFiles bool // Do we chown the file? (Needs root or CAP_CHOWN). Logger *logrus.Entry } @@ -133,7 +130,7 @@ func (out *OutputDir) Validate(secret *Secret, state secretState) bool { if err != nil { return false } - fileinfo, err := GetFileInfo(f) + fileinfo, err := output.GetFileInfo(f) if err != nil { return false } @@ -192,125 +189,37 @@ func (out *OutputDir) Cleanup(secrets map[string]Secret) error { return nil } -// FileInfo returns the filesystem properties atomicWrite wrote -type FileInfo struct { - Mode os.FileMode - UID uint32 - GID uint32 -} - -// 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 := stat.Sys().(*syscall.Stat_t).Uid - gid := stat.Sys().(*syscall.Stat_t).Gid - - return &FileInfo{filemode, uid, gid}, nil -} - -// atomicWrite 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. -// 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) { - filename, err := secret.Filename() - if err != nil { - return nil, pkgerr.Wrap(err, "cannot write to file") - } - - if err := os.MkdirAll(out.WriteDirectory, 0775); err != nil { - return nil, fmt.Errorf("making client directory '%s': %v", out.WriteDirectory, 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(out.WriteDirectory, 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 out.ChownFiles { - ownership := secret.OwnershipValue(out.DefaultOwnership) - - err = f.Chown(int(ownership.UID), int(ownership.GID)) - if err != nil { - return nil, err - } - } - mode, err := secret.ModeValue() if err != nil { return nil, err } - // Always Chmod after the Chown, so we don't expose secret with the wrong owner. - err = f.Chmod(mode) + filename, err := secret.Filename() if err != nil { return nil, err - - } - - if out.EnforceFilesystem != 0 { - good, err := isFilesystem(f, out.EnforceFilesystem) - if err != nil { - return nil, fmt.Errorf("checking filesystem type: %v", err) - } - if !good { - return nil, fmt.Errorf("unexpected filesystem writing %s", filename) - } - } - _, err = f.Write(secret.Content) - if err != nil { - return nil, fmt.Errorf("failed writing filesystem content: %v", err) } - filemode, err := GetFileInfo(f) - if err != nil { - return nil, fmt.Errorf("failed to get file mode back from file: %v", err) + var uid, gid int + if out.ChownFiles { + owner := secret.OwnershipValue(out.DefaultOwnership) + uid = int(owner.UID) + gid = int(owner.GID) } - // 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) + fileinfo, err := output.WriteFileAtomically(out.WriteDirectory, filename, mode, out.ChownFiles, uid, gid, out.EnforceFilesystem, secret.Content) if err != nil { return nil, err } + state := secretState{ ContentHash: sha256.Sum256(secret.Content), Checksum: secret.Checksum, - FileInfo: *filemode, + FileInfo: *fileinfo, Owner: secret.Owner, Group: secret.Group, Mode: secret.Mode, } return &state, err } - -// 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 - -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 -}