Skip to content

Offer better errors for certificate validation errors#917

Closed
djmitche wants to merge 2 commits intopuppetlabs:masterfrom
djmitche:better-ssl-verify-error-message
Closed

Offer better errors for certificate validation errors#917
djmitche wants to merge 2 commits intopuppetlabs:masterfrom
djmitche:better-ssl-verify-error-message

Conversation

@djmitche
Copy link
Copy Markdown
Contributor

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.

This is my first patch to ruby, and I'm about 45 minutes away from first learning about Mocha, so be kind! But I take review comments well :)

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.
@djmitche
Copy link
Copy Markdown
Contributor Author

This should probably be on the 2.7.x branch? It applies cleanly there, anyway.

@zaphod42
Copy link
Copy Markdown
Contributor

We've added this to our trello board. Hopefully we can get to it this week.

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 think we can eliminate the "this is often..." verbiage on line 100 and therefore simplify out this if statement -- that message was added in an attempt to be helpful, but since we never actually check whether the validity period is the problem, it (according to the support team) causes as much confusion as it solves.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Sounds good.

This also removes the test for time sync, and adds an `errors` method
to the ssl_context stub.
@djmitche
Copy link
Copy Markdown
Contributor Author

Updated based on the comments above.

@HAIL9000
Copy link
Copy Markdown
Contributor

@djmitche Could you rebase this off of 2.7.x and retarget it there? That means you'll have to close this pull request and open a new one.

Let me know if you have any questions, thanks for your submission!

@HAIL9000
Copy link
Copy Markdown
Contributor

@djmitche Also, if you could create a ticket for this issue and sign the CLA that would be awesome!

@djmitche
Copy link
Copy Markdown
Contributor Author

I've already signed the CLA. I'll make a ticket and a new pull req. Thanks!

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.

4 participants