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 #151

Closed
hh opened this issue Oct 4, 2015 · 21 comments
Closed

ssl_peer_fingerprint_verification for self-signed certs #151

hh opened this issue Oct 4, 2015 · 21 comments

Comments

@hh
Copy link
Contributor

hh commented Oct 4, 2015

I've coded up a working spike for this at #150 which is in use by chef/knife-windows#298 and chef-boneyard/chef-provisioning-aws#348

@hh
Copy link
Contributor Author

hh commented Oct 4, 2015

Works out of box for aws-marketplace/CIS Microsoft Windows Server 2012 R2 Benchmark v1.1.0-26bb465c-ce26-4da9-afb8-040b2f8c9a7f-ami-7a88f312.2

$ knife winrm --winrm-port 5986 --winrm-transport ssl --winrm-password 'PASSWORD' --ssl-peer-fingerprint 758C4D9AC3E77F518529443D659E1260F5720400 -m 10.113.70.228 'winrm e winrm/config/listener'
10.113.70.228 Listener
10.113.70.228     Address = *
10.113.70.228     Transport = HTTPS
10.113.70.228     Port = 5986
10.113.70.228     Hostname
10.113.70.228     Enabled = true
10.113.70.228     URLPrefix = wsman
10.113.70.228     CertificateThumbprint = 758C4D9AC3E77F518529443D659E1260F5720400
10.113.70.228     ListeningOn = 10.113.70.228, 127.0.0.1, ::1, 2001:0:9d38:90d7:2022:2eca:f58e:b91b, fe80::5efe:10.113.70.228%14, fe80::14d9:e7a5:7df7:f42e%15, fe80::2022:2eca:f58e:b91b%13
10.113.70.228 

However after I run wsus updates something changes:

$ knife winrm --winrm-port 5986 --winrm-transport ssl --winrm-password \'PASSWORD\' --ssl-peer-fingerprint 758C4D9AC3E77F518529443D659E1260F5720400 -m 10.113.70.228 \'schtasks /query /tn chef-client\'
ERROR: Failed to authenticate to ["10.113.70.228"] as Administrator
Response: WinRM::WinRMAuthorizationError
Hint: Make sure to prefix domain usernames with the correct domain name.
Hint: Local user names should be prefixed with computer name or IP address.
EXAMPLE: my_domain\user_namer
ERROR: WinRM::WinRMAuthorizationError: WinRM::WinRMAuthorizationError

(I'll try and rerun these using only the WinRM gem, but I don't yet have decent test harness for this)

@mwrock
Copy link
Member

mwrock commented Oct 5, 2015

Yeah I think for the purposes of changes to winrm, bypassing knife-windows and working directly against the winrm gem would be best. Its possible your auth errors are domain related. knife winrm uses negotiate authentication and if your server is domain joined, only domain accounts can auth via negotiate. See https://github.com/chef/knife-windows#platform-winrm-authentication-support for details. If your node is NOT domain joined, then thats not the issue.

This auth issue asside, specifying the fingerprint of the self signed RDP cert is an interesting approach. While I dont see it in your sample, I'm assuming you are installing the cert in the root cert store of the knife workstation? Otherwise I'd expect to see errors regarding the cert not being signed by a valid authority.

While it is convenient to use the RDP cert since it is already created, I'd be inclined to:

  1. Go ahead and use that cert if you can use the computer's DNS name. Since that is the CN subject name of the rdp cert, using this name will prevent verification errors. Of course this will not work if you are in an entirely different network from the remote node - a common scenario.
  2. Create a new self-signed cert using the FQDN of the remote node (or its ip). I know its an extra step but if you are on 2012R2 or later the New-SelfSignedCertificate cmdlet makes this pretty darn simple.

I am NOT an expert in X509 certificate security, so while your suggested approach "seems" legit, I'm always nervous to try out something new in this area. Its definitely not the convention. For example, Azure creates a cert for its IAAS VMs and allows you to grab it via Get-AzureCertificate. I on't believe AWS does anything similar, but some seem to go to the trouble and create the cert in user-data.

@mwrock
Copy link
Member

mwrock commented Oct 5, 2015

Been tossing this idea around and I do think it adds convenience to an already friction saturated transport. Explicitly providing the thumbprint on the command line seems like a reasonably safe approach.

@sneal thoughts?

@hh do you mind cleaning up the pry/commenting?

@hh
Copy link
Contributor Author

hh commented Oct 5, 2015

I wish there was the AWS equivalent of Get-AzureCertificate, the closest thing we have is looking at the console output and grabbing the fingerprint... hence my taking this approach:

aws ec2 get-console-output --instance-id i-8d90b24b | jq .Output | sed -e s:\\\\r\\\\n:\\n:g | sed -e 's/^\"//' | grep RDPCERT
2015/10/04 06:48:28Z: RDPCERTIFICATE-SUBJECTNAME: IP-0A7146E4
2015/10/04 06:48:28Z: RDPCERTIFICATE-THUMBPRINT: 758C4D9AC3E77F518529443D659E1260F5720400

We don't actually have a copy of the certificate until the instance is connectable.

Then we verify the SSL Fingerprint in a couple places:

