Merge branch 'ldap-ssh-key-prefix' into 'master'

Ignore LDAP SSH key prefix and invalid keys.

See #214 and #230.

cc @job

See merge request !327

lib/gitlab/ldap/person.rb

@@ -48,7 +48,9 @@ module Gitlab
 
       def ssh_keys
         if config.sync_ssh_keys? && entry.respond_to?(config.sync_ssh_keys)
-          entry[config.sync_ssh_keys.to_sym]
+          entry[config.sync_ssh_keys.to_sym].
+            map { |key| key[/(ssh|ecdsa)-[^ ]+ [^\s]+/] }.
+            compact
         else
           []
         end

spec/lib/gitlab/ldap/access_spec.rb

@@ -73,10 +73,10 @@ describe Gitlab::LDAP::Access do
   end
 
   describe :update_ssh_keys do
-    let(:ssh_key) { 'ssh-rsa AAAAB3NzaC1yc2EAAAADAQABAAABAQCrSQHff6a1rMqBdHFt+FwIbytMZ+hJKN3KLkTtOWtSvNIriGhnTdn4rs+tjD/w+z+revytyWnMDM9dS7J8vQi006B16+hc9Xf82crqRoPRDnBytgAFFQY1G/55ql2zdfsC5yvpDOFzuwIJq5dNGsojS82t6HNmmKPq130fzsenFnj5v1pl3OJvk513oduUyKiZBGTroWTn7H/eOPtu7s9MD7pAdEjqYKFLeaKmyidiLmLqQlCRj3Tl2U9oyFg4PYNc0bL5FZJ/Z6t0Ds3i/a2RanQiKxrvgu3GSnUKMx7WIX373baL4jeM7cprRGiOY/1NcS+1cAjfJ8oaxQF/1dYj' }
-    let(:ssh_key_attribute_name) { 'sshpublickey' }
+    let(:ssh_key) { "ssh-rsa AAAAB3NzaC1yc2EAAAADAQABAAABAQCrSQHff6a1rMqBdHFt+FwIbytMZ+hJKN3KLkTtOWtSvNIriGhnTdn4rs+tjD/w+z+revytyWnMDM9dS7J8vQi006B16+hc9Xf82crqRoPRDnBytgAFFQY1G/55ql2zdfsC5yvpDOFzuwIJq5dNGsojS82t6HNmmKPq130fzsenFnj5v1pl3OJvk513oduUyKiZBGTroWTn7H/eOPtu7s9MD7pAdEjqYKFLeaKmyidiLmLqQlCRj3Tl2U9oyFg4PYNc0bL5FZJ/Z6t0Ds3i/a2RanQiKxrvgu3GSnUKMx7WIX373baL4jeM7cprRGiOY/1NcS+1cAjfJ8oaxQF/1dYj" }
+    let(:ssh_key_attribute_name) { 'altSecurityIdentities' }
     let(:entry) {
-      Net::LDAP::Entry.from_single_ldif_string("dn: cn=foo, dc=bar, dc=com\n#{ssh_key_attribute_name}: #{ssh_key}") }
+      Net::LDAP::Entry.from_single_ldif_string("dn: cn=foo, dc=bar, dc=com\n#{ssh_key_attribute_name}: SSHKey:#{ssh_key}\n#{ssh_key_attribute_name}: KerberosKey:bogus") }
 
     before do
       Gitlab::LDAP::Config.any_instance.stub(sync_ssh_keys: ssh_key_attribute_name)
@@ -84,14 +84,12 @@ describe Gitlab::LDAP::Access do
     end
 
     it "should add a SSH key if it is in LDAP but not in gitlab" do
-      entry = Net::LDAP::Entry.from_single_ldif_string("dn: cn=foo, dc=bar, dc=com\n#{ssh_key_attribute_name}: #{ssh_key}")
       Gitlab::LDAP::Adapter.any_instance.stub(:user) { Gitlab::LDAP::Person.new(entry, 'ldapmain') }
 
       expect{ access.update_ssh_keys }.to change(user.keys, :count).from(0).to(1)
     end
 
     it "should add a SSH key and give it a proper name" do
