Skip to content

Commit 6efadaa

Browse files
[ARCH-327] Address security comments. 2 (#1760)
* Minor. * Minor. * Minor. * Minor. * Minor. * Minor. * Minor. --------- Co-authored-by: Connor Stein <connor.stein2@gmail.com>
1 parent 8bde3ee commit 6efadaa

6 files changed

Lines changed: 99 additions & 5 deletions

File tree

keystore/file.go

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,7 @@ import (
55
"context"
66
"os"
77

8-
"github.com/natefinch/atomic"
8+
"github.com/smartcontractkit/chainlink-common/keystore/internal/atomicfile"
99
)
1010

1111
var _ Storage = &FileStorage{}
@@ -26,5 +26,5 @@ func (f *FileStorage) GetEncryptedKeystore(ctx context.Context) ([]byte, error)
2626
}
2727

2828
func (f *FileStorage) PutEncryptedKeystore(ctx context.Context, encryptedKeystore []byte) error {
29-
return atomic.WriteFile(f.name, bytes.NewReader(encryptedKeystore))
29+
return atomicfile.WriteFile(f.name, bytes.NewReader(encryptedKeystore), 0600)
3030
}

keystore/go.mod

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,6 @@ require (
66
github.com/ethereum/go-ethereum v1.16.2
77
github.com/jmoiron/sqlx v1.4.0
88
github.com/lib/pq v1.10.9
9-
github.com/natefinch/atomic v1.0.1
109
github.com/smartcontractkit/chainlink-common v0.9.6-0.20251107154219-ec6d8370ebbf
1110
github.com/smartcontractkit/libocr v0.0.0-20250912173940-f3ab0246e23d
1211
github.com/spf13/cobra v1.8.1

keystore/go.sum

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -235,8 +235,6 @@ github.com/mr-tron/base58 v1.2.0 h1:T/HDJBh4ZCPbU39/+c3rRvE0uKBQlU27+QI8LJ4t64o=
235235
github.com/mr-tron/base58 v1.2.0/go.mod h1:BinMc/sQntlIE1frQmRFPUoPA1Zkr8VRgBdjWI2mNwc=
236236
github.com/munnerz/goautoneg v0.0.0-20191010083416-a7dc8b61c822 h1:C3w9PqII01/Oq1c1nUAm88MOHcQC9l5mIlSMApZMrHA=
237237
github.com/munnerz/goautoneg v0.0.0-20191010083416-a7dc8b61c822/go.mod h1:+n7T8mK8HuQTcFwEeznm/DIxMOiR9yIdICNftLE1DvQ=
238-
github.com/natefinch/atomic v1.0.1 h1:ZPYKxkqQOx3KZ+RsbnP/YsgvxWQPGxjC0oBt2AhwV0A=
239-
github.com/natefinch/atomic v1.0.1/go.mod h1:N/D/ELrljoqDyT3rZrsUmtsuzvHkeB/wWjHV22AZRbM=
240238
github.com/olekukonko/tablewriter v0.0.5 h1:P2Ga83D34wi1o9J6Wh1mRuqd4mF/x/lgBS7N7AbDhec=
241239
github.com/olekukonko/tablewriter v0.0.5/go.mod h1:hPp6KlRPjbx+hW8ykQs1w3UBbZlj6HuIJcUGPhkA7kY=
242240
github.com/pelletier/go-toml/v2 v2.2.4 h1:mye9XuhQ6gvn5h28+VilKrrPoQVanw5PMw/TB0t5Ec4=
Lines changed: 71 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,71 @@
1+
package atomicfile
2+
3+
import (
4+
"fmt"
5+
"io"
6+
"os"
7+
"path/filepath"
8+
)
9+
10+
// WriteFile atomically writes the contents of r to the specified filepath with the given mode.
11+
// This is a copy of https://github.com/natefinch/atomic/blob/master/atomic.go with minor modifications allowing
12+
// to set mode of the written file. If the file already exists, its mode is preserved.
13+
func WriteFile(filename string, r io.Reader, mode os.FileMode) (err error) {
14+
// write to a temp file first, then we'll atomically replace the target file
15+
// with the temp file.
16+
dir, file := filepath.Split(filename)
17+
if dir == "" {
18+
dir = "."
19+
}
20+
21+
f, err := os.CreateTemp(dir, file)
22+
if err != nil {
23+
return fmt.Errorf("cannot create temp file: %w", err)
24+
}
25+
defer func() {
26+
if err != nil {
27+
// Don't leave the temp file lying around on error.
28+
_ = os.Remove(f.Name()) // yes, ignore the error, not much we can do about it.
29+
}
30+
}()
31+
// ensure we always close f. Note that this does not conflict with the close below, as close is idempotent while
32+
// it returns an error for repeating close operations.
33+
defer f.Close() //nolint:errcheck
34+
name := f.Name()
35+
if _, err = io.Copy(f, r); err != nil {
36+
return fmt.Errorf("cannot write data to tempfile %q: %w", name, err)
37+
}
38+
// fsync is important, otherwise os.Rename could rename a zero-length file
39+
if err = f.Sync(); err != nil {
40+
return fmt.Errorf("can't flush tempfile %q: %w", name, err)
41+
}
42+
if err = f.Close(); err != nil {
43+
return fmt.Errorf("can't close tempfile %q: %w", name, err)
44+
}
45+
46+
// get the file mode from the original file and use that for the replacement file, too.
47+
destInfo, err := os.Stat(filename)
48+
if os.IsNotExist(err) {
49+
// no original file
50+
if err = os.Chmod(name, mode); err != nil {
51+
return fmt.Errorf("can't set filemode on tempfile %q: %w", name, err)
52+
}
53+
} else if err != nil {
54+
return err
55+
} else {
56+
sourceInfo, err := os.Stat(name)
57+
if err != nil {
58+
return err
59+
}
60+
61+
if sourceInfo.Mode() != destInfo.Mode() {
62+
if err = os.Chmod(name, destInfo.Mode()); err != nil {
63+
return fmt.Errorf("can't set filemode on tempfile %q: %w", name, err)
64+
}
65+
}
66+
}
67+
if err := os.Rename(name, filename); err != nil {
68+
return fmt.Errorf("cannot replace %q with tempfile %q: %w", filename, name, err)
69+
}
70+
return nil
71+
}
Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,24 @@
1+
package atomicfile
2+
3+
import (
4+
"bytes"
5+
"os"
6+
"path/filepath"
7+
"testing"
8+
9+
"github.com/stretchr/testify/require"
10+
)
11+
12+
func TestWriteFile_WriteAndRead(t *testing.T) {
13+
mode := os.FileMode(0600)
14+
path := filepath.Join(t.TempDir(), "out.txt")
15+
data := []byte("test")
16+
err := WriteFile(path, bytes.NewReader(data), mode)
17+
require.NoError(t, err)
18+
readData, err := os.ReadFile(path)
19+
require.NoError(t, err)
20+
require.Equal(t, readData, data)
21+
info, err := os.Stat(path)
22+
require.NoError(t, err)
23+
require.Equal(t, mode, info.Mode())
24+
}

keystore/internal/raw_test.go

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,8 @@ func TestRaw_nonprintable(t *testing.T) {
2323

2424
assert.Equal(t, exp, fmt.Sprintf("%#v", r))
2525

26+
assert.Equal(t, exp, fmt.Sprintf("%x", r))
27+
2628
assert.Equal(t, exp, fmt.Sprintf("%s", r)) //nolint:gosimple // S1025 deliberately testing formatting verbs
2729

2830
got, err := json.Marshal(r) //nolint:staticcheck // SA9005 deliberately testing marshalling

0 commit comments

Comments
 (0)