Bugfix 71519 - return serialNumberHex from openssl_x509_parse #1755
Conversation
Currently, openssl_x509_parse returns an integer. This can be unexpected, as the common way of handling serial numbers is with a hex string. This is compounded as php's dechex() function cannot handle >32 bit numbers which will leave people trying to handle large serial numbers frustrated. By adding this extra return variable to openssl_x509_parse, the consumer of the variable is certain that the serialNumberHex that is returned is the exact Hex Serial number as OpenSSL returns everywhere else.
This also updates the testcases so they now return correct data with the extra return value.
|
Thanks :) |
|
@krakjoe Please revert this. It would be better to handle it using flags. That function needs a review and better handling than duplicating values (one for hex and for dec). I plan to look at it at some point but this is not ideal solution. |
This appears to be valuable functionality to me, and you haven't convinced me otherwise, so I will not revert it. This pull request was open for nearly a year; that would seem like plenty of time to review it, and or improve it, or just mention anything about it. There is nothing stopping you from proposing that we change the prototype of the function to accept a mask of flags, but since that would have BC concerns that this avoids - assuming you would add a flag for short names - or would result in shortnames being a separate parameter from other flags and so creating a pretty nasty prototype, I don't think I would be in favour of that solution anyway. |
|
@bukka I disagree that flags are a better way there. If the function would return just a single value, sure … But it does return an array where one extra element is no harm. A flag is less obvious than an explicit hex in the key name. |
|
@krakjoe The reason why I haven't had time for this PR yet is that I was busy with other (possibly more important) work on this extension like adding support for OpenSSL 1.1, AEAD, queuing errors and some other stuff. But I'm hoping to do that before 7.2. If it's not the case, happy to keep this there. There would be no BC break if the a new parameter (3rd one) is added which would allow setting flag. That would not change the default but allow switch the representation to hex if set. It could also allow other things like extended extension format. @bwoebi Of course if it was just for that, then it wouldn't probably makes sense to add flags but there many other things where the flag would be useful (e.g. like extended format for cert extensions). So if we have a flag then it would be better to use it for changing format rather than duplicating fields (just for the number format...) IMO So how about this solution. Lets revert it just from 7.1 for now and keep it just in master so I have time to propose something else? |
Fair enough, but this PR is satisfactory, and adds value, you cannot ask me to revert it after a year of waiting, sorry. I've roped in someone to improve it a little, because it's not perfect, but the basic idea and functionality are valuable for us.
As mentioned, that will create a nasty prototype, because shortnames would not be a flag, for no obvious reason ... that doesn't seem very nice to me at all.
You are free to make whatever proposal you like, but I won't have peoples time wasted any more. You know it is extremely frustrating to put time into a feature or fix for it to be ignored, for years on end in some cases. This cannot happen any more, I won't let it. You are also aware that I respect your opinion very much on these things, and that had you given it already it may have stopped me, or provoked a conversation about it at the least. I hear everything you are saying, but, I would very much prefer your flags idea to usurp the functionality of the current additional parameter, it really doesn't make any sense to have a boolean parameter for one thing, and a mask of flags for everything else. It sounds to me like a (very nice) feature for a future version of PHP. Let us move forward already ;) |
|
Well it's all about the fact that this function wasn't designed well. The serialNumber should be hex in the first place and short names never should be boolean. The same case is for Ideally we would have an object for X509 with params but that's quite a big feature and would add lots of line of code to maintain (addressing various context and error related things) which is something that I would like to have in crypto ext but that will take me quite some time as there is so much other stuff that I would like to add... :) About this specific thing I don't feel so strongly to try to block it and call for RFC. I'm not too happy about the solution but the whole function is not ideal anyway and at least it will do the job for someone. |
* pull-request/2283: Fix memleaks from #1755 and some pre-existing ones
* PHP-7.0: Fix memleaks from #1755 and some pre-existing ones
* PHP-7.1: Fix memleaks from #1755 and some pre-existing ones
Currently, openssl_x509_parse only returns an integer as the Serial Number.
This can be unexpected, as the common way of handling serial numbers is by treating them as a hex string.
This is compounded by php's dechex() function being unable to handle >32 bit numbers which will leave people trying to handle large serial numbers frustrated.
By adding this extra return variable to openssl_x509_parse, the consumer of the variable is certain that the serialNumberHex that is returned is exactly the same Hex Serial number as OpenSSL returns everywhere else.