[Work in progress] ECDH for SSH #116
Conversation
| * | ||
| * @var Math_BigInteger | ||
| */ | ||
| protected $x; |
|
Looks impressive! A few organizational thoughts. I think it'd be better if ECDH were in the Crypt directory and not the Math one. I mean, Crypt_RSA, in theory, could be in the Math directory, too, but I think the Crypt directory is a better fit. Also, cosmetically, I think it'd just look better as ECDH, all caps, instead of EcDH. idk that's just me. I think some of the helper classes would be better moved to ECDH subdirectories unless there's a (realistic) chance they can and would be re-used by third parties. A few comments on the CS.. You're using tabs to indent instead of spaces, as per the PEAR CS. petrich commented on the PHP5 stuff. And as a matter of personal preference, I really don't like public / protected / private anyway. This essay sums up my thoughts: http://aperiplus.sourceforge.net/visibility.php
In addition to that article, sometimes, as stop gap solutions for more permanent fixes, I'll tell people to make use of methods that'd normally be private (and are denoted as such in the phpdoc). eg. http://www.frostjedi.com/phpbb3/viewtopic.php?p=120079#p120079 I don't pretend to be the biggest phpdoc expert but I think phpdoc wants a function name and optionally a function description? And I think if we're going to do @return null we ought to do it consistently. I'll try to find out the phpDoc people think on the matter at some point. I realize the unit tests that have already been committed violate phpseclib's / PEAR's CS but, in my mind, they're a little exempt, anyway, since they're not included with the release zips. Really, I see them as a separate project that just happens to be in the phpseclib repo. Finally, could you make examples demo'ing how to use it? Thanks! |
|
I am aware of coding style and PHP4 issues. However, the next thing to do in my opinion is to speed up the point multiplication implementation, because that seems to be quite suboptimal. At least when connecting to OpenSSH, the first curve seems to be slower than DH. This could also be caused by the stronger hash function (sha2 instead of sha1) or be just a better security tradeoff, but I believe there is quite some room left for improvement. My original idea was to have this as a PHP5.2/3-only feature in order to encourage people to upgrade their php version. But now it seems like little PHP 5 features are used and stuff can probably easily be made PHP4 compatible. I do not claim to be PHP4 compatible, so please don't review that aspect for now. I extracted some commits that are ready for merge into #117
I agree.
I actually hated reading all the abbreviations in filenames used by other projects, how about EllipticCurveDiffieHellman?
Helper classes and filenames need some more thinking, in my opinion. Helper classes might need some redesign when other EC stuff enters the game, e.g. ECDSA which shouldn't be too far away from here.
Not sure what you mean with "function name". Most of the doc blocks are incomplete and need to be expanded.
Let's not try to change all function/method documentation at once, but file-by-file instead. In PHP, a function that has no return statement returns
Yeah, having tests with broken coding style is certainly better than having no tests. I probably wouldn't enforce coding style in tests.
What exactly are you interested in? If your SSH server supports ECDH, ECDH will be used for the key exchange. Other than that, example code explaining how to use the classes should be added to them. This is something still left to do. |
EllipticCurve or EllpiticalCurve? It's kinda ironic, actually... if I was doing diffiehellman I'd probably name the class Crypt_DiffieHellman. From there Crypt_EllipticalCurveDiffieHellman or Crypt_ECDiffieHellman would naturally follow, even though on it's own I'd, personally, probably prefer ECDH lol. Speaking about DiffieHellman, what about a DiffieHellman class as well while you're at it? SSH2.php should still, I think, have it's own independent DH implementation. Like right now there's Crypt/RSA.php and Net/SSH2.php will utilize it if you're using public key authentication but for server signature verification it uses it's own highly slimmed down implementation. The idea being to reduce the number of files you're needing to include. ie. one of the big criticisms people seem to have against phpseclib is that it's big and a lot of projects don't want to include a library that's almost as big as their project itself to support a feature that might only be utilized by 1% of users. I suppose in the case of Crypt/RSA.php you probably ought to still include it to support both passwords and RSA public keys but still... I think dependency minimization is to be encouraged. Another example: if you download Net/SSH2.php from the PEAR channel Crypt/RSA.php isn't required so it's not included. You have to separately download it. Because in my mind each of the main end-user facing classes should be seen as a separate project that all just happens to be in the main repo. Like Crypt/RSA.php is a project distinct from Net/SSH2.php, even though Net/SSH2.php does use it for one thing. Really, I see phpseclib as being the PEAR of cryptographic functionality. And when you're using PEAR you don't include all of it when all you want to use is Text_Diff - you just include Text_Diff. I don't know if this would even make sense with ECDH. Like to have RSA support built into SSH2.php for host key verification it adds all of 18 lines (some of which are just new lines and comments). Utilizing Crypt/RSA.php would utilize 11 lines per the commented out code. If it took 50,000 lines it'd be a different story but it doesn't.
For that one I would like a Crypt/DSA.php class. People ask about it often enough with SSH2. And with that one it should fully support all the standard key formats too. Like for DSA, it should support the PuTTY format and the default OpenSSL format. And the DSA parameters too. Like there was a PEAR proposal for a Crypt/DSA.php a while ago: http://pear.php.net/pepr/pepr-proposal-show.php?id=467 But that proposal had the same pitfalls as PEAR's Crypt/RSA.php. I don't want a phpseclib implementation of Crypt/DSA.php / ECDSA.php to have those same pitfalls. I don't want it to be a half-assed implementation with the just the barest minimum done. I think it should be a fully featured implementation. And both DSA / ECDSA should work with X509.php too. DSA isn't often used with X.509 but X.509 supports it none-the-less. And ECDSA... five of Firefox's root CA certs use ecdsa-with-SHA384 (OID 1.2.840.10045.4.3.3) http://www.frostjedi.com/phpbb3/viewtopic.php?p=140358#p140358 ECDH / DH don't have keys so it seems like fully featured implementations of those would be less complicated. |
ghost
assigned * master: (54 commits) ANSI: </underline> -> </u> SFTP: CS adjustment BigInteger: refactor random number generation code somewhat X509: re-use existing BER extraction code RSA: fix error when exponent isn't co-prime to lcm X509: load Math/BigInteger for ASN1 description SFTP: Fix syntax error "unexpected T_SL, expecting ',' or ';'". X509: $csr -> $spkac in loadSPKAC() function X509: change return value for validateSignature when cert isn't loaded SFTP: use variable sizes for get() SSH2: Logging updates SSH2: more window size adjustments SSH2: don't remove first byte from complex logs SSH2: attribute newly added comment SSH2: include string length in window size adjustments RSA: remove extra new line X509: adjust case of id-at-stateorprovinceName SSH2: channel handling adjustments SFTP: Add optimization for put() with NET_SFTP_LOCAL_FILE Use substr() instead of _string_shift(). SSH2: attempt to handle case where window size is smaller than packet size ...
|
"Breaks PHP 4 compatibility" Really? Who cares? Tag a new version and tell PHP 4 users to use x version or below. Semantic versioning FTW! You have a good library, I've used it myself, but holding in to PHP 4 compatibility and not embracing modern PHP is really holding back adoption. People just look elsewhere. |
|
i think what is holding back more wide spread adoption is that this lib is a few 100kb big. dumping in a third party lib that doubles the size of your project may not be that desirable. not that there's any real alternative short of just not supporting ssh / sftp in your project. |
|
There is the phpseclib-php5 repo. Although there are discussions of maybe having a new php5 branch in the phpseclib repo too. Feel free to participate: phpseclib/phpseclib-php5#1 |
|
Closing for now as this is outdated. |
Works, but partially suboptimal.