Add support to sslcafile and sslcapath to cURL adapter

[mlocati]

Socket adapter supports the options sslcafile and sslcapath.

What about supporting them in the cURL Adapter too?

[ezimuel]

I commented only about a potential improvement checking for CURLOPT_SSL_VERIFYPEER. See the details. Apart from that the PR looks great!
Please remember to add the unit test and send the PR against develop. Thanks!

[ezimuel]

I'm thinking that we should check if CURLOPT_SSL_VERIFYPEER is false and in this case throw an Exception if we set sslcafile or sslcapath. What do you think?

[mlocati]

Any reason to do that? I would do the opposite: avoid setting the CURLOPT_CAINFO and CURLOPT_CAPATH curl options if CURLOPT_SSL_VERIFYPEER is false (that because I don't know how the underlying libcurl behaves in that case...)

[ezimuel]

The idea is to prevent setting CA if CURLOPT_SSL_VERIFYPEER is false. Basically we should allow setting CA only if CURLOPT_SSL_VERIFYPEER is true.
Reading here from the CURL manual, we can use CURLOPT_CAINFO and CURLOPT_CAPATH only if CURLOPT_SSL_VERIFYPEER is true.

[mlocati]

That's exactly what I meant.
Instead of throwing an exception (like you suggested), I'd avoid setting CURLOPT_CAINFO and CURLOPT_CAPATH if CURLOPT_SSL_VERIFYPEER is false

[ezimuel]

I think it's better throwing an exception because we can prevent a potential security issue.
Let me explain, if CURLOPT_SSL_VERIFYPEER is false and someone set the CURLOPT_CAINFO/CAPATH values and we skip these settings, we will have successfully CURL calls without any evidence about the usage of CA. People can assume the usage of CA because of the successfully calls but the CA will be bypassed due to CURLOPT_SSL_VERIFYPEER .
Throwing an exception we can inform people about the correct usage of CA.

[mlocati]

What about this alternative:

• if sslverifypeer is not set and CURLOPT_CAINFO/CAPATH are set, we assume that sslverifypeer is true
• if sslverifypeer is manually set to false, we don't consider CURLOPT_CAINFO/CAPATH

[ezimuel]

CURLOPT_SSL_VERIFYPEER is true by default, so if sslverifypeer is not set it's not a problem for CA. The issue comes if sslverifypeer is set to false and we set CA. In this case I think we should throw an Exception, instead of a silent action as you proposed. This is a clear misuse of the component,with potential security implications.
@mlocati why you are against using an Exception for this case?

[mlocati]

@mlocati why you are against using an Exception for this case?

@ezimuel Because I love to keep everything as simple as possible: if someone sets sslverifypeer to false, he excepts that CURLOPT_SSL_VERIFYPEER is set to false, for any value of the other parameters

[ezimuel]

@mlocati this is not the point, I don't want to change the behaviour of any settings. I would like to prevent wrong settings. Anyway, reading the CURLOPT_SSL_VERIFYPEER specification it seems that it works already as is. If it's false, the CA settings are not used anyway.
I think we can just leave the settings as you proposed without any extra check and exceptions. We should only remember to document this behaviour in the documentation.

[mlocati]

Please remember to add the unit test

I'm going to do that.

and send the PR against develop

Done.

Thanks!

Thanks to you! 😉

[ezimuel]

@mlocati I added the unit test and the documentation to the PR. Thanks for your contribution!

[mlocati]

@mlocati I added the unit test

@ezimuel Thanks! I wanted to test these configuration keys with some real use case, but I've been very busy lately.