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

2FA in CI #282

Open
wants to merge 12 commits into
base: main
Choose a base branch
from
Open

2FA in CI #282

wants to merge 12 commits into from

Conversation

dominykas
Copy link
Member

@dominykas dominykas commented Nov 10, 2019

The primary intent of this PR is to kick off a discussion on the various options around how 2FA could be used for publishing from CI, incl. by teams.

I listed multiple options, I'd like to hear feedback about them (I might have missed some pros/cons) and it would be nice to hear some other ideas (feel free to push commits with them, if you have to).

If we can get an agreement on which ones are worth pursuing the most - we could possibly extract them into separate issues/projects either here or under @pkgjs, but for now I just wanted to have everything in one place as a starting point.

I know there's opinions about whether it is a good idea to publish from CI, but I think that's a discussion for CI/CD and publishing guidelines - the fact is that people are publishing from CI and they are not using 2FA, and that needs to be fixed (and I don't think that asking them to not use CI for publishing is something that would bring any results).

See also: #244


As a personal note, after I wrote down the "poor man's version" of the Release Manager approach (i.e. setting the 2FA flag on a published release post-factum), I've kind of grown to think that it might just be one of the more awesome ways forward.

At the same time, I'm actually quite excited about all the options, and in an ideal world all of them would be available for people to use.


To make sure that the code is a _second_ factor it should be generated by a human on an independent device. The secret seed of the OTP generator should be protected in such a way, that it cannot be obtained together with the registry token.

At the moment, the advice to use 2FA is at odds with advice to automate the testing and release processes, esp. with publishing automatically from Continuous Integration (CI) systems (e.g. Travis).

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Where is the advice that the release process should be automated? Opinions differ.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I very strongly believe that publishes - the only irrevocable thing - should never be automated, for many reasons (mostly around safety and auditing and having humans as the last line of defense)

Copy link
Member Author

@dominykas dominykas Nov 10, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I hereby give such advice ;)

Then there's also https://blog.greenkeeper.io/introduction-to-semantic-release-33f73b117c8 and similar concepts.

I disagree that humans are better at this task than machines. I do however agree that this is not a one-sided decision, it has pros and cons. That said - I'm not sure that this document is a good place to weigh them up, I think that's better left for CI/CD and publishing guidelines and I'm more than happy for those to include a balanced overview of both opinions. I will see if I can find the time to make the necessary suggestions there myself, unless there's someone who can do that sooner.

So anyways, there is the school of thought of automated releases, there are maintainers using it, there are teams that can benefit and simplify their processes - they have no simple way of doing 2FA and that is the problem we're trying to solve here.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, this document could focus on the how of automating releases, not the why. It just needs a small tweak, because the current wording makes it sounds like automating the release process is considered a best practice by the package-maintenance team. Which - judging by @ljharb's comment - is not established, or even discussed yet.

Copy link
Member Author

@dominykas dominykas Nov 10, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, I'll see how to update the wording (happy to take suggestions).

Some prior discussions in #244.

The document title does only mention 2FA as a baseline practice. It's also a draft PR for a drafts folder ;) I wasn't sure about the best approach to open up the different options for discussion - the intent is to slim them down and open separate issues if need be. Haven't had time to write an intro for the PR :)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@vweevers does 09035d7 sound better?

Also added the PR description.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perfect

@dominykas dominykas marked this pull request as ready for review November 11, 2019 16:08
@dominykas dominykas added the package-maintenance-agenda Agenda items for package-maintenance team label Nov 11, 2019
Copy link
Member

@wesleytodd wesleytodd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry I only had time to read half right now (I started on a new team on Monday and have been very busy with the switch), but this is great!

docs/drafts/2fa.md Outdated Show resolved Hide resolved
docs/drafts/2fa.md Outdated Show resolved Hide resolved
@boneskull
Copy link

This is a good overview of the problem, thanks

@isaacs
Copy link

isaacs commented Nov 14, 2019

We just had a brief discussion on this topic in our npm team standup.

I intend to write up a brief RFC describing a staged publish support in the registry package document. I'm going to avoid some of the various angles discussed here and try to keep it focused on what we can likely accomplish within a reasonable time window.

tl;dr of what I'm thinking:

  • you can "stage" a publish by adding it to the "stagedPackages" object in the packument
  • stagedPackages do not reserve the version number. However, when a version is moved out of staging and into versions, the number is reserved, and any staged packages for that version are deleted.
  • a new npm command to promote a staged publish to a real publish
  • stagedPackages will be associated with a tarball artifact, so you can install them (for testing etc) by specifying the tarball URL.
  • TBD:
    • permissions around who has rights to stage vs promote vs publish
    • possible npm option to include stagedVersions in install target list
    • Any UX (other than CLI and API) around release management, staging, etc.

I think this would give us a useful building block to start tackling some of the other stuff discussed here.

@wesleytodd
Copy link
Member

This sounds great @isaacs! I think that what you have outlined here will really help, but the critical piece of integrating 2FA into the promote doesn't seem covered. If you could do this on the registry website with a button click which required re-auth and 2FA that would be AWESOME! So the sooner we can tackle that last TBD item the better :)

@wesleytodd
Copy link
Member

Hm, actually I wonder....maybe I was wrong that we need more than a cli. I think having some user stories in the RFC would help here, because now that I am thinking about it you could have CI do the staged publish and then have the owner just run the promote from the cli with an OTP. I guess that does solve that use case in a minimalist sense.

@ljharb
Copy link
Member

ljharb commented Nov 14, 2019

That's the workflow I envision; CI staging publishes, but the user explicitly promoting it (via CLI, and optionally in the future, the web UI)

@darcyclarke
Copy link
Member

As a follow up to @isaacs's initial comment, we've just posted an RFC outlining what that initial set of work/features might look like here: npm/rfcs#92

@ruyadorno
Copy link
Member

@dominykas here's the link to the npm public events calendar that has the reference to each OpenRFC meeting: https://calendar.google.com/calendar/embed?src=npmjs.com_oonluqt8oftrt0vmgrfbg6q6go%40group.calendar.google.com (it's a bi-weekly happening meeting every other Wednesday)

@dominykas dominykas removed the package-maintenance-agenda Agenda items for package-maintenance team label Feb 17, 2020
@dominykas
Copy link
Member Author

Removed the agenda label until there's something more to discuss.

My plan is to wait for npm/rfcs#92 and I will then rephrase this to list the existing options as such, rather than the theoretical approaches on how this can be solved (might do that sooner, time permitting).

@wesleytodd wesleytodd changed the base branch from master to main October 20, 2020 20:07
@bnb
Copy link
Contributor

bnb commented Dec 30, 2020

May also want to consider adding something about npm access tokens.

https://docs.npmjs.com/creating-and-viewing-access-tokens

@thescientist13 thescientist13 added the package-maintenance-agenda Agenda items for package-maintenance team label Feb 1, 2022
@thescientist13
Copy link
Contributor

These efforts are happening as well, maybe worth including or tracking in an issue to further refine this doc as progress is made

@simoneb
Copy link

simoneb commented Feb 1, 2022

We have developed this which we're using to publish our npm packages with as much automation as having 2FA enabled in NPM allows. It's not a perfect solution but it works pretty well for us.

https://github.com/nearform/optic-release-automation-action

@mhdawson mhdawson removed the package-maintenance-agenda Agenda items for package-maintenance team label Feb 17, 2022
@dominykas
Copy link
Member Author

I'd appreciate if someone could re-read this and make some suggestions with updates on the state of things in 2022!

Copy link
Contributor

@thescientist13 thescientist13 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure we want to add a link to this to the repo's docs TOC as well?


## Related docs

* [Publish guidelines (draft)](https://github.com/nodejs/package-maintenance/blob/master/docs/drafts/PUBLISH-GUIDELINES.md)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think these all got promoted out of drafts recently, so we may want to double check the link and remove the draft label

* [CI/CD guidelines (draft)](https://github.com/nodejs/package-maintenance/blob/master/docs/drafts/ci-cd-guidelines.md)
* [Testing guidelines (draft)](https://github.com/nodejs/package-maintenance/blob/master/docs/drafts/testing-guidelines.md)

Note: at the time of writing neither documents or gives advice on using two-factor authentication when publishing.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note: at the time of writing neither documents or gives advice on using two-factor authentication when publishing.

Suggest removing or


### Extension in CI providers

As 2FA in CI is partly a problem because there is no way to enter the OTP, it can be solved by simply adding a way to enter an OTP in CI.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if manual approval, now available in GitHub Actions could be "hacked" to make this work? From what I see in that blog post, folks can leave a comment. Maybe that comment could be the OTP and then the build script could leverage the GitHub API or even the current process / action to grab that and supply it to npm publish?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting idea but wouldn’t the comment be public?

(time based) OTPs can be used multiple times within valid time window. Bad actors can watch for comments and immediately reuse same OTP for another action.

@dominykas
Copy link
Member Author

It seems staged publishing is no longer on the roadmap: npm/roadmap#9 (comment)

I might actually see if I should start revisiting this doc sooner than I thought...

@ljharb
Copy link
Member

ljharb commented Mar 20, 2022

Yeah, it’s very disappointing, since it’s the only path to securely and reproducibly publishing anything.

OlawaleSC23

This comment was marked as spam.

@simoneb
Copy link

simoneb commented Jun 5, 2023

Is this PR getting anywhere? It's been amusingly open for several years now

@ljharb
Copy link
Member

ljharb commented Jun 5, 2023

At this point, the only approach for actual 2FA in CI remains something like https://github.com/GoogleCloudPlatform/wombat-dressing-room or https://github.com/step-security/wait-for-secrets (the latter of which i use on eslint-plugin-react, the only package i publish from CI from so far).

With the "web" login, I suppose you could potentially remove the need for an external server, but I haven't investigated that yet.

@dominykas
Copy link
Member Author

I'm not sure I have enough capacity right now to rewrite any of the documentation here - these are mostly just design proposals.

In terms of implementation - given the age of this - I'm not sure any of the implementations are moving ahead or gaining significant traction to be called a common pattern... Maybe there's people somewhere else working on/discussing something similar?

Optic is as close as it gets (although it's grown up with a bunch more opinions), but the UX of all of these tools is just not there.

From my perspective, hitting the "Close" button on this PR is easy enough 🤷‍♂️ Maybe I should just do that?

@wesleytodd
Copy link
Member

From my perspective, hitting the "Close" button on this PR is easy enough 🤷‍♂️ Maybe I should just do that?

I am not sure there is any benefit to doing that. At least with it being open that is a signal to new folks who might like to pick it up and get it updated. Maybe editing the original post with an update that it is "looking for help" and then you could just fun-ollow the thread?

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

Successfully merging this pull request may close these issues.