Skip to content

(#15595) Offer better errors for certificate validation errors#953

Closed
djmitche wants to merge 3 commits intopuppetlabs:2.7.xfrom
djmitche:better-ssl-verify-error-message-2.7.x
Closed

(#15595) Offer better errors for certificate validation errors#953
djmitche wants to merge 3 commits intopuppetlabs:2.7.xfrom
djmitche:better-ssl-verify-error-message-2.7.x

Conversation

@djmitche
Copy link
Copy Markdown
Contributor

This is an update of #917 against the 2.7.x branch, as requested by @hkenney. It's also squashed and the bug number added to the commit branch.

The verify_callback callback gets an OpenSSL::SSL::SSLContext for each
certificate in the chain that's verified.  If the verification failed,
then SSL provides a nice error to the callback, but that error doesn't
appear in the subsequent OpenSSL::SSL::SSLError.

This patch uses a technique similar to that used for peer_certs to
collect those errors and then add them to the Puppet::Error message
later.

Remove the guess at the error (time sync).
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I don't think there is any reason for the unless on this line. The join will just return an empty string if verify_errors are empty. The only thing this saves is that we won't end up with an extra space character. I think this could be dealt with by just formatting the error message a little differently.

Maybe change the list of stringified verify_errors to be displayed as a list in square brackets. So that it appears as '... [verify error 1; verify error 2]' Then in the case were there are no errors it would be '.... []'.

* combine conditionals in verify_callback
* change formatting of verify errors
@djmitche
Copy link
Copy Markdown
Contributor Author

updated!

@zaphod42
Copy link
Copy Markdown
Contributor

After looking into this more, I've found that the error method on StoreContext is probably not what you want to be using for creating a meaningful error messages. That method returns an error number. I think there is an error_string method that will give what you might actually be looking for.

Unfortunately there isn't much documentation about what is available in ruby's OpenSSL bindings, and the best way of figuring this out seems to be inspecting the objects in irb.

@djmitche
Copy link
Copy Markdown
Contributor Author

Hmm, when I hacked this in trying to track down verification numbers, error was what worked for me. Perhaps this is version-specific?

@zaphod42
Copy link
Copy Markdown
Contributor

Interesting, what version of ruby were you using? I've just tried it out on 1.8.7 and 1.9.3 and got the following:

> irb
1.8.7-p358 :001 > require 'net/https'
 => true 
1.8.7-p358 :002 > conn = Net::HTTP.new("github.com", 443)
 => #<Net::HTTP github.com:443 open=false> 
1.8.7-p358 :003 > conn.use_ssl = true
 => true 
1.8.7-p358 :004 > conn.verify_callback = proc do |ok, context| require 'debug'; end
 => #<Proc:0x00000001071cecf8@(irb):4> 
1.8.7-p358 :005 > conn.get('/')
warning: peer certificate won't be verified in this SSL session
Debug.rb
Emacs support available.

(irb):4:
(rdb:1) pp ok
false
(rdb:1) pp context
#<OpenSSL::X509::StoreContext:0x107271d90>
(rdb:1) pp context.error
27
(rdb:1) pp context.error_string
"certificate not trusted"
(rdb:1)

@djmitche
Copy link
Copy Markdown
Contributor Author

OK, I'll switch it, then.

@djmitche
Copy link
Copy Markdown
Contributor Author

Well, I made the switch. I strongly suspect the tests won't pass anymore, but I just switched to doing development on my laptop, which means a new Ruby environment, and now I can't get the tests to run at all. So, in the interests of expediency, do you mind running them and making the necessary updates (which are, I'm sure, s/error/error_string/ in a few select places).

@zaphod42
Copy link
Copy Markdown
Contributor

I've opened up another pull request (#972) with the changes that I made. Closing this out in favor of the new one.

@zaphod42 zaphod42 closed this Jul 26, 2012
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.

2 participants