Skip to content

Commit c602f87

Browse files
joelanfordjoelanford
committed
EnsureEmptyDirectory should recursively set writable perms prior to delete
Signed-off-by: Joe Lanford <joe.lanford@gmail.com>
1 parent 9b08aea commit c602f87

File tree

7 files changed

+172
-173
lines changed

7 files changed

+172
-173
lines changed

‎catalogd/internal/source/containers_image.go‎

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -155,7 +155,7 @@ func (i *ContainersImageRegistry) Unpack(ctx context.Context, catalog *catalogdv
155155
//
156156
//////////////////////////////////////////////////////
157157
if err := i.unpackImage(ctx, unpackPath, layoutRef, specIsCanonical, srcCtx); err != nil {
158-
if cleanupErr := source.DeleteReadOnlyRecursive(unpackPath); cleanupErr != nil {
158+
if cleanupErr := fsutil.DeleteReadOnlyRecursive(unpackPath); cleanupErr != nil {
159159
err = errors.Join(err, cleanupErr)
160160
}
161161
return nil, fmt.Errorf("error unpacking image: %w", err)
@@ -196,7 +196,7 @@ func successResult(unpackPath string, canonicalRef reference.Canonical, lastUnpa
196196
}
197197

198198
func (i *ContainersImageRegistry) Cleanup(_ context.Context, catalog *catalogdv1.ClusterCatalog) error {
199-
if err := source.DeleteReadOnlyRecursive(i.catalogPath(catalog.Name)); err != nil {
199+
if err := fsutil.DeleteReadOnlyRecursive(i.catalogPath(catalog.Name)); err != nil {
200200
return fmt.Errorf("error deleting catalog cache: %w", err)
201201
}
202202
return nil
@@ -316,10 +316,10 @@ func (i *ContainersImageRegistry) unpackImage(ctx context.Context, unpackPath st
316316
l.Info("applied layer", "layer", i)
317317
return nil
318318
}(); err != nil {
319-
return errors.Join(err, source.DeleteReadOnlyRecursive(unpackPath))
319+
return errors.Join(err, fsutil.DeleteReadOnlyRecursive(unpackPath))
320320
}
321321
}
322-
if err := source.SetReadOnlyRecursive(unpackPath); err != nil {
322+
if err := fsutil.SetReadOnlyRecursive(unpackPath); err != nil {
323323
return fmt.Errorf("error making unpack directory read-only: %w", err)
324324
}
325325
return nil
@@ -363,7 +363,7 @@ func (i *ContainersImageRegistry) deleteOtherImages(catalogName string, digestTo
363363
continue
364364
}
365365
imgDirPath := filepath.Join(catalogPath, imgDir.Name())
366-
if err := source.DeleteReadOnlyRecursive(imgDirPath); err != nil {
366+
if err := fsutil.DeleteReadOnlyRecursive(imgDirPath); err != nil {
367367
return fmt.Errorf("error removing image directory: %w", err)
368368
}
369369
}

‎internal/fsutil/helpers.go‎

Lines changed: 56 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
package fsutil
22

33
import (
4+
"fmt"
45
"io/fs"
56
"os"
67
"path/filepath"
@@ -17,9 +18,63 @@ func EnsureEmptyDirectory(path string, perm fs.FileMode) error {
1718
return err
1819
}
1920
for _, entry := range entries {
20-
if err := os.RemoveAll(filepath.Join(path, entry.Name())); err != nil {
21+
if err := DeleteReadOnlyRecursive(filepath.Join(path, entry.Name())); err != nil {
2122
return err
2223
}
2324
}
2425
return os.MkdirAll(path, perm)
2526
}
27+
28+
const (
29+
ownerWritableFileMode os.FileMode = 0700
30+
ownerWritableDirMode os.FileMode = 0700
31+
ownerReadOnlyFileMode os.FileMode = 0400
32+
ownerReadOnlyDirMode os.FileMode = 0500
33+
)
34+
35+
// SetReadOnlyRecursive recursively sets files and directories under the path given by `root` as read-only
36+
func SetReadOnlyRecursive(root string) error {
37+
return setModeRecursive(root, ownerReadOnlyFileMode, ownerReadOnlyDirMode)
38+
}
39+
40+
// SetWritableRecursive recursively sets files and directories under the path given by `root` as writable
41+
func SetWritableRecursive(root string) error {
42+
return setModeRecursive(root, ownerWritableFileMode, ownerWritableDirMode)
43+
}
44+
45+
// DeleteReadOnlyRecursive deletes read-only directory with path given by `root`
46+
func DeleteReadOnlyRecursive(root string) error {
47+
if err := SetWritableRecursive(root); err != nil {
48+
return fmt.Errorf("error making directory writable for deletion: %w", err)
49+
}
50+
return os.RemoveAll(root)
51+
}
52+
53+
func setModeRecursive(path string, fileMode os.FileMode, dirMode os.FileMode) error {
54+
return filepath.WalkDir(path, func(path string, d os.DirEntry, err error) error {
55+
if os.IsNotExist(err) {
56+
return nil
57+
}
58+
if err != nil {
59+
return err
60+
}
61+
fi, err := d.Info()
62+
if err != nil {
63+
return err
64+
}
65+
66+
switch typ := fi.Mode().Type(); typ {
67+
case os.ModeSymlink:
68+
// do not follow symlinks
69+
// 1. if they resolve to other locations in the root, we'll find them anyway
70+
// 2. if they resolve to other locations outside the root, we don't want to change their permissions
71+
return nil
72+
case os.ModeDir:
73+
return os.Chmod(path, dirMode)
74+
case 0: // regular file
75+
return os.Chmod(path, fileMode)
76+
default:
77+
return fmt.Errorf("refusing to change ownership of file %q with type %v", path, typ.String())
78+
}
79+
})
80+
}

‎internal/fsutil/helpers_test.go‎

Lines changed: 102 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -1,13 +1,12 @@
1-
package fsutil_test
1+
package fsutil
22

33
import (
4+
"io/fs"
45
"os"
56
"path/filepath"
67
"testing"
78

89
"github.com/stretchr/testify/require"
9-
10-
"github.com/operator-framework/operator-controller/internal/fsutil"
1110
)
1211

1312
func TestEnsureEmptyDirectory(t *testing.T) {
@@ -16,7 +15,7 @@ func TestEnsureEmptyDirectory(t *testing.T) {
1615
dirPerms := os.FileMode(0755)
1716

1817
t.Log("Ensure directory is created with the correct perms if it does not already exist")
19-
require.NoError(t, fsutil.EnsureEmptyDirectory(dirPath, dirPerms))
18+
require.NoError(t, EnsureEmptyDirectory(dirPath, dirPerms))
2019

2120
stat, err := os.Stat(dirPath)
2221
require.NoError(t, err)
@@ -25,15 +24,16 @@ func TestEnsureEmptyDirectory(t *testing.T) {
2524

2625
t.Log("Create a file inside directory")
2726
file := filepath.Join(dirPath, "file1")
28-
// nolint:gosec
29-
require.NoError(t, os.WriteFile(file, []byte("test"), 0640))
27+
// write file as read-only to verify EnsureEmptyDirectory can still delete it.
28+
require.NoError(t, os.WriteFile(file, []byte("test"), 0400))
3029

3130
t.Log("Create a sub-directory inside directory")
3231
subDir := filepath.Join(dirPath, "subdir")
33-
require.NoError(t, os.Mkdir(subDir, dirPerms))
32+
// write subDir as read-execute-only to verify EnsureEmptyDirectory can still delete it.
33+
require.NoError(t, os.Mkdir(subDir, 0500))
3434

3535
t.Log("Call EnsureEmptyDirectory against directory with different permissions")
36-
require.NoError(t, fsutil.EnsureEmptyDirectory(dirPath, 0640))
36+
require.NoError(t, EnsureEmptyDirectory(dirPath, 0640))
3737

3838
t.Log("Ensure directory is now empty")
3939
entries, err := os.ReadDir(dirPath)
@@ -45,3 +45,97 @@ func TestEnsureEmptyDirectory(t *testing.T) {
4545
require.NoError(t, err)
4646
require.Equal(t, dirPerms, stat.Mode().Perm())
4747
}
48+
49+
func TestSetReadOnlyRecursive(t *testing.T) {
50+
tempDir := t.TempDir()
51+
targetFilePath := filepath.Join(tempDir, "target")
52+
nestedDir := filepath.Join(tempDir, "nested")
53+
filePath := filepath.Join(nestedDir, "testfile")
54+
symlinkPath := filepath.Join(nestedDir, "symlink")
55+
56+
t.Log("Create symlink target file outside directory with its own permissions")
57+
// nolint:gosec
58+
require.NoError(t, os.WriteFile(targetFilePath, []byte("something"), 0644))
59+
60+
t.Log("Create a nested directory structure that contains a file and sym. link")
61+
require.NoError(t, os.Mkdir(nestedDir, ownerWritableDirMode))
62+
require.NoError(t, os.WriteFile(filePath, []byte("test"), ownerWritableFileMode))
63+
require.NoError(t, os.Symlink(targetFilePath, symlinkPath))
64+
65+
t.Log("Set directory structure as read-only")
66+
require.NoError(t, SetReadOnlyRecursive(nestedDir))
67+
68+
t.Log("Check file permissions")
69+
stat, err := os.Stat(filePath)
70+
require.NoError(t, err)
71+
require.Equal(t, ownerReadOnlyFileMode, stat.Mode().Perm())
72+
73+
t.Log("Check directory permissions")
74+
nestedStat, err := os.Stat(nestedDir)
75+
require.NoError(t, err)
76+
require.Equal(t, ownerReadOnlyDirMode, nestedStat.Mode().Perm())
77+
78+
t.Log("Check symlink target file permissions - should not be affected")
79+
stat, err = os.Stat(targetFilePath)
80+
require.NoError(t, err)
81+
require.Equal(t, fs.FileMode(0644), stat.Mode().Perm())
82+
83+
t.Log("Make directory writable to enable test clean-up")
84+
require.NoError(t, SetWritableRecursive(tempDir))
85+
}
86+
87+
func TestSetWritableRecursive(t *testing.T) {
88+
tempDir := t.TempDir()
89+
targetFilePath := filepath.Join(tempDir, "target")
90+
nestedDir := filepath.Join(tempDir, "nested")
91+
filePath := filepath.Join(nestedDir, "testfile")
92+
symlinkPath := filepath.Join(nestedDir, "symlink")
93+
94+
t.Log("Create symlink target file outside directory with its own permissions")
95+
// nolint:gosec
96+
require.NoError(t, os.WriteFile(targetFilePath, []byte("something"), 0644))
97+
98+
t.Log("Create a nested directory (writable) structure that contains a file (read-only) and sym. link")
99+
require.NoError(t, os.Mkdir(nestedDir, ownerWritableDirMode))
100+
require.NoError(t, os.WriteFile(filePath, []byte("test"), ownerReadOnlyFileMode))
101+
require.NoError(t, os.Symlink(targetFilePath, symlinkPath))
102+
103+
t.Log("Make directory read-only")
104+
require.NoError(t, os.Chmod(nestedDir, ownerReadOnlyDirMode))
105+
106+
t.Log("Call SetWritableRecursive")
107+
require.NoError(t, SetWritableRecursive(nestedDir))
108+
109+
t.Log("Check file is writable")
110+
stat, err := os.Stat(filePath)
111+
require.NoError(t, err)
112+
require.Equal(t, ownerWritableFileMode, stat.Mode().Perm())
113+
114+
t.Log("Check directory is writable")
115+
nestedStat, err := os.Stat(nestedDir)
116+
require.NoError(t, err)
117+
require.Equal(t, ownerWritableDirMode, nestedStat.Mode().Perm())
118+
119+
t.Log("Check symlink target file permissions - should not be affected")
120+
stat, err = os.Stat(targetFilePath)
121+
require.NoError(t, err)
122+
require.Equal(t, fs.FileMode(0644), stat.Mode().Perm())
123+
}
124+
125+
func TestDeleteReadOnlyRecursive(t *testing.T) {
126+
tempDir := t.TempDir()
127+
nestedDir := filepath.Join(tempDir, "nested")
128+
filePath := filepath.Join(nestedDir, "testfile")
129+
130+
t.Log("Create a nested read-only directory structure that contains a file and sym. link")
131+
require.NoError(t, os.Mkdir(nestedDir, ownerWritableDirMode))
132+
require.NoError(t, os.WriteFile(filePath, []byte("test"), ownerReadOnlyFileMode))
133+
require.NoError(t, os.Chmod(nestedDir, ownerReadOnlyDirMode))
134+
135+
t.Log("Set directory structure as read-only")
136+
require.NoError(t, DeleteReadOnlyRecursive(nestedDir))
137+
138+
t.Log("Ensure directory was deleted")
139+
_, err := os.Stat(nestedDir)
140+
require.ErrorIs(t, err, os.ErrNotExist)
141+
}

‎internal/rukpak/source/containers_image.go‎

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -148,7 +148,7 @@ func (i *ContainersImageRegistry) Unpack(ctx context.Context, bundle *BundleSour
148148
//
149149
//////////////////////////////////////////////////////
150150
if err := i.unpackImage(ctx, unpackPath, layoutRef, srcCtx); err != nil {
151-
if cleanupErr := DeleteReadOnlyRecursive(unpackPath); cleanupErr != nil {
151+
if cleanupErr := fsutil.DeleteReadOnlyRecursive(unpackPath); cleanupErr != nil {
152152
err = errors.Join(err, cleanupErr)
153153
}
154154
return nil, fmt.Errorf("error unpacking image: %w", err)
@@ -176,7 +176,7 @@ func successResult(bundleName, unpackPath string, canonicalRef reference.Canonic
176176
}
177177

178178
func (i *ContainersImageRegistry) Cleanup(_ context.Context, bundle *BundleSource) error {
179-
return DeleteReadOnlyRecursive(i.bundlePath(bundle.Name))
179+
return fsutil.DeleteReadOnlyRecursive(i.bundlePath(bundle.Name))
180180
}
181181

182182
func (i *ContainersImageRegistry) bundlePath(bundleName string) string {
@@ -285,10 +285,10 @@ func (i *ContainersImageRegistry) unpackImage(ctx context.Context, unpackPath st
285285
l.Info("applied layer", "layer", i)
286286
return nil
287287
}(); err != nil {
288-
return errors.Join(err, DeleteReadOnlyRecursive(unpackPath))
288+
return errors.Join(err, fsutil.DeleteReadOnlyRecursive(unpackPath))
289289
}
290290
}
291-
if err := SetReadOnlyRecursive(unpackPath); err != nil {
291+
if err := fsutil.SetReadOnlyRecursive(unpackPath); err != nil {
292292
return fmt.Errorf("error making unpack directory read-only: %w", err)
293293
}
294294
return nil
@@ -325,7 +325,7 @@ func (i *ContainersImageRegistry) deleteOtherImages(bundleName string, digestToK
325325
continue
326326
}
327327
imgDirPath := filepath.Join(bundlePath, imgDir.Name())
328-
if err := DeleteReadOnlyRecursive(imgDirPath); err != nil {
328+
if err := fsutil.DeleteReadOnlyRecursive(imgDirPath); err != nil {
329329
return fmt.Errorf("error removing image directory: %w", err)
330330
}
331331
}

‎internal/rukpak/source/containers_image_test.go‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,7 @@ import (
2222
"github.com/stretchr/testify/require"
2323
"sigs.k8s.io/controller-runtime/pkg/reconcile"
2424

25+
"github.com/operator-framework/operator-controller/internal/fsutil"
2526
"github.com/operator-framework/operator-controller/internal/rukpak/source"
2627
)
2728

@@ -286,7 +287,7 @@ func TestUnpackUnexpectedFile(t *testing.T) {
286287
require.True(t, stat.IsDir())
287288

288289
// Unset read-only to allow cleanup
289-
require.NoError(t, source.SetWritableRecursive(unpackPath))
290+
require.NoError(t, fsutil.SetWritableRecursive(unpackPath))
290291
}
291292

292293
func TestUnpackCopySucceedsMountFails(t *testing.T) {

‎internal/rukpak/source/helpers.go‎

Lines changed: 0 additions & 56 deletions
Original file line numberDiff line numberDiff line change
@@ -2,37 +2,10 @@ package source
22

33
import (
44
"errors"
5-
"fmt"
65
"os"
7-
"path/filepath"
86
"time"
97
)
108

11-
const (
12-
OwnerWritableFileMode os.FileMode = 0700
13-
OwnerWritableDirMode os.FileMode = 0700
14-
OwnerReadOnlyFileMode os.FileMode = 0400
15-
OwnerReadOnlyDirMode os.FileMode = 0500
16-
)
17-
18-
// SetReadOnlyRecursive recursively sets files and directories under the path given by `root` as read-only
19-
func SetReadOnlyRecursive(root string) error {
20-
return setModeRecursive(root, OwnerReadOnlyFileMode, OwnerReadOnlyDirMode)
21-
}
22-
23-
// SetWritableRecursive recursively sets files and directories under the path given by `root` as writable
24-
func SetWritableRecursive(root string) error {
25-
return setModeRecursive(root, OwnerWritableFileMode, OwnerWritableDirMode)
26-
}
27-
28-
// DeleteReadOnlyRecursive deletes read-only directory with path given by `root`
29-
func DeleteReadOnlyRecursive(root string) error {
30-
if err := SetWritableRecursive(root); err != nil {
31-
return fmt.Errorf("error making directory writable for deletion: %w", err)
32-
}
33-
return os.RemoveAll(root)
34-
}
35-
369
// IsImageUnpacked checks whether an image has been unpacked in `unpackPath`.
3710
// If true, time of unpack will also be returned. If false unpack time is gibberish (zero/epoch time).
3811
// If `unpackPath` is a file, it will be deleted and false will be returned without an error.
@@ -49,32 +22,3 @@ func IsImageUnpacked(unpackPath string) (bool, time.Time, error) {
4922
}
5023
return true, unpackStat.ModTime(), nil
5124
}
52-
53-
func setModeRecursive(path string, fileMode os.FileMode, dirMode os.FileMode) error {
54-
return filepath.WalkDir(path, func(path string, d os.DirEntry, err error) error {
55-
if os.IsNotExist(err) {
56-
return nil
57-
}
58-
if err != nil {
59-
return err
60-
}
61-
fi, err := d.Info()
62-
if err != nil {
63-
return err
64-
}
65-
66-
switch typ := fi.Mode().Type(); typ {
67-
case os.ModeSymlink:
68-
// do not follow symlinks
69-
// 1. if they resolve to other locations in the root, we'll find them anyway
70-
// 2. if they resolve to other locations outside the root, we don't want to change their permissions
71-
return nil
72-
case os.ModeDir:
73-
return os.Chmod(path, dirMode)
74-
case 0: // regular file
75-
return os.Chmod(path, fileMode)
76-
default:
77-
return fmt.Errorf("refusing to change ownership of file %q with type %v", path, typ.String())
78-
}
79-
})
80-
}

0 commit comments

Comments
 (0)