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

Workaround for slow sftp put/get on Windows. #102

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
3 participants
@warthog618
Contributor

warthog618 commented Nov 4, 2012

This patch improves performance of sftp put/gets for Windows clients using *-ctr ciphers.

Both the approach and the code need a review before being merged.

@bitprophet

This comment has been minimized.

Member

bitprophet commented Feb 3, 2013

Note to readers: see #101 for background.

@warthog618: thanks for submitting this! I agree that the approach needs a 2nd opinion; while your (thorough, thanks again for that) explanation makes sense to me, I'm afraid my background in the low level crypto aspects is weak. If we can get another contributor to +1 the change (ideally without just saying "+1" and nothing else...) that'll be good enough for me.

Non-crypto comments/concerns:

  • Given the symptom addressed only manifests on Windows platforms, it might be worthwhile to limit this modification to just that platform. (Other parts of the codebase reference sys.platform to accomplish this.)
  • While it's unlikely that much non-paramiko code is using the Packetizer class, it'd be more backwards compatible to set a default value for the new sdctr kwarg, e.g. sdctr=False, so that the method in question can be used with the previous signature.
@mvanderkolff

This comment has been minimized.

Contributor

mvanderkolff commented Feb 3, 2013

Yes, CTR mode means that in effect we make a stream cipher, that is, a cryptographically secure PRNG, from the block cipher algorithm. Since that's the case, each byte tells you nothing about the preceding one (assuming your block cipher meets certain assumptions). So this should be safe, as the RFC states.

@warthog618

This comment has been minimized.

Contributor

warthog618 commented Feb 5, 2013

Non-crypto comments/concerns:

  • Given the symptom addressed only manifests on Windows platforms, it
    might be worthwhile to limit this modification to just that platform.
    (Other parts of the codebase reference sys.platform to accomplish
    this.)

My preference is to not introduce platform specific code unless it is
absolutely required.

The optimisation happens to workaround the performance problem on Windows,
but if it is valid generally then why limit it to Windows?

  • While it's unlikely that much non-paramiko code is using the
    Packetizer class, it'd be more backwards compatible to set a default value
    for the new sdctr kwarg, e.g. sdctr=False, so that the method in
    question can be used with the previous signature.

That is totally reasonable. I must've been assuming the Packetizer was
private to paramiko.

@bitprophet

This comment has been minimized.

Member

bitprophet commented Feb 28, 2013

I guess that's a fair point (re: platform), I just tend to be very conservative about lower level changes (which have a higher chance of causing unforeseen problems), even moreso in Paramiko because I inherited it and didn't write it. But it's true that this is pretty generic and was "Windows specific" only in the manifestation of symptoms.

Thanks a lot for the +1, @mvanderkolff -- appreciated.

bitprophet added a commit that referenced this pull request Feb 28, 2013

bitprophet added a commit that referenced this pull request Feb 28, 2013

@bitprophet

This comment has been minimized.

Member

bitprophet commented Feb 28, 2013

All done. Did have to give that default value to ensure the tests ran, so all good there too.

@bitprophet bitprophet closed this Feb 28, 2013

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment