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,114 @@
+package atomicwriter
+
+import (
+	"io"
+	"io/ioutil"
+	"os"
+	"path/filepath"
+
+	"github.com/cyberark/conjur-authn-k8s-client/pkg/log"
+	"github.com/cyberark/secrets-provider-for-k8s/pkg/log/messages"
+)
+
+// OS Function table
+type osFuncs struct {
+	chmod    func(string, os.FileMode) error
+	rename   func(string, string) error
+	remove   func(string) error
+	truncate func(string, int64) error
+}
+
+// Instantiation of OS Function table using std OS
+var stdOSFuncs = osFuncs{
+	chmod:    os.Chmod,
+	rename:   os.Rename,
+	remove:   os.Remove,
+	truncate: os.Truncate,
+}
+
+type atomicWriter struct {
+	path        string
+	permissions os.FileMode
+	tempFile    *os.File
+	os          osFuncs
+}
+
+// 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.
+func NewAtomicWriter(path string, permissions os.FileMode) (io.WriteCloser, error) {
+	return newAtomicWriter(path, permissions, stdOSFuncs)
+}
+
+func newAtomicWriter(path string, permissions os.FileMode, osFuncs osFuncs) (io.WriteCloser, error) {
+	dir, file := filepath.Split(path)
+
+	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,
+		os:          osFuncs,
+	}, 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 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 = w.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 = w.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() {
+	err := w.os.remove(w.tempFile.Name())
+	if err == nil {
+		return
+	}
+
+	// If we can't remove the temporary directory, truncate the file to remove all secret content
+	err = w.os.truncate(w.tempFile.Name(), 0)
+	if err == nil || os.IsNotExist(err) {
+		log.Error(messages.CSPFK059E, w.tempFile.Name(), w.path)
+		return
+	}
+
+	// If that failed as well, log the error
+	log.Error(messages.CSPFK060E, w.tempFile.Name(), w.path)
+}

pkg/atomicwriter/atomic_writer_test.go

@@ -0,0 +1,255 @@
+package atomicwriter
+
+import (
+	"bytes"
+	"io"
+	"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, tempFilePath string, t *testing.T, err error)
+type errorAssertFunc func(buf *bytes.Buffer, wc io.WriteCloser, 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, tempFilePath 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())
+				// Check that the temp file was deleted
+				assert.NoFileExists(t, tempFilePath)
+			},
+		},
+	}
+
+	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, tempFilePath := writeFile(path, tc.permissions, []byte(tc.content))
+			tc.assert(path, tempFilePath, 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 TestLogsErrors(t *testing.T) {
+	testCases := []struct {
+		name          string
+		path          string
+		osFuncs       osFuncs
+		errorOnCreate bool
+		assert        errorAssertFunc
+	}{
+		{
+			name:          "nonexistent directory",
+			path:          "nonexistent_directory/test_file.txt",
+			osFuncs:       stdOSFuncs,
+			errorOnCreate: true,
+			assert: func(buf *bytes.Buffer, wc io.WriteCloser, t *testing.T, err error) {
+				assert.Error(t, err)
+				assert.Contains(t, buf.String(), "Could not create temporary file")
+			},
+		},
+		{
+			name: "unable to remove temporary file",
+			path: "test_file.txt",
+			osFuncs: osFuncs{
+				remove: func(name string) error {
+					return os.ErrPermission
+				},
+				rename: func(oldpath, newpath string) error {
+					return os.ErrPermission
+				},
+				truncate: os.Truncate,
+				chmod:    os.Chmod,
+			},
+			assert: func(buf *bytes.Buffer, wc io.WriteCloser, t *testing.T, err error) {
+				assert.Error(t, err)
+
+				// The file should be truncated instead of being deleted
+				assert.Contains(t, buf.String(), "Could not delete temporary file")
+				assert.Contains(t, buf.String(), "Truncated file")
+
+				// Check that the temp file was truncated
+				writer, ok := wc.(*atomicWriter)
+				assert.True(t, ok)
+				assert.FileExists(t, writer.tempFile.Name())
+				content, err := ioutil.ReadFile(writer.tempFile.Name())
+				assert.NoError(t, err)
+				assert.Equal(t, "", string(content))
+			},
+		},
+		{
+			name: "unable to remove or truncate temporary file",
+			path: "test_file.txt",
+			osFuncs: osFuncs{
+				remove: func(name string) error {
+					return os.ErrPermission
+				},
+				rename: func(oldpath, newpath string) error {
+					return os.ErrPermission
+				},
+				truncate: func(name string, size int64) error {
+					return os.ErrPermission
+				},
+				chmod: os.Chmod,
+			},
+			assert: func(buf *bytes.Buffer, wc io.WriteCloser, t *testing.T, err error) {
+				assert.Error(t, err)
+
+				assert.Contains(t, buf.String(), "Could not delete temporary file")
+				assert.Contains(t, buf.String(), "File may be left on disk")
+			},
+		},
+		{
+			name: "unable to chmod",
+			path: "test_file.txt",
+			osFuncs: osFuncs{
+				remove:   os.RemoveAll,
+				rename:   os.Rename,
+				truncate: os.Truncate,
+				chmod: func(name string, mode os.FileMode) error {
+					return os.ErrPermission
+				},
+			},
+			assert: func(buf *bytes.Buffer, wc io.WriteCloser, t *testing.T, err error) {
+				assert.NoError(t, err)
+				assert.Contains(t, buf.String(), "Could not set permissions on temporary file")
+
+				// Check that the file was still renamed
+				writer, ok := wc.(*atomicWriter)
+				assert.True(t, ok)
+				assert.NoFileExists(t, writer.tempFile.Name())
+				assert.FileExists(t, writer.path)
+			},
+		},
+	}
+
+	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)
+
+			// Mock the logger output
+			buf := mockErrorLog()
+			defer unmockErrorLog()
+
+			writer, err := newAtomicWriter(path, 0644, tc.osFuncs)
+
+			if tc.errorOnCreate {
+				assert.Error(t, err)
+				tc.assert(buf, writer, t, err)
+				if writer != nil {
+					writer.Close()
+				}
+				return
+			}
+
+			assert.NoError(t, err)
+
+			// Try to write the file
+			_, err = writer.Write([]byte("test content"))
+			assert.NoError(t, err)
+
+			err = writer.Close()
+			tc.assert(buf, writer, t, err)
+		})
+	}
+}
+
+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) (err error, tempFilePath string) {
+	writer, err := NewAtomicWriter(path, permissions)
+	if err != nil {
+		return err, ""
+	}
+
+	tempFilePath = writer.(*atomicWriter).tempFile.Name()
+
+	_, err = writer.Write(content)
+	if err != nil {
+		return err, tempFilePath
+	}
+
+	return writer.Close(), tempFilePath
+}
+
+// Mocks the logger output to a buffer
+func mockErrorLog() *bytes.Buffer {
+	buf := &bytes.Buffer{}
+	log.ErrorLogger.SetOutput(buf)
+	return buf
+}
+
+// Unmocks the logger output, sending it back to os.Stderr
+func unmockErrorLog() {
+	log.ErrorLogger.SetOutput(os.Stderr)
+}

pkg/log/messages/error_messages.go

@@ -73,3 +73,11 @@ const CSPFK051E string = "CSPFK051E 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'. Truncated file."
+const CSPFK060E string = "CSPFK060E Could not delete temporary file '%s'. File may be left on disk."