feat(xo-server,fs): stricter perms for backup dirs (#5378)

Fixes xoa-support#3088

@xen-orchestra/fs/src/abstract.js

@@ -120,13 +120,14 @@ export default class RemoteHandlerAbstract {
   }
 
   // TODO: remove method
-  async createOutputStream(file: File, { checksum = false, ...options }: Object = {}): Promise<LaxWritable> {
+  async createOutputStream(file: File, { checksum = false, dirMode, ...options }: Object = {}): Promise<LaxWritable> {
     if (typeof file === 'string') {
       file = normalizePath(file)
     }
     const path = typeof file === 'string' ? file : file.path
     const streamP = timeout.call(
       this._createOutputStream(file, {
+        dirMode,
         flags: 'wx',
         ...options,
       }),
@@ -210,11 +211,14 @@ export default class RemoteHandlerAbstract {
   async outputStream(
     input: Readable | Promise<Readable>,
     path: string,
-    { checksum = true }: { checksum?: boolean } = {}
+    { checksum = true, dirMode }: { checksum?: boolean, dirMode?: number } = {}
   ): Promise<void> {
     path = normalizePath(path)
     input = await input
-    return this._outputStream(await input, normalizePath(path), { checksum })
+    return this._outputStream(await input, normalizePath(path), {
+      checksum,
+      dirMode,
+    })
   }
 
   // Free the resources possibly dedicated to put the remote at work, when it
@@ -257,20 +261,24 @@ export default class RemoteHandlerAbstract {
     return entries
   }
 
-  async mkdir(dir: string): Promise<void> {
+  async mkdir(dir: string, { mode }: { mode?: number } = {}): Promise<void> {
-    await this.__mkdir(normalizePath(dir))
+    await this.__mkdir(normalizePath(dir), { mode })
   }
 
-  async mktree(dir: string): Promise<void> {
+  async mktree(dir: string, { mode }: { mode?: number } = {}): Promise<void> {
-    await this._mktree(normalizePath(dir))
+    await this._mktree(normalizePath(dir), { mode })
   }
 
   openFile(path: string, flags: string): Promise<FileDescriptor> {
     return this.__openFile(path, flags)
   }
 
-  async outputFile(file: string, data: Data, { flags = 'wx' }: { flags?: string } = {}): Promise<void> {
+  async outputFile(
-    await this._outputFile(normalizePath(file), data, { flags })
+    file: string,
+    data: Data,
+    { dirMode, flags = 'wx' }: { dirMode?: number, flags?: string } = {}
+  ): Promise<void> {
+    await this._outputFile(normalizePath(file), data, { dirMode, flags })
   }
 
   async read(file: File, buffer: Buffer, position?: number): Promise<{| bytesRead: number, buffer: Buffer |}> {
@@ -372,9 +380,9 @@ export default class RemoteHandlerAbstract {
     await timeout.call(this._closeFile(fd.fd), this._timeout)
   }
 
-  async __mkdir(dir: string): Promise<void> {
+  async __mkdir(dir: string, { mode }: { mode?: number } = {}): Promise<void> {
     try {
-      await this._mkdir(dir)
+      await this._mkdir(dir, { mode })
     } catch (error) {
       if (error == null || error.code !== 'EEXIST') {
         throw error
@@ -400,7 +408,7 @@ export default class RemoteHandlerAbstract {
     throw new Error('Not implemented')
   }
 
-  async _createOutputStream(file: File, options: Object): Promise<LaxWritable> {
+  async _createOutputStream(file: File, { dirMode, ...options }: Object = {}): Promise<LaxWritable> {
     try {
       return await this._createWriteStream(file, options)
     } catch (error) {
@@ -409,7 +417,7 @@ export default class RemoteHandlerAbstract {
       }
     }
 
-    await this._mktree(dirname(file))
+    await this._mktree(dirname(file), { mode: dirMode })
     return this._createOutputStream(file, options)
   }
 
@@ -440,39 +448,42 @@ export default class RemoteHandlerAbstract {
     throw new Error('Not implemented')
   }
 
-  async _mktree(dir: string): Promise<void> {
+  async _mktree(dir: string, { mode }: { mode?: number } = {}): Promise<void> {
     try {
-      return await this.__mkdir(dir)
+      return await this.__mkdir(dir, { mode })
     } catch (error) {
       if (error.code !== 'ENOENT') {
         throw error
       }
     }
 
-    await this._mktree(dirname(dir))
+    await this._mktree(dirname(dir), { mode })
-    return this._mktree(dir)
+    return this._mktree(dir, { mode })
   }
 
   async _openFile(path: string, flags: string): Promise<mixed> {
     throw new Error('Not implemented')
   }
 
-  async _outputFile(file: string, data: Data, options: { flags?: string }): Promise<void> {
+  async _outputFile(file: string, data: Data, { dirMode, flags }: { dirMode?: number, flags?: string }): Promise<void> {
     try {
-      return await this._writeFile(file, data, options)
+      return await this._writeFile(file, data, { flags })
     } catch (error) {
       if (error.code !== 'ENOENT') {
         throw error
       }
     }
 
-    await this._mktree(dirname(file))
+    await this._mktree(dirname(file), { mode: dirMode })
-    return this._outputFile(file, data, options)
+    return this._outputFile(file, data, { flags })
   }
 
-  async _outputStream(input, path, { checksum }) {
+  async _outputStream(input: Readable, path: string, { checksum, dirMode }: { checksum?: boolean, dirMode?: number }) {
     const tmpPath = `${dirname(path)}/.${basename(path)}`
-    const output = await this.createOutputStream(tmpPath, { checksum })
+    const output = await this.createOutputStream(tmpPath, {
+      checksum,
+      dirMode,
+    })
     try {
       input.pipe(output)
       await fromEvent(output, 'finish')

@xen-orchestra/fs/src/local.js

@@ -71,8 +71,8 @@ export default class LocalHandler extends RemoteHandlerAbstract {
     return fs.readdir(this._getFilePath(dir))
   }
 
-  _mkdir(dir) {
+  _mkdir(dir, { mode }) {
-    return fs.mkdir(this._getFilePath(dir))
+    return fs.mkdir(this._getFilePath(dir), { mode })
   }
 
   async _openFile(path, flags) {

@xen-orchestra/fs/src/smb.js

@@ -90,8 +90,8 @@ export default class SmbHandler extends RemoteHandlerAbstract {
     return this._client.readdir(this._getFilePath(dir)).catch(normalizeDirError)
   }
 
-  _mkdir(dir) {
+  _mkdir(dir, { mode }) {
-    return this._client.mkdir(this._getFilePath(dir)).catch(normalizeDirError)
+    return this._client.mkdir(this._getFilePath(dir), mode).catch(normalizeDirError)
   }
 
   // TODO: add flags

CHANGELOG.unreleased.md

@@ -20,6 +20,7 @@
 - [Remotes/NFS] Only mount with `vers=3` when no other options [#4940](https://github.com/vatesfr/xen-orchestra/issues/4940) (PR [#5354](https://github.com/vatesfr/xen-orchestra/pull/5354))
 - [VM/network] Don't change VIF's locking mode automatically (PR [#5357](https://github.com/vatesfr/xen-orchestra/pull/5357))
 - [Import OVA] Fix 'Max payload size exceeded' error when importing huge OVAs (PR [#5372](https://github.com/vatesfr/xen-orchestra/pull/5372))
+- [Backup] Make backup directories only accessible by root users (PR [#5378](https://github.com/vatesfr/xen-orchestra/pull/5378))
 
 ### Packages to release
 
@@ -40,7 +41,7 @@
 
 - xo-server-auth-ldap patch
 - @vates/multi-key-map minor
-- @xen-orchestra/fs patch
+- @xen-orchestra/fs minor
 - vhd-lib major
 - xo-vmdk-to-vhd major
 - xo-server minor

packages/xo-server/config.toml

@@ -68,6 +68,11 @@ mergeProvidersUsers = true
 defaultSignInPage = '/signin'
 
 [backup]
+# Mode to use for newly created backup directories
+#
+# https://en.wikipedia.org/wiki/File-system_permissions#Numeric_notation
+dirMode = 0o700
+
 # Delay for which backups listing on a remote is cached
 listingDebounce = '1 min'
 

packages/xo-server/src/xo-mixins/backups-ng/index.js

@@ -1304,6 +1304,7 @@ export default class BackupNg {
                 await deleteOldBackups()
               }
 
+              const { dirMode } = this._backupOptions
               await wrapTask(
                 {
                   logger,
@@ -1311,14 +1312,18 @@ export default class BackupNg {
                   parentId: taskId,
                   result: () => ({ size: xva.size }),
                 },
-                handler.outputStream(fork, dataFilename)
+                handler.outputStream(fork, dataFilename, {
+                  dirMode,
+                })
               )
 
               if (handler._getFilePath !== undefined) {
                 await isValidXva(handler._getFilePath('/' + dataFilename))
               }
 
-              await handler.outputFile(metadataFilename, jsonMetadata)
+              await handler.outputFile(metadataFilename, jsonMetadata, {
+                dirMode,
+              })
 
               if (!deleteFirst) {
                 await deleteOldBackups()
@@ -1612,6 +1617,8 @@ export default class BackupNg {
                 await deleteOldBackups()
               }
 
+              const { dirMode } = this._backupOptions
+
               await wrapTask(
                 {
                   logger,
@@ -1647,6 +1654,7 @@ export default class BackupNg {
                       // no checksum for VHDs, because they will be invalidated by
                       // merges and chainings
                       checksum: false,
+                      dirMode,
                     })
                     $defer.onFailure.call(handler, 'unlink', path)
 
@@ -1665,7 +1673,9 @@ export default class BackupNg {
                   })
                 ).then(sum)
               )
-              await handler.outputFile(metadataFilename, jsonMetadata)
+              await handler.outputFile(metadataFilename, jsonMetadata, {
+                dirMode,
+              })
 
               if (!deleteFirst) {
                 await deleteOldBackups()

packages/xo-server/src/xo-mixins/metadata-backups.js

@@ -129,6 +129,7 @@ export default class metadataBackup {
 
   constructor(app: any, { backup }) {
     this._app = app
+    this._backupOptions = backup
     this._logger = undefined
     this._runningMetadataRestores = new Set()
     this._poolMetadataTimeout = parseDuration(backup.poolMetadataTimeout)
@@ -187,7 +188,13 @@ export default class metadataBackup {
         })
 
         try {
-          await Promise.all([handler.outputFile(fileName, data), handler.outputFile(metaDataFileName, metadata)])
+          const { dirMode } = this._backupOptions
+          await Promise.all([
+            handler.outputFile(fileName, data, { dirMode }),
+            handler.outputFile(metaDataFileName, metadata, {
+              dirMode,
+            }),
+          ])
 
           await deleteOldBackups(handler, scheduleDir, retention, (error, backupDir) => {
             logger.warning(
@@ -298,9 +305,12 @@ export default class metadataBackup {
 
         let outputStream
         try {
+          const { dirMode } = this._backupOptions
           await waitAll([
             (async () => {
-              outputStream = await handler.createOutputStream(fileName)
+              outputStream = await handler.createOutputStream(fileName, {
+                dirMode,
+              })
 
               // 'readable-stream/pipeline' not call the callback when an error throws
               // from the readable stream
@@ -314,7 +324,9 @@ export default class metadataBackup {
                 this._poolMetadataTimeout
               )
             })(),
-            handler.outputFile(metaDataFileName, metadata),
+            handler.outputFile(metaDataFileName, metadata, {
+              dirMode,
+            }),
           ])
 
           await deleteOldBackups(handler, poolDir, retention, (error, backupDir) => {