DoH: some options from user's transfer are not properly inherited #4578
Comments
Prior to this change some doh handle options inherited from the user's easy handle were only inherited if they were turned on, not if they were off. [API option] : [Default API setting] CURLOPT_NOSIGNAL : OFF CURLOPT_PROXY_SSL_VERIFYHOST : ON <-- affected CURLOPT_PROXY_SSL_VERIFYPEER : ON <-- affected CURLOPT_SSL_FALSESTART : OFF CURLOPT_SSL_VERIFYHOST : ON <-- affected CURLOPT_SSL_VERIFYPEER : ON <-- affected CURLOPT_SSL_VERIFYSTATUS : OFF CURLOPT_VERBOSE : OFF For example if CURLOPT_SSL_VERIFYPEER was turned off by the user then that would not have been inherited by the doh handle and its verify peer setting would have defaulted to on. Prior to this change users were not able to disable SSL verification of the DOH server. Reported-by: 3dyd@users.noreply.github.com Fixes curl#4578 Closes #xxxx
|
Ref: 9e6af11 My fault, looks like that was my intention: I did inherit insecure because I think that should still be in control of the user. #4579 proposed fix
Yes, if those option values are expanded the code you cited should be amended. All the current internal members are bool, and they would have to be updated everywhere if they were changed from bool. |
Prior to this change some doh handle options inherited from the user's easy handle were only inherited if they were turned on, not if they were off. [API option] : [Default API setting] CURLOPT_NOSIGNAL : OFF CURLOPT_PROXY_SSL_VERIFYHOST : ON <-- affected CURLOPT_PROXY_SSL_VERIFYPEER : ON <-- affected CURLOPT_SSL_FALSESTART : OFF CURLOPT_SSL_VERIFYHOST : ON <-- affected CURLOPT_SSL_VERIFYPEER : ON <-- affected CURLOPT_SSL_VERIFYSTATUS : OFF CURLOPT_VERBOSE : OFF For example if CURLOPT_SSL_VERIFYPEER was turned off by the user then that would not have been inherited by the doh handle and its verify peer setting would have defaulted to on. Prior to this change users were not able to disable SSL verification of the DOH server. Reported-by: 3dyd@users.noreply.github.com Fixes curl#4578 Closes #xxxx
Prior to this change some doh handle options inherited from the user's easy handle were only inherited if they were turned on, not if they were off. [API option] : [Default API setting] CURLOPT_CERTINFO : OFF CURLOPT_NOSIGNAL : OFF CURLOPT_PROXY_SSL_VERIFYHOST : ON <-- affected CURLOPT_PROXY_SSL_VERIFYPEER : ON <-- affected CURLOPT_SSL_FALSESTART : OFF CURLOPT_SSL_VERIFYHOST : ON <-- affected CURLOPT_SSL_VERIFYPEER : ON <-- affected CURLOPT_SSL_VERIFYSTATUS : OFF CURLOPT_VERBOSE : OFF For example if CURLOPT_SSL_VERIFYPEER was turned off by the user then that would not have been inherited by the doh handle and its verify peer setting would have defaulted to on. Prior to this change users were not able to disable SSL verification of the DOH server. Reported-by: 3dyd@users.noreply.github.com Fixes curl#4578 Closes #xxxx
|
I think what's possibly more complicated is that |
|
Isn't this concern relevant for all inherited options? One of strong libcurl sides is reasonable defaults. In context of this topic it would be great to keep current approach as it is simple to use, better than noop, and (subjectively) more expected behavior. Then maybe add as alternative some CURLOPT_XXXFUNCTION to tune DoH handle if one would need grained control over it? |
That was the intention though, rather than establish a separate set of DOH options inherit relevant options from the user's handle, and I think that's true of --insecure. This bug was just a mistake on my part. |
- New libcurl options CURLOPT_DOH_SSL_VERIFYHOST, CURLOPT_DOH_SSL_VERIFYPEER and CURLOPT_DOH_SSL_VERIFYSTATUS do the same as their respective counterparts. - New curl tool options --doh-insecure and --doh-cert-status do the same as their respective counterparts. Ref: curl#4579 (comment) Fixes curl#4578 Closes #xxxx
- New libcurl options CURLOPT_DOH_SSL_VERIFYHOST, CURLOPT_DOH_SSL_VERIFYPEER and CURLOPT_DOH_SSL_VERIFYSTATUS do the same as their respective counterparts. - New curl tool options --doh-insecure and --doh-cert-status do the same as their respective counterparts. Prior to this change DOH SSL certificate verification settings for verifyhost and verifypeer were supposed to be inherited respectively from CURLOPT_SSL_VERIFYHOST and CURLOPT_SSL_VERIFYPEER, but due to a bug were not. As a result DOH verification remained at the default, ie enabled, and it was not possible to disable. This commit changes behavior so that the DOH verification settings are independent and not inherited. Ref: curl#4579 (comment) Fixes curl#4578 Closes #xxxx
- New libcurl options CURLOPT_DOH_SSL_VERIFYHOST, CURLOPT_DOH_SSL_VERIFYPEER and CURLOPT_DOH_SSL_VERIFYSTATUS do the same as their respective counterparts. - New curl tool options --doh-insecure and --doh-cert-status do the same as their respective counterparts. Prior to this change DOH SSL certificate verification settings for verifyhost and verifypeer were supposed to be inherited respectively from CURLOPT_SSL_VERIFYHOST and CURLOPT_SSL_VERIFYPEER, but due to a bug were not. As a result DOH verification remained at the default, ie enabled, and it was not possible to disable. This commit changes behavior so that the DOH verification settings are independent and not inherited. Ref: curl#4579 (comment) Fixes curl#4578 Closes #xxxx
- New libcurl options CURLOPT_DOH_SSL_VERIFYHOST, CURLOPT_DOH_SSL_VERIFYPEER and CURLOPT_DOH_SSL_VERIFYSTATUS do the same as their respective counterparts. - New curl tool options --doh-insecure and --doh-cert-status do the same as their respective counterparts. Prior to this change DOH SSL certificate verification settings for verifyhost and verifypeer were supposed to be inherited respectively from CURLOPT_SSL_VERIFYHOST and CURLOPT_SSL_VERIFYPEER, but due to a bug were not. As a result DOH verification remained at the default, ie enabled, and it was not possible to disable. This commit changes behavior so that the DOH verification settings are independent and not inherited. Ref: #4579 (comment) Fixes #4578 Closes #6597
- Add description: Explain that some options aren't inherited because they are not relevant for the DoH SSL connections or may result in unexpected behavior. - Remove the reference to #4578 (SSL verify options not inherited) since that was fixed by #6597 (separate DoH-specific options for verify). - Explain that DoH-specific options (those created by #6597) are available: CURLOPT_DOH_SSL_VERIFYHOST, CURLOPT_DOH_SSL_VERIFYPEER and CURLOPT_DOH_SSL_VERIFYSTATUS. - Add a reference to #6605 and explain that the user's debug function is not inherited because it would be unexpected to pass internal handles (ie DoH handles) to the user's callback. Closes #6605
Take a look at these lines from
dohprobe:curl/lib/doh.c
Lines 277 to 282 in 8063c32
If
data->set.ssl.primary.verifyhostis set to zero, appropriate option in DoH handle stays untouched. But its default value is 2. I.e. currently you cannot disable certificate's name verification for DoH request.Not sure if this is intended behavior. And if it is, at least the comment is a bit misleading.
Affected options (whose default value is not zero):
CURLOPT_SSL_VERIFYHOSTCURLOPT_PROXY_SSL_VERIFYHOSTCURLOPT_SSL_VERIFYPEERCURLOPT_PROXY_SSL_VERIFYPEERSimilar concern for the code constructions like the following one:
(Not sure though if this may generally happen in the library but) If option (
falsestartin this case) would be expanded in further library versions (get more values than '0' or '1') then code like this will need to be revisited what is easy to overlook.The text was updated successfully, but these errors were encountered: