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