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

fingerprint_type_mask returns typemask before checking keytype? #6496

Open
vytasmk opened this issue Feb 22, 2023 · 12 comments
Open

fingerprint_type_mask returns typemask before checking keytype? #6496

vytasmk opened this issue Feb 22, 2023 · 12 comments

Comments

@vytasmk
Copy link

vytasmk commented Feb 22, 2023

By following "the rabbit hole" got here. Long story short: I am using Redmine project management system with Gitolite hosting plugin which uses Rugged Gem which depends on this libgit2 library. Gitolite hosting plugin needs to connect to SSH and I was getting an error invalid or unknown remote ssh hostkey and if I try to connect using simple ssh command everything works just fine.

So started looking from where this error message comes from and found that it is here libgit2/transports/ssh.c and by following further I noticed that fingerprint_type_mask function returns data in the beginning even before checking keytype (it looks like switch statement is never reached?). Can somebody also check that and confirm?

This was first introduced with commit 6d63afcee71ef943a1fcd69c24f0562e9c8e429a on Jan 19, 2023.

@vytasmk
Copy link
Author

vytasmk commented Feb 22, 2023

Update: found that this code update was committed on Nov 2, 2022 74c2b4b

@ethomson
Copy link
Member

I’m pretty sure this is fixed by e0220e6

@vytasmk
Copy link
Author

vytasmk commented Feb 22, 2023

Emm... but if I open that file in the Main branch that function is unchanged https://github.com/libgit2/libgit2/blob/main/src/libgit2/transports/ssh.c#L587

@ethomson
Copy link
Member

That does look like a bug, but I don't think that it's the bug you're describing, which has otherwise been reported and fixed in main.

@vytasmk
Copy link
Author

vytasmk commented Feb 22, 2023

So how the code works here (taken from main branch now).

static int fingerprint_type_mask(int keytype)
{
	int mask = LIBSSH2_KNOWNHOST_TYPE_PLAIN | LIBSSH2_KNOWNHOST_KEYENC_RAW;
	return mask;

	switch (keytype) {
	case LIBSSH2_HOSTKEY_TYPE_RSA:
		mask |= LIBSSH2_KNOWNHOST_KEY_SSHRSA;
		break;
	case LIBSSH2_HOSTKEY_TYPE_DSS:
	....

As I understand switch (keytype) { line will never be executed due to return mask; before it... at least I understand so, may be I am wrong?

@ethomson
Copy link
Member

I understand what you're saying. That function either needs to not return mask2 or to have the rest of the lines removed. (cc @carlosmn)

But again - what you're reporting sounds exactly like the bug we fixed in main and had nothing to do with this. Can you test with main and let us know if it's fixed it for you, too?

@ethomson
Copy link
Member

Here's the original bug report: #6453

@vytasmk
Copy link
Author

vytasmk commented Feb 23, 2023

OK. Now I understand what You were saying regarding real source of the problem. I am not using this library directly it is included in one plugin in Ruby so it will be quite hard to test for me with latest version. Now I have simply found solution how to use previous version which is working. I will wait until new release will be available so all needed packages will get updated.

That code simply confused me as there is no comment that that return is made intentionally and the rest of the code is not needed at all. I think it is good reason that I close this issue now. Thank You @ethomson

@vytasmk vytasmk closed this as completed Feb 23, 2023
@ethomson
Copy link
Member

Thanks @vytasmk! I hope that you don't mind, I'm going to keep this open since it is an issue, but I also think that we've sorted out the issue that you're seeing and will have a v1.4 and v1.5 minor release available to fix it 🔜 .

@ethomson ethomson reopened this Feb 23, 2023
@vytasmk
Copy link
Author

vytasmk commented Feb 23, 2023

OK. That's fine to have it open and clean that code up or at least have a comment there.

I will try to test when Rugged gets updated and if there will be any issues will let You know.

@sseide
Copy link

sseide commented Mar 8, 2023

I had a similar problem with different error message on Redmine -> Ruby rugged 1.5.1 -> libgit2 running in Linux Debian 11 (amd64).

[ERROR] failed to set hostkey preference: The requested method(s) are not currently supported

At least this message seems to point to the methods updated in the commit mentioned above.
Therefor i can report back if it is fixed by this commit as soon as an updated version of rugged library is available.

Needed to go back to rugged 1.5.0.1 to solve it by now.

@fauno
Copy link

fauno commented Sep 22, 2023

Not sure if it's related to this particular issue but FWIW I've tried to push to a remote with a ssh://user@host:port/repo.git URL and got the same exception:

invalid or unknown remote ssh hostkey (Rugged::SshError)

It works with rugged 1.5.0.1 but fails with 1.6.3 and 1.7.1. If the URL is changed to user@host:repo.git with custom port on ~/.ssh/config it hangs on all versions, but I'm not sure if it's picking up the port from SSH config. A simple ssh -p 22 user@host fails with permission denied for this host.

I'll keep 1.5.0.1 for now.

Code is here: https://0xacab.org/sutty/sutty/-/blob/issue-12919/app/models/site/repository.rb#L162-L182

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

4 participants