fix wrong compression applied ('none',) when compress = True, but no compression tuple provided to cnopts

[shashfrankenstien]

In the current version of the code, test_compression_enabled fails and it's been disabled in CI tests. I was able to find the issue and implement a fix (also enabled in CI tests).

In the _start_transport method, if cnopts.compress is True, it calls self._transport.use_compression(compress=bool(compress)). This sets the compression options within paramiko Transport -

        if compress:
            self._preferred_compression = ("zlib@openssh.com", "zlib", "none")
        else:
            self._preferred_compression = ("none",)

After this step however, if cnopts.compression is not provided, it defaults to ("none",), and _start_transport method assigns this back into the transport through security options. This resets paramiko's _preferred_compression to ("none",) and disables compression. This is effectively equivalent to self._transport.use_compression(compress=False).

It also makes the behavior inaccurate per cnopts docstring which states that the default cnopts.compression is paramiko.Transport.SecurityOptions.compression

            compression = self._cnopts.compression
            self._transport.get_security_options().compression = compression

The fix is to only set security options compression if cnopts compress is enabled and compression is not the default value ('none',)

            compression = self._cnopts.compression
            if bool(compress) and compression != ('none',):
                self._transport.get_security_options().compression = compression

---

Additionally, I have created one instance of security options so the object is not initialized multiple times for each option. This also keeps line width within pep8 recommendations

security_options = self._transport.get_security_options()

[byteskeptical]

Allowing the default compression algorithm list set by paramiko is fine and I see your point from a user's perspective of thinking something is enabled only to have it not be. I'm not sure about enabling the compression test in CI though. Also unsure about the benefit of th security_options line as everything is within the pep8 line width already and direct calling attributes of the object doesn't reinit it every time as far as I'm aware, though aesthetically I do think it may looks better.

[byteskeptical]

What server supporting compression have you tested this against? The default pytest-sftpserver the CI runs against doesn't support it.

[shashfrankenstien]

This test isn't performed against pytest-sftpserver, but the OS level local sftp server, which supports compression. I was able to successfully run the CI on all OS platforms

[byteskeptical]

Your right about the test not running against pytest-sftpserver in this case but OpenSSH doesn't have compression enabled by default and hasn't for quite some time now (7.4). As stated in my previous comment above paramiko relies on the underlying ssh connection to handle compression and the default server on all OS test platforms has it disabled. I think this test will require some work before it does something actually useful as right now it basically just checks connection option and transport values.

[byteskeptical]

A bit confused by this one. By default compression is disabled in paramiko and that sets self._preferred_compression = ("none",) as the cnopts documentation states and mirrors. Now I do think the documentation could be amended to include the need to set your preferred compression algorithm(s) if you enable compress. That being said I don't mind falling back to the default list like your doing here if self._cnopts.compression is not set. I also don't see why the compress lines above shouldn't be moved just above the compression = self._cnopts.compression line.

[shashfrankenstien]

I do think falling back to default does make sense here since paramiko itself sets the compression when we call self._transport.use_compression(compress=True). The change I've submitted retains this behavior in sftpretty. I definitely do agree that the compress boolean option should be moved just above the compression options. I will move it over

[byteskeptical]

I remember now why I had it seperated after re-reading the paramiko docs/code. Since paramiko doesn't handle the actual compression and relies on the underlying ssh connection this has to be set prior to the connect (start_client) call. Should still be okay in this case but just an FYI.

[byteskeptical]

Compression changes/ line moves look good. Lets see what happens with the test on actions. Will need you to add those secrets to your fork for all OS platforms to run correctly (Linux & Windows).

[byteskeptical]

I remember now why I had it seperated after re-reading the paramiko docs/code. Since paramiko doesn't handle the actual compression and relies on the underlying ssh connection this has to be set prior to the connect (start_client) call. Should still be okay in this case but just an FYI.

[byteskeptical]

Your right about the test not running against pytest-sftpserver in this case but OpenSSH doesn't have compression enabled by default and hasn't for quite some time now (7.4). As stated in my previous comment above paramiko relies on the underlying ssh connection to handle compression and the default server on all OS test platforms has it disabled. I think this test will require some work before it does something actually useful as right now it basically just checks connection option and transport values.