-
Notifications
You must be signed in to change notification settings - Fork 2.2k
(PUP-8213) Display correct message when certname is mismatched #7401
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
Conversation
|
jenkins please test this with servertests |
CLA signed by all contributors. |
@reidmv , I think you recently mentioned a similar/same problem in Slack. |
Previously, if the server's cert did not match the hostname we tried to connect to, puppet would output a confusing message: certificate verify failed: [ok for /CN=XXX] This occurs because ruby 2.4 introduced a new security feature whereby the cert is automatically verified during the call to `SSLSocket#connect`[1]. In earlier ruby versions, the application had to call `SSLSocket#post_connection_check`, but of course, many people forgot to, or didn't know they had to, leading to MITM vulnerabilities. However, when a mismatch occurs, ruby 2.4 invokes our `verify_callback` with `preverify_ok=false`, but `store_context.error=0` which is `OpenSSL::SSL::V_OK`. Ruby then raises an `SSLError` whose message is 'certificate verify failed', which matches the first "if" statement in our error handler. This commit changes the order so that if an SSLError is rescued, we check to see if there's a host mismatch first. If not, we check if there were *any* verify errors, or raise the original error. This change is compatible with ruby versions prior to 2.4, because both `SSLSocket#post_connection_check` and our error handler use `OpenSSL::SSL.verify_certificate_identity` to detect the certname mismatch. [1] ruby/openssl#60
337eee2
to
1f632ae
Compare
Also normal agent run where
The fact that the agent keeps continuing after node, pluginsync, etc failures is long standing behavior. Would be good to fix that later, eg https://tickets.puppetlabs.com/browse/PUP-1763 |
Revert "Merge pull request #7401 from joshcooper/certmismatch_8213"
* upstream/5.5.x: Revert "Merge pull request puppetlabs#7401 from joshcooper/certmismatch_8213" Conflicts: lib/puppet/network/http/connection.rb spec/unit/network/http/connection_spec.rb
* upstream/5.5.x: (packaging) Updating manpage file for 5.5.x Revert "Merge pull request puppetlabs#7401 from joshcooper/certmismatch_8213" Conflicts: lib/puppet/network/http/connection.rb man/man8/puppet-ca.8 man/man8/puppet-cert.8 man/man8/puppet-certificate.8 man/man8/puppet-certificate_request.8 man/man8/puppet-certificate_revocation_list.8 man/man8/puppet-master.8 spec/unit/network/http/connection_spec.rb
* upstream/5.5.x: (packaging) Updating manpage file for 5.5.x Revert "Merge pull request puppetlabs#7401 from joshcooper/certmismatch_8213" Conflicts: lib/puppet/network/http/connection.rb man/man8/puppet-ca.8 man/man8/puppet-cert.8 man/man8/puppet-certificate.8 man/man8/puppet-certificate_request.8 man/man8/puppet-certificate_revocation_list.8 man/man8/puppet-master.8 spec/unit/network/http/connection_spec.rb
* upstream/5.5.x: (packaging) Updating manpage file for 5.5.x Revert "Merge pull request puppetlabs#7401 from joshcooper/certmismatch_8213" Conflicts: lib/puppet/network/http/connection.rb man/man8/puppet-ca.8 man/man8/puppet-cert.8 man/man8/puppet-certificate.8 man/man8/puppet-certificate_request.8 man/man8/puppet-certificate_revocation_list.8 man/man8/puppet-master.8 spec/unit/network/http/connection_spec.rb
Generated by CI * commit 'fd1cb18fadfe7a967567606306e6aa270af4bed6': (packaging) Updating manpage file for 5.5.x Revert "Merge pull request #7401 from joshcooper/certmismatch_8213" (packaging) Updating manpage file for 6.0.x (packaging) Updating the puppet.pot file (maint) Remove unnecessary condition (PUP-9357) Redact checks if sensitive (packaging) Updating the puppet.pot file (maint) Enable windows tests (PUP-8213) Display correct message when certname is mismatched (packaging) Updating the puppet.pot file (PUP-9076) Do not push nil on server context (maint) modify test to regex, rather than exact (maint) Clarify error on Timeout::Error exception
Previously, if the server's cert did not match the hostname we tried to connect
to, puppet would output a confusing message:
This occurs because ruby 2.4 introduced a new security feature whereby the cert
is automatically verified during the call to
SSLSocket#connect
[1]. In earlierruby versions, the application had to call
SSLSocket#post_connection_check
,but of course, many people forgot to, or didn't know they had to, leading to
MITM vulnerabilities.
However, when a mismatch occurs, ruby 2.4 invokes our
verify_callback
withpreverify_ok=false
, butstore_context.error=0
which isOpenSSL::SSL::V_OK
.Ruby then raises an
SSLError
whose message is 'certificate verify failed',which matches the first "if" statement in our error handler.
This commit changes the order so that if an SSLError is rescued, we
check to see if there's a host mismatch first. If not, we check if there
were any verify errors, or raise the original error.
This change is compatible with ruby versions prior to 2.4, because both
SSLSocket#post_connection_check
and our error handler useOpenSSL::SSL.verify_certificate_identity
to detect the certname mismatch.[1] ruby/openssl#60