fix crash when fetched key is rejected (e.g. too short, etc), report errors in the request, prevent most crashes by catching exceptions.

5 files changed, 41 insertions, 13 deletions

diff --git a/lib/nickserver/hkp/fetch_key.rb b/lib/nickserver/hkp/fetch_key.rb
index 6f91d8c..c24b2c7 100644
--- a/lib/nickserver/hkp/fetch_key.rb
+++ b/lib/nickserver/hkp/fetch_key.rb
@@ -36,7 +36,7 @@ module Nickserver; module HKP
         end
       }
       http.errback {
-        self.fail 0, http.error
+        self.fail 500, http.error
       }
     end
 
diff --git a/lib/nickserver/hkp/fetch_key_info.rb b/lib/nickserver/hkp/fetch_key_info.rb
index b341aad..2cfff43 100644
--- a/lib/nickserver/hkp/fetch_key_info.rb
+++ b/lib/nickserver/hkp/fetch_key_info.rb
@@ -17,12 +17,17 @@ module Nickserver; module HKP
       params = {:op => 'vindex', :search => uid, :exact => 'on', :options => 'mr', :fingerprint => 'on'}
       EventMachine::HttpRequest.new(Config.hkp_url).get(:query => params).callback {|http|
         if http.response_header.status != 200
-          self.fail http.response_header.status, "Could net fetch keyinfo"
+          self.fail http.response_header.status, "Could net fetch keyinfo."
         else
-          self.succeed parse(uid, http.response)
+          keys, errors = parse(uid, http.response)
+          if keys.empty?
+            self.fail 500, errors.join("\n")
+          else
+            self.succeed keys
+          end
         end
       }.errback {|http|
-        self.fail 0, http.error
+        self.fail 500, http.error
       }
       self
     end
@@ -33,33 +38,36 @@ module Nickserver; module HKP
     #  vindex_result -- raw output from a vindex hkp query (machine readable)
     #
     # returns:
-    #   an array of eligible keys (as HKPKeyInfo objects) matching uid.
+    #   an array of:
+    #   [0] -- array of eligible keys (as HKPKeyInfo objects) matching uid.
+    #   [1] -- array of error messages
     #
     # keys are eliminated from eligibility for a number of reasons, including expiration,
     # revocation, uid match, key length, and so on...
     #
     def parse(uid, vindex_result)
       keys = []
+      errors = []
       now = Time.now
       vindex_result.scan(MATCH_PUB_KEY).each do |match|
         key_info = KeyInfo.new(match[0])
         if key_info.uids.include?(uid)
-          if key_info.keylen <= 1024
-            #puts 'key length is too short'
+          if key_info.keylen < 2048
+            errors << "Ignoring key #{key_info.keyid} for #{uid}: key length is too short."
           elsif key_info.expired?
-            #puts 'ignoring expired key'
+            errors << "Ignoring key #{key_info.keyid} for #{uid}: key expired."
           elsif key_info.revoked?
-            #puts 'ignoring revoked key'
+            errors << "Ignoring key #{key_info.keyid} for #{uid}: key revoked."
           elsif key_info.disabled?
-            #puts 'ignoring disabled key'
+            errors << "Ignoring key #{key_info.keyid} for #{uid}: key disabled."
           elsif key_info.expirationdate && key_info.expirationdate < now
-            #puts 'ignoring expired key'
+            errors << "Ignoring key #{key_info.keyid} for #{uid}: key expired"
           else
             keys << key_info
           end
         end
       end
-      keys
+      [keys, errors]
     end
   end
 
diff --git a/lib/nickserver/server.rb b/lib/nickserver/server.rb
index 8434759..7f74564 100644
--- a/lib/nickserver/server.rb
+++ b/lib/nickserver/server.rb
@@ -42,6 +42,8 @@ module Nickserver
       else
         send_key(uid)
       end
+    rescue RuntimeError => exc
+      send_error(exc.to_s)
     end
 
     private
diff --git a/test/files/short_key_vindex_result b/test/files/short_key_vindex_result
new file mode 100644
index 0000000..a410144
--- /dev/null
+++ b/test/files/short_key_vindex_result
@@ -0,0 +1,7 @@
+info:1:1
+pub:9A753A6B:17:1024:1252683790::
+uid:Tom%C3%A1s Touceda <chiiph@leap.se>:1368645663::
+uid:Tomas Touceda <chiiph@riseup.net>:1361376077::
+uid:Tom%C3%A1s Touceda <chiiph@gmail.com>:1252683790::
+uid:Tom%C3%A1s Touceda <chiiph@gentoo.org>:1270489372::
+uid:Tom%C3%A1s Touceda <chiiph@torproject.org>:1306498697::
\ No newline at end of file
diff --git a/test/unit/hkp_test.rb b/test/unit/hkp_test.rb
index 1b7b2c9..c211690 100644
--- a/test/unit/hkp_test.rb
+++ b/test/unit/hkp_test.rb
@@ -21,7 +21,7 @@ class HkpTest < MiniTest::Unit::TestCase
 
   def test_key_info_reject_keysize
     fetch_key_info :hkp_vindex_result, 'frog@leap.se' do |keys|
-      assert_equal 1, keys.length, 'should find one key'
+      assert_equal 1, keys.length, 'should find one key' # because short key gets ignored
       assert_equal '00440025', keys.first.keyid
     end
   end
@@ -67,6 +67,17 @@ class HkpTest < MiniTest::Unit::TestCase
     end
   end
 
+  def test_fetch_key_too_short
+    uid    = 'chiiph@leap.se'
+    key_id = '9A753A6B'
+
+    stub_sks_vindex_reponse(uid, :body => file_content(:short_key_vindex_result))
+    test_em_errback "Nickserver::HKP::FetchKey.new.get '#{uid}'" do |error|
+      assert_equal 500, error
+    end
+  end
+
+
   protected
 
   #