From 4168854d769b508cc3789ef1e8b28a7ae5c1da85 Mon Sep 17 00:00:00 2001 From: Kyle Drake Date: Wed, 3 Sep 2014 17:14:11 -0700 Subject: [PATCH] fixes and improvements to ssl verification --- Rakefile | 6 +- app.rb | 89 +++++++--- migrations/042_pull_intermediate_ssl.rb | 9 + models/site.rb | 2 +- tests/acceptance/settings_tests.rb | 217 ++++++++++++++---------- views/custom_domain.erb | 9 +- 6 files changed, 207 insertions(+), 125 deletions(-) create mode 100644 migrations/042_pull_intermediate_ssl.rb diff --git a/Rakefile b/Rakefile index 75942c3b..149c2c22 100644 --- a/Rakefile +++ b/Rakefile @@ -91,21 +91,21 @@ end desc 'Produce SSL config package for proxy' task :buildssl => [:environment] do - sites = Site.select(:id, :username, :domain, :ssl_key, :ssl_cert, :ssl_cert_intermediate). + sites = Site.select(:id, :username, :domain, :ssl_key, :ssl_cert). exclude(domain: nil). exclude(ssl_key: nil). exclude(ssl_cert: nil). - exclude(ssl_cert_intermediate: nil). all payload = [] + FileUtils.rm './files/sslsites.zip' Zip::Archive.open('./files/sslsites.zip', Zip::CREATE) do |ar| ar.add_dir 'ssl' sites.each do |site| ar.add_buffer "ssl/#{site.username}.key", site.ssl_key - ar.add_buffer "ssl/#{site.username}.crt", "#{site.ssl_cert_intermediate}\n\n#{site.ssl_cert}" + ar.add_buffer "ssl/#{site.username}.crt", site.ssl_cert payload << {username: site.username, domain: site.domain} end diff --git a/app.rb b/app.rb index 6ffb430b..e4d95256 100644 --- a/app.rb +++ b/app.rb @@ -489,8 +489,8 @@ end post '/settings/ssl' do require_login - unless params[:key] && params[:cert] && params[:cert_intermediate] - flash[:error] = 'SSL key, certificate, and intermediate certificate are required to continue.' + unless params[:key] && params[:cert] + flash[:error] = 'SSL key and certificate are required.' redirect '/custom_domain' end @@ -506,34 +506,83 @@ post '/settings/ssl' do redirect '/custom_domain' end - begin - cert = OpenSSL::X509::Certificate.new params[:cert][:tempfile].read - rescue => e - flash[:error] = 'Could not process SSL certificate, file may be incorrect or damaged.' + certs_string = params[:cert][:tempfile].read + + cert_array = certs_string.lines.slice_before(/-----BEGIN CERTIFICATE-----/).to_a.collect {|a| a.join} + + if cert_array.empty? + flash[:error] = 'Cert file does not contain any certificates.' redirect '/custom_domain' end - if cert.not_after < Time.now - flash[:error] = 'SSL Certificate is expired, please create a new one.' + cert_valid_for_domain = false + + cert_array.each do |cert_string| + begin + cert = OpenSSL::X509::Certificate.new cert_string + rescue => e + flash[:error] = 'Could not process SSL certificate, file may be incorrect or damaged.' + redirect '/custom_domain' + end + + if cert.not_after < Time.now + flash[:error] = 'SSL Certificate has expired, please create a new one.' + redirect '/custom_domain' + end + + cert_cn = cert.subject.to_a.select {|a| a.first == 'CN'}.flatten[1] + cert_valid_for_domain = true if cert_cn && cert_cn.match(current_site.domain) + end + + unless cert_valid_for_domain + flash[:error] = "Your certificate CN (common name) does not match your domain: #{current_site.domain}" redirect '/custom_domain' end - cert_cn = cert.subject.to_a.select {|a| a.first == 'CN'}.flatten[1] - if !cert_cn.match(current_site.domain) - flash[:error] = "The certificate CN (common name) #{cert_cn} does not match your domain: #{current_site.domain}" - redirect '/custom_domain' - end + # Everything else was worse. - begin - cert_intermediate = OpenSSL::X509::Certificate.new params[:cert_intermediate][:tempfile].read - rescue => e - flash[:error] = 'Could not process intermediate SSL certificate, file may be incorrect or damaged.' - redirect '/custom_domain' + crtfile = Tempfile.new 'crtfile' + crtfile.write cert_array.join + crtfile.close + + keyfile = Tempfile.new 'keyfile' + keyfile.write key.to_pem + keyfile.close + + if ENV['TRAVIS'] != 'true' + nginx_testfile = Tempfile.new 'nginx_testfile' + nginx_testfile.write %{ + pid /tmp/throwaway.pid; + events {} + http { + access_log off; + error_log /dev/null error; + server { + listen 60000 ssl; + server_name #{current_site.domain} *.#{current_site.domain}; + ssl_certificate #{crtfile.path}; + ssl_certificate_key #{keyfile.path}; + } + } + } + nginx_testfile.close + + line = Cocaine::CommandLine.new( + "nginx", "-t -c :path", + expected_outcodes: [0], + swallow_stderr: true + ) + + begin + output = line.run path: nginx_testfile.path + rescue Cocaine::ExitStatusError => e + flash[:error] = "There is something wrong with your certificate, please check with your issuing CA." + redirect '/custom_domain' + end end current_site.ssl_key = key.to_pem - current_site.ssl_cert = cert.to_pem - current_site.ssl_cert_intermediate = cert_intermediate.to_pem + current_site.ssl_cert = cert_array.join current_site.save flash[:success] = 'Updated SSL key/certificate.' diff --git a/migrations/042_pull_intermediate_ssl.rb b/migrations/042_pull_intermediate_ssl.rb new file mode 100644 index 00000000..3bb06542 --- /dev/null +++ b/migrations/042_pull_intermediate_ssl.rb @@ -0,0 +1,9 @@ +Sequel.migration do + up { + DB.drop_column :sites, :ssl_cert_intermediate + } + + down { + DB.add_column :sites, :ssl_cert_intermediate, :text + } +end \ No newline at end of file diff --git a/models/site.rb b/models/site.rb index 42c4652d..cb66080f 100644 --- a/models/site.rb +++ b/models/site.rb @@ -799,7 +799,7 @@ class Site < Sequel::Model end def ssl_installed? - ssl_key && ssl_cert && ssl_cert_intermediate + ssl_key && ssl_cert end def to_rss diff --git a/tests/acceptance/settings_tests.rb b/tests/acceptance/settings_tests.rb index 18b1d722..e521d9f3 100644 --- a/tests/acceptance/settings_tests.rb +++ b/tests/acceptance/settings_tests.rb @@ -1,83 +1,101 @@ require_relative './environment.rb' +def generate_ssl_certs(opts={}) + # https://github.com/kyledrake/ruby-openssl-cheat-sheet/blob/master/certificate_authority.rb + res = {} + + ca_keypair = OpenSSL::PKey::RSA.new(2048) + ca_cert = OpenSSL::X509::Certificate.new + ca_cert.not_before = Time.now + ca_cert.subject = OpenSSL::X509::Name.new([ + ["C", "US"], + ["ST", "Oregon"], + ["L", "Portland"], + ["CN", "Neocities CA"] + ]) + ca_cert.issuer = ca_cert.subject + ca_cert.not_after = Time.now + 1000000000 # 40 or so years + ca_cert.serial = 1 + ca_cert.public_key = ca_keypair.public_key + ef = OpenSSL::X509::ExtensionFactory.new + ef.subject_certificate = ca_cert + ef.issuer_certificate = ca_cert + # Read more about the various extensions here: http://www.openssl.org/docs/apps/x509v3_config.html + ca_cert.add_extension(ef.create_extension("basicConstraints", "CA:TRUE", true)) + ca_cert.add_extension(ef.create_extension("keyUsage","keyCertSign, cRLSign", true)) + ca_cert.add_extension(ef.create_extension("subjectKeyIdentifier", "hash", false)) + ca_cert.add_extension(ef.create_extension("authorityKeyIdentifier", "keyid:always", false)) + ca_cert.sign(ca_keypair, OpenSSL::Digest::SHA256.new) + res[:ca_cert] = ca_cert + res[:ca_keypair] = ca_keypair + + ca_cert = OpenSSL::X509::Certificate.new(res[:ca_cert].to_pem) + our_cert_keypair = OpenSSL::PKey::RSA.new(2048) + our_cert_req = OpenSSL::X509::Request.new + our_cert_req.subject = OpenSSL::X509::Name.new([ + ["C", "US"], + ["ST", "Oregon"], + ["L", "Portland"], + ["O", "Neocities User"], + ["CN", "*.#{opts[:domain]}"] + ]) + our_cert_req.public_key = our_cert_keypair.public_key + our_cert_req.sign our_cert_keypair, OpenSSL::Digest::SHA1.new + our_cert = OpenSSL::X509::Certificate.new + our_cert.subject = our_cert_req.subject + our_cert.issuer = ca_cert.subject + our_cert.not_before = Time.now + if opts[:expired] + our_cert.not_after = Time.now - 100000000 + else + our_cert.not_after = Time.now + 100000000 + end + our_cert.serial = 123 # Should be an unique number, the CA probably has a database. + our_cert.public_key = our_cert_req.public_key + # To make the certificate valid for both wildcard and top level domain name, we need an extension. + ef = OpenSSL::X509::ExtensionFactory.new + ef.subject_certificate = our_cert + ef.issuer_certificate = ca_cert + our_cert.add_extension(ef.create_extension("subjectAltName", "DNS:#{@domain}, DNS:*.#{@domain}", false)) + our_cert.sign res[:ca_keypair], OpenSSL::Digest::SHA1.new + + our_cert_tmpfile = Tempfile.new 'our_cert' + our_cert_tmpfile.write our_cert.to_pem + our_cert_tmpfile.close + res[:cert_path] = our_cert_tmpfile.path + + our_cert_keypair_tmpfile = Tempfile.new 'our_cert_keypair' + our_cert_keypair_tmpfile.write our_cert_keypair.to_pem + our_cert_keypair_tmpfile.close + res[:key_path] = our_cert_keypair_tmpfile.path + + ca_cert_tmpfile = Tempfile.new 'ca_cert' + ca_cert_tmpfile.write res[:ca_cert].to_pem + ca_cert_tmpfile.close + res[:cert_intermediate_path] = ca_cert_tmpfile.path + + combined_cert_tmpfile = Tempfile.new 'combined_cert' + combined_cert_tmpfile.write "#{File.read(res[:cert_path])}\n#{File.read(res[:cert_intermediate_path])}" + combined_cert_tmpfile.close + res[:combined_cert_path] = combined_cert_tmpfile.path + + bad_combined_cert_tmpfile = Tempfile.new 'combined_cert' + bad_combined_cert_tmpfile.write "#{File.read(res[:cert_intermediate_path])}\n#{File.read(res[:cert_path])}" + bad_combined_cert_tmpfile.close + res[:bad_combined_cert_path] = bad_combined_cert_tmpfile.path + + res +end + describe 'site/settings' do describe 'ssl' do include Capybara::DSL before do - # https://github.com/kyledrake/ruby-openssl-cheat-sheet/blob/master/certificate_authority.rb - # TODO make ca generation run only once - ca_keypair = OpenSSL::PKey::RSA.new(2048) - ca_cert = OpenSSL::X509::Certificate.new - ca_cert.not_before = Time.now - ca_cert.subject = OpenSSL::X509::Name.new([ - ["C", "US"], - ["ST", "Oregon"], - ["L", "Portland"], - ["CN", "Neocities CA"] - ]) - ca_cert.issuer = ca_cert.subject - # All issued certs will be unusuable after this time. - ca_cert.not_after = Time.now + 1000000000 # 40 or so years - ca_cert.serial = 1 - ca_cert.public_key = ca_keypair.public_key - ef = OpenSSL::X509::ExtensionFactory.new - ef.subject_certificate = ca_cert - ef.issuer_certificate = ca_cert - # Read more about the various extensions here: http://www.openssl.org/docs/apps/x509v3_config.html - ca_cert.add_extension(ef.create_extension("basicConstraints", "CA:TRUE", true)) - ca_cert.add_extension(ef.create_extension("keyUsage","keyCertSign, cRLSign", true)) - ca_cert.add_extension(ef.create_extension("subjectKeyIdentifier", "hash", false)) - ca_cert.add_extension(ef.create_extension("authorityKeyIdentifier", "keyid:always", false)) - ca_cert.sign(ca_keypair, OpenSSL::Digest::SHA256.new) - @ca_cert = ca_cert - @ca_keypair = ca_keypair - @domain = SecureRandom.uuid.gsub('-', '')+'.com' @site = Fabricate :site, domain: @domain page.set_rack_session id: @site.id - - ca_cert = OpenSSL::X509::Certificate.new(@ca_cert.to_pem) - our_cert_keypair = OpenSSL::PKey::RSA.new(2048) - our_cert_req = OpenSSL::X509::Request.new - our_cert_req.subject = OpenSSL::X509::Name.new([ - ["C", "US"], - ["ST", "Oregon"], - ["L", "Portland"], - ["O", "Neocities User"], - ["CN", "*.#{@domain}"] - ]) - our_cert_req.public_key = our_cert_keypair.public_key - our_cert_req.sign our_cert_keypair, OpenSSL::Digest::SHA1.new - our_cert = OpenSSL::X509::Certificate.new - our_cert.subject = our_cert_req.subject - our_cert.issuer = ca_cert.subject - our_cert.not_before = Time.now - our_cert.not_after = Time.now + 100000000 # 3 or so years. - our_cert.serial = 123 # Should be an unique number, the CA probably has a database. - our_cert.public_key = our_cert_req.public_key - # To make the certificate valid for both wildcard and top level domain name, we need an extension. - ef = OpenSSL::X509::ExtensionFactory.new - ef.subject_certificate = our_cert - ef.issuer_certificate = ca_cert - our_cert.add_extension(ef.create_extension("subjectAltName", "DNS:#{@domain}, DNS:*.#{@domain}", false)) - our_cert.sign @ca_keypair, OpenSSL::Digest::SHA1.new - - our_cert_tmpfile = Tempfile.new 'our_cert' - our_cert_tmpfile.write our_cert.to_pem - our_cert_tmpfile.close - @cert_path = our_cert_tmpfile.path - - our_cert_keypair_tmpfile = Tempfile.new 'our_cert_keypair' - our_cert_keypair_tmpfile.write our_cert_keypair.to_pem - our_cert_keypair_tmpfile.close - @key_path = our_cert_keypair_tmpfile.path - - ca_cert_tmpfile = Tempfile.new 'ca_cert' - ca_cert_tmpfile.write @ca_cert.to_pem - ca_cert_tmpfile.close - @cert_intermediate_path = ca_cert_tmpfile.path - + end it 'fails without domain set' do @@ -87,71 +105,82 @@ describe 'site/settings' do page.must_have_content /Cannot upload SSL certificate until domain is added/i end - it 'works with valid key, cert and intermediate cert' do + it 'fails with expired key' do + @ssl = generate_ssl_certs domain: @domain, expired: true visit '/custom_domain' + attach_file 'key', @ssl[:key_path] + attach_file 'cert', @ssl[:combined_cert_path] + click_button 'Upload SSL Key and Certificate' + page.must_have_content /ssl certificate has expired/i + end + + it 'works with valid key and unified cert' do + @ssl = generate_ssl_certs domain: @domain + visit '/custom_domain' + key = File.read @ssl[:key_path] + combined_cert = File.read @ssl[:combined_cert_path] page.must_have_content /status: inactive/i - attach_file 'key', @key_path - attach_file 'cert', @cert_path - attach_file 'cert_intermediate', @cert_intermediate_path + attach_file 'key', @ssl[:key_path] + attach_file 'cert', @ssl[:combined_cert_path] click_button 'Upload SSL Key and Certificate' page.current_path.must_equal '/custom_domain' page.must_have_content /Updated SSL/ page.must_have_content /status: installed/i @site.reload - @site.ssl_key.must_equal File.read(@key_path) - @site.ssl_cert.must_equal File.read(@cert_path) - @site.ssl_cert_intermediate.must_equal File.read(@cert_intermediate_path) + @site.ssl_key.must_equal key + @site.ssl_cert.must_equal combined_cert end it 'fails with no uploads' do visit '/custom_domain' click_button 'Upload SSL Key and Certificate' page.current_path.must_equal '/custom_domain' - page.must_have_content /ssl key.+certificate.+intermediate.+required to continue/i + page.must_have_content /ssl key.+certificate.+required/i @site.reload @site.ssl_key.must_equal nil @site.ssl_cert.must_equal nil - @site.ssl_cert_intermediate.must_equal nil end - it 'fails with encrypted key' do + it 'fails gracefully with encrypted key' do + @ssl = generate_ssl_certs domain: @domain visit '/custom_domain' attach_file 'key', './tests/files/ssl/derpie.com-encrypted.key' - attach_file 'cert', @cert_path - attach_file 'cert_intermediate', @cert_intermediate_path + attach_file 'cert', @ssl[:cert_path] click_button 'Upload SSL Key and Certificate' page.current_path.must_equal '/custom_domain' page.must_have_content /could not process ssl key/i end it 'fails with junk key' do + @ssl = generate_ssl_certs domain: @domain visit '/custom_domain' attach_file 'key', './tests/files/index.html' - attach_file 'cert', @cert_path - attach_file 'cert_intermediate', @cert_intermediate_path + attach_file 'cert', @ssl[:cert_path] click_button 'Upload SSL Key and Certificate' page.current_path.must_equal '/custom_domain' page.must_have_content /could not process ssl key/i end it 'fails with junk cert' do + @ssl = generate_ssl_certs domain: @domain visit '/custom_domain' - attach_file 'key', @key_path + attach_file 'key', @ssl[:key_path] attach_file 'cert', './tests/files/index.html' - attach_file 'cert_intermediate', @cert_intermediate_path click_button 'Upload SSL Key and Certificate' page.current_path.must_equal '/custom_domain' page.must_have_content /could not process ssl certificate/i end - it 'fails with junk intermediate cert' do - visit '/custom_domain' - attach_file 'key', @key_path - attach_file 'cert', @cert_path - attach_file 'cert_intermediate', './tests/files/index.html' - click_button 'Upload SSL Key and Certificate' - page.current_path.must_equal '/custom_domain' - page.must_have_content /could not process intermediate ssl certificate/i + if ENV['TRAVIS'] != 'true' + it 'fails with bad cert chain' do + @ssl = generate_ssl_certs domain: @domain + visit '/custom_domain' + attach_file 'key', @ssl[:key_path] + attach_file 'cert', @ssl[:bad_combined_cert_path] + click_button 'Upload SSL Key and Certificate' + page.current_path.must_equal '/custom_domain' + page.must_have_content /there is something wrong with your certificate/i + end end end diff --git a/views/custom_domain.erb b/views/custom_domain.erb index 720b753a..3ee9d4f9 100644 --- a/views/custom_domain.erb +++ b/views/custom_domain.erb @@ -46,7 +46,7 @@

Add SSL Certificate

- This allows you to add an SSL key and certificate for your domain, enabling encryption for your site (https). It can take up to 5-30 minutes for the changes to go live, so please be patient. + This allows you to add an SSL key and certificate for your domain, enabling encryption for your site (https). It can take up to 5-30 minutes for the changes to go live, so please be patient. All files must be in PEM format. If your certificate is not bundled with the root and intermediate certificates, ask your certificate provider for help on how to do that.

<% if current_site.domain.nil? || current_site.domain.empty? %> @@ -68,15 +68,10 @@

- Primary Certificate (yourdomain.com.crt): + Bundled Certificates (yourdomain.com-bundle.crt):

-

- Intermediate Certificate (ca.crt/yourca.crt): - -

-