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

BUG: verifyAuth should not try SSH in the presence of a token in the environment; and should not call debug() #2053

Open
EvanCarroll opened this issue Jul 26, 2021 · 11 comments

Comments

@EvanCarroll
Copy link

EvanCarroll commented Jul 26, 2021

There is a routine called verifyAuth, there are a few problems with this routine,
two

  1. It's called multiple to find a method to verify rw repo access. This is probably not needed at all. This should be configured not brute forced.
  2. Even if needed in some capacity, it's not needed in the first call here. This doesn't make sense because for me I'm told "semantic-release:get-git-auth-url Verifying ssh auth by attempting to push to https://acme.net/foo/bar.git": why would it say it's validating ssh auth when it's connecting https? Moreover, why would it attempt to connect without a token before it deduces with total certainty that the token is required "skip verification if there is no ambiguity on which env var to use for authentication". If a user provides a _TOKEN, why would we want to try ssh verification first?
  3. Because it's called iteratively, a blanket debug that informs you that it failed is not useful. For example, I filed this issue on semantic-release/gitlab Fail on git push --dry-run --no-verify: remote: HTTP Basic: Access denied gitlab#259 which was a mistake because the GitLab authentication and implementation of verifyAuth happens not in the GitLab module but in core. that means on that on the first call that I linked above, 100% of users (afaik) that use --debug, GitLab, and CI tokens (rather than SSH keys) will receive a spurious message,
semantic-release:git Error: Command failed with exit code 128: git push --dry-run --no-verify https://acme.net/foo/bar.git HEAD:main
remote: HTTP Basic: Access denied
fatal: Authentication failed for 'https://acme.net/foo/bar.git/'
    at makeError (/usr/local/lib/node_modules/semantic-release/node_modules/execa/lib/error.js:60:11)
    at handlePromise (/usr/local/lib/node_modules/semantic-release/node_modules/execa/index.js:118:26)
    at processTicksAndRejections (node:internal/process/task_queues:96:5)
    at async verifyAuth (/usr/local/lib/node_modules/semantic-release/lib/git.js:208:13)
    at async module.exports (/usr/local/lib/node_modules/semantic-release/lib/get-git-auth-url.js:91:5)
    at async run (/usr/local/lib/node_modules/semantic-release/index.js:56:27)
    at async module.exports (/usr/local/lib/node_modules/semantic-release/index.js:260:22)
    at async module.exports (/usr/local/lib/node_modules/semantic-release/cli.js:55:5)
2021-07-26T17:06:52.442Z semantic-release:get-git-auth-url SSH key auth failed, falling back to https.
  1. My suggestion would be to use HTTPS if a token is detected that can be used to authenticate.
  2. To remove the call to debug(error) in the call the verifyAuth(), and to handle that from the caller's perspective. No one should turn on --debug and see "remote: HTTP Basic: Access denied; fatal: Authentication failed" as a result of a pre-flight or fail over.

If we agree on these implementation I can submit a patch.

@EvanCarroll EvanCarroll changed the title verifyAuth is very confusing, and --debug makes it worse. BUG: verifyAuth should not try SSH in the presence of a token in the environment; and should not call debug() Jul 26, 2021
@wlfbck
Copy link

wlfbck commented Mar 2, 2022

We are in the process of trying to use this plugin, and run into this exact confusing message thinking we had done something wrong, when we haven't. So highly agree to fix this.

@EvanCarroll
Copy link
Author

@wlfbck I already provided the code, it was rejected.

@engineerakki
Copy link

We are hitting this same issue as well,
Even if we have GITHUB_TOKEN configured,

semantic release plugin tries to authenticate with SSH first.

@EdieLemoine
Copy link

I just noticed this because one of my release workflows now takes 2 minutes longer than it used to.

https://github.com/myparcelnl/js-sdk/runs/6387333718 / semantic-release took 15s :)
https://github.com/myparcelnl/js-sdk/runs/6508349023 / semantic-release took 2:22m :(

Running semantic-release locally with --debug showed it was trying to connect through SSH, which brought me here.

@dawidbojarowicz
Copy link

dawidbojarowicz commented Jun 7, 2022

It looks like the issue is in get-config.js

repositoryUrl is either taken from package.json (repository or repository.url based on the setup) or from the result of git config --get remote.origin.url command.

In case you have https url set up in your package.json or you/CI cloned the repo using https then the value of repositoryUrl will be always with https.

By the way https is the correct setup. See npm docs.

repositoryUrl value is then used in get-git-auth-url.js with wrong assumption that it's SSH URL.

Potential solutions:

  1. check if repositoryUrl is ssh if not fall back to https - this should speed up execution by skipping unnecessary push
  2. convert https to ssh however this might be tricky as sometimes companies might be using different ports for SSH etc.
  3. rewrite the logic to validate access based on the user setup (prefer git setting over package.json parsing result)

@EvanCarroll
Copy link
Author

EvanCarroll commented Jun 7, 2022

@dawidbojarowicz try my fork. See if it solves your problem

@prein
Copy link

prein commented Oct 12, 2022

2. If a user provides a _TOKEN, why would we want to try ssh verification first?

In CircleCI, for example, environment variables can be shared across projects using contexts. I may want to use some but not all env vars from a context in my workflow. Just because an env var with a certain name is present doesn't mean specific job should make implicit assumptions based on that fact. Especially when this takes precedence over the repositoryUrl format leaving the user with hacks (unset GH_TOKEN ...)

@EvanCarroll
Copy link
Author

EvanCarroll commented Oct 12, 2022

I disagree with the framing of "implicit" there. Whether you're reading package.json as configuration or the environment as configuration, it's no more or less implicit. There are defaults either way. They're used implicitly. There are fall-backs currently. The only question is how these fall-backs should read package.json and environmental variables. In your case, you have environmental variables provided to a process that you want ignored. In my case, (forking workflow) I have a package.json which I want ignored (as it doesn't represent the forked package). I imagine my workflow is far more common, as 100% of forks encounter it and that's pretty common with a process built around Git.

I'd also urge you to unset your tokens regardless. Supplying tokens to every process in your pipeline sounds like a bad idea.

@stepanbaghdasaryan
Copy link

it looks like the issue still exists in version 20.1.0. Is there any updates here?

@PaulSinghDev
Copy link

If it helps anyone I was getting this issue and scratching my head for while until I ended up stumbling on the idea that the issue is with the https: protocol being used for the link to test authentication with SSH. To solve the issue I have:

  • Added my Personal Access Token to .env as GITHUB_TOKEN
  • Added node-env-run as a dev dependency
  • Added a release:dry script to the package.json file
  • Add the following in the contents of the release:dry script node-env-run --exec \"semantic-release --dry-run --debug --no-ci --repositoryUrl \"$(git remote get-url origin | tr -d '\n')\"\"

The key thing to note is this is running the semantic-release with the .env I have locally (via node-env-run and is then specifying the url used for the git origin (in my case this is starting with the git protocol). The key thing for me was specifying a URL which is not using https.

@EvanCarroll
Copy link
Author

Just as a reminder, the fork here solves this problem https://github.com/EvanCarroll/semantic-release

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

No branches or pull requests

8 participants