Passing username as SSHClientConnectionOptions attribute does not work, local username is used instead

[cdleonard]

Sample pseudocode:

def test(myhost: str, myuser: str):
    options = SSHClientConnectionOptions()
    options.username = myuser
    async with asyncssh.connect(
        host=host,
        options=options,
    ) as conn:
        result = await conn.run("whoami", check=True)
        assert isinstance(result.stdout, str)
        assert result.stdout.strip() == myuser

The username passed in SSHClientConnectionOptions is ignored, local username is used instead. Works as expected if username is passed into asyncssh.connect kwargs instead.

Can be trivially reproduced without a target, this sample fails:

async def test_options_construct_ignores_username():
    options = SSHClientConnectionOptions()
    options.username = "user"
    new_options = await SSHClientConnectionOptions.construct(options)
    assert new_options.username == "user"

[ronf]

This isn't the correct way to populate SSHClientConnectionOptions(). You must pass the arguments in via keyword arguments on the constructor. For example:

    options = await SSHClientConnectionOptions(username=username)

What you did was ignored, as the settings are not stored as direct attributes of SSHClientConnectionOptions. Also, in some cases, the values you pass in are validated or sanitized before being stored. For instance, usernames are passed through the saslprep() function. This computation is only done as part of object construction.

[cdleonard]

It took me a while to realize what is going on here. It seems that SSHClientConnectionOptions keeps constructor arguments inside the kwargs attribute and this is the only thing that is check by SSHClientConnectionOptions.construct(options). Direct assignment to SSHConnectionOptions attributes is lost. Here is a test demonstrating the difference between correct and incorrect initialization of SSHClientConnectionOptions:

def get_random_username() -> str:
    return f"u{random.randint(1000000, 9999999)}"


async def test_options_init_kwarg_good():
    myuser = get_random_username()
    options = SSHClientConnectionOptions(username=myuser)
    assert options.username == myuser
    new_options = await SSHClientConnectionOptions.construct(options)
    assert new_options.username == myuser


async def test_options_init_attr_fail():
    myuser = get_random_username()
    options = SSHClientConnectionOptions()
    options.username = myuser
    assert options.username == myuser
    new_options = await SSHClientConnectionOptions.construct(options)
    assert new_options.username != myuser

I would have expected for direct assignment to SSHClientConnectionOptions attributes to work, but it does not. The only way to build an options instance one-by-one is via kwargs, losing any typing checks.

Feel free to close this "as designed". But maybe it would be possible to use both options.__dict__ and options.kwargs?

[ronf]

The challenge here is the around needing to call the prepare() method any time a value is changed. That function handles things like checking if there's a match in the SSH config file, which can be dependent on some of the initial values passed when constructing a new Options instance. In some cases, it also does things like loading certificates/keys, which involves referencing multiple option names. So, this needs to be done only after all of the arguments have been passed in. Setting one attribute at a time would cause a problem in those cases, but passing them in through keyword arguments to the constructor makes sure that everything is available to pass into prepare(). If you want to change one or more option values, it actually goes back to the original argument values you initially provided, updates that with the new values, and then calls prepare() again as if it were constructing a new Options object from scratch.

Also, I misspoke about Options objects not using attributes. AsyncSSH actually does store the results of the call to prepare() into direct attributes on the Options object. However, the names of these don't map 1-to-1 with the keyword arguments passed into the constructor, and neither do the types of these attributes. The kwargs and the object attributes need to be kept separate from one another, with prepare() being how you convert from one to the other.

Unfortunately, you're right that using kwargs like this doesn't allow for type checking on the Options constructor, but I don't really see a good way to solve this.