I do see the benefits to generating self-signed certs, however:

  • It only works easily on 2012+
  • We don't have access even a certificate fingerprint on initial connection to the host... (I feel the trust chain has been broken at this point)
  • It can be difficult to know what the CN should be at provisioning time

Overall I think it would be nice to have an option to do it either way.

@mwrock
Copy link
Member

mwrock commented Oct 5, 2015

This also reminds me of how the Fog API works with VMWare/vSphere: https://github.com/fog/fog/blob/master/lib/fog/vsphere/compute.rb#L455

@hh
Copy link
Contributor Author

hh commented Oct 5, 2015

I'm digging the way they rescue then compare the fingerprint

@hh
Copy link
Contributor Author

hh commented Oct 5, 2015

#150 cleaned up a bit

@mwrock
Copy link
Member

mwrock commented Oct 5, 2015

Yeah. We could duplicate here where in the event that that the SSL negotiation succeeds, we just wouldn't even deal with the thumbprint. However, it would seem wrong not to check it if you explicitly are saying THIS is the cert I want to use.

@hh
Copy link
Contributor Author

hh commented Oct 5, 2015

Yes, if you specify the exact cert you want THAT cert. You don't care if something in your cert chain says it's ok.

@hh
Copy link
Contributor Author

hh commented Oct 5, 2015

Going to debug that wsus_update issue noted above.... the machine isn't joining a domain. Maybe it's something specific to the CIS 2012 Benchmark: https://benchmarks.cisecurity.org/downloads/show-single/?file=windows2012R2.200

@hh
Copy link
Contributor Author

hh commented Oct 26, 2015

I've not run into any issue with the fingerprinting so far with this patch. I'd like to get at checkpoint here with others and see if we are ready to merge.

@mwrock
Copy link
Member

mwrock commented Oct 26, 2015

Here is what I see for merge requirements:

  • get the PR green (rubocop stuff)
  • Add tests
  • +1 from @sneal

@hh
Copy link
Contributor Author

hh commented Nov 30, 2015

PR is green (rubocop done).
Not sure on how to approach testing, any direction would be useful.

@hh
Copy link
Contributor Author

hh commented Nov 30, 2015

I've looked at the current tests... these changes actually connect to a tcp socket and retrieve the certificate / fingerprint. Is this something we want to stub out in ssl_socket.connect just for testing?

@hh
Copy link
Contributor Author

hh commented Dec 1, 2015

@sneal / @mwrock I've cleaned up the PR regarding the rubocop stuff and added a simple test. At some point we'll want to add integration tests, but I wanted to see if we could merge with what we have now.

@hh
Copy link
Contributor Author

hh commented Dec 1, 2015

I tried to create an integration test but I'm not sure how we would create a winrm listener that we would know the fingerprint / certificate for. (on ec2 it's provided via the api) I thought about running a helper but I'm not sure how / where to save it and make sure it doesn't run again against a box it's already run on:

  def create_https_listener
    winrm = winrm_connection
    #reuse the rdp certificate for winrm:
    winrm.run_powershell_script(<<POWERSHELL)
netsh advfirewall firewall add rule name="WinRM 5986" protocol=TCP dir=in localport=5986 action=allow

$SourceStoreScope = 'LocalMachine'
$SourceStorename = 'Remote Desktop'

$SourceStore = New-Object  -TypeName System.Security.Cryptography.X509Certificates.X509Store  -ArgumentList $SourceStorename, $SourceStoreScope
$SourceStore.Open([System.Security.Cryptography.X509Certificates.OpenFlags]::ReadOnly)

$cert = $SourceStore.Certificates | Where-Object  -FilterScript {
$_.subject -like '*'
}

$DestStoreScope = 'LocalMachine'
$DestStoreName = 'My'

$DestStore = New-Object  -TypeName System.Security.Cryptography.X509Certificates.X509Store  -ArgumentList $DestStoreName, $DestStoreScope
$DestStore.Open([System.Security.Cryptography.X509Certificates.OpenFlags]::ReadWrite)
$DestStore.Add($cert)

$SourceStore.Close()
$DestStore.Close()

winrm create winrm/config/listener?Address=*+Transport=HTTPS  `@`{Hostname=`"($certId)`"`;CertificateThumbprint=`"($cert.Thumbprint)`"`}

net stop winrm
sc config winrm start=auto
net start winrm
POWERSHELL
  end

@hh
Copy link
Contributor Author

hh commented Dec 3, 2015

@sneal / @mwrock hoping to touch base on this and the overall approach to getting fingerprint support for chef-provisioning-aws, knife winrm (ec2), and test-kitchen. I'm hoping to be at the chef-provisioning Office Hours today.

@sneal
Copy link
Member

sneal commented Dec 3, 2015

@hh I'm fine with the approach. It seems like the most straight forward way to implement this with little risk. I'm letting you and @mwrock drive this.

@hh
Copy link
Contributor Author

hh commented Dec 3, 2015

I think it might be easiest just to update the base image for this and include a known ssl fiingerprint...

@mwrock
Copy link
Member

mwrock commented Dec 3, 2015

sorry @hh but I've been a bit swamped the last few days.I'll do some testing on my end and we'll try to get this merged soon. Thanks!

@mwrock
Copy link
Member

mwrock commented Jan 11, 2016

fixed via #170

@mwrock mwrock closed this as completed Jan 11, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants