documentation icon indicating copy to clipboard operation
documentation copied to clipboard

Update docker-and-private-modules.mdx

Open matzar opened this issue 3 years ago • 6 comments

~~Add explanation on how to safely set local NPM_TOKEN environment variable.~~

~~The current documentation instructs to use local NPM_TOKEN environment variable with docker build --build-arg NPM_TOKEN=${NPM_TOKEN} . command which is safe for building docker images but it does not mention:~~

  • ~~how to set local NPM_TOKEN variable~~
  • ~~to clean terminal history after setting local NPM_TOKEN variable~~

~~Everyone seems to be setting their local NPM_TOKEN environment variable even going as far as putting it into .zshenv or .bashrc to have them always available during each terminal session which I think is an overkill and we should either promote a different approach or explain how to safely use the most common one.~~

~~I'm open to discussion and happy to turn this PR into a google searchable answer on how to safely build your docker images with personal access tokens as it seems the best-practices are not clear, or are not easily found or I haven't found them in which case - I'm happy to be shown the proper way of doing things or to be LMGTFYed.~~


edit

I think there's a better approach and this section needs a big rewrite. The approach has been explained really well here.

Currently, npm docs encourages to use ARG parameter and --build-arg option. When we follow the link in the documentation we're going to end up reading a Warning and will be taken to a better and newer solution which does not use ARG but the global .npmrc file and which I've shown below as well.

I think this should be a more appropriate way of doing it:

  1. Remember to login with npm login if you haven't already.

  2. Dockerfile:

# https://docs.npmjs.com/docker-and-private-modules
FROM node:14

ENV APP_HOME="/app/"

WORKDIR ${APP_HOME}

COPY package*.json ${APP_HOME}/

RUN --mount=type=secret,id=npm,target=/root/.npmrc npm install

COPY . ${APP_HOME}/
CMD npm start
  1. Build docker image with the command (on mac in this case)
docker build . -t secure-app-secrets:1.0 --secret id=npm,src=$HOME/.npmrc

If this solution finds some traction and acceptance I'm going to change this PR to include this solution but I feel like this would require a bigger re-write of this entire page if we're going to agree that this is the solution that should be promoted.

My reasons for promoting it would be:

  • Does not require setting of local NPM_TOKEN environment variable as the token is being taken from the global .npmrc which uses the token created during npm login phase and does not store it either
  • No need to create local .npmrc and risk that someone will put their access token there and commit to source control
  • No risk that someone will pass their access token to docker build command and make it visible in the docker image
  • No need to worry about token getting into your terminal history
  • Seems to be using the latest docker features

Again, happy to be shown a better way or maybe there is nothing wrong with the current solution. Either way, I'm opened to discussion and with the recent leak of npm access token I feel like it's an important one to have.

References

matzar avatar Apr 26 '22 13:04 matzar

I've made a proposition of changes to the documentation which would drop the usage of ARG and environment variables and would instead promote the usage of Docker build secrets.

matzar avatar Apr 27 '22 12:04 matzar

@MylesBorins @monishcm any chance you could have a look please?

matzar avatar May 17 '22 13:05 matzar

@ljharb @MylesBorins @trevrosen any chance on getting feedback on this please?

matzar avatar Jun 21 '22 08:06 matzar

@MylesBorins @monishcm I think this is ready for another review.

matzar avatar Aug 01 '22 09:08 matzar

I've run npm install and pushed a commit with updated package-lock.json so all the test pass but it seems to have caused merge conflict which I can't solve w/out the write access (I'm not asking for ones - just saying :)).

Would you rather not have package-lock.json in the PR @MylesBorins @ljharb @lukekarrys @spicycode ?

matzar avatar Aug 18 '22 13:08 matzar

@matzar Yes, can you try removing the package-lock.json from this PR and see if the deployment succeeds?

lukekarrys avatar Sep 01 '22 15:09 lukekarrys

@matzar Yes, can you try removing the package-lock.json from this PR and see if the deployment succeeds?

@lukekarrys I've removed package-lock.json and build-and-upload fails but it's on installing npm packages.

When I run npm install locally and update package-lock.json then the build and publish passes (btw running gatsby build locally also passes).

Maybe we should update package-lock.json on the main with another PR first to fix

npm ERR! `npm ci` can only install packages when your package.json and package-lock.json or npm-shrinkwrap.json are in sync. Please update your lock file with `npm install` before continuing.
npm ERR! 

?

matzar avatar Sep 12 '22 13:09 matzar

@matzar can you try rebasing your PR against the main branch and force pushing. I'm not sure how you ended up in the situation you are in right now but npm ci is working locally without any conflicts

MylesBorins avatar Sep 12 '22 15:09 MylesBorins

@matzar can you try rebasing your PR against the main branch and force pushing. I'm not sure how you ended up in the situation you are in right now but npm ci is working locally without any conflicts

@MylesBorins I've opened up a new PR #208 from the main, updated docker-and-private-modules.mdx with the changes that are in this PR, and the build passes (strangely enough, rebasing did not help).

I propose we close this PR and merge #208 instead which has all the same changes and is a lot less convoluted.

matzar avatar Sep 15 '22 10:09 matzar