fix wrong error raised when private key is missing or invalid

[shashfrankenstien]

In the Connection class, if the private key file is not in key_types, or if the file path provided doesn't exist, it raises

UnboundLocalError: local variable 'key' referenced before assignment

This is because key.from_private_key_file is called in a finally block that will execute regardless of whether the try succeeds or fails. The correct branch to use is else.

Additionally, self._transport.auth_publickey and self._transport.auth_password were being called even when password and/or private_key inputs were not strings. This PR fixes this and raises CredentialException

Added required tests!

[byteskeptical]

I appreciate the approach but I think fixing the catching of the correct error is enough here. Too much hand holding can muddy the code base without any real distinction in the outcome for a user that provides garbage inputs. Falling into the trap of checking or limiting the data types of user input is a slippery slope, at least in the python context.

[shashfrankenstien]

Ah! I was looking into your comments, but I see now you have removed the tests I added and fixed my mistake. I would recommend the below check so that paths built with pathlib will also work -

if isinstance(private_key, str) should be updated to if isinstance(private_key, (str, Path))

Also, I would recommend adding a connection test with PKey object since it is supported. It would have helped me catch my mistake

Let me know if you'd like me to make these changes

[byteskeptical]

Adding Path to the if block for file path resolution is fine and has been pushed. Pretty much all tests (except password specific ones) use private key authentication by default. I think the confusion is that .auth_privatekey only accepts PKey objects. The only reason for the if block is to resolve a file path to a PKey object. I did however re-add a modified missing file test that includes passing a valid path to a directory instead of a file as well. Additionally a deprecated and unsupported key format test. Can you please add two secrets to your fork so the rest of the test suite for Linux and Windows will run. The password can be set to the PASS in tests/common.py and the private_key is simply the contents of the id_sftpretty.

[shashfrankenstien]

Can you please add two secrets to your fork so the rest of the test suite for Linux and Windows will run. The password can be set to the PASS in tests/common.py and the private_key is simply the contents of the id_sftpretty.

I've added the required PASSWORD and PRIVATE_KEY variables to my fork.

I couldn't figure this part out initially. So I ended up making a branch where I removed secret files and changed the workflow to dynamically create fresh key files and run the full CI on all platforms. If this is something that might be good to contribute, please let me know. I'd be glad to clean it up and send over a PR for your review

[shashfrankenstien]

Pretty much all tests (except password specific ones) use private key authentication by default. I think the confusion is that .auth_privatekey only accepts PKey objects.

I was thinking of something like this to pass an actual PKey object to Connection (this is a working test) -

def test_connection_good_pkey_obj(sftpserver):
    '''attempt connection using `paramiko.pkey.PKey` object'''
    copts = conn(sftpserver)
    pkey = Ed25519Key(
        filename=copts['private_key'],
        password=copts['private_key_pass'])
    del copts['private_key_pass']
    copts['private_key'] = pkey
    with sftpserver.serve_content(VFS):
        with Connection(**copts) as sftp:
            sftp.listdir()

[byteskeptical]

What new code path is being tested that isn't already covered by every other non-password test? Your manually doing what happens in the if isinstance(private_key block. In my view the value add for this kind of test would be to parametrize (@pytest.mark.parametrize) over all supported private key types by replacing the LOCAL['private_key'] value. This would mean adding at least two more keys and probably renaming the existing id_sftpretty to something explicit like id_ed25519. I tried to handle something similar with the test_connection_bad_private_key_format tests. The real key method would be harder as it involves additional setup on the test host in actions for each OS type.

[byteskeptical]

On that note could you please add the aforementioned secrets to your fork so the Linux and Windows test can properly run.