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

Requiring 2FA for npm publishes #10631

Closed
not-an-aardvark opened this issue Jul 20, 2018 · 11 comments
Closed

Requiring 2FA for npm publishes #10631

not-an-aardvark opened this issue Jul 20, 2018 · 11 comments
Labels
accepted There is consensus among the team that this change meets the criteria for inclusion archived due to age This issue has been archived; please open a new issue for any further discussion infrastructure Relates to the tools used in the ESLint development process

Comments

@not-an-aardvark
Copy link
Member

not-an-aardvark commented Jul 20, 2018

In light of recent events, I think we should start requiring 2FA in order to publish npm packages in the eslint npm organization. This would significantly reduce the risk of malicious publishes due to compromised accounts, like the one last week.

Since our publishes are performed by a bot account on a Jenkins server, the process of doing this will be nontrivial. I think the best way to do this would be for all the people authorized to start a release to scan the same TOTP private key to their phone. Then they would enter a TOTP code through the Jenkins interface while the release happens, and the Jenkins server would use it to complete the release.

This would prevent malicious publishes in the following attack scenarios:

  • A TSC member's GitHub account is compromised (without 2FA, this would allow an attacker to log into Jenkins and start a release).
  • The Jenkins server itself is compromised (without 2FA, this would allow an attacker to steal an npm access token from the server and start a release).

Releases take several minutes, since the server runs all of the tests before publishing. As a result, entering a TOTP code at the start of the jenkins build won't work, because the token will expire by the time the package needs to be published. We have a few options:

I would prefer the first option, since we don't know when the second option will be available.

@not-an-aardvark not-an-aardvark added infrastructure Relates to the tools used in the ESLint development process evaluating The team will evaluate this issue to decide whether it meets the criteria for inclusion labels Jul 20, 2018
@platinumazure
Copy link
Member

If we get all of our GitHub accounts secured (2FA), so that only Jenkins itself could be a practical attack vector here, how urgently do we need npm 2FA in your view? (i.e., if we secure the GitHub accounts, could we wait for time-limited tokens?)

@not-an-aardvark
Copy link
Member Author

I would still prefer to set up npm 2FA regardless, but I think securing the GitHub accounts would be a big improvement. If it turns out that setting up npm 2FA is too difficult now, we could wait for time-limited tokens.

@platinumazure
Copy link
Member

Awesome. I agree we should set up npm 2FA "eventually" regardless-- just wanted to get your sense of how urgent the need was.

