Skip to content

(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

Merged
merged 2 commits into from
Feb 26, 2019

Conversation

joshcooper
Copy link
Contributor

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

@joshcooper
Copy link
Contributor Author

$ bx ruby --version
ruby 2.5.3p105 (2018-10-18 revision 65156) [x86_64-darwin18]
$ bx puppet catalog find --server puppet-evil --terminus rest
Error: Could not call 'find' on 'catalog': Server hostname 'puppet-evil' did not match server certificate; expected one of puppet, DNS:puppet, DNS:puppet.delivery.puppetlabs.net

@joshcooper
Copy link
Contributor Author

jenkins please test this with servertests

@puppetcla
Copy link

CLA signed by all contributors.

@justinstoller
Copy link
Member

@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
@joshcooper
Copy link
Contributor Author

joshcooper commented Feb 21, 2019

Also normal agent run where puppet-evil maps to the IP of my puppetserver in /etc/hosts:

$ bx puppet agent -t --server puppet-evil
Warning: Unable to fetch my node definition, but the agent run will continue:
Warning: Server hostname 'puppet-evil' did not match server certificate; expected one of puppet, DNS:puppet, DNS:puppet.delivery.puppetlabs.net
Info: Retrieving pluginfacts
Error: /File[/Users/josh/.puppetlabs/opt/puppet/cache/facts.d]: Failed to generate additional resources using 'eval_generate': Server hostname 'puppet-evil' did not match server certificate; expected one of puppet, DNS:puppet, DNS:puppet.delivery.puppetlabs.net
Error: /File[/Users/josh/.puppetlabs/opt/puppet/cache/facts.d]: Could not evaluate: Could not retrieve file metadata for puppet:///pluginfacts: Server hostname 'puppet-evil' did not match server certificate; expected one of puppet, DNS:puppet, DNS:puppet.delivery.puppetlabs.net
Info: Retrieving plugin
Error: /File[/Users/josh/.puppetlabs/opt/puppet/cache/lib]: Failed to generate additional resources using 'eval_generate': Server hostname 'puppet-evil' did not match server certificate; expected one of puppet, DNS:puppet, DNS:puppet.delivery.puppetlabs.net
Error: /File[/Users/josh/.puppetlabs/opt/puppet/cache/lib]: Could not evaluate: Could not retrieve file metadata for puppet:///plugins: Server hostname 'puppet-evil' did not match server certificate; expected one of puppet, DNS:puppet, DNS:puppet.delivery.puppetlabs.net
Error: Could not retrieve catalog from remote server: Server hostname 'puppet-evil' did not match server certificate; expected one of puppet, DNS:puppet, DNS:puppet.delivery.puppetlabs.net
Warning: Not using cache on failed catalog
Error: Could not retrieve catalog; skipping run
Error: Could not send report: Server hostname 'puppet-evil' did not match server certificate; expected one of puppet, DNS:puppet, DNS:puppet.delivery.puppetlabs.net

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

@jhelwig jhelwig merged commit aab7b62 into puppetlabs:5.5.x Feb 26, 2019
@joshcooper joshcooper deleted the certmismatch_8213 branch February 26, 2019 21:23
joshcooper added a commit to joshcooper/puppet that referenced this pull request Mar 4, 2019
…ch_8213"

This reverts commit aab7b62, reversing
changes made to 2c3f51b.
jhelwig added a commit that referenced this pull request Mar 4, 2019
Revert "Merge pull request #7401 from joshcooper/certmismatch_8213"
joshcooper added a commit to joshcooper/puppet that referenced this pull request Mar 4, 2019
* 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
joshcooper added a commit to joshcooper/puppet that referenced this pull request Mar 4, 2019
* 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
joshcooper added a commit to joshcooper/puppet that referenced this pull request Mar 4, 2019
* 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
joshcooper added a commit to joshcooper/puppet that referenced this pull request Mar 4, 2019
* 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
puppetlabs-jenkins added a commit that referenced this pull request Mar 5, 2019
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants