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

Do not recommend adding --insecure to solve certificate issues #1810

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
4 participants
@jedisct1

jedisct1 commented Aug 20, 2017

There is a massive amount of scripts, examples and tutorials unconditionally adding the --insecure option to the curl command in order to access public servers with perfectly valid certificates.

A possible culprit is the curl command itself. As soon as a certificate error is returned, it suggests simply retrying the same command with --insecure added to it. So, people follow the advice, and since it "fixes things", end up using it unconditionally.

This is terrible.

The main causes of certificates that don't validate are:

  • A host name that doesn't match the one in the certificate
  • A client with a clock not being properly set
  • The absence of a CA cert bundle, since this has to be explicitly installed on some Linux distributions
  • A totally outdated CA cert bundle.
  • Self-signed certificates.

The following diff displays some hints about what the root cause of a certificate error is likely to be, instead of suggesting --insecure.

@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Aug 20, 2017

Coverage Status

Coverage decreased (-0.1%) to 73.233% when pulling 19bbd72 on jedisct1:jedisct1/dont-recommend-insecure into 16c71fa on curl:master.

coveralls commented Aug 20, 2017

Coverage Status

Coverage decreased (-0.1%) to 73.233% when pulling 19bbd72 on jedisct1:jedisct1/dont-recommend-insecure into 16c71fa on curl:master.

@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Aug 20, 2017

Coverage Status

Coverage decreased (-0.1%) to 73.233% when pulling d5b83ba on jedisct1:jedisct1/dont-recommend-insecure into 16c71fa on curl:master.

coveralls commented Aug 20, 2017

Coverage Status

Coverage decreased (-0.1%) to 73.233% when pulling d5b83ba on jedisct1:jedisct1/dont-recommend-insecure into 16c71fa on curl:master.

@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Aug 20, 2017

Coverage Status

Coverage decreased (-0.09%) to 73.248% when pulling d5b83ba on jedisct1:jedisct1/dont-recommend-insecure into 16c71fa on curl:master.

coveralls commented Aug 20, 2017

Coverage Status

Coverage decreased (-0.09%) to 73.248% when pulling d5b83ba on jedisct1:jedisct1/dont-recommend-insecure into 16c71fa on curl:master.

@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Aug 20, 2017

Coverage Status

Coverage decreased (-0.1%) to 73.233% when pulling d5b83ba on jedisct1:jedisct1/dont-recommend-insecure into 16c71fa on curl:master.

coveralls commented Aug 20, 2017

Coverage Status

Coverage decreased (-0.1%) to 73.233% when pulling d5b83ba on jedisct1:jedisct1/dont-recommend-insecure into 16c71fa on curl:master.

@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Aug 20, 2017

Coverage Status

Coverage decreased (-0.004%) to 73.335% when pulling d5b83ba on jedisct1:jedisct1/dont-recommend-insecure into 16c71fa on curl:master.

coveralls commented Aug 20, 2017

Coverage Status

Coverage decreased (-0.004%) to 73.335% when pulling d5b83ba on jedisct1:jedisct1/dont-recommend-insecure into 16c71fa on curl:master.

@bagder

This comment has been minimized.

Show comment
Hide comment
@bagder

bagder Aug 20, 2017

Member

Thanks.

Yes, recommending switching off the certificate checks is a rather terrible thing to do. That's also why the option is called --insecure? in its long form: to underscore it makes the transfer insecure.

This said, I'm pretty sure this error message is already a wall of text that is way too big. I think that it due to its size mostly scare people away and I doubt very many actually read the whole thing and weigh their options.

Since this already refers to a URL with most of this described in detail (and it makes sense to make that the single home for those instructions and recommendations), I think we should instead strive toward shortening this error message to a few lines.

Perhaps something like this:

More details here: https://curl.haxx.se/docs/sslcerts.html

curl failed to verify the legitimacy of the server and therefore could not establish a secure connection to it. To learn more about this situation and how to fix it, please visit the web page mentioned above.

Member

bagder commented Aug 20, 2017

Thanks.

Yes, recommending switching off the certificate checks is a rather terrible thing to do. That's also why the option is called --insecure? in its long form: to underscore it makes the transfer insecure.

This said, I'm pretty sure this error message is already a wall of text that is way too big. I think that it due to its size mostly scare people away and I doubt very many actually read the whole thing and weigh their options.

Since this already refers to a URL with most of this described in detail (and it makes sense to make that the single home for those instructions and recommendations), I think we should instead strive toward shortening this error message to a few lines.

Perhaps something like this:

More details here: https://curl.haxx.se/docs/sslcerts.html

curl failed to verify the legitimacy of the server and therefore could not establish a secure connection to it. To learn more about this situation and how to fix it, please visit the web page mentioned above.

@jedisct1

This comment has been minimized.

Show comment
Hide comment
@jedisct1

jedisct1 Aug 20, 2017

Indeed, this message is already way too long.

Your short version is great!

That web page still lists --insecure as the first thing to try :) But unlike a static error message, the page content can be easily kept up to date.

jedisct1 commented Aug 20, 2017

Indeed, this message is already way too long.

Your short version is great!

That web page still lists --insecure as the first thing to try :) But unlike a static error message, the page content can be easily kept up to date.

@bagder

This comment has been minimized.

Show comment
Hide comment
@bagder

bagder Aug 20, 2017

Member

That web page is simply the docs/SSLCERTS.md file rendered on the web site. It can certainly be improved!

Member

bagder commented Aug 20, 2017

That web page is simply the docs/SSLCERTS.md file rendered on the web site. It can certainly be improved!

bagder added a commit that referenced this pull request Aug 22, 2017

curl: shorten and clean up CA cert verification error message
The previuous message was just too long for ordinary people and it was
encouraging users to use `--insecure` a little too easy.

Based-on-work-by: Frank Denis in #1810
@vszakats

This comment has been minimized.

Show comment
Hide comment
@vszakats

vszakats Aug 22, 2017

Member

Here's some rough numbers about turning off certificate verification:
https://github.com/search?l=&q=--insecure+OR+--no-check-certificate+OR+--proxy-insecure&type=Code
(without -k hits, which is likely also common.)

Ref: https://twitter.com/vszakats/status/839812059814588416

Member

vszakats commented Aug 22, 2017

Here's some rough numbers about turning off certificate verification:
https://github.com/search?l=&q=--insecure+OR+--no-check-certificate+OR+--proxy-insecure&type=Code
(without -k hits, which is likely also common.)

Ref: https://twitter.com/vszakats/status/839812059814588416

@bagder bagder closed this in f412a5a Aug 22, 2017

@bagder

This comment has been minimized.

Show comment
Hide comment
@bagder

bagder Aug 22, 2017

Member

thanks @jedisct1!

Member

bagder commented Aug 22, 2017

thanks @jedisct1!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment