Matthew Fonda [2005-03-24 02:49 UTC] Looks alright to me.
Is there any way you could make this package not require PHP5? It doesnt seem like you necessarily need to use it. Perhaps just use PEAR_Error instead of exceptions. Also, PEAR and the GPL don't mix. Maybe consider using the PHP license.
Also, make sure everything is up to par with the PEAR coding standards. Private methods should still follow standards, so for example you would use _openSubprocess() instead of open_subprocess(). And on your if statements, make sure you use { and } even if it is just a one liner. Your class should also be named Crypt_GPG.
Other than that it looks pretty good.
Daniel Convissor [2005-03-24 16:44 UTC] This is an important and helpful package. Your approach is generally good, though needs some refinement.
Make the directory structure like this:
Crypt/GPG/package.xml
Crypt/GPG/GPG.php
Remove the README file. All of the information in there should be in the GPG.php file's docblocks.
Follow the coding standards. Notable problems in your code are:
* header comment blocks.
* use of brackets for functions and control structures.
* function docblock layout. See the Sample File in the coding standards.
* just because a method is private doesn't mean it doesn't need a docblock.
* spacing around operators (+, ., etc).
* method naming convention (for example "process_with_passphrase" should be "_process", "open_subprocess" should be "_openSubprocess", etc).
* put docblocks for the properties.
* use ' instead of " for strings that don't need evaluation.
* wrap lines at 80 columns.
* declare the $process property.
* use full open tags <?php, not short tags.
* nest with spaces, not tabs.
Define the properties to the data types they are:
$gpg_binary = '';
$pipes = array();
The listKeys() method could produce much more useful information. For example, make the key id the index for the array and make the data a sub-array of that, don't ignore sub-keys, provide uid's, etc. So, instead of returning this:
[0] => pub 1024D/8FFE1FFC 2002-12-23
return this:
[8FFE1FFC] => Array
[type] => pub
[size] => 1024D
[created] => 2002-12-23
[uid] => Array
[danielc@analysisandsolutions.com] => Daniel Convissor (Office)
In close_subprocess():
if ($this->process != null && is_resource($this->process)) {
can just be:
if (is_resource($this->process)) {
and set
$this->pipes = array();
instead of null.
There doesn't seem to be a compelling reason to make this package PHP 5 only, though the package does require 4.3.0 due to proc_open().
The package.xml file says the license is LGPL but the docblock says it's GPL. Use the LGPL @license tag from the Header Comment Block page of the coding standards.
I may have more feedback later. This is an important package. Please don't rush it to vote.
Daniel Convissor [2005-03-24 22:56 UTC] The encrypt() method should have an optional $armor parameter:
encrypt($data, $username, $armor = true)
If it is true, the encrypted data would be ASCII, but if false the output would be binary.
Perhaps the encrypt() method should be renamed encryptString().
Similarly, I'd merge the sign() and clearsign() methods into this:
signString($data, $passphrase, $clear = true)
Then, adding the following methods would be great:
encryptFile($file, $username, $armor = true)
decryptFile($file, $passphrase)
signFile($file, $passphrase, $clear = true)
It's not clear to me what to do with the verify() method. Here too, it might be good to produce verifyString() and verifyFile() or something along those lines.
Daniel Convissor [2005-04-13 20:12 UTC] Nathan, I hope you haven't given up on this package/proposal.
Nathan Fredrickson [2005-05-27 19:23 UTC] We at silverorange have been working on bringing this package up to PEAR standards so it will be accepted into PEAR.
Thank you Matthew and Daniel for your constructive comments.
The many changes are listed in the changelog of the second release on the main proposal page.
The package still requires PHP5. We don't have a need to make the package use PHP4 and we would not test or use it with PHP4. As well PHP5 exception handling is quite nice.
We did not add file methods to Crypt_GPG. We see how this could be useful, but ideally this would be implemented by someone who needs such a feature. We are unsure the best way to implement this functionality (eg, whether to pass filenames or data to the GPG subprocess).
Klaus Guenther [2005-05-27 21:57 UTC] Other packages have separate string and file methods. Basically, the file method reads the file into a string and then uses the parseString (or whatever you call it) method to do what is necessary. Then you could have toString and toFile methods or something like that. Just some suggestions. This package will be useful at any rate.
Daniel Convissor [2005-05-27 23:18 UTC] Thanks for improving the package. I look forward to reviewing your changes, but can't do so at this moment. Please don't rush it to a vote.
Nikolas Coukouma [2005-05-27 23:19 UTC] Re: "The package still requires PHP5. We don't have a need to make the package use PHP4 and we would not test or use it with PHP4."
This alone doesn't seem like a good reason to me. Refraining from using PHP 5 features isn't terribly difficult. You should write unit tests anyway, so all you need to do is run them under PHP 4 to make sure everything works there and you didn't accidentally use something from PHP 5. Releasing a beta version and getting PHP 4 users to test it should be good enough for QA before releasing it as stable.
The better and more debatable justification is "PHP 5 is maturing, and we expect that most new scripts will use it be the time we make a stable PEAR release."
Still, that doesn't consider the people with existing PHP 4 scripts and would like to use encryption. I hope that this is a pretty common occurance because signing and encryption are features I like to see. For example, forums, blogging software, etc. could have some Javascript encrypt a form field with a public key and decrypt it with a private one. Or perhaps they could let users upload a public key and sign their posts with a private one.
Nathan Fredrickson [2005-05-28 01:32 UTC] Re: 'The better and more debatable justification is "PHP 5 is maturing, and we expect that most new scripts will use it be the time we make a stable PEAR release."'
Yes, PHP 5 is maturing; our company has just completed migrating exisitng sites to PHP 5 and we're doing all new development work in PHP 5. The PEAR community may want to / have to migrate more slowly and that's fine. We wrote this package to scratch our own itch and if it is useful to others that's great. The feedback has been excellent and the second release is far better than the first. However error handling is important when running external binaries and we're not really interested in stripping out the nice exception handling to make this package work in PHP 4. Of course somebody else is free to back port it to PHP4...
Daniel Convissor [2005-06-17 21:08 UTC] Excellent job!
While "import" is the name of the command line switch for importing keys, please consider a more informative name for the import() method, like importKey().
You have getFingerprint() return a solid string (F15AB7E29D356C64CEB2C00FE8142F028FFE1FFC). But, fingerprints are generally displayed to be human readable (F15A B7E2 9D35 6C64 CEB2 C00F E814 2F02 8FFE 1FFC). I see you're running str_replace() to strip out the spaces. I'd suggest there be a second parameter to the method allowing users to choose which they want and have the spaces left in by default. OH, also, you need to run trim() on the result. It comes back on my system with a trailing LF.
Please come up with file methods. Perhaps it can be as simple as adding the file name as an element of $args:
public function encryptFile($file, $username, $armor = true)
{
$args = array('--recipient "' . $username . '"', '--encrypt');
if ($armor) {
$args[] = '--armor';
}
$args[] = $file;
// ... snip ...
}
When using the test script with my own existing key, things worked fine in the encrypt() call, but the call to decrypt() threw the following error:
gpg: failed to translate osfhandle 00000004
Dude, putting a call to deleteSecretKey() inside a publicly distributed test script is dangerous. If you want to keep it there, you MUST place a VERY bold warning above the $key_id = '' on line 85. For example, I just zoomed ahead and set it to my key id and POOF went my secret key. Fortunately, I have backups. :)
In several of the docblocks, I see reference to a file named "doc/DETAILS." That directory doesn't exist in the tarball. Perhaps you forgot to include it in the package.xml file. REGARDLESS, that information should really be in the docblocks. For example, the docblock for listKeys() should explain the format of the arrays/objects returned.
SO... What IS that array key? I'm used to seeing things like "8FFE1FFC" but that's returning "E8142F028FFE1FFC". Also, make the key a property of the object as well.
In listKeys() you use $key->creation_date but in the Crypt_GPG_Key class you defined $create_date.
There are a few nit-picky coding standards things...
Put the license stuff into page-level docblock instead of a separate block comment above it.
Use docblocks, not just block comments, above the include/require statements.
Have the short descriptions in the page-level docblocks truly reflect the contents of the given file rather than just using the same description for each file.
Daniel Convissor [2005-06-18 02:57 UTC] Oh, yes. It seems best if the exception classes extended PEAR_Exception instead of plain Exception.
Philippe Jausions [2006-04-20 05:06 UTC] Is this proposal still alive? I hope it will make it as a package...
Cipriano Groenendal [2006-04-30 17:50 UTC] Once this package gets released, I'd like to use it to implement GPG signed Emails via Mail_Mime. Havn't had the chance to look at code much, but I'd like to see this proposal finished :)
James Carr [2006-05-03 15:30 UTC] Whatever happened to this? I really think it would be great to get several Encryption based PEAR packages out there, but this seems to have died a year ago?
James Carr [2006-05-03 15:31 UTC] sorry, I thought the last comment was last year.
Keep up the good work. ;)
Christian Weiske [2007-11-20 06:19 UTC] Great job you did.
Christian Weiske [2007-11-20 06:20 UTC] What I would like to see is an example how to use your package with Mail_Mime to send signed/crypted messages.
Arnaud Limbourg [2007-11-26 21:57 UTC] Nice !
What do you need to call for votes ?
|
