On Fri, 10 Oct 2014, Patrick Monnerat wrote:
1) QsoSSL is obsolete. I propose to remove it completely from the code. Runs only on AS/400 where GSKit is always available and much better in many senses. In addition, it's hard to make it evolve. Any objection ?
Not at all. Rip it out!
2) In Curl_ssl_random(), I propose to return -1 if curlssl_random is not defined.
I didn't provide a fallback for curlssl_random not being defined simply to make sure we discover the affected backends clearly and make each backend make an active choice on how to provide it.
Right now I believe the PolarSSL backend is the only one that sets a define that kind of "ducks" for the problem:
#define curlssl_random(x,y,z) (x=x, y=y, z=z, CURLE_NOT_BUILT_IN)
3) I can understand the backend-specific md5sum function (i.e.: for speed purpose), but we already have our own implementation and it would be wise to use it, at least when have_curlssl_md5sum is not defined.
Oh right. That would indeed be a sensible default alternative if the TLS/crypto library doesn't have one provided.
In addition, ifdefs on have_curlssl_md5sum can be replaced by ifdefs on curlssl_md5sum. Your opinion ?
I agree, as they're tightly associated already.
4) I would like to split the backend-specific pkp_pin_peer_pubkey() (of openssl and gtls) into a backend-specific part that gets the key from the certificate and a generic part cooking the pinned public key and comparing: this would avoid repeating the PPK cooking in each backend. Any objection ?
Sounds wise. I think we should view the current pinning code as a first-shot and a good entry, but I'm sure we can polish it up going forward to get even better.
5) The misunderstanding I make 2 days ago about PPK inspired me the following possible improvement. If the PPK file does not exist, the curl_easy_setopt() string is checked for being a PEM public key (direct data). In addition, the file data is checked for PEM format. Else it behaves has today (DER).
I can see how it can try to detect the file format, as I believe DER and PEM are quite different, but is it really wise to offer direct data using the same option? I'm just scared that there's a risk that if you provide wrong file name or just remove the file name, libcurl will then treat that as a key and that could lead to some untended consequences.
I think I would favour a separate option instead. Or am I being silly?
Of course, all these changes can be delayed after the feature freeze...
I don't think we have to wait. These all sounds like fixes to me, and when it comes to the pinning code I think it is a better time to adjust things now if we should, before we ship it in a public release the first time.
We just need to be a bit careful when we change how the TLS backends work etc since they are many and not all of them are built and tested very frequently.
-- / daniel.haxx.se ------------------------------------------------------------------- List admin: http://cool.haxx.se/list/listinfo/curl-library Etiquette: http://curl.haxx.se/mail/etiquette.html
