Merge pull request #8908 from joshcooper/external_ca_with_client_certs_11522

(PUP-11522) Support mutual TLS when using an external CA

Author: joshcooper

lib/puppet/http/client.rb

@@ -426,7 +426,7 @@ def system_ssl_context
     cacerts = cert_provider.load_cacerts || []
 
     ssl = Puppet::SSL::SSLProvider.new
-    @default_system_ssl_context = ssl.create_system_context(cacerts: cacerts)
+    @default_system_ssl_context = ssl.create_system_context(cacerts: cacerts, include_client_cert: true)
     ssl.print(@default_system_ssl_context)
     @default_system_ssl_context
   end

lib/puppet/ssl/ssl_provider.rb

@@ -42,15 +42,18 @@ def create_root_context(cacerts:, crls: [], revocation: Puppet[:certificate_revo
   # refers to the cacerts bundle in the puppet-agent package.
   #
   # Connections made from the returned context will authenticate the server,
-  # i.e. `VERIFY_PEER`, but will not use a client certificate and will not
-  # perform revocation checking.
+  # i.e. `VERIFY_PEER`, but will not use a client certificate (unless requested)
+  # and will not perform revocation checking.
   #
   # @param cacerts [Array<OpenSSL::X509::Certificate>] Array of trusted CA certs
   # @param path [String, nil] A file containing additional trusted CA certs.
+  # @param include_client_cert [true, false] If true, the client cert will be added to the context
+  #   allowing mutual TLS authentication. The default is false. If the client cert doesn't exist
+  #   then the option will be ignored.
   # @return [Puppet::SSL::SSLContext] A context to use to create connections
   # @raise (see #create_context)
   # @api private
-  def create_system_context(cacerts:, path: Puppet[:ssl_trust_store])
+  def create_system_context(cacerts:, path: Puppet[:ssl_trust_store], include_client_cert: false)
     store = create_x509_store(cacerts, [], false, include_system_store: true)
 
     if path
@@ -71,6 +74,29 @@ def create_system_context(cacerts:, path: Puppet[:ssl_trust_store])
       end
     end
 
+    if include_client_cert
+      cert_provider = Puppet::X509::CertProvider.new
+      private_key = cert_provider.load_private_key(Puppet[:certname], required: false)
+      unless private_key
+        Puppet.warning("Private key for '#{Puppet[:certname]}' does not exist")
+      end
+
+      client_cert = cert_provider.load_client_cert(Puppet[:certname], required: false)
+      unless client_cert
+        Puppet.warning("Client certificate for '#{Puppet[:certname]}' does not exist")
+      end
+
+      if private_key && client_cert
+        client_chain = resolve_client_chain(store, client_cert, private_key)
+
+        return Puppet::SSL::SSLContext.new(
+          store: store, cacerts: cacerts, crls: [],
+          private_key: private_key, client_cert: client_cert, client_chain: client_chain,
+          revocation: false
+        ).freeze
+      end
+    end
+
     Puppet::SSL::SSLContext.new(store: store, cacerts: cacerts, crls: [], revocation: false).freeze
   end
 
@@ -107,15 +133,7 @@ def create_context(cacerts:, crls:, private_key:, client_cert:, revocation: Pupp
     raise ArgumentError, _("Client cert is missing") unless client_cert
 
     store = create_x509_store(cacerts, crls, revocation, include_system_store: include_system_store)
-    client_chain = verify_cert_with_store(store, client_cert)
-
-    if !private_key.is_a?(OpenSSL::PKey::RSA) && !private_key.is_a?(OpenSSL::PKey::EC)
-      raise Puppet::SSL::SSLError, _("Unsupported key '%{type}'") % { type: private_key.class.name }
-    end
-
-    unless client_cert.check_private_key(private_key)
-      raise Puppet::SSL::SSLError, _("The certificate for '%{name}' does not match its private key") % { name: subject(client_cert) }
-    end
+    client_chain = resolve_client_chain(store, client_cert, private_key)
 
     Puppet::SSL::SSLContext.new(
       store: store, cacerts: cacerts, crls: crls,
@@ -241,6 +259,20 @@ def revocation_mode(mode)
     end
   end
 
+  def resolve_client_chain(store, client_cert, private_key)
+    client_chain = verify_cert_with_store(store, client_cert)
+
+    if !private_key.is_a?(OpenSSL::PKey::RSA) && !private_key.is_a?(OpenSSL::PKey::EC)
+      raise Puppet::SSL::SSLError, _("Unsupported key '%{type}'") % { type: private_key.class.name }
+    end
+
+    unless client_cert.check_private_key(private_key)
+      raise Puppet::SSL::SSLError, _("The certificate for '%{name}' does not match its private key") % { name: subject(client_cert) }
+    end
+
+    client_chain
+  end
+
   def verify_cert_with_store(store, cert)
     # StoreContext#initialize accepts a chain argument, but it's set to [] because
     # puppet requires any intermediate CA certs needed to complete the client's

spec/integration/http/client_spec.rb

@@ -77,13 +77,13 @@
       }
     }
 
-    let(:systemstore) do
-      res = tmpfile('systemstore')
+    let(:cert_file) do
+      res = tmpfile('cert_file')
       File.write(res, https_server.ca_cert)
       res
     end
 
-    it "mutually authenticates the connection" do
+    it "mutually authenticates the connection using an explicit context" do
       client_context = ssl_provider.create_context(
         cacerts: [https_server.ca_cert], crls: [https_server.ca_crl],
         client_cert: https_server.server_cert, private_key: https_server.server_key
@@ -95,10 +95,27 @@
       end
     end
 
+    it "mutually authenticates the connection when the client and server certs are issued from different CAs" do
+      # this is the client cert's CA, key and cert
+      Puppet[:localcacert] = fixtures('ssl/unknown-ca.pem')
+      Puppet[:hostprivkey] = fixtures('ssl/unknown-127.0.0.1-key.pem')
+      Puppet[:hostcert] = fixtures('ssl/unknown-127.0.0.1.pem')
+
+      # this is the server cert's CA that the client needs in order to authenticate the server
+      Puppet[:ssl_trust_store] = fixtures('ssl/ca.pem')
+
+      # need to pass both the client and server CAs. The former is needed so the server can authenticate our client cert
+      https_server = PuppetSpec::HTTPSServer.new(ca_cert: [cert_fixture('ca.pem'), cert_fixture('unknown-ca.pem')])
+      https_server.start_server(ctx_proc: ctx_proc) do |port|
+        res = client.get(URI("https://127.0.0.1:#{port}"), options: {include_system_store: true})
+        expect(res).to be_success
+      end
+    end
+
     it "connects when the server's CA is in the system store and the connection is mutually authenticated using create_context" do
-      Puppet::Util.withenv("SSL_CERT_FILE" => systemstore) do
+      Puppet::Util.withenv("SSL_CERT_FILE" => cert_file) do
         client_context = ssl_provider.create_context(
-          cacerts: [https_server.ca_cert], crls: [https_server.ca_crl],
+          cacerts: [], crls: [],
           client_cert: https_server.server_cert, private_key: https_server.server_key,
           revocation: false, include_system_store: true
         )
@@ -109,8 +126,8 @@
       end
     end
 
-    it "connects when the server's CA is in the system store and the connection is mutually authenticated uning load_context" do
-      Puppet::Util.withenv("SSL_CERT_FILE" => systemstore) do
+    it "connects when the server's CA is in the system store and the connection is mutually authenticated using load_context" do
+      Puppet::Util.withenv("SSL_CERT_FILE" => cert_file) do
         client_context = ssl_provider.load_context(revocation: false, include_system_store: true)
         https_server.start_server(ctx_proc: ctx_proc) do |port|
           res = client.get(URI("https://127.0.0.1:#{port}"), options: {ssl_context: client_context})
@@ -132,12 +149,12 @@
 
     it "connects when the server's CA is in the system store" do
       # create a temp cacert bundle
-      ssl_file = tmpfile('systemstore')
-      File.write(ssl_file, https_server.ca_cert)
+      cert_file = tmpfile('cert_file')
+      File.write(cert_file, https_server.ca_cert)
 
       # override path to system cacert bundle, this must be done before
       # the SSLContext is created and the call to X509::Store.set_default_paths
-      Puppet::Util.withenv("SSL_CERT_FILE" => ssl_file) do
+      Puppet::Util.withenv("SSL_CERT_FILE" => cert_file) do
         system_context = ssl_provider.create_system_context(cacerts: [])
         https_server.start_server do |port|
           res = client.get(URI("https://127.0.0.1:#{port}"), options: {ssl_context: system_context})

spec/lib/puppet_spec/https.rb

@@ -40,7 +40,7 @@ def start_server(ctx_proc: nil, response_proc: nil, &block)
 
     IO.pipe {|stop_pipe_r, stop_pipe_w|
       store = OpenSSL::X509::Store.new
-      store.add_cert(@ca_cert)
+      Array(@ca_cert).each { |c| store.add_cert(c) }
       store.purpose = OpenSSL::X509::PURPOSE_SSL_CLIENT
       ctx = OpenSSL::SSL::SSLContext.new
       ctx.cert_store = store

spec/unit/ssl/ssl_provider_spec.rb

@@ -113,12 +113,21 @@
       }.to raise_error(/can't modify frozen/)
     end
 
-    it 'trusts system ca store' do
+    it 'trusts system ca store by default' do
       expect_any_instance_of(OpenSSL::X509::Store).to receive(:set_default_paths)
 
       subject.create_system_context(cacerts: [])
     end
 
+    it 'trusts an external ca store' do
+      path = tmpfile('system_cacerts')
+      File.write(path, cert_fixture('ca.pem').to_pem)
+
+      expect_any_instance_of(OpenSSL::X509::Store).to receive(:add_file).with(path)
+
+      subject.create_system_context(cacerts: [], path: path)
+    end
+
     it 'verifies peer' do
       sslctx = subject.create_system_context(cacerts: [])
       expect(sslctx.verify_peer).to eq(true)
@@ -135,6 +144,47 @@
       expect(sslctx.private_key).to be_nil
     end
 
+    it 'includes the client cert and private key when requested' do
+      Puppet[:hostcert] = fixtures('ssl/signed.pem')
+      Puppet[:hostprivkey] = fixtures('ssl/signed-key.pem')
+      sslctx = subject.create_system_context(cacerts: [], include_client_cert: true)
+      expect(sslctx.client_cert).to be_an(OpenSSL::X509::Certificate)
+      expect(sslctx.private_key).to be_an(OpenSSL::PKey::RSA)
+    end
+
+    it 'ignores non-existent client cert and private key when requested' do
+      Puppet[:certname] = 'doesnotexist'
+      sslctx = subject.create_system_context(cacerts: [], include_client_cert: true)
+      expect(sslctx.client_cert).to be_nil
+      expect(sslctx.private_key).to be_nil
+    end
+
+    it 'warns if the client cert does not exist' do
+      Puppet[:certname] = 'missingcert'
+      Puppet[:hostprivkey] = fixtures('ssl/signed-key.pem')
+
+      expect(Puppet).to receive(:warning).with("Client certificate for 'missingcert' does not exist")
+      subject.create_system_context(cacerts: [], include_client_cert: true)
+    end
+
+    it 'warns if the private key does not exist' do
+      Puppet[:certname] = 'missingkey'
+      Puppet[:hostcert] = fixtures('ssl/signed.pem')
+
+      expect(Puppet).to receive(:warning).with("Private key for 'missingkey' does not exist")
+      subject.create_system_context(cacerts: [], include_client_cert: true)
+    end
+
+    it 'raises if client cert and private key are mismatched' do
+      Puppet[:hostcert] = fixtures('ssl/signed.pem')
+      Puppet[:hostprivkey] = fixtures('ssl/127.0.0.1-key.pem')
+
+      expect {
+        subject.create_system_context(cacerts: [], include_client_cert: true)
+      }.to raise_error(Puppet::SSL::SSLError,
+        "The certificate for 'CN=signed' does not match its private key")
+    end
+
     it 'trusts additional system certs' do
       path = tmpfile('system_cacerts')
       File.write(path, cert_fixture('ca.pem').to_pem)
@@ -448,6 +498,18 @@
       sslctx = subject.create_context(**config)
       expect(sslctx.verify_peer).to eq(true)
     end
+
+    it 'does not trust the system ca store by default' do
+      expect_any_instance_of(OpenSSL::X509::Store).to receive(:set_default_paths).never
+
+      subject.create_context(**config)
+    end
+
+    it 'trusts the system ca store' do
+      expect_any_instance_of(OpenSSL::X509::Store).to receive(:set_default_paths)
+
+      subject.create_context(**config.merge(include_system_store: true))
+    end
   end
 
   context 'when loading an ssl context' do
@@ -528,6 +590,18 @@
         subject.load_context(password: 'wrongpassword')
       }.to raise_error(Puppet::SSL::SSLError, /Failed to load private key for host 'signed': Could not parse PKey/)
     end
+
+    it 'does not trust the system ca store by default' do
+      expect_any_instance_of(OpenSSL::X509::Store).to receive(:set_default_paths).never
+
+      subject.load_context
+    end
+
+    it 'trusts the system ca store' do
+      expect_any_instance_of(OpenSSL::X509::Store).to receive(:set_default_paths)
+
+      subject.load_context(include_system_store: true)
+    end
   end
 
   context 'when verifying requests' do