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

ssh-rsa sigantures broken with 1.12.0 #1316

Closed
axkibe opened this issue Jul 20, 2023 · 5 comments
Closed

ssh-rsa sigantures broken with 1.12.0 #1316

axkibe opened this issue Jul 20, 2023 · 5 comments

Comments

@axkibe
Copy link

axkibe commented Jul 20, 2023

ssh-rsa signatures worked for me with 1.11.0 since 1.12.0 (up to including 1.14.0) they stopped worked.

unfortunally I do not have more info at hand, except that after upgrading node-ssh the handshakes stopped working, downgrading to 1.11.0 they worked again.

PS: I forgot to say, node-ssh working in server mode.

@axkibe
Copy link
Author

axkibe commented Jul 20, 2023

I found the issue, in my server code I had to change this line (taken from examples)

  •           if( ctx.signature && ssh2Parsed.verify( ctx.blob, ctx.signature ) !== true ) continue;
    
  •           if( ctx.signature && ssh2Parsed.verify( ctx.blob, ctx.signature, ctx.hashAlgo ) !== true ) continue;
    

I may have overlooked this, but it is a there an UPDATES.md file or something of this sort that lists these kind of breaking changes?

Kind regards,

@mscdex
Copy link
Owner

mscdex commented Jul 20, 2023

#989 (comment)

For ssh2 servers, ctx.key.algo will always be 'ssh-rsa' for RSA keys and a ctx.hashAlgo will now be available to make things a bit easier. Technically this would be a breaking change, but the only situation I can think of where this would be a problem would be clients sending sha2-based RSA offers (perhaps via trial and error) and implementors checking that ctx.key.algo against those sha2-based RSA signature names. Seeing as most SSH clients either only send 'ssh-rsa' or at least support server-sig-algs, I don't think this should be a problem, so the proposed changes won't need to land in a new major version.

@axkibe
Copy link
Author

axkibe commented Jul 20, 2023

To be honest, I don't fully understand this. It was tough a breaking change for me (openssh clients), my server code: https://gitlab.com/csc1/gitengine/-/blob/main/src/Ssh/Self.js#L131

I think in essence I was close to the example code back then, now the major difference is that I cache parsed keys in a map and check serveral keys.

Debugging I just noticed it all looked almost the same, execpt ssh2Parsed.verify() returned true with .11 and false with .14, and the noted the new parameter I was supposed to hand down to.

No biggie, it all works now, just noting unless I'm doing something wrong here, an update notice would have been nice.

PS: I think not many people use servermode to be honest, most use client mode.. but I'm still very thankful for the whole thing.

@mscdex
Copy link
Owner

mscdex commented Jul 20, 2023

What OpenSSH client versions? AFAIK this shouldn't be a problem as OpenSSH will fall back to ssh-rsa unless the client has explicitly set PubkeyAcceptedKeyTypes to not include it.

I don't remember if there are any potential issues or not with having the server automatically send server-sig-algs to clients, using the same logic as OpenSSH when it comes to deciding if it should send such a message to the client. We're already doing that.

@axkibe
Copy link
Author

axkibe commented Jul 20, 2023

OpenSSH_8.4p1 Debian-5+deb11u1, OpenSSL 1.1.1n 15 Mar 2022
with no special config was failing.

The other machine I tested with, same result:
OpenSSH_9.2p1 Debian-2, OpenSSL 3.0.9 30 May 2023
but here .ssh/config has:
addKeysToAgent yes
PubkeyAcceptedKeyTypes=+ssh-rsa

I forgot about this, was due some issues in the past I excplizitly turned it on.

ed25519 worked all the time, with .11 and .14 without the 3rd parameter.
Thats why I though at first, that mysterious rsa issue popped up in past I could now finally reproduce it, but only to discover it was something different this time :)

But IMO you can close this, the issue is resolved for me, and since its already 3 minor versions ago, I guess no one else had that problem.

@mscdex mscdex closed this as completed Dec 11, 2023
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

2 participants