Skip to content

Wrong value set for CURLOPT_SSL_VERIFYHOST #58

Closed
wants to merge 1 commit into from

4 participants

@tmuras
tmuras commented Nov 25, 2012

CURLOPT_SSL_VERIFYHOST should be set to 2 not to 1.

From the libcurl documentation:

When CURLOPT_SSL_VERIFYHOST is 2, that certificate must indicate that the
server is the server to which you meant to connect, or the connection fails.

Curl considers the server the intended one when the Common Name field or a
Subject Alternate Name field in the certificate matches the host name in the
URL to which you told Curl to connect.

When the value is 1, the certificate must contain a Common Name field, but it
doesn't matter what name it says. (This is not ordinarily a useful setting).

Thanks for ghedo from debian.org for reporting.

@olberger

Have you been able to test the fix ?

What's the author's opinion on this change ?

@tmuras
tmuras commented Nov 26, 2012

Hi, no I did not test the fix. It looks to me like a valid change based on the code review only.

@jfritschi
Apereo Foundation member

The patch makes sense but there is an error in the patch and existing code in my view. The "else" an explicit "no verification of the SSL connection (cert/host)" that is requested by the user so it should be 0 for the HOST_VERIFY. I guess it's not used anyway and could simply be removed all together.

I also think we should have an option to only verify the SSL cert but skip the HOST verification.

@jfritschi jfritschi was assigned Nov 26, 2012
@jfritschi jfritschi added a commit that referenced this pull request Nov 28, 2012
@jfritschi jfritschi #58 Enable full CN valdiation of SSL certifcate and create a manual user
override to disable it. The new default is a proper CN
validation.
aa00f35
@jfritschi
Apereo Foundation member

I have commited a patch to fix this issue. I have tested the solution locally and it works.

The commit also includes an example how to fall back to the old (unsafe) implementation if necessary. The new default is a full CN validation of the SSL certificate.

Any feedback is welcome. The only "ugly" thing was changing the RequestInterface. Maybe @adamfranco has some ideas or opinions?

@adamfranco
Apereo Foundation member

Adding the optional parameter to the interface seems OK since in theory the extra validation should be done by any request-handling method, not just CURL. The only change needed is that I think the interface should define the parameter as optional to avoid PHP strict notices.

@jfritschi
Apereo Foundation member

Done. I hope this is what you had in mind?

@adamfranco
Apereo Foundation member

Looks good. :-)

@jfritschi
Apereo Foundation member

Thanks, this takes care of one of the open items on the release preparation list. I will add it to the changelog and release announcement.

@jfritschi
Apereo Foundation member

I have added all the info to the docs.

@jfritschi jfritschi closed this Dec 15, 2012
@jfritschi jfritschi added a commit that referenced this pull request Dec 16, 2012
@jfritschi jfritschi #58 fix bug introduced in previous patches. Setting of the ssl certs was
not performing properly.
0e75d13
@jfritschi jfritschi added a commit that referenced this pull request Dec 31, 2012
@jfritschi jfritschi #60 fix testcases that were broken in #58 5d64fbe
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.