Merge cf59400 into 572ff3a

CHANGES.md

@@ -1,3 +1,10 @@
+### Unreleased
+
+* [#67](https://github.com/square/rails-auth/pull/67)
+  Remove `ca_file`, `require_cert`, and `truststore` options to X509 middleware
+  as we no longer verify the certificate chain.
+  ([@drcapulet])
+
 ### 2.2.2 (2020-07-02)
 
 * [#65](https://github.com/square/rails-auth/pull/65)

lib/rails/auth/x509/middleware.rb

@@ -3,30 +3,20 @@
 module Rails
   module Auth
     module X509
-      # Raised when certificate verification is mandatory
-      CertificateVerifyFailed = Class.new(NotAuthorizedError)
-
-      # Validates X.509 client certificates and adds credential objects for valid
-      # clients to the rack environment as env["rails-auth.credentials"]["x509"]
+      # Extracts X.509 client certificates and adds credential objects to the
+      # rack environment as env["rails-auth.credentials"]["x509"]
       class Middleware
         # Create a new X.509 Middleware object
         #
-        # @param [Object]               app next app in the Rack middleware chain
-        # @param [String]               ca_file path to the CA bundle to verify client certs with
-        # @param [Hash]                 cert_filters maps Rack environment names to cert extractors
-        # @param [Logger]               logger place to log verification successes & failures
-        # @param [Boolean]              require_cert causes middleware to raise if certs are unverified
-        # @param [OpenSSL::X509::Store] truststore (optional) provide your own truststore (for e.g. CRLs)
+        # @param [Object] app next app in the Rack middleware chain
+        # @param [Hash]   cert_filters maps Rack environment names to cert extractors
+        # @param [Logger] logger place to log certificate extraction issues
         #
         # @return [Rails::Auth::X509::Middleware] new X509 middleware instance
-        def initialize(app, ca_file: nil, cert_filters: {}, logger: nil, require_cert: false, truststore: nil)
-          raise ArgumentError, "no ca_file or truststore given" unless ca_file || truststore
-
+        def initialize(app, cert_filters: {}, logger: nil)
           @app          = app
           @cert_filters = cert_filters
           @logger       = logger
-          @require_cert = require_cert
-          @truststore   = truststore || OpenSSL::X509::Store.new.add_file(ca_file)
 
           @cert_filters.each do |key, filter|
             next unless filter.is_a?(Symbol)
@@ -53,17 +43,9 @@ def extract_credential(env)
             cert = extract_certificate_with_filter(filter, env[key])
             next unless cert
 
-            if @truststore.verify(cert)
-              log("Verified", cert)
-              return Rails::Auth::X509::Certificate.new(cert)
-            else
-              log("Verify FAILED", cert)
-              raise CertificateVerifyFailed, "verify failed: #{subject(cert)}" if @require_cert
-            end
+            return Rails::Auth::X509::Certificate.new(cert)
           end
 
-          raise CertificateVerifyFailed, "no client certificate in request" if @require_cert
-
           nil
         end
 
@@ -79,10 +61,6 @@ def extract_certificate_with_filter(filter, raw_cert)
           nil
         end
 
-        def log(message, cert)
-          @logger.debug("rails-auth: #{message} (#{subject(cert)})") if @logger
-        end
-
         def subject(cert)
           cert.subject.to_a.map { |attr, data| "#{attr}=#{data}" }.join(",")
         end

spec/rails/auth/x509/middleware_spec.rb

@@ -3,41 +3,32 @@
 require "logger"
 
 RSpec.describe Rails::Auth::X509::Middleware do
-  let(:request) { Rack::MockRequest.env_for("https://www.example.com") }
   let(:app)     { ->(env) { [200, env, "Hello, world!"] } }
+  let(:request) { Rack::MockRequest.env_for("https://www.example.com") }
 
-  let(:valid_cert_pem) { cert_path("valid.crt").read }
-  let(:bad_cert_pem)   { cert_path("invalid.crt").read }
-  let(:cert_required)  { false }
-  let(:cert_filter)    { :pem }
-  let(:example_key)    { "X-SSL-Client-Cert" }
+  let(:cert_filter) { :pem }
+  let(:cert_pem)    { cert_path("valid.crt").read }
+  let(:example_key) { "X-SSL-Client-Cert" }
 
   let(:middleware) do
     described_class.new(
       app,
-      logger: Logger.new(STDERR),
-      ca_file: cert_path("ca.crt").to_s,
       cert_filters: { example_key => cert_filter },
-      require_cert: cert_required
+      logger: Logger.new(STDERR)
     )
   end
 
   context "certificate types" do
     describe "PEM certificates" do
       it "extracts Rails::Auth::X509::Certificate from a PEM certificate in the Rack environment" do
-        _response, env = middleware.call(request.merge(example_key => valid_cert_pem))
+        _response, env = middleware.call(request.merge(example_key => cert_pem))
 
         credential = Rails::Auth.credentials(env).fetch("x509")
         expect(credential).to be_a Rails::Auth::X509::Certificate
       end
 
-      it "ignores unverified certificates" do
-        _response, env = middleware.call(request.merge(example_key => bad_cert_pem))
-        expect(Rails::Auth.credentials(env)).to be_empty
-      end
-
       it "normalizes abnormal whitespace" do
-        _response, env = middleware.call(request.merge(example_key => valid_cert_pem.tr("\n", "\t")))
+        _response, env = middleware.call(request.merge(example_key => cert_pem.tr("\n", "\t")))
 
         credential = Rails::Auth.credentials(env).fetch("x509")
         expect(credential).to be_a Rails::Auth::X509::Certificate
@@ -46,11 +37,11 @@
 
     # :nocov:
     describe "Java certificates" do
-      let(:example_key) { "javax.servlet.request.X509Certificate" }
       let(:cert_filter) { :java }
+      let(:example_key) { "javax.servlet.request.X509Certificate" }
 
       let(:java_cert) do
-        ruby_cert = OpenSSL::X509::Certificate.new(valid_cert_pem)
+        ruby_cert = OpenSSL::X509::Certificate.new(cert_pem)
         input_stream = Java::JavaIO::ByteArrayInputStream.new(ruby_cert.to_der.to_java_bytes)
         java_cert_klass = Java::JavaSecurityCert::CertificateFactory.getInstance("X.509")
         java_cert_klass.generateCertificate(input_stream)
@@ -67,21 +58,4 @@
     end
     # :nocov:
   end
-
-  describe "require_cert: true" do
-    let(:cert_required) { true }
-
-    it "functions normally for valid certificates" do
-      _response, env = middleware.call(request.merge(example_key => valid_cert_pem))
-
-      credential = Rails::Auth.credentials(env).fetch("x509")
-      expect(credential).to be_a Rails::Auth::X509::Certificate
-    end
-
-    it "raises Rails::Auth::X509::CertificateVerifyFailed for unverified certificates" do
-      expect do
-        middleware.call(request.merge(example_key => bad_cert_pem))
-      end.to raise_error Rails::Auth::X509::CertificateVerifyFailed
-    end
-  end
 end

spec/support/create_certs.rb

@@ -95,20 +95,3 @@
 
 File.write valid_cert_with_ext_path, valid_cert_with_ext.to_pem
 File.write valid_key_with_ext_path,  valid_cert_with_ext.key_material.private_key.to_pem
-
-#
-# Create evil MitM self-signed certificate
-#
-
-self_signed_cert = CertificateAuthority::Certificate.new
-self_signed_cert.subject.common_name = "127.0.0.1"
-self_signed_cert.subject.organizational_unit = "ponycopter"
-self_signed_cert.serial_number.number = 2
-self_signed_cert.key_material.generate_key
-self_signed_cert.sign!
-
-self_signed_cert_path = File.join(cert_path, "invalid.crt")
-self_signed_key_path  = File.join(cert_path, "invalid.key")
-
-File.write self_signed_cert_path, self_signed_cert.to_pem
-File.write self_signed_key_path,  self_signed_cert.key_material.private_key.to_pem