Skip to content
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

Update kex classes to have a proper hierarchy/docs/basic safety checks #1452

Open
bitprophet opened this issue Jun 8, 2019 · 0 comments
Open
Labels
Milestone

Comments

@bitprophet
Copy link
Member

Followup from #1384 - a bunch of time was wasted troubleshooting what should have been an obvious documented fact: that kex 'engines' must exhibit a hash_algo attribute or post-kex messages will implicitly get hashed with SHA1 (inside Packetizer).

This can cause SHA256-hash-based algorithm families to yield Bad packet length errors from an sshd (and in Paramiko's end, usually auth errors [until #387?] because of its awful exception trapping) if that attribute is not actually used when implementing the kex class.

It'd be nice to make a basic KexEngine that documents this and other assumptions made by either Transport or Packetizer, and then move the extant kex classes to subclass it.

Further, we should IMHO change the behavior of the current fallback to except instead - I don't think we accept arbitrary user objects here, which means we should be able to ensure all of our kex classes have an explicit hash_algo set, with the old ones using SHA1, and this will be backwards compatible for most users not doing deep hacks.

Finally, it'd be nice for us to catch this in the test suite too, but right now the kex tests (if not most others) use a very stripped down dummy Transport class, so most of Transport and all of Packetizer get totally skipped, which would otherwise have surfaced this problem. I suspect that's orthogonal but it may be worth poking at when doing this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

1 participant