Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Copy private NPM credentials if exists #104

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

feynmanliang
Copy link

Description

Motivation and Context

Some projects have dependencies in private NPM registries. Currently, faas-cli build will release an incomplete image which dependency errors in production.

This change will copy a .npmrc containing the NPM registry auth token, if it is present.

  • I have raised an issue to propose this change (required)

Which issue(s) this PR fixes

Fixes #103

@derek
Copy link

derek bot commented Dec 19, 2018

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.

@derek derek bot added the no-dco label Dec 19, 2018
feynmanliang and others added 3 commits December 18, 2018 20:38
Signed-off-by: Feynman Liang <feynman.liang@gmail.com>
* 'patch-1' of github.com:feynmanliang/templates:
  Update Dockerfile

Signed-off-by: Feynman Liang <feynman.liang@gmail.com>
@alexellis
Copy link
Member

Hi @feynmanliang thanks for your interest in the project.

I see you're having a bit of trouble with the bot and sign-off process. You'd be better off resetting all those commits and then adding your change on the top of master.

I am not sure this is a good change though since this could lead to leaking private npm credentials within your Docker image. cc @padiazg @pyramation

Maybe you could pass it via a build-arg or similar?

Alex

@pyramation
Copy link

Great idea and thanks for helping out! However, this is honestly a bad security practice to put in the main repo IMHO. Not secure. The pattern I use that works today using Dockerfile is this:

place a .npmrc locally

contents of .npmrc:

//registry.npmjs.org/:_authToken=${NPM_TOKEN}

edit your Dockerfile to use ARG:

ARG NPM_TOKEN  
COPY .npmrc .npmrc 

ENV NODE_ENV production
RUN npm install -g @yourNpmAccount/your-private-module@0.0.5
RUN rm .npmrc
  • Notice it removes the .npmrc!

build image

Then make sure you have NPM_TOKEN in your env, and run

faas build -f production-stack.yml --build-arg NPM_TOKEN=${NPM_TOKEN}

@feynmanliang
Copy link
Author

Thanks for catching that security flaw; that would have been bad >.<

I will revise this to accept a build arg.

Not 100% sure if copying .npmrcwill work (Docker might complain if none of the files in a COPY command exist) but will test it out shortly.

@pyramation
Copy link

Yes @feynmanliang looks like it does error out:

COPY failed: stat /var/lib/docker/tmp/docker-builder325671983/function/.npmrc: no such file or directory

https://travis-ci.org/openfaas/templates/builds/469855203#L1348

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Node template fails to install dependencies in private npm registries
3 participants