[WIP] non-root user for NodeJS template #83

austinfrey · 2017-09-06T09:36:29Z

Initial draft of non-root user in the NodeJS template.

Description

update the template/node/Dockerfile to use the node user rather than root. function directory structure needed to changed from /root/ to /usr/local/app/ as well

Motivation and Context

Allows users to run function as non-root which may increase security

This relates to #81

I have raised an issue to propose this change (required)
already raised by @alex Proposal: Alter templates to run as non-privileged user #81

How Has This Been Tested?

Manually built test function after changes, deployed and tested through the UI gateway

Linux 16+ on Docker CE 17.06

Will only impact the NodeJS template

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

My code follows the code style of this project.
My change requires a change to the documentation.
I have updated the documentation accordingly.
I've read the CONTRIBUTION guide
I have signed-off my commits with git commit -s
I have added tests to cover my changes.
All new and existing tests passed.

austinfrey · 2017-09-06T09:37:47Z

Initial draft to get feedback before moving on to other templates. No need to merge yet.

alexellis · 2017-09-06T09:45:33Z

template/node/Dockerfile

@@ -12,27 +12,31 @@ WORKDIR /root/
 # Turn down the verbosity to default level.
 ENV NPM_CONFIG_LOGLEVEL warn

+RUN mkdir -p /usr/local/app/function


What about a regular home directory like /home/app/ ?

sounds good

This should be: /home/app/ as per the comment, could you update? I'd like to move forward and merge your change. Thank you for helping lead this @aafrey

alexellis · 2017-09-06T09:46:05Z

template/node/Dockerfile

-WORKDIR /root/
+WORKDIR /usr/local/app/
+
+USER node


Great start! Maybe the username could be app and we could use that across the board - like a copy/paste segment for all the templates?

that makes the most sense. I'll work on it this evening

Just picking up on the user, and acknowledging that its largely academic, but why dont we use faas or openfaas? Setting this as a sort of principle might be helpful further down the road, when 3rd party templates start to emerge.

I'd just keep it generic like app for now. Always good to think forward though so thanks for your input.

alexellis · 2017-09-10T16:15:19Z

@aafrey - really appreciate your work on this and the "serverless inc" template. 💯

When can we get this fixed-up and merged? @rgee0 @rorpage any other feedback?

alexellis

Changes needed.

austinfrey · 2017-09-10T18:37:11Z

I'll allocate some time tomorrow for this if no one else has any feedback @alex @rgee0 @rorpage

ericstoekl · 2017-09-11T02:40:53Z

When I try this change, I get the following error:

$ curl http://localhost:8080/function/nodejs-echo -d "stuff"
exit status 1
fs.js:641
  return binding.open(pathModule._makeLong(path), stringToFlags(flags), mode);
                 ^

Error: EACCES: permission denied, open '/usr/local/app/index.js'
    at Error (native)
    at Object.fs.openSync (fs.js:641:18)
    at Object.fs.readFileSync (fs.js:509:33)
    at Object.Module._extensions..js (module.js:578:20)
    at Module.load (module.js:487:32)
    at tryModuleLoad (module.js:446:12)
    at Function.Module._load (module.js:438:3)
    at Module.runMain (module.js:604:10)
    at run (bootstrap_node.js:389:7)
    at startup (bootstrap_node.js:149:9)

I believe this can be fixed by the following line just before the USER node line:

RUN chown -R node .

alexellis · 2017-09-11T03:53:23Z

@ems5311 thank you for testing.

austinfrey · 2017-09-11T09:17:45Z

@ems5311 appreciate the feedback. I couldn't reproduce so I'm unsure what the issue is. I wouldn't think you'd need to chown since the node user is setup by default when building from that image. I made some changes as asked to use /home/app directory and the app user. I tested and it seems to be working for me, but it worked for me last time too, so would you mind testing again, and let me know if you see that error again? i thought docker was supposed to fix the "works on my machine" issue :)

