Check backup filename when downloading

apiserver/apiserver.go

@@ -864,8 +864,9 @@ func (srv *Server) endpoints() ([]apihttp.Endpoint, error) {
 		pattern: modelRoutePrefix + "/units/:unit/resources/:resource",
 		handler: unitResourcesHandler,
 	}, {
-		pattern: modelRoutePrefix + "/backups",
-		handler: backupHandler,
+		pattern:    modelRoutePrefix + "/backups",
+		handler:    backupHandler,
+		authorizer: controllerAdminAuthorizer,
 	}, {
 		pattern:    "/migrate/charms",
 		handler:    migrateCharmsHTTPHandler,

apiserver/backup.go

@@ -8,6 +8,7 @@ import (
 	"io"
 	"io/ioutil"
 	"net/http"
+	"os"
 
 	"github.com/juju/errors"
 
@@ -41,7 +42,25 @@ func (h *backupHandler) ServeHTTP(resp http.ResponseWriter, req *http.Request) {
 	switch req.Method {
 	case "GET":
 		logger.Infof("handling backups download request")
-		id, err := h.download(newBackups(), resp, req)
+		model, err := st.Model()
+		if err != nil {
+			h.sendError(resp, err)
+			return
+		}
+		modelConfig, err := model.ModelConfig()
+		if err != nil {
+			h.sendError(resp, err)
+			return
+		}
+		backupDir := modelConfig.BackupDir()
+		if backupDir == "" {
+			backupDir = os.TempDir()
+		}
+
+		paths := &backups.Paths{
+			BackupDir: backupDir,
+		}
+		id, err := h.download(newBackups(paths), resp, req)
 		if err != nil {
 			h.sendError(resp, err)
 			return

apiserver/backup_test.go

@@ -37,7 +37,7 @@ func (s *backupsSuite) SetUpTest(c *gc.C) {
 	s.backupURL = s.server.URL + fmt.Sprintf("/model/%s/backups", s.State.ModelUUID())
 	s.fake = &backupstesting.FakeBackups{}
 	s.PatchValue(apiserver.NewBackups,
-		func() backups.Backups {
+		func(path *backups.Paths) backups.Backups {
 			return s.fake
 		},
 	)
@@ -98,7 +98,10 @@ func (s *backupsSuite) TestAuthRequiresClientNotMachine(c *gc.C) {
 		URL:      s.backupURL,
 		Nonce:    "fake_nonce",
 	})
-	s.assertErrorResponse(c, resp, http.StatusInternalServerError, "tag kind machine not valid")
+	c.Assert(resp.StatusCode, gc.Equals, http.StatusForbidden)
+	body, err := ioutil.ReadAll(resp.Body)
+	c.Assert(err, jc.ErrorIsNil)
+	c.Assert(string(body), gc.Equals, "authorization failed: machine 0 is not a user\n")
 
 	// Now try a user login.
 	resp = s.sendHTTPRequest(c, apitesting.HTTPRequestParams{Method: "POST", URL: s.backupURL})

apiserver/facades/client/backups/backups.go

@@ -4,6 +4,8 @@
 package backups
 
 import (
+	"os"
+
 	"github.com/juju/errors"
 	"github.com/juju/mgo/v2"
 	"github.com/juju/names/v4"
@@ -109,6 +111,9 @@ func NewAPI(backend Backend, resources facade.Resources, authorizer facade.Autho
 		return nil, errors.Trace(err)
 	}
 	backupDir := modelConfig.BackupDir()
+	if backupDir == "" {
+		backupDir = os.TempDir()
+	}
 
 	paths := backups.Paths{
 		BackupDir: backupDir,

apiserver/facades/client/backups/backups_test.go

@@ -78,7 +78,7 @@ func (s *backupsSuite) setBackups(c *gc.C, meta *backups.Metadata, err string) *
 		fake.Error = errors.Errorf(err)
 	}
 	s.PatchValue(backupsAPI.NewBackups,
-		func() backups.Backups {
+		func(paths *backups.Paths) backups.Backups {
 			return &fake
 		},
 	)

apiserver/facades/client/backups/create.go

@@ -33,7 +33,7 @@ func (a *API) Create(args params.BackupsCreateArgs) (params.BackupsMetadataResul
 }
 
 func (a *APIv2) Create(args params.BackupsCreateArgs) (params.BackupsMetadataResult, error) {
-	backupsMethods := newBackups()
+	backupsMethods := newBackups(a.paths)
 
 	session := a.backend.MongoSession().Copy()
 	defer session.Close()
@@ -88,7 +88,7 @@ func (a *APIv2) Create(args params.BackupsCreateArgs) (params.BackupsMetadataRes
 	}
 	meta.Controller.HANodes = int64(len(nodes))
 
-	fileName, err := backupsMethods.Create(meta, a.paths, dbInfo)
+	fileName, err := backupsMethods.Create(meta, dbInfo)
 	if err != nil {
 		return result, errors.Trace(err)
 	}

state/backups/backups.go

@@ -6,8 +6,10 @@ package backups
 import (
 	"fmt"
 	"io"
+	"io/fs"
 	"os"
 	"path/filepath"
+	"strings"
 	"time"
 
 	"github.com/dustin/go-humanize"
@@ -46,17 +48,21 @@ var (
 type Backups interface {
 	// Create creates a new juju backup archive. It updates
 	// the provided metadata.
-	Create(meta *Metadata, paths *Paths, dbInfo *DBInfo) (string, error)
+	Create(meta *Metadata, dbInfo *DBInfo) (string, error)
 
 	// Get returns the metadata and specified archive file.
 	Get(fileName string) (*Metadata, io.ReadCloser, error)
 }
 
-type backups struct{}
+type backups struct {
+	paths *Paths
+}
 
 // NewBackups creates a new Backups value using the FileStorage provided.
-func NewBackups() Backups {
-	return &backups{}
+func NewBackups(paths *Paths) Backups {
+	return &backups{
+		paths: paths,
+	}
 }
 
 func totalDirSize(path string) (int64, error) {
@@ -75,7 +81,7 @@ func totalDirSize(path string) (int64, error) {
 
 // Create creates and stores a new juju backup archive (based on arguments)
 // and updates the provided metadata.  A filename to download the backup is provided.
-func (b *backups) Create(meta *Metadata, paths *Paths, dbInfo *DBInfo) (string, error) {
+func (b *backups) Create(meta *Metadata, dbInfo *DBInfo) (string, error) {
 	// TODO(fwereade): 2016-03-17 lp:1558657
 	meta.Started = time.Now().UTC()
 
@@ -90,7 +96,7 @@ func (b *backups) Create(meta *Metadata, paths *Paths, dbInfo *DBInfo) (string,
 	}
 
 	// Create the archive.
-	filesToBackUp, err := getFilesToBackUp("", paths)
+	filesToBackUp, err := getFilesToBackUp("", b.paths)
 	if err != nil {
 		return "", errors.Annotate(err, "while listing files to back up")
 	}
@@ -108,11 +114,7 @@ func (b *backups) Create(meta *Metadata, paths *Paths, dbInfo *DBInfo) (string,
 	logger.Infof("backing up %dMiB (files) and %dMiB (database) = %dMiB",
 		totalFizeSizesMiB, dbInfo.ApproxSizeMB, int(totalFizeSizesMiB)+dbInfo.ApproxSizeMB)
 
-	destinationDir := paths.BackupDir
-	if destinationDir == "" {
-		destinationDir = os.TempDir()
-	}
-
+	destinationDir := b.paths.BackupDir
 	if _, err := os.Stat(destinationDir); err != nil {
 		if os.IsNotExist(err) {
 			return "", errors.Errorf("backup destination directory %q does not exist", destinationDir)
@@ -171,8 +173,36 @@ func (b *backups) Create(meta *Metadata, paths *Paths, dbInfo *DBInfo) (string,
 	return result.filename, nil
 }
 
+func isValidFilepath(root string, filePath string) (bool, error) {
+	if !filepath.IsAbs(filePath) {
+		return false, nil
+	}
+	if !strings.HasPrefix(filepath.Base(filePath), FilenamePrefix) {
+		return false, nil
+	}
+	result := false
+	err := filepath.WalkDir(root, func(path string, d fs.DirEntry, err error) error {
+		if d.IsDir() {
+			return nil
+		}
+		if path == filePath {
+			result = true
+			return nil
+		}
+		return nil
+	})
+	return result, err
+}
+
 // Get retrieves the associated metadata and archive file a file on the machine.
 func (b *backups) Get(fileName string) (_ *Metadata, _ io.ReadCloser, err error) {
+	valid, err := isValidFilepath(b.paths.BackupDir, fileName)
+	if err != nil {
+		return nil, nil, errors.Trace(err)
+	}
+	if !valid {
+		return nil, nil, errors.NotValidf("backup file %q", fileName)
+	}
 	defer func() {
 		// On success, remove the retrieved file.
 		if err != nil {

state/backups/backups_test.go

@@ -9,6 +9,7 @@ import (
 	"io/ioutil"
 	"os"
 	"path"
+	"path/filepath"
 
 	"github.com/dustin/go-humanize"
 	"github.com/juju/collections/set"
@@ -24,7 +25,8 @@ import (
 type backupsSuite struct {
 	backupstesting.BaseSuite
 
-	api backups.Backups
+	paths *backups.Paths
+	api   backups.Backups
 
 	totalDiskMiB     uint64
 	availableDiskMiB uint64
@@ -37,7 +39,11 @@ var _ = gc.Suite(&backupsSuite{}) // Register the suite.
 func (s *backupsSuite) SetUpTest(c *gc.C) {
 	s.BaseSuite.SetUpTest(c)
 
-	s.api = backups.NewBackups()
+	s.paths = &backups.Paths{
+		BackupDir: c.MkDir(),
+		DataDir:   c.MkDir(),
+	}
+	s.api = backups.NewBackups(s.paths)
 	s.PatchValue(backups.AvailableDisk, func(string) uint64 {
 		return s.availableDiskMiB
 	})
@@ -60,7 +66,6 @@ func (s *backupsSuite) checkFailure(c *gc.C, expected string) {
 		return &fakeDumper{}, nil
 	})
 
-	paths := backups.Paths{DataDir: "/var/lib/juju"}
 	targets := set.NewStrings("juju", "admin")
 	dbInfo := backups.DBInfo{
 		Address: "a", Username: "b", Password: "c",
@@ -69,20 +74,18 @@ func (s *backupsSuite) checkFailure(c *gc.C, expected string) {
 	meta := backupstesting.NewMetadataStarted()
 	meta.Notes = "some notes"
 
-	_, err := s.api.Create(meta, &paths, &dbInfo)
+	_, err := s.api.Create(meta, &dbInfo)
 	c.Check(err, gc.ErrorMatches, expected)
 }
 
 func (s *backupsSuite) TestCreateOkay(c *gc.C) {
-	dataDir := c.MkDir()
-	backupDir := c.MkDir()
 	// Patch the internals.
 	archiveFile := ioutil.NopCloser(bytes.NewBufferString("<compressed tarball>"))
 	result := backups.NewTestCreateResult(
 		archiveFile,
 		10,
 		"<checksum>",
-		path.Join(backupDir, "test-backup.tar.gz"))
+		path.Join(s.paths.BackupDir, "test-backup.tar.gz"))
 	received, testCreate := backups.NewTestCreate(result)
 	s.PatchValue(backups.RunCreate, testCreate)
 
@@ -99,7 +102,6 @@ func (s *backupsSuite) TestCreateOkay(c *gc.C) {
 	})
 
 	// Run the backup.
-	paths := backups.Paths{BackupDir: backupDir, DataDir: dataDir}
 	targets := set.NewStrings("juju", "admin")
 	dbInfo := backups.DBInfo{
 		Address: "a", Username: "b", Password: "c",
@@ -108,13 +110,13 @@ func (s *backupsSuite) TestCreateOkay(c *gc.C) {
 	meta := backupstesting.NewMetadataStarted()
 	backupstesting.SetOrigin(meta, "<model ID>", "<machine ID>", "<hostname>")
 	meta.Notes = "some notes"
-	resultFilename, err := s.api.Create(meta, &paths, &dbInfo)
+	resultFilename, err := s.api.Create(meta, &dbInfo)
 	c.Assert(err, jc.ErrorIsNil)
-	c.Assert(resultFilename, gc.Equals, path.Join(backupDir, "test-backup.tar.gz"))
+	c.Assert(resultFilename, gc.Equals, path.Join(s.paths.BackupDir, "test-backup.tar.gz"))
 
 	// Test the call values.
 	resultBackupDir, filesToBackUp, _ := backups.ExposeCreateArgs(received)
-	c.Check(resultBackupDir, gc.Equals, backupDir)
+	c.Check(resultBackupDir, gc.Equals, s.paths.BackupDir)
 	c.Check(filesToBackUp, jc.SameContents, []string{"<some file>"})
 
 	c.Check(receivedDBInfo.Address, gc.Equals, "a")
@@ -198,15 +200,18 @@ func (s *backupsSuite) TestNotEnoughDiskSpaceSmallDisk(c *gc.C) {
 }
 
 func (s *backupsSuite) TestGetFileName(c *gc.C) {
-	backupDir := c.MkDir()
-	err := os.MkdirAll(backupDir, 0644)
+	backupSubDir := filepath.Join(s.paths.BackupDir, "a", "b")
+	err := os.MkdirAll(backupSubDir, 0755)
 	c.Assert(err, jc.ErrorIsNil)
-	backupFilename := path.Join(backupDir, "test-backup.tar.gz")
+	backupFilename := path.Join(backupSubDir, "juju-backup-123.tar.gz")
 	backupFile, err := os.Create(backupFilename)
 	c.Assert(err, jc.ErrorIsNil)
 	_, err = backupFile.Write([]byte("archive file testing"))
 	c.Assert(err, jc.ErrorIsNil)
 
+	_, _, err = s.api.Get("/etc/hostname")
+	c.Assert(err, gc.ErrorMatches, `backup file "/etc/hostname" not valid`)
+
 	resultMeta, resultArchive, err := s.api.Get(backupFilename)
 	c.Assert(err, jc.ErrorIsNil)
 	defer resultArchive.Close()

state/backups/testing/fakes.go

@@ -31,8 +31,6 @@ type FakeBackups struct {
 
 	// IDArg holds the ID that was passed in.
 	IDArg string
-	// PathsArg holds the Paths that was passed in.
-	PathsArg *backups.Paths
 	// DBInfoArg holds the ConnInfo that was passed in.
 	DBInfoArg *backups.DBInfo
 	// MetaArg holds the backup metadata that was passed in.
@@ -51,12 +49,10 @@ var _ backups.Backups = (*FakeBackups)(nil)
 // its associated metadata.
 func (b *FakeBackups) Create(
 	meta *backups.Metadata,
-	paths *backups.Paths,
 	dbInfo *backups.DBInfo,
 ) (string, error) {
 	b.Calls = append(b.Calls, "Create")
 
-	b.PathsArg = paths
 	b.DBInfoArg = dbInfo
 	b.MetaArg = meta