From ae2a92d229786791f3a249dbc5e689f30614a064 Mon Sep 17 00:00:00 2001 From: badrAZ Date: Thu, 26 Nov 2020 11:02:33 +0100 Subject: [PATCH] feat(xo-server,fs): stricter perms for backup dirs (#5378) Fixes xoa-support#3088 --- @xen-orchestra/fs/src/abstract.js | 57 +++++++++++-------- @xen-orchestra/fs/src/local.js | 4 +- @xen-orchestra/fs/src/smb.js | 4 +- CHANGELOG.unreleased.md | 3 +- packages/xo-server/config.toml | 5 ++ .../src/xo-mixins/backups-ng/index.js | 16 +++++- .../src/xo-mixins/metadata-backups.js | 18 +++++- 7 files changed, 73 insertions(+), 34 deletions(-) diff --git a/@xen-orchestra/fs/src/abstract.js b/@xen-orchestra/fs/src/abstract.js index 066955e86..b2c465e46 100644 --- a/@xen-orchestra/fs/src/abstract.js +++ b/@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 { + async createOutputStream(file: File, { checksum = false, dirMode, ...options }: Object = {}): Promise { 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, path: string, - { checksum = true }: { checksum?: boolean } = {} + { checksum = true, dirMode }: { checksum?: boolean, dirMode?: number } = {} ): Promise { 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 { - await this.__mkdir(normalizePath(dir)) + async mkdir(dir: string, { mode }: { mode?: number } = {}): Promise { + await this.__mkdir(normalizePath(dir), { mode }) } - async mktree(dir: string): Promise { - await this._mktree(normalizePath(dir)) + async mktree(dir: string, { mode }: { mode?: number } = {}): Promise { + await this._mktree(normalizePath(dir), { mode }) } openFile(path: string, flags: string): Promise { return this.__openFile(path, flags) } - async outputFile(file: string, data: Data, { flags = 'wx' }: { flags?: string } = {}): Promise { - await this._outputFile(normalizePath(file), data, { flags }) + async outputFile( + file: string, + data: Data, + { dirMode, flags = 'wx' }: { dirMode?: number, flags?: string } = {} + ): Promise { + 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 { + async __mkdir(dir: string, { mode }: { mode?: number } = {}): Promise { 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 { + async _createOutputStream(file: File, { dirMode, ...options }: Object = {}): Promise { 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 { + async _mktree(dir: string, { mode }: { mode?: number } = {}): Promise { 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)) - return this._mktree(dir) + await this._mktree(dirname(dir), { mode }) + return this._mktree(dir, { mode }) } async _openFile(path: string, flags: string): Promise { throw new Error('Not implemented') } - async _outputFile(file: string, data: Data, options: { flags?: string }): Promise { + async _outputFile(file: string, data: Data, { dirMode, flags }: { dirMode?: number, flags?: string }): Promise { 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)) - return this._outputFile(file, data, options) + await this._mktree(dirname(file), { mode: dirMode }) + 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') diff --git a/@xen-orchestra/fs/src/local.js b/@xen-orchestra/fs/src/local.js index b48b7002a..b6e97ea4a 100644 --- a/@xen-orchestra/fs/src/local.js +++ b/@xen-orchestra/fs/src/local.js @@ -71,8 +71,8 @@ export default class LocalHandler extends RemoteHandlerAbstract { return fs.readdir(this._getFilePath(dir)) } - _mkdir(dir) { - return fs.mkdir(this._getFilePath(dir)) + _mkdir(dir, { mode }) { + return fs.mkdir(this._getFilePath(dir), { mode }) } async _openFile(path, flags) { diff --git a/@xen-orchestra/fs/src/smb.js b/@xen-orchestra/fs/src/smb.js index 407227298..d1cf021f2 100644 --- a/@xen-orchestra/fs/src/smb.js +++ b/@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) { - return this._client.mkdir(this._getFilePath(dir)).catch(normalizeDirError) + _mkdir(dir, { mode }) { + return this._client.mkdir(this._getFilePath(dir), mode).catch(normalizeDirError) } // TODO: add flags diff --git a/CHANGELOG.unreleased.md b/CHANGELOG.unreleased.md index 05b16378e..863434436 100644 --- a/CHANGELOG.unreleased.md +++ b/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 diff --git a/packages/xo-server/config.toml b/packages/xo-server/config.toml index 72d69eda4..7fe0bec35 100644 --- a/packages/xo-server/config.toml +++ b/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' diff --git a/packages/xo-server/src/xo-mixins/backups-ng/index.js b/packages/xo-server/src/xo-mixins/backups-ng/index.js index daffb9578..bc6f3db86 100644 --- a/packages/xo-server/src/xo-mixins/backups-ng/index.js +++ b/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() diff --git a/packages/xo-server/src/xo-mixins/metadata-backups.js b/packages/xo-server/src/xo-mixins/metadata-backups.js index 1e17f0161..693ddeef4 100644 --- a/packages/xo-server/src/xo-mixins/metadata-backups.js +++ b/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) => {