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

Fix RSA certificate key determination. #602

Merged
merged 8 commits into from
Jul 31, 2020

Conversation

vladimirlagunov
Copy link
Contributor

Fixes #599.

@fmeum
Copy link
Contributor

fmeum commented Jul 28, 2020

Now that #607 has been merged, it would make sense to ensure that RSA certificates work with both SHA1 and the SHA2 family. The logic in getClientKeyAlgorithm will need to deal with this added complexity anyway.

@hierynomus
Copy link
Owner

@werehuman Can you add a test to test the functional change? i.e. the test that would've failed without these changes... The current test you added/extended only tests the KeyType class itself, but not the actual use case for the change.

@vladimirlagunov
Copy link
Contributor Author

@hierynomus Let's discuss where it's better to put a new test.

The best functional test is the test that verifies the primary goal of changes. Initially I created the pull request to fix connections using SSH keys with certificates. At the moment of creating the pull-request, this patch helped to proceed the authentication further but not until the success. Something still didn't work, and I had to switch to other tasks.

Tell me your suggestions about the new test. If you don't know how to test this particular change better, I'll verify the whole ability to authenticate with ssh-rsa-cert-v01 and with merging the fresh master. Either I'll write a functional test, or I'll write where I'm stuck.

@vladimirlagunov
Copy link
Contributor Author

No, still can't authenticate.

debug1: userauth-request for user user service ssh-connection method publickey [preauth]                                                                                          
debug1: attempt 1 failures 0 [preauth]                                                                                                                                            
debug2: input_userauth_request: try method publickey [preauth]                           
key_from_blob: incorrect signature [preauth]                                             
userauth_pubkey: cannot decode key: ssh-rsa-cert-v01@openssh.com [preauth]               
debug2: userauth_pubkey: authenticated 0 pkalg ssh-rsa-cert-v01@openssh.com [preauth]    
debug1: userauth-request for user user service ssh-connection method password [preauth]  
debug1: attempt 2 failures 1 [preauth]

And no debugging logs for SSHJ.

I have to investigate further. And I can't imagine a functional test for now.

# Conflicts:
#	src/main/java/net/schmizz/sshj/transport/TransportImpl.java
@hierynomus
Copy link
Owner

@werehuman I would imagine you'd add an integration test that authetnicates using a certificate. If that works, that is the proof right?

@vladimirlagunov
Copy link
Contributor Author

Ready for review.

The new test verifies only RSA client certificates and only in the PEM format. Of course, to tell that we fully support certificate keys, we should verify the whole variety of keys, formats, and also verify host keys. But it's more than nothing even now.

@hierynomus hierynomus merged commit 0e0d730 into hierynomus:master Jul 31, 2020
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.

RSA certificate key determination is broken
3 participants