fix: [CURLRequest] skip hostname checks if options 'verify' false #8258

NicolaeIotu · 2023-11-26T12:59:07Z

Supersedes #8257

Description
When CURLRequest options 'verify' is set to false, some CURLOPT_SSL_... options should be disabled in such a way as to allow requests to pass through in case the destination is for example on private networks.

Avoids SSL errors: SSL: certificate subject name 'CA' does not match target host name 'localhost'

Checklist:

Securely signed commits
Component(s) with PHPDoc blocks, only if necessary or adds value
Unit testing, with >80% coverage
User guide updated
Conforms to style guide

kenjis · 2023-11-26T20:36:49Z

Thank you!
But some checks were not successful.
Please fix the code to pass all checks.

kenjis · 2023-11-27T22:38:26Z

system/HTTP/CURLRequest.php

-                $curlOptions[CURLOPT_SSL_VERIFYPEER] = 1;
+                $curlOptions[CURLOPT_CAINFO] = $file;
+                if ($config['verify'] === 'yes') {
+                    $curlOptions[CURLOPT_SSL_VERIFYPEER] = 1;


Why do you set 1?
It seems this parameter should be boolean.

CURLOPT_SSL_VERIFYPEER
false to stop cURL from verifying the peer's certificate. Alternate certificates to verify against can be specified with the CURLOPT_CAINFO option or a certificate directory can be specified with the CURLOPT_CAPATH option.
https://www.php.net/manual/en/function.curl-setopt.php#refsect1-function.curl-setopt-parameters

kenjis

There is no test case for options 'verify' false.
Please add it.

kenjis

LGTM!

kenjis · 2023-11-28T08:05:16Z

Thank you!
But why did you send to 4.5?
This is a bug fix, so you can send to develop.
Do you think this is a breaking change?

NicolaeIotu · 2023-11-28T08:13:53Z

I don't think it's breaking anything. It's just that that the label bug was added afterwards.
So I have to close this PR and open a new one while choosing branch develop? Is this ok?

kenjis · 2023-11-28T08:18:48Z

I think this is a bug fix, so I added the label bug.
I'm okay that you change to develop.
See https://github.com/codeigniter4/CodeIgniter4/blob/develop/contributing/workflow.md#if-you-sent-to-the-wrong-branch

…alse. When CURLRequest options 'verify' is set to false, some CURLOPT_SSL_... options should be disabled in such a way as to allow requests to pass through in case the destination is for example on private networks. Avoids SSL errors: SSL: certificate subject name 'CA' does not match target host name 'localhost'

…_VERIFYPEER and CURLOPT_SSL_VERIFYHOST

NicolaeIotu · 2023-11-28T08:35:14Z

Completed the switch to branch develop.

michalsn · 2023-11-28T19:50:22Z

system/HTTP/CURLRequest.php

-                $curlOptions[CURLOPT_CAINFO]         = $file;
-                $curlOptions[CURLOPT_SSL_VERIFYPEER] = 1;
+                $curlOptions[CURLOPT_CAINFO] = $file;
+                if ($config['verify'] === 'yes') {


There might be something I don't understand, but how can we have yes as a value here?

The user guide says about verify option:

This option describes the SSL certificate verification behavior. If the verify option is true, it enables the SSL certificate verification and uses the default CA bundle provided by the operating system. If set to false it will disable the certificate verification (this is insecure, and allows man-in-the-middle attacks!). You can set it to a string that contains the path to a CA bundle to enable verification with a custom certificate. The default value is true:

Dunno why, but we use ssl_key instead of verify when it's a string. The ssl_key option is not documented at all.

I feel like something has to be changed here. Either the code or the documentation.

I missed it! Thank you.
yes is wrong.

Dunno why, but we use ssl_key instead of verify when it's a string.

This seems a bug.
We don't support ssl_key.
See https://docs.guzzlephp.org/en/stable/request-options.html#ssl-key

Yeah... since we should be compatible with guzzle, we should use verify.

When fixed, this should be noted in the changelog with some additional description, because some users could dig into the code and set ssl_key just to make it work.

kenjis · 2023-11-28T20:53:49Z

@NicolaeIotu We don't use git merge in PR branches. Please use git rebase if you can.
See https://github.com/codeigniter4/CodeIgniter4/blob/develop/contributing/workflow.md#updating-your-branch

yes is invalid.

NicolaeIotu · 2023-11-28T21:24:05Z

So in case verify it's a string then it can only be "the path to a CA bundle".
Also will try to make git rebase.

kenjis · 2023-11-30T01:17:52Z

There is still two merge commits. Can you run git rebase to remove the commits?

NicolaeIotu · 2023-11-30T10:54:21Z

I'm doing some git action to revert my mistakes. Please ignore for a while.

…RLOPT_SSL_VERIFYPEER and CURLOPT_SSL_VERIFYHOST

NicolaeIotu · 2023-11-30T11:45:06Z

I think it's ok now.

michalsn

We need a changelog update, which will inform users about these fixes: https://github.com/codeigniter4/CodeIgniter4/blob/develop/user_guide_src/source/changelogs/v4.4.4.rst

Both: Bugs and Breaking sections should be filled.

michalsn · 2023-11-30T14:33:26Z

system/HTTP/CURLRequest.php

@@ -548,17 +548,21 @@ protected function setCURLOptions(array $curlOptions = [], array $config = [])

        // SSL Verification
        if (isset($config['verify'])) {
-            if (is_string($config['verify'])) {
-                $file = realpath($config['ssl_key']) ?: $config['ssl_key'];
+            $configVerify = $config['verify'];


Please don't introduce an additional variable, it makes the code less readable - use $config['verify'].

Just to clarify. Array element $config['verify'] is accessed 8 times. Isn't this less efficient than using a local variable?

No, why would it be?

If the element of the array it's not changing value, you get a performance increase if assigning its value to a local variable and using that instead of accessing the array every time especially if the key is not an integer and the value of the element is required many times.

This is pretty much valid for all languages when data type is using hash tables and others alike.

In this case there is a performance increase if using the local variable, but it may be more important to make the code readable.

I forgot to ask. The breaking change is that $config['verify'] cannot take the value 'yes' anymore. This is the only breaking change?

If the element of the array it's not changing value, you get a performance increase...

In this case, I'm pretty sure we would not see the difference. That's why I choose the readability.

I forgot to ask. The breaking change is that $config['verify'] cannot take the value 'yes' anymore. This is the only breaking change?

Was using the value yes was documented somewhere? If not this can be skipped.
The breaking change is not using the ssl_key array key anymore.

Revert to ['verify'] instead of

michalsn

The bug description should also go to the user_guide_src/source/changelogs/v4.4.4.rst. You can check the previous versions for a reference.

michalsn · 2023-12-01T07:02:37Z

CHANGELOG.md

+## [v4.4.4](https://github.com/codeigniter4/CodeIgniter4/tree/v4.4.4) (Unreleased)
+[Full Changelog](https://github.com/codeigniter4/CodeIgniter4/compare/v4.4.3...v4.4.4)
+
+### Fixed Bugs
+
+* fix: [CURLRequest] skip hostname checks if options 'verify' false by @NicolaeIotu in https://github.com/codeigniter4/CodeIgniter4/pull/8258
+


I think that this file is handled automatically when we prepare the release. @kenjis can confirm or deny.

Yes, it is generated by GitHub.
Please add in changelog in user guide.
https://github.com/codeigniter4/CodeIgniter4/blob/develop/user_guide_src/source/changelogs/v4.4.4.rst#breaking

File user_guide_src/source/changelogs/v4.4.4.rst was updated.
Now I'll revert the changes to CHANGELOG.md.

user_guide_src/source/changelogs/v4.4.4.rst

Co-authored-by: Michal Sniatala <michal@sniatala.pl>

kenjis · 2023-12-01T08:36:51Z

The last thing. Add short instruction how to change when upgrading in
https://github.com/codeigniter4/CodeIgniter4/blob/develop/user_guide_src/source/installation/upgrade_444.rst#breaking-changes

Although confusing with changelog, the changelog describes what has changed and the upgrading guide describes what users need to do when upgrading.

kenjis · 2023-12-02T20:31:16Z

@NicolaeIotu Thank you!

NicolaeIotu · 2023-12-03T07:22:31Z

@kenjis Thank you!

kenjis changed the title ~~add: skip hostname checks if CURLRequest options 'verify' false~~ fix: [CURLRequest] skip hostname checks if options 'verify' false Nov 26, 2023

kenjis added the bug Verified issues on the current code behavior or pull requests that will fix them label Nov 26, 2023

kenjis added the docs needed Pull requests needing documentation write-ups and/or revisions. label Nov 27, 2023

kenjis reviewed Nov 27, 2023

View reviewed changes

kenjis added the tests needed Pull requests that need tests label Nov 27, 2023

kenjis requested changes Nov 27, 2023

View reviewed changes

kenjis removed the docs needed Pull requests needing documentation write-ups and/or revisions. label Nov 27, 2023

NicolaeIotu requested a review from kenjis November 28, 2023 07:52

kenjis approved these changes Nov 28, 2023

View reviewed changes

kenjis removed the tests needed Pull requests that need tests label Nov 28, 2023

kenjis changed the base branch from 4.5 to develop November 28, 2023 08:16

kenjis changed the base branch from develop to 4.5 November 28, 2023 08:16

NicolaeIotu added 3 commits November 28, 2023 08:28

add: SSL verify options when config.verify is string and boolean

3685232

fix: CURLOPT_SSL_VERIFYPEER values and tests related with CURLOPT_SSL…

e159bfd

…_VERIFYPEER and CURLOPT_SSL_VERIFYHOST

NicolaeIotu force-pushed the add-curlrequest-curlopt_ssl_verifyhost branch from 8207e01 to e159bfd Compare November 28, 2023 08:29

NicolaeIotu changed the base branch from 4.5 to develop November 28, 2023 08:34

kenjis previously approved these changes Nov 28, 2023

View reviewed changes

michalsn reviewed Nov 28, 2023

View reviewed changes

NicolaeIotu force-pushed the add-curlrequest-curlopt_ssl_verifyhost branch from 779d5c7 to e159bfd Compare November 30, 2023 10:50

recreate fix: CURLOPT_SSL_VERIFYPEER values and tests related with CU…

e601954

…RLOPT_SSL_VERIFYPEER and CURLOPT_SSL_VERIFYHOST

michalsn requested changes Nov 30, 2023

View reviewed changes

NicolaeIotu added 2 commits November 30, 2023 17:49

Updated Bugs and Breaking sections

f8dd0b7

Revert to ['verify'] instead of

Fix sphinx-action warning 'Title underline too short'

25b6d40

michalsn reviewed Dec 1, 2023

View reviewed changes

Update user_guide_src/source/changelogs/v4.4.4.rst

ab25092

Co-authored-by: Michal Sniatala <michal@sniatala.pl>

kenjis added the breaking change Pull requests that may break existing functionalities label Dec 1, 2023

NicolaeIotu added 2 commits December 1, 2023 07:39

Added the summary of the bug

6df07b0

Revert manual changes to CHANGELOG.md

bfb9ab4

Added upgrading instructions

5b4482c

michalsn approved these changes Dec 2, 2023

View reviewed changes

kenjis approved these changes Dec 2, 2023

View reviewed changes

kenjis merged commit 54cbc32 into codeigniter4:develop Dec 2, 2023
62 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: [CURLRequest] skip hostname checks if options 'verify' false #8258

fix: [CURLRequest] skip hostname checks if options 'verify' false #8258

NicolaeIotu commented Nov 26, 2023 •

edited by kenjis

Loading

kenjis commented Nov 26, 2023

kenjis Nov 27, 2023

kenjis left a comment

kenjis left a comment

kenjis commented Nov 28, 2023

NicolaeIotu commented Nov 28, 2023

kenjis commented Nov 28, 2023

NicolaeIotu commented Nov 28, 2023

michalsn Nov 28, 2023

kenjis Nov 28, 2023 •

edited

Loading

kenjis Nov 28, 2023

michalsn Nov 28, 2023

kenjis commented Nov 28, 2023

NicolaeIotu commented Nov 28, 2023

kenjis commented Nov 30, 2023 •

edited

Loading

NicolaeIotu commented Nov 30, 2023

NicolaeIotu commented Nov 30, 2023

michalsn left a comment

michalsn Nov 30, 2023

NicolaeIotu Nov 30, 2023

michalsn Nov 30, 2023

NicolaeIotu Nov 30, 2023

michalsn Nov 30, 2023

michalsn left a comment

michalsn Dec 1, 2023

kenjis Dec 1, 2023

NicolaeIotu Dec 1, 2023

kenjis Dec 1, 2023

kenjis commented Dec 1, 2023

kenjis commented Dec 2, 2023

NicolaeIotu commented Dec 3, 2023

fix: [CURLRequest] skip hostname checks if options 'verify' false #8258

fix: [CURLRequest] skip hostname checks if options 'verify' false #8258

Conversation

NicolaeIotu commented Nov 26, 2023 • edited by kenjis Loading

kenjis commented Nov 26, 2023

Choose a reason for hiding this comment

kenjis left a comment

Choose a reason for hiding this comment

kenjis left a comment

Choose a reason for hiding this comment

kenjis commented Nov 28, 2023

NicolaeIotu commented Nov 28, 2023

kenjis commented Nov 28, 2023

NicolaeIotu commented Nov 28, 2023

Choose a reason for hiding this comment

kenjis Nov 28, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kenjis commented Nov 28, 2023

NicolaeIotu commented Nov 28, 2023

kenjis commented Nov 30, 2023 • edited Loading

NicolaeIotu commented Nov 30, 2023

NicolaeIotu commented Nov 30, 2023

michalsn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michalsn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kenjis commented Dec 1, 2023

kenjis commented Dec 2, 2023

NicolaeIotu commented Dec 3, 2023

NicolaeIotu commented Nov 26, 2023 •

edited by kenjis

Loading

kenjis Nov 28, 2023 •

edited

Loading

kenjis commented Nov 30, 2023 •

edited

Loading