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

WIP: Add a non OpenSSL RSA impl #252

Closed
wants to merge 10 commits into from

Conversation

darleybarreto
Copy link

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 darleybarreto changed the title Add a non OpenSSL RSA impl WIP: Add a non OpenSSL RSA impl Feb 17, 2024
@Eugeny
Copy link
Owner

Eugeny commented Feb 18, 2024

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

@darleybarreto
Copy link
Author

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.

@samuela
Copy link
Contributor

samuela commented Apr 9, 2024

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

@Eugeny
Copy link
Owner

Eugeny commented Apr 10, 2024

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

@robertabcd
Copy link
Contributor

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: #270

@darleybarreto
Copy link
Author

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
Copy link
Author

Thanks @robertabcd , now we are down to

failures:
    test::test_agent
    test::test_loewenheim

let pss = match hash {
SignatureHash::SHA1 => Pss::new::<Sha1>(),
SignatureHash::SHA2_256 => Pss::new::<Sha256>(),
SignatureHash::SHA2_512 => Pss::new::<Sha512>(),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't debug but those tests are about signing/verifying. I think this should use PKCS#1 v1.5 padding scheme, instead of PSS. See rsa::pkcs1v15.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that would be Pkcs1v15Sign, then?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the pointers, I have changed this part and rsa_signature, but the same tests are failling

failures:
    test::test_agent
    test::test_loewenheim

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

buffer needs to be hashed before passing into verify. I guess you could use VerifyingKey, that can hash it for you.

@robertabcd
Copy link
Contributor

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.

@darleybarreto
Copy link
Author

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.

@Eugeny Eugeny closed this May 5, 2024
@samuela
Copy link
Contributor

samuela commented May 7, 2024

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?

@robertabcd
Copy link
Contributor

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.

@samuela
Copy link
Contributor

samuela commented May 7, 2024

Thanks @robertabcd !

@TheBlindM
Copy link

TheBlindM commented Jun 5, 2024

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

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

Successfully merging this pull request may close these issues.

5 participants