Write secret files atomically

CHANGELOG.md

@@ -6,6 +6,9 @@ and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0.
 
 ## [Unreleased]
 
+### Added
+- Secrets files are written in an atomic operation. [cyberark/secrets-provider-for-k8s#440](https://github.com/cyberark/secrets-provider-for-k8s/pull/440)
+
 ## [1.4.0] - 2022-02-15
 
 ### Added

pkg/atomicwriter/atomic_writer.go

@@ -0,0 +1,108 @@
+package atomicwriter
+
+import (
+	"io/ioutil"
+	"os"
+	"path/filepath"
+	"time"
+
+	"github.com/cyberark/conjur-authn-k8s-client/pkg/log"
+	"github.com/cyberark/secrets-provider-for-k8s/pkg/log/messages"
+)
+
+const deleteTempFileRetryInterval = time.Second * 5
+const deleteTempFileMaxRetries = 5
+
+// This package provides a simple atomic file writer which implements the
+// io.WriteCloser interface. This allows us to use AtomicWriter the way we
+// would use any other Writer, such as a Buffer. Additonally, this struct
+// takes the file path during construction, so the code which calls
+// `Write()` doesn't need to be concerned with the destination, just like
+// any other writer.
+type AtomicWriter struct {
+	path        string
+	permissions os.FileMode
+	tempFile    *os.File
+}
+
+func NewAtomicWriter(path string, permissions os.FileMode) (*AtomicWriter, error) {
+	dir, file := filepath.Split(path)
+	if dir == "" {
+		dir = "."
+	}
+
+	f, err := ioutil.TempFile(dir, file)
+	if err != nil {
+		log.Error(messages.CSPFK055E, path)
+		return nil, err
+	}
+
+	return &AtomicWriter{
+		path:        path,
+		tempFile:    f,
+		permissions: permissions,
+	}, nil
+}
+
+func (w *AtomicWriter) Write(content []byte) (n int, err error) {
+	// Write to the temporary file
+	return w.tempFile.Write(content)
+}
+
+func (w *AtomicWriter) Close() error {
+	defer func() {
+		go w.Cleanup()
+	}()
+
+	// Flush and close the temporary file
+	err := w.tempFile.Sync()
+	if err != nil {
+		log.Error(messages.CSPFK056E, w.tempFile.Name())
+		return err
+	}
+	w.tempFile.Close()
+
+	// Set the file permissions
+	err = os.Chmod(w.tempFile.Name(), w.permissions)
+	if err != nil {
+		log.Error(messages.CSPFK057E, w.tempFile.Name())
+		// Try to rename the file anyway
+	}
+
+	// Rename the temporary file to the destination
+	err = os.Rename(w.tempFile.Name(), w.path)
+	if err != nil {
+		log.Error(messages.CSPFK058E, w.tempFile.Name(), w.path)
+		return err
+	}
+
+	return nil
+}
+
+// Cleanup attempts to remove the temporary file. This function is called by
+// the `Close()` method, but can also be called manually in cases where `Close()`
+// is not called.
+func (w *AtomicWriter) Cleanup() {
+	for i := 0; i < deleteTempFileMaxRetries; i++ {
+		if w.removeTempFile() {
+			// File was removed successfully
+			break
+		}
+
+		if i+1 == deleteTempFileMaxRetries {
+			// This was the last attempt, so log the error
+			log.Error(messages.CSPFK059E, w.tempFile.Name())
+		} else {
+			// Wait a bit before trying again
+			time.Sleep(deleteTempFileRetryInterval)
+		}
+	}
+}
+
+func (w *AtomicWriter) removeTempFile() bool {
+	err := os.Remove(w.tempFile.Name())
+	if err != nil && !os.IsNotExist(err) {
+		return false
+	}
+	return true
+}

pkg/atomicwriter/atomic_writer_test.go

@@ -0,0 +1,148 @@
+package atomicwriter
+
+import (
+	"bytes"
+	"io/ioutil"
+	"os"
+	"path/filepath"
+	"testing"
+
+	"github.com/cyberark/conjur-authn-k8s-client/pkg/log"
+	"github.com/stretchr/testify/assert"
+)
+
+type assertFunc func(path string, t *testing.T, err error)
+
+func TestWriteFile(t *testing.T) {
+	testCases := []struct {
+		name        string
+		path        string
+		permissions os.FileMode
+		content     string
+		assert      assertFunc
+	}{
+		{
+			name:        "happy path",
+			path:        "test_file.txt",
+			permissions: 0644,
+			content:     "test content",
+			assert: func(path string, t *testing.T, err error) {
+				assert.NoError(t, err)
+				// Check that the file exists
+				assert.FileExists(t, path)
+				// Check the contents of the file
+				contents, err := ioutil.ReadFile(path)
+				assert.NoError(t, err)
+				assert.Equal(t, "test content", string(contents))
+				// Check the file permissions
+				mode, err := os.Stat(path)
+				assert.NoError(t, err)
+				assert.Equal(t, os.FileMode(0644), mode.Mode())
+			},
+		},
+	}
+
+	for _, tc := range testCases {
+		t.Run(tc.name, func(t *testing.T) {
+			tmpDir, _ := ioutil.TempDir("", "atomicwriter")
+			defer os.RemoveAll(tmpDir)
+
+			path := filepath.Join(tmpDir, tc.path)
+			err := writeFile(path, tc.permissions, []byte(tc.content))
+			tc.assert(path, t, err)
+		})
+	}
+}
+
+func TestWriterAtomicity(t *testing.T) {
+	tmpDir, _ := ioutil.TempDir("", "atomicwriter")
+	defer os.RemoveAll(tmpDir)
+	path := filepath.Join(tmpDir, "test_file.txt")
+
+	// Create 2 writers for the same path
+	writer1, err := NewAtomicWriter(path, 0600)
+	assert.NoError(t, err)
+	writer2, err := NewAtomicWriter(path, 0644)
+	assert.NoError(t, err)
+	// Write different content to each writer
+	writer1.Write([]byte("writer 1 line 1\n"))
+	writer2.Write([]byte("writer 2 line 1\n"))
+	writer1.Write([]byte("writer 1 line 2\n"))
+	writer2.Write([]byte("writer 2 line 2\n"))
+	// Close the first writer and ensure only the contents of the first writer are written
+	err = writer1.Close()
+
+	assert.NoError(t, err)
+	// Check that the file exists
+	assert.FileExists(t, path)
+	// Check the contents of the file match the first writer (which was closed)
+	contents, err := ioutil.ReadFile(path)
+	assert.NoError(t, err)
+	assert.Equal(t, "writer 1 line 1\nwriter 1 line 2\n", string(contents))
+	// Check the file permissions match the first writer
+	mode, err := os.Stat(path)
+	assert.NoError(t, err)
+	assert.Equal(t, os.FileMode(0600), mode.Mode())
+}
+
+func TestUnableToCreateTempFile(t *testing.T) {
+	path := "non/existent/dir/test_file.txt"
+
+	// Mock the logger output
+	buf := &bytes.Buffer{}
+	log.ErrorLogger.SetOutput(buf)
+
+	_, err := NewAtomicWriter(path, 0644)
+	assert.Error(t, err)
+	assert.Contains(t, buf.String(), "Could not create temporary file")
+}
+
+func TestUnableToWriteFile(t *testing.T) {
+	tmpDir, _ := ioutil.TempDir("", "atomicwriter")
+	defer os.RemoveAll(tmpDir)
+
+	path := filepath.Join(tmpDir, "test_file.txt")
+	writer, err := NewAtomicWriter(path, 0644)
+	assert.NoError(t, err)
+
+	// Mock the logger output
+	buf := &bytes.Buffer{}
+	log.ErrorLogger.SetOutput(buf)
+
+	// Remove the temp directory to cause the atomic writer to fail
+	err = os.RemoveAll(tmpDir)
+	assert.NoError(t, err)
+
+	// Try to write the file
+	_, err = writer.Write([]byte("test content"))
+	assert.NoError(t, err)
+
+	err = writer.Close()
+	assert.Error(t, err)
+	assert.Contains(t, buf.String(), "Could not rename temporary file")
+}
+
+func TestDefaultDirectory(t *testing.T) {
+	writer, err := NewAtomicWriter("test_file.txt", 0644)
+	assert.NoError(t, err)
+	defer os.Remove("test_file.txt")
+
+	writer.Write([]byte("test content"))
+	err = writer.Close()
+	assert.NoError(t, err)
+	assert.FileExists(t, "./test_file.txt")
+}
+
+func writeFile(path string, permissions os.FileMode, content []byte) error {
+	writer, err := NewAtomicWriter(path, permissions)
+	if err != nil {
+		return err
+	}
+
+	_, err = writer.Write(content)
+	if err != nil {
+		return err
+	}
+
+	return writer.Close()
+}

pkg/log/messages/error_messages.go

@@ -73,3 +73,10 @@ const CSPFK051E string = "CSPFK050E Invalid secrets refresh configuration: %s %s
 // Push to File
 const CSPFK053E string = "CSPFK053E Unable to initialize Secrets Provider: unable to create secret group collection"
 const CSPFK054E string = "CSPFK054E Unable to initialize Secrets Provider: unrecognized Store Type '%s'"
+
+// Atomic Writer
+const CSPFK055E string = "CSPFK055E Could not create temporary file for '%s'"
+const CSPFK056E string = "CSPFK056E Could not flush temporary file '%s'"
+const CSPFK057E string = "CSPFK057E Could not set permissions on temporary file '%s'"
+const CSPFK058E string = "CSPFK058E Could not rename temporary file '%s' to '%s'"
+const CSPFK059E string = "CSPFK059E Could not delete temporary file '%s'"

pkg/secrets/pushtofile/push_to_writer.go

@@ -10,6 +10,7 @@ import (
 	"text/template"
 
 	"github.com/cyberark/conjur-authn-k8s-client/pkg/log"
+	"github.com/cyberark/secrets-provider-for-k8s/pkg/atomicwriter"
 	"github.com/cyberark/secrets-provider-for-k8s/pkg/log/messages"
 )
 
@@ -51,17 +52,13 @@ func openFileAsWriteCloser(path string, permissions os.FileMode) (io.WriteCloser
 		return nil, fmt.Errorf("unable to mkdir when opening file to write at %q: %s", path, err)
 	}
 
-	wc, err := os.OpenFile(path, os.O_WRONLY|os.O_CREATE, permissions)
+	// Return an instance of an atomic writer
+	atomicWriter, err := atomicwriter.NewAtomicWriter(path, permissions)
 	if err != nil {
-		return nil, fmt.Errorf("unable to open file to write at %q: %s", path, err)
+		return nil, fmt.Errorf("unable to create atomic writer: %s", err)
 	}
 
-	//Chmod file to set permissions regardless of 'umask'
-	if err := os.Chmod(path, permissions); err != nil {
-		return nil, fmt.Errorf("unable to chmod file %q: %s", path, err)
-	}
-
-	return wc, nil
+	return atomicWriter, nil
 }
 
 // pushToWriter takes a (group's) path, template and secrets, and processes the template
@@ -125,15 +122,18 @@ func writeContent(writer io.Writer, fileContent *bytes.Buffer, groupName string)
 	// Calculate a sha256 checksum on the content
 	checksum, _ := fileChecksum(fileContent)
 
-	// If file contents have changed, write the file and update checksum
-	if contentHasChanged(groupName, checksum) {
-		if _, err := writer.Write(fileContent.Bytes()); err != nil {
-			return err
-		}
-		prevFileChecksums[groupName] = checksum
-	} else {
+	// If file contents haven't changed, don't rewrite the file
+	if !contentHasChanged(groupName, checksum) {
 		log.Info(messages.CSPFK018I)
+		return nil
 	}
+
+	// Write the file and update the checksum
+	if _, err := writer.Write(fileContent.Bytes()); err != nil {
+		return err
+	}
+	prevFileChecksums[groupName] = checksum
+
 	return nil
 }
 

pkg/secrets/pushtofile/secret_group_test.go

@@ -946,6 +946,6 @@ func TestSecretGroup_PushToFile(t *testing.T) {
 			},
 		})
 		assert.Error(t, err)
-		assert.Contains(t, err.Error(), "unable to open file")
+		assert.Contains(t, err.Error(), "unable to create")
 	})
 }