From 3fe8b0294a10aca5550e48c622811b46800404a4 Mon Sep 17 00:00:00 2001
From: Nicolas Bigler <nicolas.bigler@vshn.ch>
Date: Wed, 12 Feb 2020 16:50:25 +0100
Subject: [PATCH] Remove feature that exports client certs and keys to facts

Exporting private keys via facts is unsafe as facts should not contain
sensitive information, as they might be accessible from undesired
systems (eg. dashboards)

This feature has been added in commit voxpupuli@d0fd9f3 as a result of
voxpupuli/puppet-openvpn#231

This commit reverts the changes and removes the added feature for
security reasons.
---
 lib/facter/openvpn.rb                      | 62 ------------------
 manifests/deploy/client.pp                 | 42 ------------
 manifests/deploy/export.pp                 | 76 ----------------------
 manifests/deploy/install.pp                |  8 ---
 manifests/deploy/prepare.pp                | 15 -----
 manifests/deploy/service.pp                | 13 ----
 spec/defines/openvpn_deploy_client_spec.rb | 45 -------------
 spec/defines/openvpn_deploy_export_spec.rb | 46 -------------
 spec/unit/openvpn_module_spec.rb           | 58 -----------------
 9 files changed, 365 deletions(-)
 delete mode 100644 lib/facter/openvpn.rb
 delete mode 100644 manifests/deploy/client.pp
 delete mode 100644 manifests/deploy/export.pp
 delete mode 100644 manifests/deploy/install.pp
 delete mode 100644 manifests/deploy/prepare.pp
 delete mode 100644 manifests/deploy/service.pp
 delete mode 100644 spec/defines/openvpn_deploy_client_spec.rb
 delete mode 100644 spec/defines/openvpn_deploy_export_spec.rb

