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

Allow setting hash algorithm to use for signing requests of SSH agent #449

Merged
merged 1 commit into from
Jan 15, 2025

Conversation

wiktor-k
Copy link
Contributor

@wiktor-k wiktor-k commented Jan 14, 2025

This is a suggestion PR that I've tested locally. I'm happy to adjust it in any way you see fit.

@nightmared would it be possible for you to test it, too? (just like in the ticket you've described).

Fixes: #444
Fixes: #445

Note that since I'm using my own agent server implementation and upstream ssh-key has not released a version with some critical fixes such as RustCrypto/SSH#263 making signatures with SHA-1 is totally broken :( so I'd be really happy if something like this has been merged. Thanks for your time! 🙇

@Eugeny
Copy link
Owner

Eugeny commented Jan 14, 2025

I think a more flexible solution would be to update the Signer trait to accept an Option<HashAlg> parameter, similar to how PrivateKeyWithHashAlg is used for direct key auth. If you have time to implement this change, please do, otherwise I'll merge it as-is and do it later, thanks!

It will need to be updated later anyway to accept an enum of a public key OR certificate later for OpenSSH certificate keys agent auth.

@wiktor-k
Copy link
Contributor Author

Thanks for the feedback! I'll try to implement your idea tomorrow and see how far I go...

@nightmared
Copy link

This PR does fix the test (and would close #444)!

However, I'm not convinced hardcoding SHA512 (instead of leaving the option to specify the algorithm as @Eugeny suggested) is the best choice here. E.g., what would happen when talking to old systems/SSH servers that may not support it? I fear this could introduce regressions.

@Eugeny
Copy link
Owner

Eugeny commented Jan 14, 2025

(sorry for the conflict, client.rs got moved from russh-keys/src to russh/src/keys on main)

@wiktor-k wiktor-k force-pushed the wiktor-k/use-sha-512-with-rsa branch from ed1399e to d4e840b Compare January 15, 2025 13:03
@wiktor-k wiktor-k changed the title Use SHA-512 when signing using RSA keys instead of SHA-1 Allow setting hash algorithm to use for signing requests of SSH agent Jan 15, 2025
@wiktor-k
Copy link
Contributor Author

(sorry for the conflict, client.rs got moved from russh-keys/src to russh/src/keys on main)

Fortunately git took care of that seamlessly when rebasing 😅

Okay, I've passed the hash_alg throughout the code and verified that this PR (still) works with my other code... phew. (Note that I've adjusted only signing with the agent, the thing that I use, maybe some other parts need adjusting, too? 🤔 )

See if that's more like what you meant and if not I can re-adjust.

Thanks for your time! 👋

@wiktor-k wiktor-k force-pushed the wiktor-k/use-sha-512-with-rsa branch from d4e840b to fe58322 Compare January 15, 2025 13:08
@Eugeny
Copy link
Owner

Eugeny commented Jan 15, 2025

LGTM, thank you!

@Eugeny Eugeny merged commit 902010f into Eugeny:main Jan 15, 2025
7 checks passed
@wiktor-k wiktor-k deleted the wiktor-k/use-sha-512-with-rsa branch January 15, 2025 19:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants