Merge pull request #1617 from dosire/reject-ssh-keys-that-break-gitolite

Reject ssh keys that break gitolite

app/models/key.rb

@@ -14,7 +14,7 @@ class Key < ActiveRecord::Base
   before_save :set_identifier
   before_validation :strip_white_space
   delegate :name, :email, to: :user, prefix: true
-  validate :unique_key
+  validate :unique_key, :fingerprintable_key
 
   def strip_white_space
     self.key = self.key.strip unless self.key.blank?
@@ -28,6 +28,21 @@ class Key < ActiveRecord::Base
     end
   end
 
+  def fingerprintable_key
+    return true unless key # Don't test if there is no key.
+    # `ssh-keygen -lf /dev/stdin <<< "#{key}"` errors with: redirection unexpected
+    file = Tempfile.new('key_file')
+    begin
+      file.puts key
+      file.rewind
+      fingerprint_output = `ssh-keygen -lf #{file.path} 2>&1` # Catch stderr.
+    ensure
+      file.close
+      file.unlink # deletes the temp file
+    end
+    errors.add(:key, "can't be fingerprinted") if fingerprint_output.match("failed")
+  end
+
   def set_identifier
     if is_deploy_key
       self.identifier = "deploy_#{Digest::MD5.hexdigest(key)}"

features/steps/profile/profile_ssh_keys.rb

@@ -13,7 +13,7 @@ class ProfileSshKeys < Spinach::FeatureSteps
 
   And 'I submit new ssh key "Laptop"' do
     fill_in "key_title", :with => "Laptop"
-    fill_in "key_key", :with => "ssh-rsa publickey234="
+    fill_in "key_key", :with => "ssh-rsa AAAAB3NzaC1yc2EAAAABIwAAAQEAzrEJUIR6Y03TCE9rIJ+GqTBvgb8t1jI9h5UBzCLuK4VawOmkLornPqLDrGbm6tcwM/wBrrLvVOqi2HwmkKEIecVO0a64A4rIYScVsXIniHRS6w5twyn1MD3sIbN+socBDcaldECQa2u1dI3tnNVcs8wi77fiRe7RSxePsJceGoheRQgC8AZ510UdIlO+9rjIHUdVN7LLyz512auAfYsgx1OfablkQ/XJcdEwDNgi9imI6nAXhmoKUm1IPLT2yKajTIC64AjLOnE0YyCh6+7RFMpiMyu1qiOCpdjYwTgBRiciNRZCH8xIedyCoAmiUgkUT40XYHwLuwiPJICpkAzp7Q== user@laptop"
     click_button "Save"
   end
 

spec/factories.rb

@@ -89,11 +89,7 @@ FactoryGirl.define do
   factory :key do
     title
     key do
-      """
-      ssh-rsa AAAAB3NzaC1yc2EAAAABJQAAAIEAiPWx6WM4lhHNedGfBpPJNPpZ7yKu+dnn1SJejgt4
-      596k6YjzGGphH2TUxwKzxcKDKKezwkpfnxPkSMkuEspGRt/aZZ9wa++Oi7Qkr8prgHc4
-      soW6NUlfDzpvZK2H5E7eQaSeP3SAwGmQKUFHCddNaP0L+hM7zhFNzjFvpaMgJw0=
-      """
+      "ssh-rsa AAAAB3NzaC1yc2EAAAABJQAAAIEAiPWx6WM4lhHNedGfBpPJNPpZ7yKu+dnn1SJejgt4596k6YjzGGphH2TUxwKzxcKDKKezwkpfnxPkSMkuEspGRt/aZZ9wa++Oi7Qkr8prgHc4soW6NUlfDzpvZK2H5E7eQaSeP3SAwGmQKUFHCddNaP0L+hM7zhFNzjFvpaMgJw0="
     end
 
     factory :deploy_key do
@@ -103,6 +99,12 @@ FactoryGirl.define do
     factory :personal_key do
       user
     end
+
+    factory :key_with_a_space_in_the_middle do
+      key do
+        "ssh-rsa AAAAB3NzaC1yc2EAAAABJQAAAIEAiPWx6WM4lhHNedGfBpPJNPpZ7yKu+dnn1SJejgt4596k6YjzGGphH2TUxwKzxcKDKKezwkpfnxPkSMkuEspGRt/aZZ9wa ++Oi7Qkr8prgHc4soW6NUlfDzpvZK2H5E7eQaSeP3SAwGmQKUFHCddNaP0L+hM7zhFNzjFvpaMgJw0="
+      end
+    end
   end
 
   factory :milestone do

spec/factories_spec.rb

@@ -1,6 +1,7 @@
 require 'spec_helper'
 
 FactoryGirl.factories.map(&:name).each do |factory_name|
+  next if :key_with_a_space_in_the_middle == factory_name
   describe "#{factory_name} factory" do
     it 'should be valid' do
       build(factory_name).should be_valid

spec/models/key_spec.rb

@@ -51,4 +51,16 @@ describe Key do
       end
     end
   end
+
+  context "validate it is a fingerprintable key" do
+    let(:user) { Factory.create(:user) }
+
+    it "accepts the fingerprintable key" do
+      build(:key, user: user).should be_valid
+    end
+
+    it "rejects the unfingerprintable key" do
+      build(:key_with_a_space_in_the_middle).should_not be_valid
+    end
+  end
 end

spec/requests/projects_deploy_keys_spec.rb

@@ -42,7 +42,7 @@ describe "Projects", "DeployKeys" do
     describe "fill in" do
       before do
         fill_in "key_title", with: "laptop"
-        fill_in "key_key", with: "ssh-rsa publickey234="
+        fill_in "key_key", with: "ssh-rsa AAAAB3NzaC1yc2EAAAABIwAAAQEAzrEJUIR6Y03TCE9rIJ+GqTBvgb8t1jI9h5UBzCLuK4VawOmkLornPqLDrGbm6tcwM/wBrrLvVOqi2HwmkKEIecVO0a64A4rIYScVsXIniHRS6w5twyn1MD3sIbN+socBDcaldECQa2u1dI3tnNVcs8wi77fiRe7RSxePsJceGoheRQgC8AZ510UdIlO+9rjIHUdVN7LLyz512auAfYsgx1OfablkQ/XJcdEwDNgi9imI6nAXhmoKUm1IPLT2yKajTIC64AjLOnE0YyCh6+7RFMpiMyu1qiOCpdjYwTgBRiciNRZCH8xIedyCoAmiUgkUT40XYHwLuwiPJICpkAzp7Q== user@laptop"
       end
 
       it { expect { click_button "Save" }.to change {Key.count}.by(1) }