Fallback to CRLs if there is OCSP parsing or timeout error (#184)

**Why**: If we fail to get a proper response from an OCSP server, we should fallback to our CRLs instead of marking the cert invalid.

app/models/certificate.rb

@@ -64,14 +64,6 @@ def validate_cert
   end
 
   def validate_untrusted_root
-    validate_untrusted_root_with_exceptions
-  rescue OpenSSL::OCSP::OCSPError
-    'ocsp_error'
-  rescue Timeout::Error
-    'timeout'
-  end
-
-  def validate_untrusted_root_with_exceptions
     if self_signed?
       'self-signed cert'
     elsif !signature_verified?

app/services/ocsp_service.rb

@@ -81,9 +81,12 @@ def handle_response(response, limit)
 
   def make_single_http_request(uri, request, retries = 1)
     make_single_http_request!(uri, request)
-  rescue Errno::ECONNRESET
+  rescue Errno::ECONNRESET, Timeout::Error => e
     retries -= 1
-    return if retries.negative?
+    if retries.negative?
+      log_ocsp_error(:ocsp_timeout, e)
+      return
+    end
     sleep(1)
     retry
   end
@@ -98,5 +101,18 @@ def make_single_http_request!(uri, request)
 
   def process_http_response_body(body)
     OpenSSL::OCSP::Response.new(body) if body.present?
+  rescue OpenSSL::OCSP::OCSPError => e
+    log_ocsp_error(:ocsp_response_error, e)
+    nil
+  end
+
+  def log_ocsp_error(error_type, exception)
+    info = {
+      error_type: error_type,
+      ocsp_url_for_subject: ocsp_url_for_subject,
+      key_id: subject.key_id,
+    }
+    Rails.logger.warn("OCSP error: #{info.to_json}")
+    NewRelic::Agent.notice_error(exception)
   end
 end

spec/controllers/identify_controller_spec.rb

@@ -285,21 +285,20 @@
             stub_request(:post, 'http://ocsp.example.com/').to_timeout
           end
 
-          it 'returns a token as timeout' do
+          it 'returns a valid response after checking CRLs' do
             ca = CertificateAuthority.find_or_create_for_certificate(
                 Certificate.new(root_cert)
             )
 
             @request.headers['X-Client-Cert'] = CGI.escape(client_cert_pem)
-            expect(CertificateLoggerService).to receive(:log_certificate)
 
             get :create, params: { nonce: '123', redirect_uri: 'http://example.com/' }
             expect(response).to have_http_status(:found)
             expect(response.has_header?('Location')).to be_truthy
             expect(token).to be_truthy
 
-            expect(token_contents['error']).to eq 'certificate.timeout'
-            expect(token_contents['key_id']).to be_present
+            expect(token_contents['error']).to be_nil
+            expect(token_contents['uuid']).to be_present
             expect(token_contents['nonce']).to eq '123'
           end
         end
@@ -310,21 +309,20 @@
               to_return(status: 200, body: 'not-a-valid-cert', headers: {})
           end
 
-          it 'returns a token as ocsp error' do
+          it 'returns a valid response after checking CRLs' do
             ca = CertificateAuthority.find_or_create_for_certificate(
                 Certificate.new(root_cert)
             )
 
             @request.headers['X-Client-Cert'] = CGI.escape(client_cert_pem)
-            expect(CertificateLoggerService).to receive(:log_certificate)
 
             get :create, params: { nonce: '123', redirect_uri: 'http://example.com/' }
             expect(response).to have_http_status(:found)
             expect(response.has_header?('Location')).to be_truthy
             expect(token).to be_truthy
 
-            expect(token_contents['error']).to eq 'certificate.ocsp_error'
-            expect(token_contents['key_id']).to be_present
+            expect(token_contents['error']).to be_nil
+            expect(token_contents['uuid']).to be_present
             expect(token_contents['nonce']).to eq '123'
           end
         end

spec/services/ocsp_service_spec.rb

@@ -278,6 +278,16 @@
 
     let(:status) { :invalid }
 
+    before do
+      described_class.clear_ocsp_response_cache
+      allow(IO).to receive(:binread).with(ca_file_path).and_return(ca_file_content)
+      allow(Figaro.env).to receive(:trusted_ca_root_identifiers).and_return(
+        root_cert_key_ids.join(',')
+      )
+      certificate_store.clear_root_identifiers
+      certificate_store.add_pem_file(ca_file_path)
+    end
+
     context "that isn't an OCSP response at all" do
       before(:each) do
         stub_request(:post, 'http://ocsp.example.com/').