Azure-pipelines-tasks: "npm Authenticate (for task runners)" should use environment variables to improve security

[octogonz]

Environment

• Server - Azure Pipelines
• Agent - Hosted or Private: either

Issue Description

The "npm Authenticate (for task runners)" task allows a shell command such as npm publish to obtain its NPM registry credentials from a VSTS "service connection". This is more secure/maintainable than storing an authentication token in an environment variable.

However, the design of "npm Authenticate (for task runners)" task seems to work like this:

1. Inject the credentials into an .npmrc file on disk (whose path is specified as part of the build step)
2. Allow the rest of the build steps to execute
3. In a special post-build step: Revert the changes from step 1, restoring the .npmrc file to its original state

There are a couple problems with this design:

1. If the post-build step never executes (e.g. because the machine crashed), the credentials will be saved on disk forever. This seems like a security concern, given that the NPM publishing token is a very powerful and sensitive credential.
2. The NPM tool only searches for .npmrc in a few places. In a monorepo with hundreds of projects, we need the file to be in a central location that will be found by many different npm publish invocations. We can put the file in the user home directory, but that will affect other jobs running on the same VM. That's a security concern also.

Proposed improvement

I'd like to propose a different design: The "npm Authenticate (for task runners)" task should store its authentication token in an environment variable. The task would be configured to invoke a shell command similar to the "Command Line" task. The environment variable would only be visible to this process; it would not be accessible from any other build steps.

This could be implemented as an optional mode for the existing task, or maybe it could be a new task type.

What do you think? Does this sound like a useful improvement? Or is there some way to solve the above security problems using the existing design?

@nickpape-msft @iclanton

[infin8x]

@pgonzal unfortunately you've hit on a fundamental challege with build. When someone has access to your build environment, lots of nefarious things can happen. We mitigate this when you're working within the same organization by using a token scoped to the execution of the build, but that obviously doesn't help when you're publishing to e.g. npmjs.

The npm Authenticate task is actually designed to leave the token available for you to use for the scope of the build. This allows you to call npm in whatever way you wish (through gulp, grunt, ps, bash, etc.) and have npm self-discover the token it needs to perform your desired action.

The npm task works closer to the way you describe. It still creates a temporary file but it cleans it up at the end of that task, rather than the end of the build. If we made the changes you proposed, it would be to this task. I don't think the environment variable support was there the last time we updated the task - good to know they're improving security here.

Adding @bryanmacfarlane, who's thought a lot about token security in build - he may have additional thoughts.

[infin8x]

Or, thinking more, we might also switch the npm Authenticate task to use the environment variable (as that's just generally better than having a token on disk). But, we'd probably make the environment variable available for the scope of the build, or have that configurable, to enable the scripting scenarios I described.

[octogonz]

@alexmullans FWIW when I first tried "npm Authenticate (for task runners)", I was dismayed to find that our npmjs.com authentication token was accidentally pushed to our public GitHub repo. (!) This happened because the target .npmrc file was under our repo folder, and one of the build steps ran a version-bumping operation that made changes to package.json and then did something akin to git add && git commit && git push. That script did not anticipate other processes writing files into the Git working folder, so the .npmrc change inadvertently got included. Coming from that experience, I feel like environment variables are inherently MUCH safer than writing tokens into a file on disk.

I don't think the environment variable support was there the last time we updated the task - good to know they're improving security here.

In our case, we don't run npm publish directly. Instead we use rush publish which figures out which package(s) have changes to be published, and automates running npm publish (or actually pnpm publish, or yarn publish in some cases) for each individual project. Rush supports a command line sort of like this:

rush publish --npm-auth-token $(YOUR_TOKEN_HERE) ...

So the environment variable is useful even if you aren't using the NPM package manager.

But, we'd probably make the environment variable available for the scope of the build, or have that configurable, to enable the scripting scenarios I described.

I suppose you have a fair point: If any of the build steps executes malicious code, then theoretically all subsequent build steps could have the binary that they invoked replaced by a malicious binary that captures the environment variable. However I think it's still less secure: For example, a non-malicious person might do something like dump all the environment variables to the console for diagnostic purposes. If the authentication token is available to ALL steps, then this person would inadvertently dump the token into the log files. (Even if VSTS detected this and masked the password, it could still happen via a build artifact.)

Given the high risk of a public npmjs.com authentication token, I would encourage the VSTS design to be as conservative as possible, versus by default sharing the environment as widely as possible. Security is arguably more important than convenience for this case.

[bryanmacfarlane]

@alexmullans - we'll discuss and loop back.

[infin8x]

I've tracked this work internally as part of a set of updates we'd like to make to the npm task. In general, @pgonzal, I think you're right that we need to move to using an environment variable.

[octogonz]

Awesome, thanks Alex!

[infin8x]

@pgonzal we're tracking design work for this here, fyi. https://github.com/Microsoft/azure-pipelines-yaml/pull/40#pullrequestreview-170819959

[aasim]

This has been fixed and released as part of this design: https://github.com/Microsoft/azure-pipelines-yaml/blob/master/design/artifacts-in-pipelines.md#authentication-1.

The new NpmAuthenticate task uses environment variables to authenticate feeds. You can read more about this task here: https://docs.microsoft.com/en-us/azure/devops/pipelines/tasks/package/npm-authenticate?view=azure-devops