From 8846efe7ce08d39906db6b7a54321b998bc77a89 Mon Sep 17 00:00:00 2001 From: Jakob Ackermann Date: Fri, 26 Jun 2020 12:29:49 +0100 Subject: [PATCH 1/4] [misc] RequestParser: restrict imageName to an allow list and add tests --- services/clsi/app/js/RequestParser.js | 2 +- services/clsi/config/settings.defaults.js | 10 +++ services/clsi/docker-compose-config.yml | 2 + .../test/acceptance/js/AllowedImageNames.js | 73 +++++++++++++++++++ .../clsi/test/unit/js/RequestParserTests.js | 38 ++++++++++ 5 files changed, 124 insertions(+), 1 deletion(-) create mode 100644 services/clsi/test/acceptance/js/AllowedImageNames.js diff --git a/services/clsi/app/js/RequestParser.js b/services/clsi/app/js/RequestParser.js index f2be556af1..ecc92c0836 100644 --- a/services/clsi/app/js/RequestParser.js +++ b/services/clsi/app/js/RequestParser.js @@ -61,7 +61,7 @@ module.exports = RequestParser = { response.imageName = this._parseAttribute( 'imageName', compile.options.imageName, - { type: 'string' } + { type: 'string', validValues: settings.allowedImageNamesFlat } ) response.draft = this._parseAttribute('draft', compile.options.draft, { default: false, diff --git a/services/clsi/config/settings.defaults.js b/services/clsi/config/settings.defaults.js index 3e115e8bf5..fd2c28af1e 100644 --- a/services/clsi/config/settings.defaults.js +++ b/services/clsi/config/settings.defaults.js @@ -73,6 +73,16 @@ if (process.env.ALLOWED_COMPILE_GROUPS) { process.exit(1) } } +if (process.env.ALLOWED_IMAGE_NAMES_FLAT) { + try { + module.exports.allowedImageNamesFlat = process.env.ALLOWED_IMAGE_NAMES_FLAT.split( + ' ' + ) + } catch (error) { + console.error(error, 'could not apply allowed image names setting') + process.exit(1) + } +} if (process.env.DOCKER_RUNNER) { let seccompProfilePath diff --git a/services/clsi/docker-compose-config.yml b/services/clsi/docker-compose-config.yml index 392f8feae9..d1b72ee216 100644 --- a/services/clsi/docker-compose-config.yml +++ b/services/clsi/docker-compose-config.yml @@ -3,6 +3,7 @@ version: "2.3" services: dev: environment: + ALLOWED_IMAGE_NAMES_FLAT: "quay.io/sharelatex/texlive-full:2017.1" TEXLIVE_IMAGE: quay.io/sharelatex/texlive-full:2017.1 TEXLIVE_IMAGE_USER: "tex" SHARELATEX_CONFIG: /app/config/settings.defaults.coffee @@ -18,6 +19,7 @@ services: ci: environment: + ALLOWED_IMAGE_NAMES_FLAT: ${TEXLIVE_IMAGE} TEXLIVE_IMAGE: quay.io/sharelatex/texlive-full:2017.1 TEXLIVE_IMAGE_USER: "tex" SHARELATEX_CONFIG: /app/config/settings.defaults.coffee diff --git a/services/clsi/test/acceptance/js/AllowedImageNames.js b/services/clsi/test/acceptance/js/AllowedImageNames.js new file mode 100644 index 0000000000..1ea0a36b64 --- /dev/null +++ b/services/clsi/test/acceptance/js/AllowedImageNames.js @@ -0,0 +1,73 @@ +const Client = require('./helpers/Client') +const ClsiApp = require('./helpers/ClsiApp') +const { expect } = require('chai') + +describe('AllowedImageNames', function() { + beforeEach(function(done) { + this.project_id = Client.randomId() + this.request = { + options: { + imageName: undefined + }, + resources: [ + { + path: 'main.tex', + content: `\ +\\documentclass{article} +\\begin{document} +Hello world +\\end{document}\ +` + } + ] + } + ClsiApp.ensureRunning(done) + }) + + describe('with a valid name', function() { + beforeEach(function(done) { + this.request.options.imageName = process.env.TEXLIVE_IMAGE + + Client.compile(this.project_id, this.request, (error, res, body) => { + this.error = error + this.res = res + this.body = body + done(error) + }) + }) + it('should return success', function() { + expect(this.res.statusCode).to.equal(200) + }) + + it('should return a PDF', function() { + let pdf + try { + pdf = Client.getOutputFile(this.body, 'pdf') + } catch (e) {} + expect(pdf).to.exist + }) + }) + + describe('with an invalid name', function() { + beforeEach(function(done) { + this.request.options.imageName = 'something/evil:1337' + Client.compile(this.project_id, this.request, (error, res, body) => { + this.error = error + this.res = res + this.body = body + done(error) + }) + }) + it('should return non success', function() { + expect(this.res.statusCode).to.not.equal(200) + }) + + it('should not return a PDF', function() { + let pdf + try { + pdf = Client.getOutputFile(this.body, 'pdf') + } catch (e) {} + expect(pdf).to.not.exist + }) + }) +}) diff --git a/services/clsi/test/unit/js/RequestParserTests.js b/services/clsi/test/unit/js/RequestParserTests.js index e2d8b02610..16955bb670 100644 --- a/services/clsi/test/unit/js/RequestParserTests.js +++ b/services/clsi/test/unit/js/RequestParserTests.js @@ -114,6 +114,44 @@ describe('RequestParser', function() { }) }) + describe('when image restrictions are present', function() { + beforeEach(function() { + this.settings.allowedImageNamesFlat = ['repo/name:tag1', 'repo/name:tag2'] + }) + + describe('with imageName set to something invalid', function() { + beforeEach(function() { + const request = this.validRequest + request.compile.options.imageName = 'something/different:latest' + this.RequestParser.parse(request, (error, data) => { + this.error = error + this.data = data + }) + }) + + it('should throw an error for imageName', function() { + expect(String(this.error)).to.include( + 'imageName attribute should be one of' + ) + }) + }) + + describe('with imageName set to something valid', function() { + beforeEach(function() { + const request = this.validRequest + request.compile.options.imageName = 'repo/name:tag1' + this.RequestParser.parse(request, (error, data) => { + this.error = error + this.data = data + }) + }) + + it('should set the imageName', function() { + this.data.imageName.should.equal('repo/name:tag1') + }) + }) + }) + describe('with flags set', function() { beforeEach(function() { this.validRequest.compile.options.flags = ['-file-line-error'] From c857371fede8936a63be2f6d2cef7f3de496b1b6 Mon Sep 17 00:00:00 2001 From: Jakob Ackermann Date: Fri, 26 Jun 2020 13:17:45 +0100 Subject: [PATCH 2/4] [misc] wordcount: restrict image to an allow list and add tests --- services/clsi/app/js/CompileController.js | 7 +++ .../test/acceptance/js/AllowedImageNames.js | 29 +++++++++++++ .../clsi/test/acceptance/js/helpers/Client.js | 9 ++++ .../test/unit/js/CompileControllerTests.js | 43 ++++++++++++++++++- 4 files changed, 86 insertions(+), 2 deletions(-) diff --git a/services/clsi/app/js/CompileController.js b/services/clsi/app/js/CompileController.js index c76d0d50e2..857d0504a3 100644 --- a/services/clsi/app/js/CompileController.js +++ b/services/clsi/app/js/CompileController.js @@ -218,6 +218,13 @@ module.exports = CompileController = { const { project_id } = req.params const { user_id } = req.params const { image } = req.query + if ( + image && + Settings.allowedImageNamesFlat && + Settings.allowedImageNamesFlat.indexOf(image) === -1 + ) { + return res.status(400).send('invalid image') + } logger.log({ image, file, project_id }, 'word count request') return CompileManager.wordcount(project_id, user_id, file, image, function( diff --git a/services/clsi/test/acceptance/js/AllowedImageNames.js b/services/clsi/test/acceptance/js/AllowedImageNames.js index 1ea0a36b64..a9b3996e1e 100644 --- a/services/clsi/test/acceptance/js/AllowedImageNames.js +++ b/services/clsi/test/acceptance/js/AllowedImageNames.js @@ -70,4 +70,33 @@ Hello world expect(pdf).to.not.exist }) }) + + describe('wordcount', function() { + beforeEach(function(done) { + Client.compile(this.project_id, this.request, done) + }) + it('should error out with an invalid imageName', function() { + Client.wordcountWithImage( + this.project_id, + 'main.tex', + 'something/evil:1337', + (error, result) => { + expect(String(error)).to.include('statusCode=400') + } + ) + }) + + it('should produce a texcout a valid imageName', function() { + Client.wordcountWithImage( + this.project_id, + 'main.tex', + process.env.TEXLIVE_IMAGE, + (error, result) => { + expect(error).to.not.exist + expect(result).to.exist + expect(result.texcount).to.exist + } + ) + }) + }) }) diff --git a/services/clsi/test/acceptance/js/helpers/Client.js b/services/clsi/test/acceptance/js/helpers/Client.js index d65941671a..c940e305f8 100644 --- a/services/clsi/test/acceptance/js/helpers/Client.js +++ b/services/clsi/test/acceptance/js/helpers/Client.js @@ -189,6 +189,11 @@ module.exports = Client = { }, wordcount(project_id, file, callback) { + const image = undefined + Client.wordcountWithImage(project_id, file, image, callback) + }, + + wordcountWithImage(project_id, file, image, callback) { if (callback == null) { callback = function(error, pdfPositions) {} } @@ -196,6 +201,7 @@ module.exports = Client = { { url: `${this.host}/project/${project_id}/wordcount`, qs: { + image, file } }, @@ -203,6 +209,9 @@ module.exports = Client = { if (error != null) { return callback(error) } + if (response.statusCode !== 200) { + return callback(new Error(`statusCode=${response.statusCode}`)) + } return callback(null, JSON.parse(body)) } ) diff --git a/services/clsi/test/unit/js/CompileControllerTests.js b/services/clsi/test/unit/js/CompileControllerTests.js index 4480c8804e..f3f3fa4ad8 100644 --- a/services/clsi/test/unit/js/CompileControllerTests.js +++ b/services/clsi/test/unit/js/CompileControllerTests.js @@ -12,6 +12,7 @@ const SandboxedModule = require('sandboxed-module') const sinon = require('sinon') require('chai').should() +const { expect } = require('chai') const modulePath = require('path').join( __dirname, '../../../app/js/CompileController' @@ -287,21 +288,59 @@ describe('CompileController', function() { this.CompileManager.wordcount = sinon .stub() .callsArgWith(4, null, (this.texcount = ['mock-texcount'])) - return this.CompileController.wordcount(this.req, this.res, this.next) }) it('should return the word count of a file', function() { + this.CompileController.wordcount(this.req, this.res, this.next) return this.CompileManager.wordcount .calledWith(this.project_id, undefined, this.file, this.image) .should.equal(true) }) - return it('should return the texcount info', function() { + it('should return the texcount info', function() { + this.CompileController.wordcount(this.req, this.res, this.next) return this.res.json .calledWith({ texcount: this.texcount }) .should.equal(true) }) + + describe('when allowedImageNamesFlat is set', function() { + beforeEach(function() { + this.Settings.allowedImageNamesFlat = [ + 'repo/image:tag1', + 'repo/image:tag2' + ] + this.res.send = sinon.stub() + this.res.status = sinon.stub().returns({ send: this.res.send }) + }) + + describe('with an invalid image', function() { + beforeEach(function() { + this.req.query.image = 'something/evil:1337' + this.CompileController.wordcount(this.req, this.res, this.next) + }) + it('should return a 400', function() { + expect(this.res.status.calledWith(400)).to.equal(true) + }) + it('should not run the query', function() { + expect(this.CompileManager.wordcount.called).to.equal(false) + }) + }) + + describe('with a valid image', function() { + beforeEach(function() { + this.req.query.image = 'repo/image:tag1' + this.CompileController.wordcount(this.req, this.res, this.next) + }) + it('should not return a 400', function() { + expect(this.res.status.calledWith(400)).to.equal(false) + }) + it('should run the query', function() { + expect(this.CompileManager.wordcount.called).to.equal(true) + }) + }) + }) }) }) From aa02df7b81b2b761bf85cbb54a05f883d726f503 Mon Sep 17 00:00:00 2001 From: Jakob Ackermann Date: Tue, 30 Jun 2020 12:00:18 +0100 Subject: [PATCH 3/4] [misc] apply review feedback - move setting into clsi.docker namespace - rename the variable for images to allowedImages / ALLOWED_IMAGES - add an additional check for the image name into the DockerRunner Co-Authored-By: Brian Gough --- services/clsi/app/js/CompileController.js | 6 +- services/clsi/app/js/DockerRunner.js | 7 +++ services/clsi/app/js/RequestParser.js | 8 ++- services/clsi/config/settings.defaults.js | 21 +++---- services/clsi/docker-compose-config.yml | 4 +- .../test/unit/js/CompileControllerTests.js | 5 +- .../clsi/test/unit/js/DockerRunnerTests.js | 58 ++++++++++++++++++- .../clsi/test/unit/js/RequestParserTests.js | 6 +- 8 files changed, 96 insertions(+), 19 deletions(-) diff --git a/services/clsi/app/js/CompileController.js b/services/clsi/app/js/CompileController.js index 857d0504a3..9c18830de8 100644 --- a/services/clsi/app/js/CompileController.js +++ b/services/clsi/app/js/CompileController.js @@ -220,8 +220,10 @@ module.exports = CompileController = { const { image } = req.query if ( image && - Settings.allowedImageNamesFlat && - Settings.allowedImageNamesFlat.indexOf(image) === -1 + Settings.clsi && + Settings.clsi.docker && + Settings.clsi.docker.allowedImages && + !Settings.clsi.docker.allowedImages.includes(image) ) { return res.status(400).send('invalid image') } diff --git a/services/clsi/app/js/DockerRunner.js b/services/clsi/app/js/DockerRunner.js index 5874a51fb4..5f04fe0c59 100644 --- a/services/clsi/app/js/DockerRunner.js +++ b/services/clsi/app/js/DockerRunner.js @@ -91,6 +91,13 @@ module.exports = DockerRunner = { image = `${Settings.texliveImageNameOveride}/${img[2]}` } + if ( + Settings.clsi.docker.allowedImages && + !Settings.clsi.docker.allowedImages.includes(image) + ) { + return callback(new Error('image not allowed')) + } + const options = DockerRunner._getContainerOptions( command, image, diff --git a/services/clsi/app/js/RequestParser.js b/services/clsi/app/js/RequestParser.js index ecc92c0836..342dbc86b8 100644 --- a/services/clsi/app/js/RequestParser.js +++ b/services/clsi/app/js/RequestParser.js @@ -61,7 +61,13 @@ module.exports = RequestParser = { response.imageName = this._parseAttribute( 'imageName', compile.options.imageName, - { type: 'string', validValues: settings.allowedImageNamesFlat } + { + type: 'string', + validValues: + settings.clsi && + settings.clsi.docker && + settings.clsi.docker.allowedImages + } ) response.draft = this._parseAttribute('draft', compile.options.draft, { default: false, diff --git a/services/clsi/config/settings.defaults.js b/services/clsi/config/settings.defaults.js index fd2c28af1e..823f1f737f 100644 --- a/services/clsi/config/settings.defaults.js +++ b/services/clsi/config/settings.defaults.js @@ -73,16 +73,6 @@ if (process.env.ALLOWED_COMPILE_GROUPS) { process.exit(1) } } -if (process.env.ALLOWED_IMAGE_NAMES_FLAT) { - try { - module.exports.allowedImageNamesFlat = process.env.ALLOWED_IMAGE_NAMES_FLAT.split( - ' ' - ) - } catch (error) { - console.error(error, 'could not apply allowed image names setting') - process.exit(1) - } -} if (process.env.DOCKER_RUNNER) { let seccompProfilePath @@ -139,6 +129,17 @@ if (process.env.DOCKER_RUNNER) { process.exit(1) } + if (process.env.ALLOWED_IMAGES) { + try { + module.exports.clsi.docker.allowedImages = process.env.ALLOWED_IMAGES.split( + ' ' + ) + } catch (error) { + console.error(error, 'could not apply allowed images setting') + process.exit(1) + } + } + module.exports.path.synctexBaseDir = () => '/compile' module.exports.path.sandboxedCompilesHostDir = process.env.COMPILES_HOST_DIR diff --git a/services/clsi/docker-compose-config.yml b/services/clsi/docker-compose-config.yml index d1b72ee216..afe56bbec5 100644 --- a/services/clsi/docker-compose-config.yml +++ b/services/clsi/docker-compose-config.yml @@ -3,7 +3,7 @@ version: "2.3" services: dev: environment: - ALLOWED_IMAGE_NAMES_FLAT: "quay.io/sharelatex/texlive-full:2017.1" + ALLOWED_IMAGES: "quay.io/sharelatex/texlive-full:2017.1" TEXLIVE_IMAGE: quay.io/sharelatex/texlive-full:2017.1 TEXLIVE_IMAGE_USER: "tex" SHARELATEX_CONFIG: /app/config/settings.defaults.coffee @@ -19,7 +19,7 @@ services: ci: environment: - ALLOWED_IMAGE_NAMES_FLAT: ${TEXLIVE_IMAGE} + ALLOWED_IMAGES: ${TEXLIVE_IMAGE} TEXLIVE_IMAGE: quay.io/sharelatex/texlive-full:2017.1 TEXLIVE_IMAGE_USER: "tex" SHARELATEX_CONFIG: /app/config/settings.defaults.coffee diff --git a/services/clsi/test/unit/js/CompileControllerTests.js b/services/clsi/test/unit/js/CompileControllerTests.js index f3f3fa4ad8..8bb83e66af 100644 --- a/services/clsi/test/unit/js/CompileControllerTests.js +++ b/services/clsi/test/unit/js/CompileControllerTests.js @@ -306,9 +306,10 @@ describe('CompileController', function() { .should.equal(true) }) - describe('when allowedImageNamesFlat is set', function() { + describe('when allowedImages is set', function() { beforeEach(function() { - this.Settings.allowedImageNamesFlat = [ + this.Settings.clsi = { docker: {} } + this.Settings.clsi.docker.allowedImages = [ 'repo/image:tag1', 'repo/image:tag2' ] diff --git a/services/clsi/test/unit/js/DockerRunnerTests.js b/services/clsi/test/unit/js/DockerRunnerTests.js index 16ecbbc51e..9c1731a809 100644 --- a/services/clsi/test/unit/js/DockerRunnerTests.js +++ b/services/clsi/test/unit/js/DockerRunnerTests.js @@ -273,7 +273,7 @@ describe('DockerRunner', function() { }) }) - return describe('with image override', function() { + describe('with image override', function() { beforeEach(function() { this.Settings.texliveImageNameOveride = 'overrideimage.com/something' this.DockerRunner._runAndWaitForContainer = sinon @@ -296,6 +296,62 @@ describe('DockerRunner', function() { return image.should.equal('overrideimage.com/something/image:2016.2') }) }) + + describe('with image restriction', function() { + beforeEach(function() { + this.Settings.clsi.docker.allowedImages = [ + 'repo/image:tag1', + 'repo/image:tag2' + ] + this.DockerRunner._runAndWaitForContainer = sinon + .stub() + .callsArgWith(3, null, (this.output = 'mock-output')) + }) + + describe('with a valid image', function() { + beforeEach(function() { + this.DockerRunner.run( + this.project_id, + this.command, + this.directory, + 'repo/image:tag1', + this.timeout, + this.env, + this.compileGroup, + this.callback + ) + }) + + it('should setup the container', function() { + this.DockerRunner._getContainerOptions.called.should.equal(true) + }) + }) + + describe('with a invalid image', function() { + beforeEach(function() { + this.DockerRunner.run( + this.project_id, + this.command, + this.directory, + 'something/different:evil', + this.timeout, + this.env, + this.compileGroup, + this.callback + ) + }) + + it('should call the callback with an error', function() { + const err = new Error('image not allowed') + this.callback.called.should.equal(true) + this.callback.args[0][0].message.should.equal(err.message) + }) + + it('should not setup the container', function() { + this.DockerRunner._getContainerOptions.called.should.equal(false) + }) + }) + }) }) describe('run with _getOptions', function() { diff --git a/services/clsi/test/unit/js/RequestParserTests.js b/services/clsi/test/unit/js/RequestParserTests.js index 16955bb670..25b6b29349 100644 --- a/services/clsi/test/unit/js/RequestParserTests.js +++ b/services/clsi/test/unit/js/RequestParserTests.js @@ -116,7 +116,11 @@ describe('RequestParser', function() { describe('when image restrictions are present', function() { beforeEach(function() { - this.settings.allowedImageNamesFlat = ['repo/name:tag1', 'repo/name:tag2'] + this.settings.clsi = { docker: {} } + this.settings.clsi.docker.allowedImages = [ + 'repo/name:tag1', + 'repo/name:tag2' + ] }) describe('with imageName set to something invalid', function() { From f703b2ca415b3011dcf16479b92d4b7e8b80b3eb Mon Sep 17 00:00:00 2001 From: Jakob Ackermann Date: Wed, 1 Jul 2020 10:01:25 +0100 Subject: [PATCH 4/4] [misc] move the image check prior to the base image override --- services/clsi/app/js/DockerRunner.js | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/services/clsi/app/js/DockerRunner.js b/services/clsi/app/js/DockerRunner.js index 5f04fe0c59..49c7f40249 100644 --- a/services/clsi/app/js/DockerRunner.js +++ b/services/clsi/app/js/DockerRunner.js @@ -86,11 +86,6 @@ module.exports = DockerRunner = { ;({ image } = Settings.clsi.docker) } - if (Settings.texliveImageNameOveride != null) { - const img = image.split('/') - image = `${Settings.texliveImageNameOveride}/${img[2]}` - } - if ( Settings.clsi.docker.allowedImages && !Settings.clsi.docker.allowedImages.includes(image) @@ -98,6 +93,11 @@ module.exports = DockerRunner = { return callback(new Error('image not allowed')) } + if (Settings.texliveImageNameOveride != null) { + const img = image.split('/') + image = `${Settings.texliveImageNameOveride}/${img[2]}` + } + const options = DockerRunner._getContainerOptions( command, image,