Add server TLS certificate verification

[takanabe]

What this PR does / why we need it:

Which issue(s) this PR fixes:
Fixes #1986

Special notes for your reviewer:

Checklist

• Documentation added
• Tests updated

[codecov-commenter]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 62.02%. Comparing base (f04bc99) to head (3a0a390).

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #2171      +/-   ##
==========================================
- Coverage   62.09%   62.02%   -0.08%     
==========================================
  Files         156      156              
  Lines       12531    12531              
==========================================
- Hits         7781     7772       -9     
- Misses       4143     4148       +5     
- Partials      607      611       +4     

see 3 files with indirect coverage changes

[jgehrcke]

"to disable server verification"? :)

[takanabe]

This explanation should be fine. The reason why we introduce this flag is to specify server TLS verification mode. Not for disabling the verification. If we think, "to disable" is better, we need to change the option to no_tls_verify or something.

[jgehrcke]

I think your thinking around this patch came off the wrong foot. You seem to have thought that we somehow want to be able to enable server certificate verification. It is enabled by default and we really look for a way to optionally disable it :).

[jgehrcke]

So, let's focus on the actual patch first and then adjust the docs, ok?

[jgehrcke]

Some high level feedback, clarifying the motivation and goal.

According to rdoc, the default value is VERIFY_NONE.

This seems to be veeeery out-of-date documentation (I don't know Ruby, otherwise I would have been a little more motivated to tackle this patch myself). The Internet and open-source software fortunately moved on around 2014 and 2015, switching stdlibs to do certificate verification by default :-) Ruby is no exception, fortunately:

https://github.com/ruby/ruby/blob/ruby_2_7/sample/drb/drbssl_c.rb#L9
https://github.com/ruby/ruby/blob/v2_7_1/lib/rubygems/request.rb#L52

    connection.use_ssl = true
    connection.verify_mode =
      Gem.configuration.ssl_verify_mode || OpenSSL::SSL::VERIFY_PEER

That is, the default is of course to verify the server certificate when doing HTTPS.

That's how this patch came off the wrong foot I think. As one can also see when comparing the title of #1986 ("add option to disable server certificate verification") and the title of this PR ("Add server TLS certificate verification") --> conflict between core of the issue, and (your previous) goal of this PR.

Really, we need a switch to optionally disable server certificate verification. Convinced?

I keep the current default for this plugin (VERIFY_PEER) but enable developers to turn off the verification with verify_tls = false 07e0def

The current patch isn't doing that! In a working patch we will find a part of the (new) code that sets

verify_mode: OpenSSL::SSL::VERIFY_NONE

but only when verify_tls is set to false :-).

Makes sense?

I'll happily review again, just ping me please.

[takanabe]

Thank you for the comment! I read the links and net/http and you are right. The default value of verify_mode is OpenSSL::SSL::VERIFY_PEER.

I pushed new commit based on your advice 4aadf60. If you are happy to the commit, I will update document as well 📝

[takanabe]

@jgehrcke could you review the changes?

[cyriltovena]

LGTM

Can you update the doc now ?

[takanabe]

@cyriltovena thank you for the review!

I pushed the commit to update document e1dac90

Should I rebase all commits into one commit or will you squash-merge this PR?

[cyriltovena]

LGTM with one nit.

[jgehrcke]

Thanks again everyone here.