SPKAC functionality per feature request #38917 #21
Conversation
|
This needs some tests. The tests attached does not really test anything except function existence. It should actually test the functions doing what they are supposed to do. Also, it would be nice if whole code structure was not built around goto's. |
|
Perhaps you could elaborate on the tests which do nothing more then test for the function existing? If you review the test cases again you will see that after each function is shown to exist it will halt script execution if the desired value is not what the various test cases expect therefore causing it to fail. As well; propose a consistent return value for each of the five implemented functions and I will move away from GOTO's which (IMO) are the best way to go in these cases. |
|
I would use a multli-exit loop instead of goto, which is usually a much cleaner strategy. "A Case for Teaching Multi-exit Loops to Beginning Programmers" would be useful to read. Basically, do something like: while(true) {
if(..) break;
if(...) break;
return ...;
}
// clean up code goes hereReplace all goto's with break and put the entire chunk of code in a loop of your choice (for, while, etc.). |
|
@jas- sorry, I was wrong on the test. If doesn't use the usual way phpt tests work - by outputting the data and comparing it to test output, which is the recommended way since if something goes wrong you can see what exactly was expected and returned without having to parse the code. I'll review the test again this evening and see if anything is missing. |
|
A quick glance shows a few problems:
|
|
Comment on behalf of yohgaki at php.net: Could you use the "CODING STANDARD"? Indent should be TAG, not spaces. |
|
Modifications have been made per the comments. First patch so should I ask for a discussion on internals? It has been tested against OpenSSL libraries 0.9.8, 1.0.0, 1.0.1 and Fips versions |
|
I would drop a note to internals, yes. |
| RETURN_NULL(); | ||
| } | ||
|
|
||
| s = emalloc(strlen(spkac) + strlen(spkstr) + 1); |
There was a problem hiding this comment.
Looks like this one definitely leaked on errors, and probably also leaked on normal return.
There was a problem hiding this comment.
Hi:
On Tue, Apr 3, 2012 at 3:39 PM, Stanislav Malyshev
[email protected]
wrote:
- RETURN_NULL();
- }
- } else if (strcmp(algo, "sha512")==0){
- if (!NETSCAPE_SPKI_sign(spki, pkey, EVP_sha512())) {
- php_error_docref(NULL TSRMLS_CC, E_WARNING, "unable to sign with sha512 algorithm");
should we make an RFC, that the error message should be in ucfirst
form, or lower case ?
I see there are both ucfirst warnig message and lowercase messages
in php-src.
thanks
- RETURN_NULL();
- }
- }
+- spkstr = NETSCAPE_SPKI_b64_encode(spki);
- if (!spkstr){
- php_error_docref(NULL TSRMLS_CC, E_WARNING, "unable encode SPKAC");
- RETURN_NULL();
- }
+- s = emalloc(strlen(spkac) + strlen(spkstr) + 1);
Looks like this one definitely leaked on errors, and probably also leaked on normal return.
Reply to this email directly or view it on GitHub:
https://github.com/php/php-src/pull/21/files#r637678Git Pull Requests Mailing List (https://github.com/php)
To unsubscribe, visit: http://www.php.net/unsub.php
Laruence Xinchen Hui
http://www.laruence.com/
There was a problem hiding this comment.
I used all lc due to additional functions in etc/openssl/openssl.c using all lc for warnings
|
Looks like in the patch multiple structures are allocated but never freed on error conditions, and some buffers also are not deallocated in normal return too. Please fix. Also, in the tests, in PHP sources it is usually custom to print the output and compare it to template in EXPECT results, not compare it in the code. So instead of:
You would do just:
and then put "sample_challenge_string" in result. This would allow if it fails to see not only the expected but the actual result and thus more easily be able to see what went wrong. Of course, when it is not possible code checks are still OK. |
|
Please use constants for the hash methods (md5&co). See https://bugs.php.net/bug.php?id=61421 for some of the new contants |
|
Wouldn't use of the newer constants for sha256, sha512 etc break compatibility with 5.3? Or should I be patching for the 5.4 branch? At the moment I have been and am working on the 5.3 branch as per https://wiki.php.net/vcs/gitworkflow If I were to manually patch without the referenced patch I feel it might mess up the workflow preservation. Also, I feel like I should close this patch request due to problems with the latest pull request due to incorrect indentation settings within my editor that I was unable to revert. Suggestions? |
…thms to requested algorithm constants (does not contain patch specified @ https://bugs.php.net/bug.php?id=61421 due to it being upstream)
…thms to requested algorithm constants (does not contain patch specified @ https://bugs.php.net/bug.php?id=61421 due to it being upstream)
|
@jas- not really, as this patch can only make it to master. 5.3 and 5.4 are not open to feature additions |
|
@jas- no worry btw, php-next come in a year or so :) |
|
@pierrejoye I plan on resubmitting to master but the last push (because of my editors indenting problems) added unnecessary bits. I am also going to remove the openssl_spki_details() function as it's not needed. |
ORIGINAL BUG:
https://bugs.php.net/bug.php?id=38917
FEATURES:
INSTALLATION:
Once it is installed you can use either test case provided to test. The CLI
version might be easier for immediate testing of applied patch.
USAGE EXAMPLES:
Here is a complete list of the functions this patch implements as well as
usage examples of how ot use them.
Creating new SPKAC's
Creating a new SPKAC with defaults (sha256 signature)
Returns SPKAC string
Creating a new SPKAC using MD5 signature
Returns SPKAC string
Creating new SPKAC using sha1 signature
Returns SPKAC string
Creating new SPKAC using sha512 signature
Returns SPKAC string
Verification
You can verify an existing SPKAC (possibly one generated from the HTML5
KeyGen element)
Verifying an existing SPKAC
Returns boolean true/false value
Extracting from SPKAC
You may wish use the SPKAC for more then just generating certificate
signing requests. The next two functions will allow you retrieve the
formatted public key as well as the associated challenge from the SPKAC.
Extracting the challenge
Returns challenge string
Extracting the public key
Returns a formatted string containing the public key
SPKAC details
This next function may be unnecessary but will provide a formatted copy of the
details of the SPKAC (the signature algorithm, the associated challenge string,
the public key modulus etc.)
Providing details of SPKAC
EXAMPLE OUTPUT
Here are the output examples if you would like to know what type of information
you can retrieve from a signed public key and challenge.
A signed public key and challenge string
Extracting the associated public key
Providing details of the SPKAC