From a68a5e42292fefffb00791e6295278f1f5ab197e 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 | 108 ++++++++++++++ pkg/atomicwriter/atomic_writer_test.go | 148 ++++++++++++++++++++ pkg/log/messages/error_messages.go | 7 + pkg/secrets/pushtofile/push_to_writer.go | 30 ++-- pkg/secrets/pushtofile/secret_group_test.go | 2 +- 6 files changed, 282 insertions(+), 16 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..998b39606 --- /dev/null +++ b/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 +} diff --git a/pkg/atomicwriter/atomic_writer_test.go b/pkg/atomicwriter/atomic_writer_test.go new file mode 100644 index 000000000..8bbce856b --- /dev/null +++ b/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() +} diff --git a/pkg/log/messages/error_messages.go b/pkg/log/messages/error_messages.go index fa3480066..b57532d6e 100644 --- a/pkg/log/messages/error_messages.go +++ b/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'" diff --git a/pkg/secrets/pushtofile/push_to_writer.go b/pkg/secrets/pushtofile/push_to_writer.go index fa5973fd8..2296463ee 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" ) @@ -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 } diff --git a/pkg/secrets/pushtofile/secret_group_test.go b/pkg/secrets/pushtofile/secret_group_test.go index f7d6de8ef..7b4819ba3 100644 --- a/pkg/secrets/pushtofile/secret_group_test.go +++ b/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") }) }