not-an-aardvark added a commit to eslint/eslint-release that referenced this issue Jul 20, 2018
(refs eslint/eslint#10631)

This updates the release API to not publish to npm when initially
called, and to only publish to npm when invoked as a separate process.
This will make it possible to pause the build to ask the user for a TOTP
code before publishing, provided that consumers of the package are
updated accordingly.
not-an-aardvark added a commit to eslint/eslint-release that referenced this issue Jul 20, 2018
(refs eslint/eslint#10631)

This updates the release API to not publish to npm when initially
called, and to only publish to npm when invoked as a separate process.
This will make it possible to pause the build to ask the user for a TOTP
code before publishing, provided that consumers of the package are
updated accordingly.
@ilyavolodin
Copy link
Member

As far as I know, there's no way to currently require 2FA for publishing of a package. There's going to be a beta starting at some point in the future, but I don't think it's running yet. Also, how do you propose we share the TOTP code with all of the members of the TSC (we also have some packages that are not published by TSC, like eslint-plugin-typescript).

@not-an-aardvark
Copy link
Member Author

As far as I know, there's no way to currently require 2FA for publishing of a package. There's going to be a beta starting at some point in the future, but I don't think it's running yet.

As of this week, it is possible to do this -- see here.

Also, how do you propose we share the TOTP code with all of the members of the TSC (we also have some packages that are not published by TSC, like eslint-plugin-typescript).

I was thinking we would create a team on Keybase, an encrypted filesystem, and use it to distribute the QR code.

You make a good point about eslint-plugin-typescript. I think the simplest solution would be to also give those publishers access to the QR code. Given that there aren't many publishers of these packages outside the TSC, this doesn't seem like it would be too big a problem (since those publishers still wouldn't be able to publish other packages without compromising the Jenkins server).

Other solutions include disabling the 2FA requirement for those packages, or publishing them from a different bot account with a different 2FA secret.

@ilyavolodin
Copy link
Member

Honestly, I have never tried to share TOTP code, so I'm not sure how that would work, but is it even supposed to generate valid code from multiple devices? I though it was coupled with device specific timestamp and can't really be shared. That's why after you scan QR code, you need to generate one (or two, for paranoid services like AWS) TOTP codes and submit them back to the server. Am I complete wrong here?

@not-an-aardvark
Copy link
Member Author

My understanding is that TOTP codes are deterministically generated from (a) the QR code, and (b) the current time. So scanning the same QR code on multiple devices should result in the same TOTP codes, provided that the clocks on the devices are accurate.

I think the reason that sites require you to send TOTP codes after scanning a QR code is to verify that you actually scanned the QR code successfully, to prevent account lockout if you did something wrong.

@btmills
Copy link
Member

btmills commented Jul 23, 2018

I can confirm @not-an-aardvark's understanding that TOTP uses the secret shared via the QR code and the current time, so sharing the secret via Keybase would be technically feasible, assuming we want to do it now and not wait for time-limited tokens.

@not-an-aardvark not-an-aardvark added the tsc agenda This issue will be discussed by ESLint's TSC at the next meeting label Jul 26, 2018
@not-an-aardvark
Copy link
Member Author

TSC Summary: npm has an option to require a 2FA code to be entered when publishing any particular package. This would improve the security of our releases because it would prevent a malicious release in the event that (a) a TSC member's GitHub or npm account is compromised, or (b) the Jenkins server is non-persistently compromised. If we enabled this, we would probably do releases by sharing the 2FA private key among the users who are allowed to do the release, and entering a TOTP code into jenkins during the release process.

TSC Question: Should we require 2FA for npm publishes?

@not-an-aardvark
Copy link
Member Author

This issue was accepted in today's TSC meeting.

@not-an-aardvark not-an-aardvark added accepted There is consensus among the team that this change meets the criteria for inclusion and removed evaluating The team will evaluate this issue to decide whether it meets the criteria for inclusion tsc agenda This issue will be discussed by ESLint's TSC at the next meeting labels Sep 28, 2018
not-an-aardvark added a commit that referenced this issue Oct 17, 2018
This upgrades `eslint-release` to 1.0.0. The new version of `eslint-release` has a separate "generate" and "publish" phase, to allow for a 2FA code to be supplied in between. As a side-effect, it's now possible to manually test most aspects of the release process (by running `npm run generate-release`) without actually publishing a release somewhere. In lieu of unit tests, this should make it easier to test changes to the release process, like this one.
not-an-aardvark added a commit that referenced this issue Oct 17, 2018
This upgrades `eslint-release` to 1.0.0. The new version of `eslint-release` has a separate "generate" and "publish" phase, to allow for a 2FA code to be supplied in between. As a side-effect, it's now possible to manually test most aspects of the release process (by running `npm run generate-release`) without actually publishing a release somewhere. In lieu of unit tests, this should make it easier to test changes to the release process, like this one.
not-an-aardvark added a commit that referenced this issue Oct 17, 2018
This upgrades `eslint-release` to 1.0.0. The new version of `eslint-release` has a separate "generate" and "publish" phase, to allow for a 2FA code to be supplied in between. As a side-effect, it's now possible to manually test most aspects of the release process (by running `npm run generate-release`) without actually publishing a release somewhere. In lieu of unit tests, this should make it easier to test changes to the release process, like this one.
not-an-aardvark added a commit to eslint/doctrine that referenced this issue Oct 17, 2018
(refs eslint/eslint#10631)

This updates `eslint-release` to allow the package to still be published now that we have 2FA enabled.
not-an-aardvark added a commit to eslint/doctrine that referenced this issue Oct 17, 2018
(refs eslint/eslint#10631)

This updates `eslint-release` to allow the package to still be published now that we have 2FA enabled.
not-an-aardvark added a commit to eslint/eslint-plugin-markdown that referenced this issue Oct 17, 2018
(refs eslint/eslint#10631)

This updates `eslint-release` to allow the package to still be published now that we have 2FA enabled.
not-an-aardvark added a commit to eslint/eslint-scope that referenced this issue Oct 17, 2018
(refs eslint/eslint#10631)

This updates `eslint-release` to allow the package to still be published from the Jenkins server now that we have 2FA enabled on the bot account.

Some changes are also needed on the Jenkins server -- I plan to make those changes after this is merged.
not-an-aardvark added a commit to eslint/eslint-transforms that referenced this issue Oct 17, 2018
(refs eslint/eslint#10631)

This updates `eslint-release` to allow the package to still be published from the Jenkins server now that we have 2FA enabled on the bot account.

Some changes are also needed on the Jenkins server -- I plan to make those changes after this is merged.
not-an-aardvark added a commit to eslint/eslint-visitor-keys that referenced this issue Oct 17, 2018
(refs eslint/eslint#10631)

This updates `eslint-release` to allow the package to still be published from the Jenkins server now that we have 2FA enabled on the bot account.

Some changes are also needed on the Jenkins server -- I plan to make those changes after this is merged.
not-an-aardvark added a commit to eslint/espree that referenced this issue Oct 17, 2018
(refs eslint/eslint#10631)

This updates `eslint-release` to allow the package to still be published from the Jenkins server now that we have 2FA enabled on the bot account.

Some changes are also needed on the Jenkins server -- I plan to make those changes after this is merged.
not-an-aardvark added a commit to eslint/generator-eslint that referenced this issue Oct 17, 2018
(refs eslint/eslint#10631)

This updates `eslint-release` to allow the package to still be published from the Jenkins server now that we have 2FA enabled on the bot account.

Some changes are also needed on the Jenkins server -- I plan to make those changes after this is merged.
not-an-aardvark added a commit to eslint/typescript-eslint-parser that referenced this issue Oct 17, 2018
(refs eslint/eslint#10631)

This updates `eslint-release` to allow the package to still be published from the Jenkins server now that we have 2FA enabled on the bot account.

Some changes are also needed on the Jenkins server -- I plan to make those changes after this is merged.
not-an-aardvark added a commit to eslint/espree that referenced this issue Oct 18, 2018
(refs eslint/eslint#10631)

This updates `eslint-release` to allow the package to still be published from the Jenkins server now that we have 2FA enabled on the bot account.

Some changes are also needed on the Jenkins server -- I plan to make those changes after this is merged.
not-an-aardvark added a commit to eslint/eslint-plugin-markdown that referenced this issue Oct 18, 2018
(refs eslint/eslint#10631)

This updates `eslint-release` to allow the package to still be published now that we have 2FA enabled.
not-an-aardvark added a commit to eslint/eslint-scope that referenced this issue Oct 18, 2018
(refs eslint/eslint#10631)

This updates `eslint-release` to allow the package to still be published from the Jenkins server now that we have 2FA enabled on the bot account.

Some changes are also needed on the Jenkins server -- I plan to make those changes after this is merged.
not-an-aardvark added a commit to eslint/eslint-transforms that referenced this issue Oct 18, 2018
(refs eslint/eslint#10631)

This updates `eslint-release` to allow the package to still be published from the Jenkins server now that we have 2FA enabled on the bot account.

Some changes are also needed on the Jenkins server -- I plan to make those changes after this is merged.
not-an-aardvark added a commit to eslint/eslint-visitor-keys that referenced this issue Oct 18, 2018
(refs eslint/eslint#10631)

This updates `eslint-release` to allow the package to still be published from the Jenkins server now that we have 2FA enabled on the bot account.

Some changes are also needed on the Jenkins server -- I plan to make those changes after this is merged.
not-an-aardvark added a commit to eslint/typescript-eslint-parser that referenced this issue Oct 18, 2018
(refs eslint/eslint#10631)

This updates `eslint-release` to allow the package to still be published from the Jenkins server now that we have 2FA enabled on the bot account.

Some changes are also needed on the Jenkins server -- I plan to make those changes after this is merged.
kaicataldo pushed a commit that referenced this issue Oct 19, 2018
This upgrades `eslint-release` to 1.0.0. The new version of `eslint-release` has a separate "generate" and "publish" phase, to allow for a 2FA code to be supplied in between. As a side-effect, it's now possible to manually test most aspects of the release process (by running `npm run generate-release`) without actually publishing a release somewhere. In lieu of unit tests, this should make it easier to test changes to the release process, like this one.
not-an-aardvark added a commit to eslint/generator-eslint that referenced this issue Oct 27, 2018
(refs eslint/eslint#10631)

This updates `eslint-release` to allow the package to still be published from the Jenkins server now that we have 2FA enabled on the bot account.

Some changes are also needed on the Jenkins server -- I plan to make those changes after this is merged.
@not-an-aardvark
Copy link
Member Author

All of eslint's packages now require 2FA to publish, and the infrastructure for doing so on the build server has been set up (see here), so this issue is complete.

JamesHenry pushed a commit to typescript-eslint/typescript-eslint that referenced this issue Jan 14, 2019
(refs eslint/eslint#10631)

This updates `eslint-release` to allow the package to still be published from the Jenkins server now that we have 2FA enabled on the bot account.

Some changes are also needed on the Jenkins server -- I plan to make those changes after this is merged.
JamesHenry pushed a commit to typescript-eslint/typescript-eslint that referenced this issue Jan 15, 2019
(refs eslint/eslint#10631)

This updates `eslint-release` to allow the package to still be published from the Jenkins server now that we have 2FA enabled on the bot account.

Some changes are also needed on the Jenkins server -- I plan to make those changes after this is merged.
@eslint-deprecated eslint-deprecated bot locked and limited conversation to collaborators Apr 26, 2019
@eslint-deprecated eslint-deprecated bot added the archived due to age This issue has been archived; please open a new issue for any further discussion label Apr 26, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
accepted There is consensus among the team that this change meets the criteria for inclusion archived due to age This issue has been archived; please open a new issue for any further discussion infrastructure Relates to the tools used in the ESLint development process
Projects
None yet
Development

No branches or pull requests

4 participants