Skip to content

Commit 1a2275d

Browse files
authored
fs storage: Use temporary files when writing (#300)
* fix: use an tmp file to flush new certs to disk * add readme
1 parent 16c9db1 commit 1a2275d

File tree

7 files changed

+466
-2
lines changed

7 files changed

+466
-2
lines changed

filestorage.go

+22-2
Original file line numberDiff line numberDiff line change
@@ -27,6 +27,8 @@ import (
2727
"path/filepath"
2828
"runtime"
2929
"time"
30+
31+
"github.com/caddyserver/certmagic/internal/atomicfile"
3032
)
3133

3234
// FileStorage facilitates forming file paths derived from a root
@@ -82,12 +84,30 @@ func (s *FileStorage) Store(_ context.Context, key string, value []byte) error {
8284
if err != nil {
8385
return err
8486
}
85-
return os.WriteFile(filename, value, 0600)
87+
fp, err := atomicfile.New(filename, 0o600)
88+
if err != nil {
89+
return err
90+
}
91+
_, err = fp.Write(value)
92+
if err != nil {
93+
// cancel the write
94+
fp.Cancel()
95+
return err
96+
}
97+
// close, thereby flushing the write
98+
return fp.Close()
8699
}
87100

88101
// Load retrieves the value at key.
89102
func (s *FileStorage) Load(_ context.Context, key string) ([]byte, error) {
90-
return os.ReadFile(s.Filename(key))
103+
// i believe it's possible for the read call to error but still return bytes, in event of something like a shortread?
104+
// therefore, i think it's appropriate to not return any bytes to avoid downstream users of the package erroniously believing that
105+
// bytes read + error is a valid response (it should not be)
106+
xs, err := os.ReadFile(s.Filename(key))
107+
if err != nil {
108+
return nil, err
109+
}
110+
return xs, nil
91111
}
92112

93113
// Delete deletes the value at key.

filestorage_test.go

+78
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,78 @@
1+
package certmagic_test
2+
3+
import (
4+
"bytes"
5+
"context"
6+
"os"
7+
"testing"
8+
9+
"github.com/caddyserver/certmagic"
10+
"github.com/caddyserver/certmagic/internal/testutil"
11+
)
12+
13+
func TestFileStorageStoreLoad(t *testing.T) {
14+
ctx := context.Background()
15+
tmpDir, err := os.MkdirTemp(os.TempDir(), "certmagic*")
16+
testutil.RequireNoError(t, err, "allocating tmp dir")
17+
defer os.RemoveAll(tmpDir)
18+
s := &certmagic.FileStorage{
19+
Path: tmpDir,
20+
}
21+
err = s.Store(ctx, "foo", []byte("bar"))
22+
testutil.RequireNoError(t, err)
23+
dat, err := s.Load(ctx, "foo")
24+
testutil.RequireNoError(t, err)
25+
testutil.RequireEqualValues(t, dat, []byte("bar"))
26+
}
27+
28+
func TestFileStorageStoreLoadRace(t *testing.T) {
29+
ctx := context.Background()
30+
tmpDir, err := os.MkdirTemp(os.TempDir(), "certmagic*")
31+
testutil.RequireNoError(t, err, "allocating tmp dir")
32+
defer os.RemoveAll(tmpDir)
33+
s := &certmagic.FileStorage{
34+
Path: tmpDir,
35+
}
36+
a := bytes.Repeat([]byte("a"), 4096*1024)
37+
b := bytes.Repeat([]byte("b"), 4096*1024)
38+
err = s.Store(ctx, "foo", a)
39+
testutil.RequireNoError(t, err)
40+
done := make(chan struct{})
41+
go func() {
42+
err := s.Store(ctx, "foo", b)
43+
testutil.RequireNoError(t, err)
44+
close(done)
45+
}()
46+
dat, err := s.Load(ctx, "foo")
47+
<-done
48+
testutil.RequireNoError(t, err)
49+
testutil.RequireEqualValues(t, 4096*1024, len(dat))
50+
}
51+
52+
func TestFileStorageWriteLock(t *testing.T) {
53+
ctx := context.Background()
54+
tmpDir, err := os.MkdirTemp(os.TempDir(), "certmagic*")
55+
testutil.RequireNoError(t, err, "allocating tmp dir")
56+
defer os.RemoveAll(tmpDir)
57+
s := &certmagic.FileStorage{
58+
Path: tmpDir,
59+
}
60+
// cctx is a cancelled ctx. so if we can't immediately get the lock, it will fail
61+
cctx, cn := context.WithCancel(ctx)
62+
cn()
63+
// should success
64+
err = s.Lock(cctx, "foo")
65+
testutil.RequireNoError(t, err)
66+
// should fail
67+
err = s.Lock(cctx, "foo")
68+
testutil.RequireError(t, err)
69+
70+
err = s.Unlock(cctx, "foo")
71+
testutil.RequireNoError(t, err)
72+
// shouldn't fail
73+
err = s.Lock(cctx, "foo")
74+
testutil.RequireNoError(t, err)
75+
76+
err = s.Unlock(cctx, "foo")
77+
testutil.RequireNoError(t, err)
78+
}

internal/atomicfile/README

+11
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,11 @@
1+
# atomic file
2+
3+
4+
this is copied from
5+
6+
https://github.com/containerd/containerd/blob/main/pkg%2Fatomicfile%2Ffile.go
7+
8+
9+
see
10+
11+
https://github.com/caddyserver/certmagic/issues/296

internal/atomicfile/file.go

+148
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,148 @@
1+
/*
2+
Copyright The containerd Authors.
3+
4+
Licensed under the Apache License, Version 2.0 (the "License");
5+
you may not use this file except in compliance with the License.
6+
You may obtain a copy of the License at
7+
8+
http://www.apache.org/licenses/LICENSE-2.0
9+
10+
Unless required by applicable law or agreed to in writing, software
11+
distributed under the License is distributed on an "AS IS" BASIS,
12+
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
13+
See the License for the specific language governing permissions and
14+
limitations under the License.
15+
*/
16+
17+
/*
18+
Package atomicfile provides a mechanism (on Unix-like platforms) to present a consistent view of a file to separate
19+
processes even while the file is being written. This is accomplished by writing a temporary file, syncing to disk, and
20+
renaming over the destination file name.
21+
22+
Partial/inconsistent reads can occur due to:
23+
1. A process attempting to read the file while it is being written to (both in the case of a new file with a
24+
short/incomplete write or in the case of an existing, updated file where new bytes may be written at the beginning
25+
but old bytes may still be present after).
26+
2. Concurrent goroutines leading to multiple active writers of the same file.
27+
28+
The above mechanism explicitly protects against (1) as all writes are to a file with a temporary name.
29+
30+
There is no explicit protection against multiple, concurrent goroutines attempting to write the same file. However,
31+
atomically writing the file should mean only one writer will "win" and a consistent file will be visible.
32+
33+
Note: atomicfile is partially implemented for Windows. The Windows codepath performs the same operations, however
34+
Windows does not guarantee that a rename operation is atomic; a crash in the middle may leave the destination file
35+
truncated rather than with the expected content.
36+
*/
37+
package atomicfile
38+
39+
import (
40+
"errors"
41+
"fmt"
42+
"io"
43+
"os"
44+
"path/filepath"
45+
"sync"
46+
)
47+
48+
// File is an io.ReadWriteCloser that can also be Canceled if a change needs to be abandoned.
49+
type File interface {
50+
io.ReadWriteCloser
51+
// Cancel abandons a change to a file. This can be called if a write fails or another error occurs.
52+
Cancel() error
53+
}
54+
55+
// ErrClosed is returned if Read or Write are called on a closed File.
56+
var ErrClosed = errors.New("file is closed")
57+
58+
// New returns a new atomic file. On Unix-like platforms, the writer (an io.ReadWriteCloser) is backed by a temporary
59+
// file placed into the same directory as the destination file (using filepath.Dir to split the directory from the
60+
// name). On a call to Close the temporary file is synced to disk and renamed to its final name, hiding any previous
61+
// file by the same name.
62+
//
63+
// Note: Take care to call Close and handle any errors that are returned. Errors returned from Close may indicate that
64+
// the file was not written with its final name.
65+
func New(name string, mode os.FileMode) (File, error) {
66+
return newFile(name, mode)
67+
}
68+
69+
type atomicFile struct {
70+
name string
71+
f *os.File
72+
closed bool
73+
closedMu sync.RWMutex
74+
}
75+
76+
func newFile(name string, mode os.FileMode) (File, error) {
77+
dir := filepath.Dir(name)
78+
f, err := os.CreateTemp(dir, "")
79+
if err != nil {
80+
return nil, fmt.Errorf("failed to create temp file: %w", err)
81+
}
82+
if err := f.Chmod(mode); err != nil {
83+
return nil, fmt.Errorf("failed to change temp file permissions: %w", err)
84+
}
85+
return &atomicFile{name: name, f: f}, nil
86+
}
87+
88+
func (a *atomicFile) Close() (err error) {
89+
a.closedMu.Lock()
90+
defer a.closedMu.Unlock()
91+
92+
if a.closed {
93+
return nil
94+
}
95+
a.closed = true
96+
97+
defer func() {
98+
if err != nil {
99+
_ = os.Remove(a.f.Name()) // ignore errors
100+
}
101+
}()
102+
// The order of operations here is:
103+
// 1. sync
104+
// 2. close
105+
// 3. rename
106+
// While the ordering of 2 and 3 is not important on Unix-like operating systems, Windows cannot rename an open
107+
// file. By closing first, we allow the rename operation to succeed.
108+
if err = a.f.Sync(); err != nil {
109+
return fmt.Errorf("failed to sync temp file %q: %w", a.f.Name(), err)
110+
}
111+
if err = a.f.Close(); err != nil {
112+
return fmt.Errorf("failed to close temp file %q: %w", a.f.Name(), err)
113+
}
114+
if err = os.Rename(a.f.Name(), a.name); err != nil {
115+
return fmt.Errorf("failed to rename %q to %q: %w", a.f.Name(), a.name, err)
116+
}
117+
return nil
118+
}
119+
120+
func (a *atomicFile) Cancel() error {
121+
a.closedMu.Lock()
122+
defer a.closedMu.Unlock()
123+
124+
if a.closed {
125+
return nil
126+
}
127+
a.closed = true
128+
_ = a.f.Close() // ignore error
129+
return os.Remove(a.f.Name())
130+
}
131+
132+
func (a *atomicFile) Read(p []byte) (n int, err error) {
133+
a.closedMu.RLock()
134+
defer a.closedMu.RUnlock()
135+
if a.closed {
136+
return 0, ErrClosed
137+
}
138+
return a.f.Read(p)
139+
}
140+
141+
func (a *atomicFile) Write(p []byte) (n int, err error) {
142+
a.closedMu.RLock()
143+
defer a.closedMu.RUnlock()
144+
if a.closed {
145+
return 0, ErrClosed
146+
}
147+
return a.f.Write(p)
148+
}

internal/atomicfile/file_test.go

+77
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,77 @@
1+
/*
2+
Copyright The containerd Authors.
3+
4+
Licensed under the Apache License, Version 2.0 (the "License");
5+
you may not use this file except in compliance with the License.
6+
You may obtain a copy of the License at
7+
8+
http://www.apache.org/licenses/LICENSE-2.0
9+
10+
Unless required by applicable law or agreed to in writing, software
11+
distributed under the License is distributed on an "AS IS" BASIS,
12+
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
13+
See the License for the specific language governing permissions and
14+
limitations under the License.
15+
*/
16+
17+
package atomicfile_test
18+
19+
import (
20+
"fmt"
21+
"os"
22+
"path/filepath"
23+
"testing"
24+
25+
"github.com/caddyserver/certmagic/internal/atomicfile"
26+
"github.com/caddyserver/certmagic/internal/testutil"
27+
)
28+
29+
func TestFile(t *testing.T) {
30+
const content = "this is some test content for a file"
31+
dir := t.TempDir()
32+
path := filepath.Join(dir, "test-file")
33+
34+
f, err := atomicfile.New(path, 0o644)
35+
testutil.RequireNoError(t, err, "failed to create file")
36+
n, err := fmt.Fprint(f, content)
37+
testutil.AssertNoError(t, err, "failed to write content")
38+
testutil.AssertEqual(t, len(content), n, "written bytes should be equal")
39+
err = f.Close()
40+
testutil.RequireNoError(t, err, "failed to close file")
41+
42+
actual, err := os.ReadFile(path)
43+
testutil.AssertNoError(t, err, "failed to read file")
44+
testutil.AssertEqual(t, content, string(actual))
45+
}
46+
47+
func TestConcurrentWrites(t *testing.T) {
48+
const content1 = "this is the first content of the file. there should be none other."
49+
const content2 = "the second content of the file should win!"
50+
dir := t.TempDir()
51+
path := filepath.Join(dir, "test-file")
52+
53+
file1, err := atomicfile.New(path, 0o600)
54+
testutil.RequireNoError(t, err, "failed to create file1")
55+
file2, err := atomicfile.New(path, 0o644)
56+
testutil.RequireNoError(t, err, "failed to create file2")
57+
58+
n, err := fmt.Fprint(file1, content1)
59+
testutil.AssertNoError(t, err, "failed to write content1")
60+
testutil.AssertEqual(t, len(content1), n, "written bytes should be equal")
61+
62+
n, err = fmt.Fprint(file2, content2)
63+
testutil.AssertNoError(t, err, "failed to write content2")
64+
testutil.AssertEqual(t, len(content2), n, "written bytes should be equal")
65+
66+
err = file1.Close()
67+
testutil.RequireNoError(t, err, "failed to close file1")
68+
actual, err := os.ReadFile(path)
69+
testutil.AssertNoError(t, err, "failed to read file")
70+
testutil.AssertEqual(t, content1, string(actual))
71+
72+
err = file2.Close()
73+
testutil.RequireNoError(t, err, "failed to close file2")
74+
actual, err = os.ReadFile(path)
75+
testutil.AssertNoError(t, err, "failed to read file")
76+
testutil.AssertEqual(t, content2, string(actual))
77+
}

internal/testutil/readme.md

+27
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,27 @@
1+
# testutil
2+
3+
some testing functions copied out of github.com/stretchr/testify, which were originally used by atomicfile
4+
5+
```
6+
MIT License
7+
8+
Copyright (c) 2012-2020 Mat Ryer, Tyler Bunnell and contributors.
9+
10+
Permission is hereby granted, free of charge, to any person obtaining a copy
11+
of this software and associated documentation files (the "Software"), to deal
12+
in the Software without restriction, including without limitation the rights
13+
to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
14+
copies of the Software, and to permit persons to whom the Software is
15+
furnished to do so, subject to the following conditions:
16+
17+
The above copyright notice and this permission notice shall be included in all
18+
copies or substantial portions of the Software.
19+
20+
THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
21+
IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
22+
FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
23+
AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
24+
LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
25+
OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
26+
SOFTWARE.
27+
```

0 commit comments

Comments
 (0)