-      entry = Net::LDAP::Entry.from_single_ldif_string("dn: cn=foo, dc=bar, dc=com\n#{ssh_key_attribute_name}: #{ssh_key}")
       Gitlab::LDAP::Adapter.any_instance.stub(:user) { Gitlab::LDAP::Person.new(entry, 'ldapmain') }
 
       access.update_ssh_keys

spec/lib/gitlab/ldap/person_spec.rb

@@ -0,0 +1,74 @@
+require "spec_helper"
+
+describe Gitlab::LDAP::Person do
+
+  describe "#ssh_keys" do
+
+    let(:ssh_key) { "ssh-rsa AAAAB3NzaC1yc2EAAAADAQABAAABAQCrSQHff6a1rMqBdHFt+FwIbytMZ+hJKN3KLkTtOWtSvNIriGhnTdn4rs+tjD/w+z+revytyWnMDM9dS7J8vQi006B16+hc9Xf82crqRoPRDnBytgAFFQY1G/55ql2zdfsC5yvpDOFzuwIJq5dNGsojS82t6HNmmKPq130fzsenFnj5v1pl3OJvk513oduUyKiZBGTroWTn7H/eOPtu7s9MD7pAdEjqYKFLeaKmyidiLmLqQlCRj3Tl2U9oyFg4PYNc0bL5FZJ/Z6t0Ds3i/a2RanQiKxrvgu3GSnUKMx7WIX373baL4jeM7cprRGiOY/1NcS+1cAjfJ8oaxQF/1dYj" }
+    let(:ssh_key_attribute_name) { 'altSecurityIdentities' }
+    let(:entry) {
+      Net::LDAP::Entry.from_single_ldif_string("dn: cn=foo, dc=bar, dc=com\n#{keys}") }
+
+    subject { Gitlab::LDAP::Person.new(entry, 'ldapmain') }
+
+    before do
+      Gitlab::LDAP::Config.any_instance.stub(sync_ssh_keys: ssh_key_attribute_name)
+    end
+
+    context "when the SSH key is literal" do
+
+      let(:keys) { "#{ssh_key_attribute_name}: #{ssh_key}" }
+
+      it "includes the SSH key" do
+        expect(subject.ssh_keys).to include(ssh_key)
+      end
+    end
+
+    context "when the SSH key is prefixed" do
+
+      let(:keys) { "#{ssh_key_attribute_name}: SSHKey:#{ssh_key}" }
+
+      it "includes the SSH key" do
+        expect(subject.ssh_keys).to include(ssh_key)
+      end
+    end
+
+    context "when the SSH key is suffixed" do
+
+      let(:keys) { "#{ssh_key_attribute_name}: #{ssh_key} (SSH key)" }
+
+      it "includes the SSH key" do
+        expect(subject.ssh_keys).to include(ssh_key)
+      end
+    end
+
+    context "when the SSH key is followed by a newline" do
+
+      let(:keys) { "#{ssh_key_attribute_name}: #{ssh_key}\n" }
+
+      it "includes the SSH key" do
+        expect(subject.ssh_keys).to include(ssh_key)
+      end
+    end
+
+    context "when the key is not an SSH key" do
+
+      let(:keys) { "#{ssh_key_attribute_name}: KerberosKey:bogus" }
+
+      it "is empty" do
+        expect(subject.ssh_keys).to be_empty
+      end
+    end
+
+    context "when there are multiple keys" do
+
+      let(:keys) { "#{ssh_key_attribute_name}: #{ssh_key}\n#{ssh_key_attribute_name}: KerberosKey:bogus\n#{ssh_key_attribute_name}: ssh-rsa keykeykey" }
+
+      it "includes both SSH keys" do
+        expect(subject.ssh_keys).to include(ssh_key)
+        expect(subject.ssh_keys).to include("ssh-rsa keykeykey")
+        expect(subject.ssh_keys).not_to include("KerberosKey:bogus")
+      end
+    end
+  end
+end