From 66215eb694ce43614e78e2d27128045eaa6be74b Mon Sep 17 00:00:00 2001 From: Nick Cabatoff Date: Mon, 27 Apr 2020 18:06:35 -0400 Subject: [PATCH 1/2] Ensure that the .vault-token file writen by `vault login` always has the correct permissions and ownership. --- command/config/util.go | 2 +- command/token/helper_internal.go | 40 +++++++++++++++----- command/token/helper_internal_test.go | 53 ++++++++++++++++++++++++++- go.mod | 1 + go.sum | 2 + vendor/modules.txt | 2 + 6 files changed, 88 insertions(+), 12 deletions(-) diff --git a/command/config/util.go b/command/config/util.go index a9d595161837e..3dd09fdec773f 100644 --- a/command/config/util.go +++ b/command/config/util.go @@ -13,7 +13,7 @@ func DefaultTokenHelper() (token.TokenHelper, error) { path := config.TokenHelper if path == "" { - return &token.InternalTokenHelper{}, nil + return token.NewInternalTokenHelper() } path, err = token.ExternalTokenHelperPath(path) diff --git a/command/token/helper_internal.go b/command/token/helper_internal.go index e5c5b53924631..6c65b9b682098 100644 --- a/command/token/helper_internal.go +++ b/command/token/helper_internal.go @@ -9,6 +9,7 @@ import ( "strings" homedir "github.com/mitchellh/go-homedir" + "github.com/natefinch/atomic" ) var _ TokenHelper = (*InternalTokenHelper)(nil) @@ -17,16 +18,21 @@ var _ TokenHelper = (*InternalTokenHelper)(nil) // token-helper is configured, and avoids shelling out type InternalTokenHelper struct { tokenPath string + homeDir string } -// populateTokenPath figures out the token path using homedir to get the user's -// home directory -func (i *InternalTokenHelper) populateTokenPath() { - homePath, err := homedir.Dir() +func NewInternalTokenHelper() (*InternalTokenHelper, error) { + homeDir, err := homedir.Dir() if err != nil { panic(fmt.Sprintf("error getting user's home directory: %v", err)) } - i.tokenPath = filepath.Join(homePath, ".vault-token") + return &InternalTokenHelper{homeDir: homeDir}, err +} + +// populateTokenPath figures out the token path using homedir to get the user's +// home directory +func (i *InternalTokenHelper) populateTokenPath() { + i.tokenPath = filepath.Join(i.homeDir, ".vault-token") } func (i *InternalTokenHelper) Path() string { @@ -53,21 +59,35 @@ func (i *InternalTokenHelper) Get() (string, error) { return strings.TrimSpace(buf.String()), nil } -// Store stores the value of the token to the file +// Store stores the value of the token to the file. We always overwrite any +// existing file atomically to ensure that ownership and permissions are set +// appropriately. func (i *InternalTokenHelper) Store(input string) error { i.populateTokenPath() - f, err := os.OpenFile(i.tokenPath, os.O_CREATE|os.O_TRUNC|os.O_WRONLY, 0600) + tmpFile := i.tokenPath + ".tmp" + f, err := os.OpenFile(tmpFile, os.O_CREATE|os.O_TRUNC|os.O_WRONLY, 0600) if err != nil { return err } defer f.Close() + defer os.Remove(tmpFile) - buf := bytes.NewBufferString(input) - if _, err := io.Copy(f, buf); err != nil { + _, err = io.WriteString(f, input) + if err != nil { + return err + } + err = f.Close() + if err != nil { return err } - return nil + // We don't care so much about atomic writes here. We're using this package + // because we don't have a portable way of verifying that the target file + // is owned by the correct user. The simplest way of ensuring that is + // to simply re-write it, and the simplest way to ensure that we don't + // damage an existing working file due to error is the write-rename pattern. + // os.Rename on Windows will return an error if the target already exists. + return atomic.ReplaceFile(tmpFile, i.tokenPath) } // Erase erases the value of the token diff --git a/command/token/helper_internal_test.go b/command/token/helper_internal_test.go index 4ac527ce4983d..c0f87662f13a8 100644 --- a/command/token/helper_internal_test.go +++ b/command/token/helper_internal_test.go @@ -1,11 +1,62 @@ package token import ( + "io/ioutil" + "os" + "path/filepath" "testing" ) // TestCommand re-uses the existing Test function to ensure proper behavior of // the internal token helper func TestCommand(t *testing.T) { - Test(t, &InternalTokenHelper{}) + helper, err := NewInternalTokenHelper() + if err != nil { + t.Fatal(err) + } + Test(t, helper) +} + +func TestInternalHelperFilePerms(t *testing.T) { + tmpDir, err := ioutil.TempDir("", t.Name()) + if err != nil { + t.Fatal(err) + } + defer os.RemoveAll(tmpDir) + + helper, err := NewInternalTokenHelper() + if err != nil { + t.Fatal(err) + } + helper.homeDir = tmpDir + + tmpFile := filepath.Join(tmpDir, ".vault-token") + f, err := os.Create(tmpFile) + if err != nil { + t.Fatal(err) + } + defer f.Close() + + fi, err := os.Stat(tmpFile) + if err != nil { + t.Fatal(err) + } + + if fi.Mode().Perm()&004 != 004 { + t.Fatalf("expected world-readable/writable permission bits, got: %o", fi.Mode().Perm()) + } + + err = helper.Store("bogus_token") + if err != nil { + t.Fatal(err) + } + + fi, err = os.Stat(tmpFile) + if err != nil { + t.Fatal(err) + } + + if fi.Mode().Perm()&004 != 0 { + t.Fatalf("expected no world-readable/writable permission bits, got: %o", fi.Mode().Perm()) + } } diff --git a/go.mod b/go.mod index c3f4e47424b6a..4f8ebd3723c41 100644 --- a/go.mod +++ b/go.mod @@ -106,6 +106,7 @@ require ( github.com/mitchellh/go-testing-interface v1.0.0 github.com/mitchellh/mapstructure v1.1.2 github.com/mitchellh/reflectwalk v1.0.1 + github.com/natefinch/atomic v0.0.0-20150920032501-a62ce929ffcc github.com/ncw/swift v1.0.47 github.com/nwaples/rardecode v1.0.0 // indirect github.com/oklog/run v1.0.0 diff --git a/go.sum b/go.sum index eed22a25adbc8..26c9b474a79b8 100644 --- a/go.sum +++ b/go.sum @@ -578,6 +578,8 @@ github.com/mwielbut/pointy v1.1.0 h1:U5/YEfoIkaGCHv0St3CgjduqXID4FNRoyZgLM1kY9vg github.com/mwielbut/pointy v1.1.0/go.mod h1:MvvO+uMFj9T5DMda33HlvogsFBX7pWWKAkFIn4teYwY= github.com/mwitkow/go-conntrack v0.0.0-20161129095857-cc309e4a2223/go.mod h1:qRWi+5nqEBWmkhHvq77mSJWrCKwh8bxhgT7d/eI7P4U= github.com/mxk/go-flowrate v0.0.0-20140419014527-cca7078d478f/go.mod h1:ZdcZmHo+o7JKHSa8/e818NopupXU1YMK5fe1lsApnBw= +github.com/natefinch/atomic v0.0.0-20150920032501-a62ce929ffcc h1:7xGrl4tTpBQu5Zjll08WupHyq+Sp0Z/adtyf1cfk3Q8= +github.com/natefinch/atomic v0.0.0-20150920032501-a62ce929ffcc/go.mod h1:1rLVY/DWf3U6vSZgH16S7pymfrhK2lcUlXjgGglw/lY= github.com/ncw/swift v1.0.47 h1:4DQRPj35Y41WogBxyhOXlrI37nzGlyEcsforeudyYPQ= github.com/ncw/swift v1.0.47/go.mod h1:23YIA4yWVnGwv2dQlN4bB7egfYX6YLn0Yo/S6zZO/ZM= github.com/nwaples/rardecode v1.0.0 h1:r7vGuS5akxOnR4JQSkko62RJ1ReCMXxQRPtxsiFMBOs= diff --git a/vendor/modules.txt b/vendor/modules.txt index 4079b8eb1ab3a..e86b88270bc72 100644 --- a/vendor/modules.txt +++ b/vendor/modules.txt @@ -613,6 +613,8 @@ github.com/modern-go/reflect2 github.com/mongodb/go-client-mongodb-atlas/mongodbatlas # github.com/mwielbut/pointy v1.1.0 github.com/mwielbut/pointy +# github.com/natefinch/atomic v0.0.0-20150920032501-a62ce929ffcc +github.com/natefinch/atomic # github.com/ncw/swift v1.0.47 github.com/ncw/swift # github.com/nwaples/rardecode v1.0.0 From 951a85e6bd7ae47e51be4d336b5db51213f2d4ca Mon Sep 17 00:00:00 2001 From: Nick Cabatoff Date: Mon, 27 Apr 2020 18:31:01 -0400 Subject: [PATCH 2/2] Run go mod vendor. --- .../hashicorp/vault/api/sys_raft.go | 4 ++ .../database/dbplugin/databasemiddleware.go | 13 ++++- vendor/github.com/natefinch/atomic/.gitignore | 24 ++++++++ vendor/github.com/natefinch/atomic/LICENSE | 22 +++++++ vendor/github.com/natefinch/atomic/README.md | 35 +++++++++++ vendor/github.com/natefinch/atomic/atomic.go | 58 +++++++++++++++++++ .../github.com/natefinch/atomic/file_unix.go | 14 +++++ .../natefinch/atomic/file_windows.go | 33 +++++++++++ .../natefinch/atomic/zfile_windows.go | 27 +++++++++ vendor/modules.txt | 1 + 10 files changed, 229 insertions(+), 2 deletions(-) create mode 100644 vendor/github.com/natefinch/atomic/.gitignore create mode 100644 vendor/github.com/natefinch/atomic/LICENSE create mode 100644 vendor/github.com/natefinch/atomic/README.md create mode 100644 vendor/github.com/natefinch/atomic/atomic.go create mode 100644 vendor/github.com/natefinch/atomic/file_unix.go create mode 100644 vendor/github.com/natefinch/atomic/file_windows.go create mode 100644 vendor/github.com/natefinch/atomic/zfile_windows.go diff --git a/vendor/github.com/hashicorp/vault/api/sys_raft.go b/vendor/github.com/hashicorp/vault/api/sys_raft.go index 908a3c4f11319..1a8aa1176b63f 100644 --- a/vendor/github.com/hashicorp/vault/api/sys_raft.go +++ b/vendor/github.com/hashicorp/vault/api/sys_raft.go @@ -92,6 +92,10 @@ func (c *Sys) RaftSnapshot(snapWriter io.Writer) error { // to determine if the body contains error message. var result *Response resp, err := c.c.config.HttpClient.Do(req) + if err != nil { + return err + } + if resp == nil { return nil } diff --git a/vendor/github.com/hashicorp/vault/sdk/database/dbplugin/databasemiddleware.go b/vendor/github.com/hashicorp/vault/sdk/database/dbplugin/databasemiddleware.go index 19cfa3374b621..e7cb0a2f5af12 100644 --- a/vendor/github.com/hashicorp/vault/sdk/database/dbplugin/databasemiddleware.go +++ b/vendor/github.com/hashicorp/vault/sdk/database/dbplugin/databasemiddleware.go @@ -8,10 +8,10 @@ import ( "sync" "time" - "github.com/hashicorp/errwrap" - metrics "github.com/armon/go-metrics" + "github.com/hashicorp/errwrap" log "github.com/hashicorp/go-hclog" + "google.golang.org/grpc/status" ) // ---- Tracing Middleware Domain ---- @@ -318,6 +318,15 @@ func (mw *DatabaseErrorSanitizerMiddleware) sanitize(err error) error { if k == "" { continue } + + // Attempt to keep the status code attached to the + // error without changing the actual error message + s, ok := status.FromError(err) + if ok { + err = status.Error(s.Code(), strings.Replace(s.Message(), k, v.(string), -1)) + continue + } + err = errors.New(strings.Replace(err.Error(), k, v.(string), -1)) } } diff --git a/vendor/github.com/natefinch/atomic/.gitignore b/vendor/github.com/natefinch/atomic/.gitignore new file mode 100644 index 0000000000000..daf913b1b347a --- /dev/null +++ b/vendor/github.com/natefinch/atomic/.gitignore @@ -0,0 +1,24 @@ +# Compiled Object files, Static and Dynamic libs (Shared Objects) +*.o +*.a +*.so + +# Folders +_obj +_test + +# Architecture specific extensions/prefixes +*.[568vq] +[568vq].out + +*.cgo1.go +*.cgo2.c +_cgo_defun.c +_cgo_gotypes.go +_cgo_export.* + +_testmain.go + +*.exe +*.test +*.prof diff --git a/vendor/github.com/natefinch/atomic/LICENSE b/vendor/github.com/natefinch/atomic/LICENSE new file mode 100644 index 0000000000000..cc38d640457ee --- /dev/null +++ b/vendor/github.com/natefinch/atomic/LICENSE @@ -0,0 +1,22 @@ +The MIT License (MIT) + +Copyright (c) 2015 Nate Finch + +Permission is hereby granted, free of charge, to any person obtaining a copy +of this software and associated documentation files (the "Software"), to deal +in the Software without restriction, including without limitation the rights +to use, copy, modify, merge, publish, distribute, sublicense, and/or sell +copies of the Software, and to permit persons to whom the Software is +furnished to do so, subject to the following conditions: + +The above copyright notice and this permission notice shall be included in all +copies or substantial portions of the Software. + +THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR +IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, +FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE +AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER +LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, +OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE +SOFTWARE. + diff --git a/vendor/github.com/natefinch/atomic/README.md b/vendor/github.com/natefinch/atomic/README.md new file mode 100644 index 0000000000000..37cd67316f286 --- /dev/null +++ b/vendor/github.com/natefinch/atomic/README.md @@ -0,0 +1,35 @@ +# atomic + import "github.com/natefinch/atomic" +atomic is a go package for atomic file writing + +By default, writing to a file in go (and generally any language) can fail +partway through... you then have a partially written file, which probably was +truncated when the write began, and bam, now you've lost data. + +This go package avoids this problem, by writing first to a temp file, and then +overwriting the target file in an atomic way. This is easy on linux, os.Rename +just is atomic. However, on Windows, os.Rename is not atomic, and so bad things +can happen. By wrapping the windows API moveFileEx, we can ensure that the move +is atomic, and we can be safe in knowing that either the move succeeds entirely, +or neither file will be modified. + + +## func ReplaceFile +``` go +func ReplaceFile(source, destination string) error +``` +ReplaceFile atomically replaces the destination file or directory with the +source. It is guaranteed to either replace the target file entirely, or not +change either file. + + +## func WriteFile +``` go +func WriteFile(filename string, r io.Reader) (err error) +``` +WriteFile atomically writes the contents of r to the specified filepath. If +an error occurs, the target file is guaranteed to be either fully written, or +not written at all. WriteFile overwrites any file that exists at the +location (but only if the write fully succeeds, otherwise the existing file +is unmodified). + diff --git a/vendor/github.com/natefinch/atomic/atomic.go b/vendor/github.com/natefinch/atomic/atomic.go new file mode 100644 index 0000000000000..6f59c15406d1a --- /dev/null +++ b/vendor/github.com/natefinch/atomic/atomic.go @@ -0,0 +1,58 @@ +// package atomic provides functions to atomically change files. +package atomic + +import ( + "fmt" + "io" + "io/ioutil" + "os" + "path/filepath" +) + +// WriteFile atomically writes the contents of r to the specified filepath. If +// an error occurs, the target file is guaranteed to be either fully written, or +// not written at all. WriteFile overwrites any file that exists at the +// location (but only if the write fully succeeds, otherwise the existing file +// is unmodified). +func WriteFile(filename string, r io.Reader) (err error) { + // write to a temp file first, then we'll atomically replace the target file + // with the temp file. + dir, file := filepath.Split(filename) + f, err := ioutil.TempFile(dir, file) + if err != nil { + return fmt.Errorf("cannot create temp file: %v", err) + } + defer func() { + if err != nil { + // Don't leave the temp file lying around on error. + _ = os.Remove(f.Name()) // yes, ignore the error, not much we can do about it. + } + }() + // ensure we always close f. Note that this does not conflict with the + // close below, as close is idempotent. + defer f.Close() + name := f.Name() + if _, err := io.Copy(f, r); err != nil { + return fmt.Errorf("cannot write data to tempfile %q: %v", name, err) + } + if err := f.Close(); err != nil { + return fmt.Errorf("can't close tempfile %q: %v", name, err) + } + + // get the file mode from the original file and use that for the replacement + // file, too. + info, err := os.Stat(filename) + if os.IsNotExist(err) { + // no original file + } else if err != nil { + return err + } else { + if err := os.Chmod(name, info.Mode()); err != nil { + return fmt.Errorf("can't set filemode on tempfile %q: %v", name, err) + } + } + if err := ReplaceFile(name, filename); err != nil { + return fmt.Errorf("cannot replace %q with tempfile %q: %v", filename, name, err) + } + return nil +} diff --git a/vendor/github.com/natefinch/atomic/file_unix.go b/vendor/github.com/natefinch/atomic/file_unix.go new file mode 100644 index 0000000000000..408f18ed01adb --- /dev/null +++ b/vendor/github.com/natefinch/atomic/file_unix.go @@ -0,0 +1,14 @@ +// +build !windows + +package atomic + +import ( + "os" +) + +// ReplaceFile atomically replaces the destination file or directory with the +// source. It is guaranteed to either replace the target file entirely, or not +// change either file. +func ReplaceFile(source, destination string) error { + return os.Rename(source, destination) +} diff --git a/vendor/github.com/natefinch/atomic/file_windows.go b/vendor/github.com/natefinch/atomic/file_windows.go new file mode 100644 index 0000000000000..e507143b2c687 --- /dev/null +++ b/vendor/github.com/natefinch/atomic/file_windows.go @@ -0,0 +1,33 @@ +package atomic + +import ( + "os" + "syscall" +) + +const ( + movefile_replace_existing = 0x1 + movefile_write_through = 0x8 +) + +//sys moveFileEx(lpExistingFileName *uint16, lpNewFileName *uint16, dwFlags uint32) (err error) = MoveFileExW + +// ReplaceFile atomically replaces the destination file or directory with the +// source. It is guaranteed to either replace the target file entirely, or not +// change either file. +func ReplaceFile(source, destination string) error { + src, err := syscall.UTF16PtrFromString(source) + if err != nil { + return &os.LinkError{"replace", source, destination, err} + } + dest, err := syscall.UTF16PtrFromString(destination) + if err != nil { + return &os.LinkError{"replace", source, destination, err} + } + + // see http://msdn.microsoft.com/en-us/library/windows/desktop/aa365240(v=vs.85).aspx + if err := moveFileEx(src, dest, movefile_replace_existing|movefile_write_through); err != nil { + return &os.LinkError{"replace", source, destination, err} + } + return nil +} diff --git a/vendor/github.com/natefinch/atomic/zfile_windows.go b/vendor/github.com/natefinch/atomic/zfile_windows.go new file mode 100644 index 0000000000000..d55db613e5467 --- /dev/null +++ b/vendor/github.com/natefinch/atomic/zfile_windows.go @@ -0,0 +1,27 @@ +// mksyscall_windows -l32 file_windows.go +// MACHINE GENERATED BY THE COMMAND ABOVE; DO NOT EDIT + +package atomic + +import ( + "syscall" + "unsafe" +) + +var ( + modkernel32 = syscall.NewLazyDLL("kernel32.dll") + + procMoveFileExW = modkernel32.NewProc("MoveFileExW") +) + +func moveFileEx(lpExistingFileName *uint16, lpNewFileName *uint16, dwFlags uint32) (err error) { + r1, _, e1 := syscall.Syscall(procMoveFileExW.Addr(), 3, uintptr(unsafe.Pointer(lpExistingFileName)), uintptr(unsafe.Pointer(lpNewFileName)), uintptr(dwFlags)) + if r1 == 0 { + if e1 != 0 { + err = error(e1) + } else { + err = syscall.EINVAL + } + } + return +} diff --git a/vendor/modules.txt b/vendor/modules.txt index e86b88270bc72..ca54dbac073b1 100644 --- a/vendor/modules.txt +++ b/vendor/modules.txt @@ -437,6 +437,7 @@ github.com/hashicorp/vault/sdk/database/helper/connutil github.com/hashicorp/vault/sdk/database/helper/credsutil github.com/hashicorp/vault/sdk/database/helper/dbutil github.com/hashicorp/vault/sdk/framework +github.com/hashicorp/vault/sdk/helper/authmetadata github.com/hashicorp/vault/sdk/helper/awsutil github.com/hashicorp/vault/sdk/helper/base62 github.com/hashicorp/vault/sdk/helper/certutil