diff --git a/lib/facter/openvpn.rb b/lib/facter/openvpn.rb
deleted file mode 100644
index 54ba7be0..00000000
--- a/lib/facter/openvpn.rb
+++ /dev/null
@@ -1,62 +0,0 @@
-require 'facter'
-
-module Openvpn
-  def self.etc_path
-    case Facter.value(:osfamily)
-    when 'FreeBSD'
-      '/usr/local/etc/openvpn'
-    when 'RedHat'
-      '/etc/openvpn'
-    when 'Debian'
-      '/etc/openvpn'
-    when 'Archlinux'
-      '/etc/openvpn'
-    when 'Linux'
-      '/etc/openvpn'
-    else
-      ''
-    end
-  end
-
-  def self.client_certs
-    path = etc_path
-    clients = {}
-    if File.directory?(path)
-      Dir.entries(path).each do |server|
-        next unless File.directory?("#{path}/#{server}/download-configs")
-        clients[server.to_s] = {}
-
-        Dir.entries("#{path}/#{server}/download-configs").each do |client|
-          next unless File.directory?("#{path}/#{server}/download-configs/#{client}") && client !~ %r{^\.\.?$} && client !~ %r{\.tblk$}
-
-          clients[server.to_s][client.to_s] = {}
-          clients[server.to_s][client.to_s]['conf'] = File.open("#{path}/#{server}/download-configs/#{client}/#{client}.conf", 'r').read
-          clients[server.to_s][client.to_s]['ca'] = File.open("#{path}/#{server}/download-configs/#{client}/keys/#{client}/ca.crt", 'r').read
-          clients[server.to_s][client.to_s]['crt'] = File.open("#{path}/#{server}/download-configs/#{client}/keys/#{client}/#{client}.crt", 'r').read
-          clients[server.to_s][client.to_s]['key'] = File.open("#{path}/#{server}/download-configs/#{client}/keys/#{client}/#{client}.key", 'r').read
-          if File.exist?("#{path}/#{server}/download-configs/#{client}/keys/#{client}/ta.key")
-            clients[server.to_s][client.to_s]['ta'] = File.open("#{path}/#{server}/download-configs/#{client}/keys/#{client}/ta.key", 'r').read
-          end
-        end
-      end
-    end
-    clients
-  end
-
-  # Method to call the Facter DSL and dynamically add facts at runtime.
-  #
-  # This method is necessary to add reasonable RSpec coverage for the custom
-  # fact
-  #
-  # @return [NilClass]
-  def self.add_facts
-    certs = client_certs
-    Facter.add('openvpn') do
-      setcode do
-        certs
-      end
-    end
-  end
-end
-
-Openvpn.add_facts
diff --git a/manifests/deploy/client.pp b/manifests/deploy/client.pp
deleted file mode 100644
index 5e6e50e0..00000000
--- a/manifests/deploy/client.pp
+++ /dev/null
@@ -1,42 +0,0 @@
-#
-# @summary Collect the exported configs for an Host and ensure a running Openvpn Service
-# @param server which Openvpn::Server[$server] does the config belong to?
-# @param manage_etc should the /etc/openvpn directory be managed? (warning, all unmanaged files will be purged!)
-#
-# @example
-#  openvpn::deploy::client { 'test-client':
-#    server => 'test_server',
-#  }
-#
-define openvpn::deploy::client (
-  String $server,
-  Boolean $manage_etc = true,
-) {
-
-  include openvpn::deploy::prepare
-
-  Class['openvpn::deploy::install']
-  -> Openvpn::Deploy::Client[$name]
-  ~> Class['openvpn::deploy::service']
-
-
-  if $manage_etc {
-    file { [
-      "${openvpn::deploy::prepare::etc_directory}/openvpn",
-      "${openvpn::deploy::prepare::etc_directory}/openvpn/keys",
-      "${openvpn::deploy::prepare::etc_directory}/openvpn/keys/${name}",
-    ]:
-      ensure  => directory,
-      require => Package['openvpn'];
-    }
-  } else {
-    file { "${openvpn::deploy::prepare::etc_directory}/openvpn/keys/${name}":
-      ensure  => directory,
-      require => Package['openvpn'];
-    }
-  }
-
-  File <<| tag == "${server}-${name}" |>>
-  ~> Class['openvpn::deploy::service']
-
-}
diff --git a/manifests/deploy/export.pp b/manifests/deploy/export.pp
deleted file mode 100644
index 3b77d21d..00000000
--- a/manifests/deploy/export.pp
+++ /dev/null
@@ -1,76 +0,0 @@
-#
-# @summary Prepare all Openvpn-Client-Configs to be exported
-#
-# @param server which Openvpn::Server[$server] does the config belong to?
-# @param tls_auth should the ta* files be exported too?
-#
-# @example
-#  openvpn::deploy::export { 'test-client':
-#    server => 'test_server',
-#  }
-#
-define openvpn::deploy::export (
-  String $server,
-  Boolean $tls_auth = false,
-) {
-
-  Openvpn::Server[$server]
-  -> Openvpn::Client[$name]
-  -> Openvpn::Deploy::Export[$name]
-
-  if fact("openvpn.${server}.${name}") {
-    $data = $facts['openvpn'][$server][$name]
-
-    @@file { "exported-${server}-${name}-config":
-      ensure  => file,
-      path    => "${openvpn::etc_directory}/openvpn/${name}.conf",
-      owner   => 'root',
-      group   => 'root',
-      mode    => '0600',
-      content => $data['conf'],
-      tag     => "${server}-${name}",
-    }
-
-    @@file { "exported-${server}-${name}-ca":
-      ensure  => file,
-      path    => "${openvpn::etc_directory}/openvpn/keys/${name}/ca.crt",
-      owner   => 'root',
-      group   => 'root',
-      mode    => '0600',
-      content => $data['ca'],
-      tag     => "${server}-${name}",
-    }
-
-    @@file { "exported-${server}-${name}-crt":
-      ensure  => file,
-      path    => "${openvpn::etc_directory}/openvpn/keys/${name}/${name}.crt",
-      owner   => 'root',
-      group   => 'root',
-      mode    => '0600',
-      content => $data['crt'],
-      tag     => "${server}-${name}",
-    }
-
-    @@file { "exported-${server}-${name}-key":
-      ensure  => file,
-      path    => "${openvpn::etc_directory}/openvpn/keys/${name}/${name}.key",
-      owner   => 'root',
-      group   => 'root',
-      mode    => '0600',
-      content => $data['key'],
-      tag     => "${server}-${name}",
-    }
-
-    if $tls_auth {
-      @@file { "exported-${server}-${name}-ta":
-        ensure  => file,
-        path    => "${openvpn::etc_directory}/openvpn/keys/${name}/ta.key",
-        owner   => 'root',
-        group   => 'root',
-        mode    => '0600',
-        content => $data['ta'],
-        tag     => "${server}-${name}",
-      }
-    }
-  }
-}
diff --git a/manifests/deploy/install.pp b/manifests/deploy/install.pp
deleted file mode 100644
index 93d3617e..00000000
--- a/manifests/deploy/install.pp
+++ /dev/null
@@ -1,8 +0,0 @@
-#
-# @summary Installs the Openvpn profile
-#
-class openvpn::deploy::install {
-
-  ensure_packages(['openvpn'])
-
-}
diff --git a/manifests/deploy/prepare.pp b/manifests/deploy/prepare.pp
deleted file mode 100644
index 9260e6c2..00000000
--- a/manifests/deploy/prepare.pp
+++ /dev/null
@@ -1,15 +0,0 @@
-#
-# @summary Base profile
-#
-# @param etc_directory Path of the configuration directory.
-# @example
-#   include openvpn::deploy::prepare
-#
-class openvpn::deploy::prepare(
-  Stdlib::Absolutepath $etc_directory
-) {
-
-  class { 'openvpn::deploy::install': }
-  ~> class { 'openvpn::deploy::service': }
-
-}
diff --git a/manifests/deploy/service.pp b/manifests/deploy/service.pp
deleted file mode 100644
index 5e6978c5..00000000
--- a/manifests/deploy/service.pp
+++ /dev/null
@@ -1,13 +0,0 @@
-#
-# @summary Base profile
-#
-class openvpn::deploy::service {
-
-  service { 'openvpn':
-    ensure     => running,
-    enable     => true,
-    hasrestart => true,
-    hasstatus  => true;
-  }
-
-}
diff --git a/spec/defines/openvpn_deploy_client_spec.rb b/spec/defines/openvpn_deploy_client_spec.rb
deleted file mode 100644
index 8d74baec..00000000
--- a/spec/defines/openvpn_deploy_client_spec.rb
+++ /dev/null
@@ -1,45 +0,0 @@
-require 'spec_helper'
-
-describe 'openvpn::deploy::client', type: :define do
-  on_supported_os.each do |os, facts|
-    context "on #{os}" do
-      let(:facts) do
-        facts
-      end
-      let(:title) { 'test_client' }
-
-      context 'with manage_etc false' do
-        let(:params) do
-          {
-            server: 'test_server',
-            manage_etc: false
-          }
-        end
-
-        it { is_expected.to compile.with_all_deps }
-        it { is_expected.to contain_file('/etc/openvpn/keys/test_client') }
-        it { is_expected.to contain_package('openvpn') }
-        it {
-          is_expected.to contain_service('openvpn').with(
-            ensure: 'running',
-            enable: true
-          )
-        }
-      end
-
-      context 'with manage_etc true' do
-        let(:params) do
-          {
-            server: 'test_server',
-            manage_etc: true
-          }
-        end
-
-        it { is_expected.to compile.with_all_deps }
-        it { is_expected.to contain_file('/etc/openvpn') }
-        it { is_expected.to contain_file('/etc/openvpn/keys') }
-        it { is_expected.to contain_file('/etc/openvpn/keys/test_client') }
-      end
-    end
-  end
-end
diff --git a/spec/defines/openvpn_deploy_export_spec.rb b/spec/defines/openvpn_deploy_export_spec.rb
deleted file mode 100644
index c9154dcc..00000000
--- a/spec/defines/openvpn_deploy_export_spec.rb
+++ /dev/null
@@ -1,46 +0,0 @@
-require 'spec_helper'
-
-describe 'openvpn::deploy::export', type: :define do
-  on_supported_os.each do |os, facts|
-    context "on #{os}" do
-      let(:pre_condition) do
-        [
-          'openvpn::server { "test_server":
-            country       => "CO",
-            province      => "ST",
-            city          => "Some City",
-            organization  => "example.org",
-            email         => "testemail@example.org"
-          }',
-          'openvpn::client { "test_client":
-            server => "test_server"
-          }'
-        ].join
-      end
-      let(:facts) do
-        facts.merge(
-          openvpn: { 'test_server' => { 'test_client' => { 'conf' => 'config', 'crt' => 'crt', 'ca' => 'ca', 'key' => 'key', 'ta' => 'ta' } } }
-        )
-      end
-      let(:title) { 'test_client' }
-      let(:params) { { 'server' => 'test_server' } }
-
-      it { is_expected.to compile.with_all_deps }
-
-      context 'exported resources' do
-        subject { exported_resources }
-
-        it { is_expected.to contain_file('exported-test_server-test_client-config').with_content('config') }
-        it { is_expected.to contain_file('exported-test_server-test_client-ca').with_content('ca') }
-        it { is_expected.to contain_file('exported-test_server-test_client-crt').with_content('crt') }
-        it { is_expected.to contain_file('exported-test_server-test_client-key').with_content('key') }
-
-        context 'with tls_auth' do
-          let(:params) { { 'server' => 'test_server', 'tls_auth' => true } }
-
-          it { is_expected.to contain_file('exported-test_server-test_client-ta').with_content('ta') }
-        end
-      end
-    end
-  end
-end
diff --git a/spec/unit/openvpn_module_spec.rb b/spec/unit/openvpn_module_spec.rb
index 731a4c2e..a88cffc1 100644
--- a/spec/unit/openvpn_module_spec.rb
+++ b/spec/unit/openvpn_module_spec.rb
@@ -42,62 +42,4 @@
       it { is_expected.to eq('') }
     end
   end