ericstoekl · 2017-09-11T15:09:48Z

@aafrey Thanks for the feedback, strangely I am still seeing the issue. I have uploaded the image that faas-cli build -f <yaml> builds for me for the nodejs-echo function, based on this latest dockerfile. https://hub.docker.com/r/ems5311/nodejs-echo-noroot/

I believe if you run faas-cli deploy --image ems5311/nodejs-echo-noroot --name ems5311-node-noroot, it will pull and run the image from the docker hub repo.

I have also made sure that I am building the faas-cli binary with this latest pull request, and the faas-cli version hash matches the latest commit. I am running Ubuntu 16.04 and I might have some unconventional docker settings on my system.

Thanks again,
Eric

austinfrey · 2017-09-11T16:05:38Z

@ems5311 Hi Eric, the image you sent over doesn't work for me either, could you try building with the build.sh script in the template/node directory rather than the cli? just to test if there is a difference

ericstoekl · 2017-09-11T16:29:39Z

@aafrey good call, I'll try this out tonight.

ericstoekl · 2017-09-12T03:10:04Z

Yep, works now. Force rebuilding the image with --no-cache did the trick. Sorry for the false alarm, this changelist is good to go. 👏

austinfrey · 2017-09-12T08:17:55Z

@ems5311 appreciate the testing! thanks!

austinfrey · 2017-09-12T14:51:41Z

@alexellis anything else needed here?

alexellis · 2017-09-12T14:56:47Z

Yes I posted a comment about using /home/app as the folder for the app.

austinfrey · 2017-09-12T15:19:31Z

@alexellis the most recent commit made that change

alexellis · 2017-09-20T16:02:06Z

@austinfrey are we good to go with this?

alexellis · 2017-09-20T16:03:24Z

template/node/Dockerfile


-WORKDIR /root/
+RUN addgroup -S app && adduser -S -g app app


We can add the group at the top - meaning this gets cached and not affected by copying code in.

i can make this change @alexellis then we should be all set. should I make the change, squash and resubmit? or can i squash and commit with this same PR?

alexellis · 2017-09-20T17:58:35Z

Yes you can squash and re-push with the --force flag. I only raise a new PR if I screw things up really bad and have to push out of a different branch.

alexellis · 2017-09-21T08:43:54Z

Ping me for merging.

ghost · 2017-09-21T14:28:52Z

Thank you for your contribution. I've just checked and your commit doesn't appear to be signed-off.
That's something we need before your Pull Request can be merged. Please see our contributing guide.

austinfrey · 2017-09-21T14:40:11Z

@alexellis i had to merge the latest master with my branch to get up to date, i squashed everything down to one commit, but attributing all the previous changes i had to merge in as being part of this commit. is that normal?

alexellis · 2017-09-23T20:12:22Z

Struggling to see the changes here. This contains 32 files and now has some merge conflicts. Maybe it would be easier to submit a new PR?

austinfrey · 2017-09-23T20:52:13Z

Agreed, I'll resubmit this evening

…om the `home/app` directory. Signed-off-by: Austin Frey <aafrey85@gmail.com>

austinfrey · 2017-09-24T01:12:25Z

@alexellis started fresh, hopefully that's the last of it :)

alexellis · 2017-10-01T07:25:56Z

Derek add label: status/testing

alexellis · 2017-10-07T18:41:40Z

template/node/Dockerfile

 WORKDIR /root/

 # Turn down the verbosity to default level.
 ENV NPM_CONFIG_LOGLEVEL warn

+RUN mkdir -p /home/app
+
 # Wrapper/boot-strapper


Just wondering if "user app" can go higher?

sure. I'll update and test

Signed-off-by: austinfrey <aafrey85@gmail.com>

austinfrey · 2017-10-16T14:53:47Z

@alexellis moved adduser to line two. tested and had no issues

alexellis · 2017-10-26T07:00:21Z

