From 311c61f56cb4391e256e6ab3fe7bc55d7e36c706 Mon Sep 17 00:00:00 2001 From: Shlomo Heigh Date: Wed, 16 Feb 2022 14:58:38 -0500 Subject: [PATCH] Write secret files atomically --- CHANGELOG.md | 3 + pkg/atomicwriter/atomic_writer.go | 114 ++++++++++ pkg/atomicwriter/atomic_writer_test.go | 255 +++++++++++++++++++++++ pkg/log/messages/error_messages.go | 8 + pkg/secrets/pushtofile/push_to_writer.go | 29 ++- 5 files changed, 398 insertions(+), 11 deletions(-) create mode 100644 pkg/atomicwriter/atomic_writer.go create mode 100644 pkg/atomicwriter/atomic_writer_test.go diff --git a/CHANGELOG.md b/CHANGELOG.md index cf08488da..20bfe9843 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/pkg/atomicwriter/atomic_writer.go b/pkg/atomicwriter/atomic_writer.go new file mode 100644 index 000000000..f25fdf963 --- /dev/null +++ b/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) +} diff --git a/pkg/atomicwriter/atomic_writer_test.go b/pkg/atomicwriter/atomic_writer_test.go new file mode 100644 index 000000000..78ac6c35d --- /dev/null +++ b/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) +} diff --git a/pkg/log/messages/error_messages.go b/pkg/log/messages/error_messages.go index d204cb3b1..237514ef1 100644 --- a/pkg/log/messages/error_messages.go +++ b/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." diff --git a/pkg/secrets/pushtofile/push_to_writer.go b/pkg/secrets/pushtofile/push_to_writer.go index fa5973fd8..9b84d5c31 100644 --- a/pkg/secrets/pushtofile/push_to_writer.go +++ b/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" ) @@ -46,6 +47,7 @@ var prevFileChecksums = map[string]checksum{} func openFileAsWriteCloser(path string, permissions os.FileMode) (io.WriteCloser, error) { dir := filepath.Dir(path) + // Create the file here to capture any errors, instead of in atomicWriter.Close() which may be deferred and ignored err := os.MkdirAll(dir, os.ModePerm) if err != nil { return nil, fmt.Errorf("unable to mkdir when opening file to write at %q: %s", path, err) @@ -55,13 +57,15 @@ func openFileAsWriteCloser(path string, permissions os.FileMode) (io.WriteCloser if err != nil { return nil, fmt.Errorf("unable to open file to write at %q: %s", path, err) } + wc.Close() - //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 an instance of an atomic writer + atomicWriter, err := atomicwriter.NewAtomicWriter(path, permissions) + if err != nil { + return nil, fmt.Errorf("unable to create atomic writer: %s", err) } - return wc, nil + return atomicWriter, nil } // pushToWriter takes a (group's) path, template and secrets, and processes the template @@ -125,15 +129,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 }