Skip to content
This repository was archived by the owner on Jan 30, 2020. It is now read-only.

Add support to sslcafile and sslcapath to cURL adapter #97

Merged
merged 2 commits into from
Jan 24, 2017
Merged

Add support to sslcafile and sslcapath to cURL adapter #97

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
c620830
Add support to sslcafile and sslcapath to cURL adapter
mlocati Dec 2, 2016
5194999
Add sslcafile and sslcapath to default options
mlocati Dec 2, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion src/Client.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -110,7 +110,9 @@ class Client implements Stdlib\DispatchableInterface
'outputstream' => false,
'encodecookies' => true,
'argseparator' => null,
'rfc3986strict' => false
'rfc3986strict' => false,
'sslcafile' => null,
'sslcapath' => null,
];

/**
Expand Down
7 changes: 7 additions & 0 deletions src/Client/Adapter/Curl.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -209,6 +209,13 @@ public function connect($host, $port = 80, $secure = false)
}
}

if (isset($this->config['sslcafile']) && $this->config['sslcafile']) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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...)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

curl_setopt($this->curl, CURLOPT_CAINFO, $this->config['sslcafile']);
}
if (isset($this->config['sslcapath']) && $this->config['sslcapath']) {
curl_setopt($this->curl, CURLOPT_CAPATH, $this->config['sslcapath']);
}

if (isset($this->config['maxredirects'])) {
// Set Max redirects
curl_setopt($this->curl, CURLOPT_MAXREDIRS, $this->config['maxredirects']);
Expand Down