tlhackque
commented
7 years ago I'm sorry for the slow reply - I didn't see this until now. Issues with Crypt::PKCS10 are supposed to go to the main repository -- this is my staging repo & I don't get notifications.
There are three issues in your note.
The default for VerifySignature - If you don't want to verify the signature (because you know it's been done once), set VerifySignature to 0 in your constructor. This is not the expected case.
The usual case is to call the constructor once & access it multiple times. Verifying the signature (once) helps ensure that if the CSR was valid when signed, any corruption in transit will be detected. This has been an issue in the past. In addition, there are security issues with processing an unverified CSR.
The constructor is expensive, in that it has to setup the ASN1 parse & decode the fields of the CSR. The extra cost of verifying the signature is relatively small. If you are repeatedly calling the constructor for the same CSR, you should consider reorganizing your code to call the constructor only once.
For those reasons, I still think that VerifySignature => 1 is the correct default.
I agree that this default is a potentially incompatible change, but only if you think it's OK to process a CSR with an invalid signature ! The real problem that you encountered is with Crypt::PKCS10's dependencies, which is the next item.
The issue with dependencies: There is a problem with the way dependencies are specified, which I was working on when the CPAN test system went down but haven't gotten back to. The problem is that Crypt::PKCS10 requires at least one module to verify signatures - but I don't want to require all of them, as many users only use one key type. (E.g. Just RSA is most common; RSA + ECDSA is being used more, and relatively few people use DSA.) There is no way to specify "at least one of" in the metadata for a package. I also can't pick just one - RSA would be the obvious choice, but there are sites that only use one of the others. The "recommends" dependency is not honored by default by the installers (cpan, cpanminus). At present, this means that Crypt::PKCS10 will install with no (or outdated) dependencies.
Thus, there are some issues with the automated test system. I probably also need to make interactive installs smarter....
For now, the best option is for you to require the "recommended" modules of Crypt::PKCS10. I'll try to do something better sometime soon.
Finally, checking for undef from the constructor is required, and Crypt::PKCS10->error returns the eval error, as shown in the documentation.
my $decoded = Crypt::PKCS10->new( $csr ) or die Crypt::PKCS10->error;For the case that you found, the error would be something like Unable to load Crypt::OpenSSL::RSA.
"warn" on error is redundant & usually causes applications to trap the warning and turn it into something that the application likes. E.g. webservers will direct warnings to the server error log, but applications want to send them to the user (or an application error log). So if I were to warn, I would expect the warning to be trapped & redirected. But it's already available from Crypt::PKCS10->error...
In addition, eval doesn't trap warn, so you have to do a local $SIG{__WARN__} = ... as well. And the other API calls can also return errors - so to be consistent, they'd have to warn() too. This doesn't seem to be the right approach.
I could add a "dieOnError" parameter to the constructor & make all the API calls that return errors die() if it is specified. This would provide exception style error reporting for the whole API, for those who prefer that style. Would this meet your needs?
I want to suggest to remove this default setting.
Reasons:
I like the extra functionality but I also like to be backwards compatible ;)
It would also be nice to echo the eval error using "warn" for API version > 0 in line 411.