Skip to content

Conversation

dberenbaum
Copy link
Contributor

Empty passwords were being passed as () instead of None to asyncssh, causing unexpected failure.

See https://discuss.dvc.org/t/error-in-pushing-experiments/2138 and the added test for how to reproduce it.

In https://github.com/ronf/asyncssh/blob/050e5c1a0f42d5f145b44c33b183ff745447a687/asyncssh/connection.py#L7790, you can see that username defaults to (), but password defaults to None.

@dberenbaum dberenbaum requested a review from skshetry July 8, 2024 13:06
@@ -8,15 +8,78 @@
import asyncssh
import paramiko
import pytest
from dulwich.contrib.test_paramiko_vendor import CLIENT_KEY, PASSWORD, USER, Server
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unfortunately, this test was moved outside of dulwich, so we have to copy all these into this test file now. See https://github.com/jelmer/dulwich/pull/1277/files#diff-71d1213e4e666bc90d260326bcd0fe35137cc595cc108abe5eaf136440f2e9ab.

skshetry
skshetry previously approved these changes Jul 8, 2024
@skshetry
Copy link
Collaborator

skshetry commented Jul 8, 2024

@dberenbaum, please fix lint errors. Code looks good to me.

@dberenbaum dberenbaum force-pushed the no-password branch 2 times, most recently from 9d01b22 to 733c599 Compare July 8, 2024 13:20
@dberenbaum
Copy link
Contributor Author

Linting is fixed now. PTAL @skshetry, thanks!

@dberenbaum
Copy link
Contributor Author

I think the force push dismissed your approval @skshetry

@dberenbaum dberenbaum merged commit 5d073d5 into main Jul 8, 2024
@dberenbaum dberenbaum deleted the no-password branch July 8, 2024 14:09
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.

2 participants