-
-  describe '.client_certs' do
-    subject(:path) { described_class.client_certs }
-
-    before do
-      allow(Facter.fact(:osfamily)).to receive(:value) { osfamily }
-    end
-
-    after { Facter.clear }
-
-    context 'with Openvpn installed' do
-      let(:osfamily) { 'Linux' }
-
-      before do
-        allow(Dir).to receive(:entries).and_call_original
-        allow(Dir).to receive(:entries).with('/etc/openvpn').and_return(%w[. .. test-server])
-        allow(Dir).to receive(:entries).with('/etc/openvpn/test-server').and_return(%w[. .. download-configs])
-        allow(Dir).to receive(:entries).with('/etc/openvpn/test-server/download-configs').and_return(%w[. .. test2 client3 other4])
-        allow(File).to receive(:directory?).and_call_original
-        allow(File).to receive(:directory?).with('/etc/openvpn').and_return(true)
-        allow(File).to receive(:directory?).with('/etc/openvpn/test-server').and_return(true)
-        allow(File).to receive(:directory?).with('/etc/openvpn/test-server/download-configs').and_return(true)
-        allow(File).to receive(:directory?).with('/etc/openvpn/test-server/download-configs/test2').and_return(true)
-        allow(File).to receive(:open).with('/etc/openvpn/test-server/download-configs/test2/test2.conf', 'r').and_return(StringIO.new('conf'))
-        allow(File).to receive(:open).with('/etc/openvpn/test-server/download-configs/test2/keys/test2/ca.crt', 'r').and_return(StringIO.new('ca'))
-        allow(File).to receive(:open).with('/etc/openvpn/test-server/download-configs/test2/keys/test2/test2.crt', 'r').and_return(StringIO.new('crt'))
-        allow(File).to receive(:open).with('/etc/openvpn/test-server/download-configs/test2/keys/test2/test2.key', 'r').and_return(StringIO.new('key'))
-      end
-      it { is_expected.to eq('test-server' => { 'test2' => { 'conf' => 'conf', 'ca' => 'ca', 'crt' => 'crt', 'key' => 'key' } }) }
-
-      context 'with tsl_auth enabled' do
-        before do
-          allow(File).to receive(:exist?).with('/etc/openvpn/test-server/download-configs/test2/keys/test2/ta.key').and_return(true)
-          allow(File).to receive(:open).with('/etc/openvpn/test-server/download-configs/test2/keys/test2/ta.key', 'r').and_return(StringIO.new('ta'))
-        end
-
-        it { is_expected.to eq('test-server' => { 'test2' => { 'conf' => 'conf', 'ca' => 'ca', 'crt' => 'crt', 'key' => 'key', 'ta' => 'ta' } }) }
-      end
-    end
-  end
-
-  describe 'openvpn fact' do
-    subject(:fact) { Facter.fact('openvpn').value }
-
-    before do
-      # Ensure we're populating Facter's internal collection with our Fact
-      described_class.add_facts
-    end
-
-    # A regular ol' RSpec example
-    it { is_expected.to eq({}) }
-
-    after do
-      # Make sure we're clearing out Facter every time
-      Facter.clear
-      Facter.clear_messages
-    end
-  end
 end