Merged. Please can you retro-fit to the armhf template? @rgee0 perhaps you could help test?

alexellis · 2017-10-26T07:00:59Z

faas-cli invoke notroot
Reading from STDIN - hit (Control + D) to stop.
test this
Server returned unexpected status code: 500 - exit status 1
fs.js:641
  return binding.open(pathModule._makeLong(path), stringToFlags(flags), mode);
                 ^

Error: EACCES: permission denied, open '/home/app/index.js'
    at Error (native)
    at Object.fs.openSync (fs.js:641:18)
    at Object.fs.readFileSync (fs.js:509:33)
    at Object.Module._extensions..js (module.js:578:20)
    at Module.load (module.js:487:32)
    at tryModuleLoad (module.js:446:12)
    at Function.Module._load (module.js:438:3)
    at Module.runMain (module.js:604:10)
    at run (bootstrap_node.js:389:7)
    at startup (bootstrap_node.js:149:9)

@austinfrey this broke :-/

austinfrey · 2017-10-26T16:11:18Z

~~Interesting. I'll look at this tonight~~ Looks like you already fixed

alexellis reviewed Sep 6, 2017

View reviewed changes

alexellis changed the title ~~initial draft of non-root user in NodeJS template~~ [WIP] non-root user for NodeJS template Sep 7, 2017

alexellis requested changes Sep 10, 2017

View reviewed changes

ericstoekl mentioned this pull request Sep 11, 2017

Final non-root changes to Python, Ruby and C# templates #91

Closed

11 tasks

alexellis approved these changes Sep 20, 2017

View reviewed changes

alexellis reviewed Sep 20, 2017

View reviewed changes

ghost added no-dco and removed no-dco labels Sep 21, 2017

Updated template/node/Dockerfile to run function as the app user fr…

392b11c

…om the `home/app` directory. Signed-off-by: Austin Frey <aafrey85@gmail.com>

open-derek bot added the status/testing label Oct 1, 2017

alexellis reviewed Oct 7, 2017

View reviewed changes

Moved adduser to line 2

81ca45b

Signed-off-by: austinfrey <aafrey85@gmail.com>

alexellis merged commit cc9ce2d into openfaas:master Oct 26, 2017

[WIP] non-root user for NodeJS template #83

[WIP] non-root user for NodeJS template #83

Conversation

austinfrey commented Sep 6, 2017 • edited Loading

Description

Motivation and Context

How Has This Been Tested?

Types of changes

Checklist:

austinfrey commented Sep 6, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexellis Sep 12, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexellis commented Sep 10, 2017

alexellis left a comment

Choose a reason for hiding this comment

austinfrey commented Sep 10, 2017

ericstoekl commented Sep 11, 2017

alexellis commented Sep 11, 2017

austinfrey commented Sep 11, 2017

ericstoekl commented Sep 11, 2017

austinfrey commented Sep 11, 2017

ericstoekl commented Sep 11, 2017

ericstoekl commented Sep 12, 2017

austinfrey commented Sep 12, 2017

austinfrey commented Sep 12, 2017

alexellis commented Sep 12, 2017

austinfrey commented Sep 12, 2017 • edited Loading

alexellis commented Sep 20, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexellis commented Sep 20, 2017

alexellis commented Sep 21, 2017

ghost commented Sep 21, 2017

austinfrey commented Sep 21, 2017

alexellis commented Sep 23, 2017

austinfrey commented Sep 23, 2017

austinfrey commented Sep 24, 2017

alexellis commented Oct 1, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

austinfrey commented Oct 16, 2017

alexellis commented Oct 26, 2017

alexellis commented Oct 26, 2017

austinfrey commented Oct 26, 2017 • edited Loading

austinfrey commented Sep 6, 2017 •

edited

Loading

alexellis Sep 12, 2017 •

edited

Loading

austinfrey commented Sep 12, 2017 •

edited

Loading

austinfrey commented Oct 26, 2017 •

edited

Loading