Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CRM-11160 - Set CURLOPT_SSL_VERIFYHOST based the system's general "verifySSL" option #1098

Merged
merged 1 commit into from Jul 4, 2013

Conversation

@totten
Copy link
Member

@totten totten commented Jul 3, 2013

We currently set Curl's VERIFYPEER option based on the CiviCRM setting, "verifySSL." This patch carries that forward to also set the VERIFYHOST option.

Per email discussion, it would be great if @deepak-srivastava and @eileenmcnaughton could test a couple of the payment processors that are affected by this.


@totten
Copy link
Member Author

@totten totten commented Jul 3, 2013

Note: In manual testing, I didn't do a full setup with each of the payment processors. Instead, I copied most of the curl_*() statements and checked to see which SSL errors arose under which conditions. (The results were as expected.)

For testing a valid SSL URL, 'https://civicrm.org/README.txt' was OK; for testing an invalid SSL URL, 'https://www-prod.civicrm.osuosl.org/README.txt' was interesting because the cert was valid but its hostname was mismatched.

@mc0e
Copy link

@mc0e mc0e commented Jul 3, 2013

I think we also need a warning to the site administrator if the SSL settings are not secure. There are good reasons why you might want to disregard the SSL issues during testing, but without a warning it's easy to forget about this when going to production, and also it's likely that most admins will not perceive a problem unless something is failing or telling them there's a problem.

@xurizaemon
Copy link
Member

@xurizaemon xurizaemon commented Jul 3, 2013

Is there a reason we'd ever want to set CURLOPT_SSL_VERIFYHOST and CURLOPT_SSL_VERIFYPEER to FALSE for these payment processors? Unless there's a need to test a payment processor which serves with invalid SSL, I say we leave these locked in a sensible config. That should eliminate the situation @mc0e suggested above too?

In my experience most test processors either spring for valid SSL or test on plain HTTP.

https://github.com/totten/civicrm-core/blob/17c04b52f3d0079795426a5e7a3444b92a3dedbc/CRM/Core/Payment/PayPalImpl.php and https://github.com/totten/civicrm-core/blob/17c04b52f3d0079795426a5e7a3444b92a3dedbc/CRM/Core/Payment/Google.php shouldn't retain the "turning off server & peer verification" comment

colemanw added a commit that referenced this pull request Jul 4, 2013
CRM-11160 - Set CURLOPT_SSL_VERIFYHOST based the system's general "verifySSL" option
@colemanw colemanw merged commit 8bd7ae5 into civicrm:4.3 Jul 4, 2013
@totten totten deleted the totten:4.3-CRM-11160 branch Aug 19, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants