Skip to content
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

ssl_peer_fingerprint_verification for self-signed certs #150

Closed
wants to merge 5 commits into from

Conversation

hh
Copy link
Contributor

@hh hh commented Oct 3, 2015

This pull request is mainly to discuss approaches to ssl fingerprint or certificate checking when self-signed cert Common Names do not match the FDQN or IP of the host we are connecting to.

conn_fingerprint = OpenSSL::Digest::SHA1.new(
resp.peer_cert.to_der).to_s.upcase
if @ssl_peer_fingerprint != conn_fingerprint
@logger.fatal("ssl fingerprint mismatch!!!!\n")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This verifies connections ongoing. See #151

@mwrock
Copy link
Member

mwrock commented Oct 21, 2015

After spending a good chunk of time writing this winrm blog post and most of the time getting some SSL scenarios working, I'm even more of a fan of this idea.

For instance, using New-SelfSignedCertificate available on 2012 and up does not work if exported to a PEM and then passed to the :ca_trust_path because it is not identified as a CA. This also includes the RDP cert already on the box. Its a pain to either download the tooling or supply the 50 lines of C# needed to add that constraint.

If we could simply pass a known thumbprint it would be much less friction than throwing self signed certs around and much safer than HTTP or disabled peer verification.

One could also submit a small vagrant pr to allow adding HTTPS attributes to a Vagrantfile making vagrant scenarios easier to secure and reducing the need to allow unencrypted basic auth.

We do need to add some tests and get the rubocop happy.

@sneal
Copy link
Member

sneal commented Oct 21, 2015

@mwrock What about 50 lines of PowerShell that just uses the .NET framework classes to do the work of generating the appropriate self signed cert? Everyone already needs a PS script to enable WinRM anyway.

@mwrock
Copy link
Member

mwrock commented Oct 21, 2015

Yeah powershell works too :) Here is a great powershell function that has more granular control than New-SelfSignedCertificate which works great with an openssl client if you ad -IsCA $true.

However, its one more thing to download or pack into your image prep artifacts.

This model proposed by @hh is very similar to the way one connects to vSphere using the Fog API. With this, one could reuse the cert that windows creates for remote desktop. Just removes another layer of friction.

@hh hh force-pushed the ssl_self_signed_verify branch from 7859a7c to 33a26d4 Compare November 30, 2015 19:10
@hh
Copy link
Contributor Author

hh commented Nov 30, 2015

@mwrock I cleaned up the rubocop, I'm not yet familiar enough with the testing framework. I'm taking a look now, but any useful/pointed directions would be great.

@hh hh changed the title notes around a possible lazy approach to self-signed verification add support for self-signed fingerprint verification Dec 1, 2015
@hh hh changed the title add support for self-signed fingerprint verification ssl_peer_fingerprint_verification for self-signed certs Dec 1, 2015
resp = @httpcli.post(@endpoint, message, hdr)
log_soap_message(resp.http_body.content)
verify_ssl_fingerprint(resp.peer_cert)
Copy link
Member

Choose a reason for hiding this comment

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

You need a if @ssl_peer_fingerprint here so it is not called for normal requests. Or perhaps even better, return from that method if its nil.

@mwrock
Copy link
Member

mwrock commented Jan 10, 2016

Merged in with #170

@mwrock mwrock closed this Jan 10, 2016
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.

3 participants