Ensure that the .vault-token file writen by vault login always has …

…the correct permissions and ownership. (#8867)

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)

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

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())
+	}
 }

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

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=