From 8719f08b55ff984cbfdb5d7693b4fbd0d21624ef Mon Sep 17 00:00:00 2001 From: Ville Aikas Date: Tue, 27 Sep 2022 14:15:55 -0700 Subject: [PATCH] Update as per feedback. Use io.FS instead of raw directories. Signed-off-by: Ville Aikas --- client/client_test.go | 32 ++++----- client/file_store.go | 90 +++++++++++++----------- client/file_store_test.go | 143 +++++++++++++++++++++++++------------- 3 files changed, 157 insertions(+), 108 deletions(-) diff --git a/client/client_test.go b/client/client_test.go index a848d104f..e266886c6 100644 --- a/client/client_test.go +++ b/client/client_test.go @@ -77,7 +77,7 @@ func (f *fakeRemoteStore) get(name string, store map[string]*fakeFile) (io.ReadC // because the set/delete methods are not part of the Interface, we need to // switch on the underlying implementation. // Also readMeta method is convenience for ease of testing. -func (s *ClientSuite) setMeta(path string, data []byte) error { +func (s *ClientSuite) setRemoteMeta(path string, data []byte) error { switch impl := s.remote.(type) { case *fakeRemoteStore: impl.meta[path] = newFakeFile(data) @@ -89,7 +89,7 @@ func (s *ClientSuite) setMeta(path string, data []byte) error { } } -func (s *ClientSuite) setTarget(path string, data []byte) error { +func (s *ClientSuite) setRemoteTarget(path string, data []byte) error { switch impl := s.remote.(type) { case *fakeRemoteStore: impl.targets[path] = newFakeFile(data) @@ -109,7 +109,7 @@ func (s *ClientSuite) deleteMeta(path string) error { case *FileRemoteStore: return impl.deleteMeta(path) default: - return fmt.Errorf("non-supoprted RemoteStore, got %+v", impl) + return fmt.Errorf("non-supported RemoteStore, got %+v", impl) } } @@ -121,7 +121,7 @@ func (s *ClientSuite) deleteTarget(path string) error { case *FileRemoteStore: return impl.deleteTarget(path) default: - return fmt.Errorf("non-supoprted RemoteStore, got %+v", impl) + return fmt.Errorf("non-supported RemoteStore, got %+v", impl) } } @@ -183,7 +183,7 @@ func (s *ClientSuite) SetUpTest(c *C) { // create a remote store containing valid repo files if s.useFileStore { - s.remote, s.tmpDir, err = newTestFileStore() + s.remote, s.tmpDir, err = newTestFileStoreFS() if err != nil { c.Fatalf("failed to create new FileStore: %v", err) } @@ -192,7 +192,7 @@ func (s *ClientSuite) SetUpTest(c *C) { } s.syncRemote(c) for path, data := range targetFiles { - s.setTarget(path, data) + s.setRemoteTarget(path, data) } s.expiredTime = time.Now().Add(time.Hour) @@ -242,7 +242,7 @@ func (s *ClientSuite) syncRemote(c *C) { meta, err := s.store.GetMeta() c.Assert(err, IsNil) for name, data := range meta { - if err := s.setMeta(name, data); err != nil { + if err := s.setRemoteMeta(name, data); err != nil { panic(fmt.Sprintf("setMetadata failed: %v", err)) } } @@ -855,7 +855,7 @@ func (s *ClientSuite) TestLocalExpired(c *C) { } func (s *ClientSuite) TestTimestampTooLarge(c *C) { - s.setMeta("timestamp.json", make([]byte, defaultTimestampDownloadLimit+1)) + s.setRemoteMeta("timestamp.json", make([]byte, defaultTimestampDownloadLimit+1)) _, err := s.newClient(c).Update() c.Assert(err, Equals, ErrMetaTooLarge{"timestamp.json", defaultTimestampDownloadLimit + 1, defaultTimestampDownloadLimit}) } @@ -997,7 +997,7 @@ func (s *ClientSuite) TestUpdateMixAndMatchAttack(c *C) { if err != nil { c.Fatal("missing remote targets.json") } - s.setMeta("targets.json", oldTargets) + s.setRemoteMeta("targets.json", oldTargets) // check update returns ErrWrongSize for targets.json _, err = client.Update() @@ -1008,7 +1008,7 @@ func (s *ClientSuite) TestUpdateMixAndMatchAttack(c *C) { c.Assert(s.repo.Snapshot(), IsNil) c.Assert(s.repo.Timestamp(), IsNil) s.syncRemote(c) - s.setMeta("targets.json", oldTargets) + s.setRemoteMeta("targets.json", oldTargets) // check update returns ErrWrongHash _, err = client.Update() @@ -1034,7 +1034,7 @@ func (s *ClientSuite) TestUpdateReplayAttack(c *C) { c.Assert(client.timestampVer > version, Equals, true) // replace remote timestamp.json with the old one - s.setMeta("timestamp.json", oldTimestamp) + s.setRemoteMeta("timestamp.json", oldTimestamp) // check update returns ErrLowVersion _, err = client.Update() @@ -1067,7 +1067,7 @@ func (s *ClientSuite) TestUpdateForkTimestamp(c *C) { c.Assert(newVersion > version, Equals, true) // generate a new, different timestamp with the *same version* - s.setMeta("timestamp.json", oldTimestamp) + s.setRemoteMeta("timestamp.json", oldTimestamp) c.Assert(s.repo.Timestamp(), IsNil) c.Assert(client.timestampVer, Equals, newVersion) // double-check: same version? s.syncRemote(c) @@ -1199,7 +1199,7 @@ func (s *ClientSuite) TestDownloadOK(c *C) { func (s *ClientSuite) TestDownloadWrongSize(c *C) { client := s.updatedClient(c) // Update with a file that's incorrect size. - s.setTarget("foo.txt", []byte("wrong-size")) + s.setRemoteTarget("foo.txt", []byte("wrong-size")) var dest testDestination c.Assert(client.Download("foo.txt", &dest), DeepEquals, ErrWrongSize{"foo.txt", 10, 3}) c.Assert(dest.deleted, Equals, true) @@ -1207,7 +1207,7 @@ func (s *ClientSuite) TestDownloadWrongSize(c *C) { func (s *ClientSuite) TestDownloadTargetTooLong(c *C) { client := s.updatedClient(c) - s.setTarget("foo.txt", []byte("foo-ooo")) + s.setRemoteTarget("foo.txt", []byte("foo-ooo")) var dest testDestination c.Assert(client.Download("foo.txt", &dest), DeepEquals, ErrWrongSize{"foo.txt", 7, 3}) c.Assert(dest.deleted, Equals, true) @@ -1215,7 +1215,7 @@ func (s *ClientSuite) TestDownloadTargetTooLong(c *C) { func (s *ClientSuite) TestDownloadTargetTooShort(c *C) { client := s.updatedClient(c) - s.setTarget("foo.txt", []byte("fo")) + s.setRemoteTarget("foo.txt", []byte("fo")) var dest testDestination c.Assert(client.Download("foo.txt", &dest), DeepEquals, ErrWrongSize{"foo.txt", 2, 3}) c.Assert(dest.deleted, Equals, true) @@ -1223,7 +1223,7 @@ func (s *ClientSuite) TestDownloadTargetTooShort(c *C) { func (s *ClientSuite) TestDownloadTargetCorruptData(c *C) { client := s.updatedClient(c) - s.setTarget("foo.txt", []byte("ooo")) + s.setRemoteTarget("foo.txt", []byte("ooo")) var dest testDestination assertWrongHash(c, client.Download("foo.txt", &dest)) c.Assert(dest.deleted, Equals, true) diff --git a/client/file_store.go b/client/file_store.go index 78d878714..520bbe73a 100644 --- a/client/file_store.go +++ b/client/file_store.go @@ -6,62 +6,76 @@ import ( "fmt" "io" "io/fs" - "os" - "path/filepath" ) -type FileStoreOptions struct { - MetadataPath string - TargetsPath string -} - // FileRemoteStore provides a RemoteStore interface compatible -// implementation that can be used where the RemoteStore is on local disk. This -// is useful for example in air-gapped environments where there's no +// implementation that can be used where the RemoteStore is backed by a +// fs.FS. This is useful for example in air-gapped environments where there's no // possibility to make outbound network connections. -// If targetsDir is "", metadataDir/targets is used. -func NewFileRemoteStore(metadataDir, targetsDir string) (*FileRemoteStore, error) { - if metadataDir == "" { - return nil, errors.New("metadataDir can't be empty") +// By having this be a fs.FS instead of directories allows the repository to +// be backed by something that's not persisted to disk. +func NewFileRemoteStore(fsys fs.FS, targetDir string) (*FileRemoteStore, error) { + if fsys == nil { + return nil, errors.New("nil fs.FS") + } + t := targetDir + if t == "" { + t = "targets" } - f := &FileRemoteStore{ - metadataDir: metadataDir, + // Make sure directory exists + d, err := fsys.Open(t) + if err != nil { + return nil, fmt.Errorf("failed to open targets directory %s: %w", t, err) + } + fi, err := d.Stat() + if err != nil { + return nil, fmt.Errorf("failed to stat targets directory %s: %w", t, err) } - if targetsDir != "" { - f.targetsDir = targetsDir - } else { - f.targetsDir = filepath.Join(f.metadataDir, "targets") + if !fi.IsDir() { + return nil, fmt.Errorf("targets directory not a directory %s", t) } - // Make sure metadataDir and targetsDir exist and are directories before - // proceeding. - for _, dir := range []string{f.metadataDir, f.targetsDir} { - s, err := os.Stat(dir) - if err != nil { - return nil, fmt.Errorf("failed to stat %s: %s", dir, err) - } - if !s.IsDir() { - return nil, fmt.Errorf("not a directory %s", dir) - } + fsysT, err := fs.Sub(fsys, t) + if err != nil { + return nil, fmt.Errorf("failed to open targets directory %s: %w", t, err) } - return f, nil + return &FileRemoteStore{fsys: fsys, targetDir: fsysT}, nil } type FileRemoteStore struct { - metadataDir string - targetsDir string + // Meta directory fs + fsys fs.FS + // Target directory fs. + targetDir fs.FS + // In order to be able to make write operations (create, delete) we can't + // use fs.FS for it (it's read only), so we have to know the underlying + // directory that add/delete test methods can use. This is only necessary + // for testing purposes. + testDir string } func (f *FileRemoteStore) GetMeta(name string) (io.ReadCloser, int64, error) { - rc, b, err := f.get(filepath.Join(f.metadataDir, name)) + rc, b, err := f.get(f.fsys, name) return handleErrors(name, rc, b, err) } func (f *FileRemoteStore) GetTarget(name string) (io.ReadCloser, int64, error) { - rc, b, err := f.get(filepath.Join(f.targetsDir, name)) + rc, b, err := f.get(f.targetDir, name) return handleErrors(name, rc, b, err) } +func (f *FileRemoteStore) get(fsys fs.FS, s string) (io.ReadCloser, int64, error) { + if !fs.ValidPath(s) { + return nil, 0, fmt.Errorf("invalid path %s", s) + } + + b, err := fs.ReadFile(fsys, s) + if err != nil { + return nil, -1, err + } + return io.NopCloser(bytes.NewReader(b)), int64(len(b)), nil +} + // handleErrors converts NotFound errors to something that TUF knows how to // handle properly. For example, when looking for n+1 root files, this is a // signal that it will stop looking. @@ -74,11 +88,3 @@ func handleErrors(name string, rc io.ReadCloser, b int64, err error) (io.ReadClo } return rc, b, err } - -func (f *FileRemoteStore) get(s string) (io.ReadCloser, int64, error) { - b, err := os.ReadFile(s) - if err != nil { - return nil, -1, err - } - return io.NopCloser(bytes.NewReader(b)), int64(len(b)), nil -} diff --git a/client/file_store_test.go b/client/file_store_test.go index 91dc17ac3..c31431602 100644 --- a/client/file_store_test.go +++ b/client/file_store_test.go @@ -1,69 +1,55 @@ package client import ( + "bytes" + "io/fs" + "io/ioutil" "os" "path/filepath" "strings" "testing" ) -func TestBadDirs(t *testing.T) { - // Note that since we create the temp directory, all the paths defined here - // in the tests are relative to that, as in, we just tack the md/td after - // it. - // Create the file store with the repository and repository/targets - // directories. - dir := t.TempDir() - os.Mkdir(filepath.Join(dir, "repository"), os.ModePerm) - os.Mkdir(filepath.Join(dir, "repository", "targets"), os.ModePerm) - os.Create(filepath.Join(dir, "repository-that-isfile")) +const targetsDir = "targets" + +func TestCreates(t *testing.T) { + tmpDir := t.TempDir() + defer os.RemoveAll(tmpDir) + dir := filepath.Join(tmpDir, "repository") + os.Mkdir(dir, os.ModePerm) + os.Mkdir(filepath.Join(dir, "targets"), os.ModePerm) os.Create(filepath.Join(dir, "targets-that-isfile")) tests := []struct { name string - md string + fsys fs.FS td string wantErr string }{{ - name: "empty, error", - wantErr: "metadataDir can't be empty", - }, { - name: "invalid metadatadir, missing", - md: "notthere", - wantErr: "notthere", + name: "nil, error", + wantErr: "nil fs.FS", }, { - name: "invalid targetsdir, missing", - md: "repository", - td: "targetsnotthere", - wantErr: "targetsnotthere", + name: "missing targets directory", + fsys: os.DirFS(dir), + td: "targets-not-there", + wantErr: "failed to open targets directory targets-not-there", }, { - name: "invalid meta dir, is file", - md: "repository-that-isfile", - wantErr: "not a directory", + name: "targets directory is not a file", + fsys: os.DirFS(dir), + td: "targets-that-isfile", + wantErr: "targets directory not a directory targets-that-isfile", }, { - name: "invalid targets dir, is file", - md: "targets-that-isfile", - wantErr: "not a directory", - }, { - name: "works, only specify meta", - md: "repository", + name: "works, explicit targets", + fsys: os.DirFS(dir), + td: "targets", }, { name: "works, explicit targets", - md: "repository", - td: "repository/targets", + fsys: os.DirFS(dir), + td: "targets", }} + for _, tc := range tests { - absoluteMeta := filepath.Join(dir, tc.md) - if tc.md == "" { - // pass in empty explicitly - absoluteMeta = tc.md - } - absoluteTargets := filepath.Join(dir, tc.td) - if tc.td == "" { - // pass in empty explicitly - absoluteTargets = tc.td - } - _, err := NewFileRemoteStore(absoluteMeta, absoluteTargets) + _, err := NewFileRemoteStore(tc.fsys, tc.td) if tc.wantErr != "" && err == nil { t.Errorf("%q wanted error %s, got none", tc.name, tc.wantErr) } else if tc.wantErr == "" && err != nil { @@ -74,30 +60,87 @@ func TestBadDirs(t *testing.T) { } } +func TestBasicOps(t *testing.T) { + metas := map[string][]byte{ + "root.json": []byte("root"), + "snapshot.json": []byte("snapshot"), + "timestamp": []byte("timestamp"), + } + + fsys, dir, err := newTestFileStoreFS() + if err != nil { + t.Fatalf("Failed to create test FileStore") + } + defer os.RemoveAll(dir) + + // Add targets and metas and check them. + for k, v := range targetFiles { + if err := fsys.addTarget(k, v); err != nil { + t.Errorf("failed to add target %s: %v", k, err) + } + rc, size, err := fsys.GetTarget(k) + if err != nil { + t.Errorf("failed to GetTarget %s: %v", k, err) + } + if size != int64(len(v)) { + t.Errorf("unexpected size returned for GetTarget: %s want %d got %d", k, len(v), size) + } + got, err := ioutil.ReadAll(rc) + if err != nil { + t.Errorf("failed to ReadAll returned ReacCloser %s: %v", k, err) + } + if bytes.Compare(v, got) != 0 { + t.Errorf("Read unexpected bytes, want: %s got: %s", string(k), string(got)) + } + } + for k, v := range metas { + if err := fsys.addMeta(k, v); err != nil { + t.Errorf("failed to add meta %s %v", k, err) + } + rc, size, err := fsys.GetMeta(k) + if err != nil { + t.Errorf("failed to GetMeta %s: %v", k, err) + } + if size != int64(len(v)) { + t.Errorf("unexpected size returned for GetMeta: %s want %d got %d", k, len(v), size) + } + got, err := ioutil.ReadAll(rc) + if err != nil { + t.Errorf("failed to ReadAll returned ReacCloser %s: %v", k, err) + } + if bytes.Compare(v, got) != 0 { + t.Errorf("Read unexpected bytes, want: %s got: %s", string(k), string(got)) + } + } +} + // Test helper methods func (f *FileRemoteStore) addMeta(name string, data []byte) error { - return os.WriteFile(filepath.Join(f.metadataDir, name), data, os.ModePerm) + return os.WriteFile(filepath.Join(f.testDir, name), data, os.ModePerm) } func (f *FileRemoteStore) addTarget(name string, data []byte) error { - return os.WriteFile(filepath.Join(f.targetsDir, name), data, os.ModePerm) + fname := filepath.Join(f.testDir, targetsDir, name) + err := os.WriteFile(fname, data, os.ModePerm) + return err } func (f *FileRemoteStore) deleteMeta(name string) error { - return os.Remove(filepath.Join(f.metadataDir, name)) + return os.Remove(filepath.Join(f.testDir, name)) } func (f *FileRemoteStore) deleteTarget(name string) error { - return os.Remove(filepath.Join(f.targetsDir, name)) + return os.Remove(filepath.Join(f.testDir, targetsDir, name)) } -func newTestFileStore() (*FileRemoteStore, string, error) { +func newTestFileStoreFS() (*FileRemoteStore, string, error) { tmpDir := os.TempDir() tufDir := filepath.Join(tmpDir, "tuf-file-store-test") // Clean it in case there is cruft left around os.RemoveAll(tufDir) os.Mkdir(tufDir, os.ModePerm) - os.Mkdir(filepath.Join(tufDir, "targets"), os.ModePerm) - fs, err := NewFileRemoteStore(tufDir, "") + os.Mkdir(filepath.Join(tufDir, targetsDir), os.ModePerm) + fs, err := NewFileRemoteStore(os.DirFS(tufDir), targetsDir) + fs.testDir = tufDir return fs, tufDir, err }