russh icon indicating copy to clipboard operation
russh copied to clipboard

WIP: Add a non OpenSSL RSA impl

Open darleybarreto opened this issue 1 year ago • 8 comments

Tests are failing:

failures:
    test::test_agent
    test::test_decode_rsa_secret_key
    test::test_gpg
    test::test_loewenheim
    test::test_nikao
    test::test_o01eg
    test::test_pkcs8
    test::test_pkcs8_encrypted

test result: FAILED. 9 passed; 8 failed; 0 ignored; 0 measured; 0 filtered out; finished in 1.19s

darleybarreto avatar Feb 17 '24 14:02 darleybarreto

Looks like it's failing in the key parser (here or here) in 8 out of 9 tests

Eugeny avatar Feb 18 '24 16:02 Eugeny

Looks like it's failing in the key parser (here or here) in 8 out of 9 tests

Thanks for the pointers, now only 4 are failing:

failures:
    test::test_agent
    test::test_loewenheim
    test::test_o01eg
    test::test_pkcs8_encrypted

Something is wrong with decode_secret_key, not sure what.

darleybarreto avatar Feb 24 '24 19:02 darleybarreto

I know there are still some test failures to sort through, but I just want to say that this is super helpful and I would love to get this PR (or similar) landed! Dealing with the whole openssl-sys nonsense is so annoying, and makes cross-compilation more difficult

samuela avatar Apr 09 '24 05:04 samuela

Agreed! I tried sorting out the test failures (it's complaining about extraneous data at the end of the test keys) but haven't been able to find time to dive deeper yet.

Would really appreciate any help with sorting these out

Eugeny avatar Apr 10 '24 15:04 Eugeny

Thanks for the pointers, now only 4 are failing:

failures:
    test::test_agent
    test::test_loewenheim
    test::test_o01eg
    test::test_pkcs8_encrypted

Something is wrong with decode_secret_key, not sure what.

This is probably caused by: https://github.com/warp-tech/russh/issues/270

robertabcd avatar Apr 22 '24 00:04 robertabcd

Hey folks, I rebased this branch and a new test is falling

failures:
    test::test_agent
    test::test_decode_pkcs8_rsa_secret_key
    test::test_loewenheim
    test::test_pkcs8_encrypted

test_decode_pkcs8_rsa_secret_key was added after I opened this PR

darleybarreto avatar Apr 25 '24 22:04 darleybarreto

Thanks @robertabcd , now we are down to

failures:
    test::test_agent
    test::test_loewenheim

darleybarreto avatar Apr 25 '24 23:04 darleybarreto

I have a general comment on this PR: There are a lot of duplications on openssl and !openssl code branches which also dup the ssh protocol handling code. For example, reading and writing mpints. This can be error-prone in the future, because we will need to manually ensure they are identical on the protocol handling part. I would suggest we only branch on part of the code that is necessary.

robertabcd avatar Apr 26 '24 16:04 robertabcd

Hey folks, was RSA support implemented already? If so, can you close this one?

Sorry for not being able to push this forward earlier, got caught up with some other stuff.

darleybarreto avatar May 04 '24 22:05 darleybarreto

Hey folks, was RSA support implemented already? If so, can you close this one?

has non-openssl RSA support been implemented? if so, how can one use it?

samuela avatar May 07 '24 03:05 samuela

Hey folks, was RSA support implemented already? If so, can you close this one?

has non-openssl RSA support been implemented? if so, how can one use it?

It's pretty bleeding-edge right now. You'll need 0.44.0-beta.1. By not enabling the openssl feature, you'll be using pure-Rust RSA.

robertabcd avatar May 07 '24 04:05 robertabcd

Thanks @robertabcd !

samuela avatar May 07 '24 04:05 samuela

Unable to run in Rust version 1.71.1 stable

error[E0658]: use of unstable library feature 'result_option_inspect'
   --> C:\Users\bangfu\.cargo\registry\src\index.crates.io-6f17d22bba15001f\russh-0.44.0-beta.2\src\server\encrypted.rs:430:26
    |
430 |                         .inspect(|hash| pubkey.set_algorithm(*hash));
    |                          ^^^^^^^
    |
    = note: see issue #91345 <https://github.com/rust-lang/rust/issues/91345> for more information


Solved it. rustup update

TheBlindM avatar Jun 05 '24 02:06 TheBlindM