Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Support OTP fetching from config.otpUrl #176

Closed
wants to merge 5 commits into from

Conversation

dominykas
Copy link
Contributor

@dominykas dominykas commented Jun 28, 2019

Tested on one of my own packages: works like magic!

I think I got the coverage right, but I'm still not sure how to assert that --otp was passed through to npm. npm will send the OTP inside the npm-otp header, but short of adding a proxy around the couch app, I'm not sure if it's even possible to observe that in any way.

See also: #93

Note: this does not yet assert that the OTP was actually sent over to npm.
@dominykas dominykas marked this pull request as ready for review July 16, 2019 22:06
@pvdlg
Copy link
Member

pvdlg commented Aug 21, 2019

Maybe I'm missing something here but that seems to be a huge security hole...
Anyone can look at the URL you configure in otpUrl calls it, get your OTP and release anything with your account.

@dominykas
Copy link
Contributor Author

dominykas commented Aug 21, 2019

No, they cannot release anything, because

a) I need to press a button on my phone (although I'll admit that someone could configure a server to respond without that, however that's no worse than no 2FA at all)
b) OTP is not enough - you still need the token (so once again - no worse than no 2FA)

@dominykas
Copy link
Contributor Author

Also the URL should be kept as secret as the token (i.e. in env). I can easily update this to only use process.env.NPM_OTP_URL if that's the preferred route?

@pvdlg
Copy link
Member

pvdlg commented Aug 21, 2019

Reading through the documentation of https://github.com/nearform/optic I don't think using it require any modification in this plugin.

In your CI you could simply set NPM_CONFIG_OTP (which is equivalent of passing the OPT via --otp).

Because the OTP is valid only for a few seconds you could do that via a prepublishOnly step in your package.json so it would be executed just before semantic-release runs npm publish:

{
 "scripts": {
    "prepublishOnly": "export NPM_CONFIG_OTP=$(curl -s $NPM_OTP_URL/$OTP_TOKEN)"
  }
}

I would rather not make any change in this plugin not recommending any particular solution in the doc out of security concern.

@dominykas
Copy link
Contributor Author

I was not aware you could set it this way - I'll try that out, thank you.

I will try this out and report back.

I have no intent of recommending any particular solution, but if this approach works - I suppose some documentation is in order - I'll see if I can find a place to PR that.

@dominykas
Copy link
Contributor Author

dominykas commented Aug 23, 2019

No luck: https://travis-ci.org/dominykas/allow-scripts/jobs/575685664#L310

I suspected this might happen, because I had tried playing around with env vars in a different context - they don't carry through, i.e. npm does not see an env vars from one of scripts inside another one.

I'll try writing the otp into an .npmrc next, but IIRC, the config for publish is loaded before prepublishOnly executes, so I'm not sure that will have an effect either.


No luck: https://travis-ci.org/dominykas/allow-scripts/jobs/575690340#L316

It can probably be worked around by using the exec plugin, but it seems to me that this is a behavior that's important enough to be built-in, rather than worked around?


No luck with exec: https://travis-ci.org/dominykas/allow-scripts/jobs/576303090#L347

I might not be writing the param correctly or it is unsupported via the .npmrc.

The only way I could get the env var to work was to set it directly before calling npx semantic-release, however this is somewhat less than ideal, as that introduces some 10s gap between when it is generated and when it is used - I don't know what is the grace period on npm side, but there is a chance it will expire.

Edit: also just realized that setting it before executing npx semantic-release also means that you'll be asking for an OTP even if you don't need it (i.e. no publish will be triggered during the run).


Would it make sense to replace otpUrl with an otpCommand to make it more generic (with curl -s $URL being one way to configure the command)?

@dominykas
Copy link
Contributor Author

dominykas commented Aug 23, 2019

(merged two comments into one)

@pvdlg
Copy link
Member

pvdlg commented Sep 23, 2019

In your first test the error is The local branch master is behind the remote one, therefore a new version won't be published. so the failure is not related to the OTP token.

@dominykas
Copy link
Contributor Author

dominykas commented Sep 24, 2019

In your first test the error is The local branch master is behind the remote one, therefore a new version won't be published. so the failure is not related to the OTP token.

The link I have points to line 310 in the log... but the log does not contain such a line. The release job also ran 1.5 hours later than the test jobs, so I suspect for whatever reason (quite possibly by accident) I restarted the build.

I can't find a way to recover the original logs, but I'm fairly certain it was an EOTP originally.

Reproducing the whole test is time consuming, but here's an alternative test case to demonstrate that env vars do not carry through:
✔ ~/devel/experiments/test-renovate [master|✚ 1…1] 
12:10 $ npm run dostuff

> @dominykas/test-things@ predostuff /Users/dominykas/devel/experiments/test-renovate
> export TEST=ohai


> @dominykas/test-things@ dostuff /Users/dominykas/devel/experiments/test-renovate
> echo $TEST


✔ ~/devel/experiments/test-renovate [master|✚ 1…1] 
12:10 $ TEST=canhazvalue npm run dostuff

> @dominykas/test-things@ predostuff /Users/dominykas/devel/experiments/test-renovate
> export TEST=ohai


> @dominykas/test-things@ dostuff /Users/dominykas/devel/experiments/test-renovate
> echo $TEST

canhazvalue

package.json:

  "scripts": {
    "predostuff": "export TEST=ohai",
    "dostuff": "echo $TEST"
  },

@pvdlg
Copy link
Member

pvdlg commented Dec 14, 2019

Closing per #234 (comment)

@pvdlg pvdlg closed this Dec 14, 2019
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants