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

doh: add options to disable ssl verification #6597

Closed
wants to merge 1 commit into from

Conversation

jay
Copy link
Member

@jay jay commented Feb 11, 2021

  • 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: #4579 (comment)

Fixes #4578
Closes #xxxx

@jay jay added the name lookup DNS and related tech label Feb 11, 2021
@ghost
Copy link

ghost commented Feb 11, 2021

DeepCode's analysis on #b95007 found:

  • ⚠️ 1 warning 👇

Top issues

Description Example fixes
Disabling the setting CURLOPT_SSL_VERIFYHOST in curl_easy_setopt may lead to Man-in-the-middle attacks. Occurrences: 🔧 Example fixes

👉 View analysis in DeepCode’s Dashboard | Configure the bot

@jay jay force-pushed the doh-insecure branch 2 times, most recently from bb5ac55 to e5bc791 Compare February 12, 2021 19:44
- 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
Copy link
Member

@bagder bagder left a comment

Choose a reason for hiding this comment

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

Makes perfect sense for DoH users.

Maybe we should create a "inherit these things for DoH" bitmask option at some point...

@jay jay closed this in 53022e1 Feb 14, 2021
@jay jay deleted the doh-insecure branch February 14, 2021 23:25
jay added a commit that referenced this pull request Mar 26, 2021
- 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
name lookup DNS and related tech
Development

Successfully merging this pull request may close these issues.

DoH: some options from user's transfer are not properly inherited
2 participants