-
Notifications
You must be signed in to change notification settings - Fork 117
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
feat: add an otpProvider option to allow users to use a module to provide an OTP to semantic-release #234
base: master
Are you sure you want to change the base?
feat: add an otpProvider option to allow users to use a module to provide an OTP to semantic-release #234
Conversation
ad45704
to
144fb63
Compare
…vide an OTP to semantic-release
144fb63
to
e11be30
Compare
Both #176 and this PR are probably not the direction we are going to take to handle npm 2FA in semantic-release. I understand the backend solution can be implemented such that it would asks the user to enter the otp somewhere before sending it to semantic-release. But there is no guarantee that it would be implemented that way and would create situation that effectively circumvent the 2FA. In addition, if you implement the backend such that is ask the user to enter the otp (so you don't circumvent 2FA), it would leave the semantic-release process hanging while the user enter the otp. If the user doesn't enter the otp, then the CI would end up killing the build on timeout and leave you with a partial release (git tag created, GitHub release created but no npm package published). Instead I'm currently working on a service that would:
Then with a CLI or a web UI the user would be able to log in with their npm credential and see the stored packages. Then they can release them to the actual npm registry which would prompt them to enter their otp. |
I would argue that that is entirely on the user for doing that (if they make that choice, regardless of how bad it is). I don't think we should limit what the Good Actors can do just in case someone does a Bad Thing.
This is fine, we have been doing it like this for well into a year now and most CI providers have configurable timeouts.
This is also a solvable problem, cleaning up a incomplete release is not difficult, delete a tag and empty github release and you're fine. We have our build timeout set to 30 minutes and we have never seen a timeout hit.
We also experimented with such a system, this results in a in-complete release until the user action of publishing to the actual registry is actually complete. So there is a github tag, release and changelog but no guarantee the npm publish actually went out (or will ever go out). While I respect the direction you wish to take with your project 🙇 I would point out that all this is going to do is force us to maintain our electron/semantic-release-npm fork indefinitely which will only cause ecosystem fragmentation as other folks pick up and use the CFA system. Some kind of plugin system here to allow us to inject an OTP token into the CLI args here is all we need to avoid that. |
For full context the CFA system was recently fully open sourced by the team at https://github.com/continuousauth |
Yes. However supporting a solution in a plugin from the That said, you obviously put a lot of work into continuous-auth and I would like to find a way to make it work with semantic-release in an easy way. See below for a potential solution.
This is not trivial for many users. Per the issues and support requests we have, a lot of users do not have a clear enough understanding of how semantic-release work internally (using the tags to figure out the new commits) and don't know how to fix those partial releases.
Yes. Both system have more or less the same problem, in your solution if the user doesn't enter the otp right away, in mine if they don't make the final release. However the solution I proposed has 2 advantages: the window of time for the user to enter the otp is a lot longer (as long as we keep the package stored) and failing to enter the otp doesn't result in situation technically problematic for semantic-release.
npm can read the otp from the environment variable
You could even add a Then configure this plugin to run just before I didn't test but that should work because the This way we would have a clean way to support continuous-auth without giving users the possibility to use a similar solution that doesn't require manual input of the OTP. Would that work for you? |
It might have been a while since I looked at it, but isn't -- The downside of having a different plugin to set the OTP provider vs a config option is that you're then forced to define the plugin order in your release config. My gut is occasionally wrong, but I do feel uneasy each time I do that vs using the default plugins. |
See semantic-release/semantic-release#1391. You can test with semantic-release on the branch |
Works for my use case: https://github.com/dominykas/test-things/blob/master/lib/poc-npm-set-otp.js
Is that the same thing mentioned here: https://twitter.com/BenjaminCoe/status/1208808072610758658? There's also other alternatives proposed, incl. npm providing a "staging" area: nodejs/package-maintenance#282. It would be valuable if you could describe your proposal in there as well. |
No it's something similar to nodejs/package-maintenance#282 (comment) I describe it there #234 (comment) But knowing that npm will implement something similar, I don't know if I'm going to finish mine. In any case if I release it, it would be there: https://github.com/airlockjs/airlockjs |
@MarshallOfSound still interested of making https://github.com/continuousauth works with semantic-release? Another solution would be to just add a |
Hey @pvdlg sorry I've been super distracted by other things recently. Looking through that proposed solution there would still be a race condition between the creation / fetch of the OTP and actually calling The best thing to do there is to get the OTP just before calling publish ourself.
This sounds super optimal if you're ok with it |
@MarshallOfSound I can work on the implementation by reusing some of the code in this PR if you are ok with it. Is there anything to verify other than |
Those are the only two environment variables.
There isn't an endpoint for that currently, though there can be quite easily. You'd just want an endpoint that takes those two variables as body / auth headers values and says "yes" or "no" right? |
Yes! |
@pvdlg You can now use |
Would it not make sense to also have a param for the host? |
@dominykas technically there is, it's just undocumented and only used for testing atm. |
This is an alternative take on #176 that allows folks to provide a generic "module" to load and provide an OTP which gives users a bit more freedom than "provide a URL".
An example usage of this is something like