Fixes voxpupuli#374 - Revocation command update and crl renew

An issue was raised informing that the revocation command is incorrect. This was diagnosed to indeed be the case. As the `$name` variable in context of `revoke.pp` does not evalute to `server name` but instead `client name`. The exec for the crl renew was updated to clarify which server it's done for and to prevent duplicate `exec` resource names. `catch_changes` in the acceptance test was taken out because a crl renew is triggrered which is seen as a change.
Rubueno · May 22, 2020 · c3f6251 · c3f6251
1 parent a176aea
commit c3f6251
Show file tree

Hide file tree

Showing 3 changed files with 54 additions and 15 deletions.
diff --git a/manifests/revoke.pp b/manifests/revoke.pp
@@ -26,28 +26,33 @@
   $server_directory = $openvpn::server_directory
 
   $revocation_command = $openvpn::easyrsa_version ? {
-    '2.0' => ". ./vars && ./revoke-full ${name}; echo \"exit $?\" | grep -qE '(error 23|exit (0|2))' && touch revoked/${name}",
-    '3.0' => ". ./vars && ./easyrsa revoke --batch ${name}; echo \"exit $?\" | grep -qE '(error 23|exit (0|2))' && touch revoked/${name}",
+    '2.0' => ". ./vars && ./revoke-full ${name}; echo \"exit $?\" | grep -qE '(error 23|exit (0|2))'",
+    '3.0' => ". ./vars && ./easyrsa --batch revoke ${name}; echo \"exit $?\" | grep -qE '(error 23|exit (0|2))'",
   }
 
   $renew_command = $openvpn::easyrsa_version ? {
-    '2.0'   => ". ./vars && KEY_CN='' KEY_OU='' KEY_NAME='' KEY_ALTNAMES='' openssl ca -gencrl -out ${openvpn::server_directory}/${name}/crl.pem -config ${openvpn::server_directory}/${name}/easy-rsa/openssl.cnf",
-    '3.0'   => ". ./vars && EASYRSA_REQ_CN='' EASYRSA_REQ_OU='' openssl ca -gencrl -out ${server_directory}/${name}/crl.pem -config ${server_directory}/${name}/easy-rsa/openssl.cnf",
+    '2.0'   => ". ./vars && KEY_CN='' KEY_OU='' KEY_NAME='' KEY_ALTNAMES='' openssl ca -gencrl -out ${server_directory}/${server}/crl.pem -config ${server_directory}/${server}/easy-rsa/openssl.cnf",
+    '3.0'   => ". ./vars && EASYRSA_REQ_CN='' EASYRSA_REQ_OU='' openssl ca -gencrl -out ${server_directory}/${server}/crl.pem -config ${server_directory}/${server}/easy-rsa/openssl.cnf",
     default => fail("unexepected value for EasyRSA version, got '${openvpn::easyrsa_version}', expect 2.0 or 3.0."),
   }
 
+  file { "${server_directory}/${server}/easy-rsa/revoked/${name}":
+    ensure  => file,
+    require => Exec["revoke certificate for ${name} in context of ${server}"],
+  }
+
   exec { "revoke certificate for ${name} in context of ${server}":
     command  => $revocation_command,
     cwd      => "${server_directory}/${server}/easy-rsa",
-    creates  => "${server_directory}/${server}/easy-rsa/revoked/${name}",
     provider => 'shell',
-    notify   => Exec["renew crl.pem on ${name}"],
+    notify   => Exec["renew crl.pem on ${server} because of revocation of ${name}"],
+    creates  => "${server_directory}/${server}/easy-rsa/revoked/${name}",
   }
 
-  exec { "renew crl.pem on ${name}":
-    command  => $renew_command,
-    cwd      => "${openvpn::server_directory}/${name}/easy-rsa",
-    provider => 'shell',
-    schedule => "renew crl.pem schedule on ${name}",
+  exec { "renew crl.pem on ${server} because of revocation of ${name}":
+    command     => $renew_command,
+    cwd         => "${server_directory}/${server}/easy-rsa",
+    provider    => 'shell',
+    refreshonly => true,
   }
 }
diff --git a/spec/acceptance/openvpn_spec.rb b/spec/acceptance/openvpn_spec.rb
@@ -43,11 +43,12 @@
           local        => '',
           management   => true,
           tls_cipher   => 'TLS-DHE-RSA-WITH-AES-128-GCM-SHA256:TLS-DHE-RSA-WITH-AES-128-CBC-SHA',
-  }
+        }
       )
       apply_manifest_on(hosts_as('vpnserver'), pp, catch_failures: true)
       apply_manifest_on(hosts_as('vpnserver'), pp, catch_changes: true)
     end
+
     it 'creates openvpn client certificate idempotently' do
       pp = %(
         openvpn::server { 'test_openvpn_server':
@@ -62,7 +63,7 @@
           tls_cipher   => 'TLS-DHE-RSA-WITH-AES-128-GCM-SHA256:TLS-DHE-RSA-WITH-AES-128-CBC-SHA',
         }
 
-  openvpn::client { 'vpnclienta' :
+        openvpn::client { 'vpnclienta' :
           server      => 'test_openvpn_server',
           require     => Openvpn::Server['test_openvpn_server'],
           remote_host => $facts['networking']['ip'],
@@ -73,6 +74,39 @@
       apply_manifest_on(hosts_as('vpnserver'), pp, catch_changes: true)
     end
 
+    it 'revokes openvpn client certificate' do
+      pp = %(
+        openvpn::server { 'test_openvpn_server':
+          country      => 'CO',
+          province     => 'ST',
+          city         => 'A city',
+          organization => 'FOO',
+          email        => '[email protected]',
+          server       => '10.0.0.0 255.255.255.0',
+          local        => '',
+          management   => true,
+          tls_cipher   => 'TLS-DHE-RSA-WITH-AES-128-GCM-SHA256:TLS-DHE-RSA-WITH-AES-128-CBC-SHA',
+        }
+
+        openvpn::client { 'vpnclientb' :
+          server      => 'test_openvpn_server',
+          require     => Openvpn::Server['test_openvpn_server'],
+          remote_host => $facts['networking']['ip'],
+          tls_cipher  => 'TLS-DHE-RSA-WITH-AES-128-GCM-SHA256:TLS-DHE-RSA-WITH-AES-128-CBC-SHA',
+        }
+
+        openvpn::revoke { 'vpnclientb':
+          server => 'test_openvpn_server',
+        }
+      )
+      apply_manifest_on(hosts_as('vpnserver'), pp, catch_failures: true)
+      apply_manifest_on(hosts_as('vpnserver'), pp, catch_changes: false)
+    end
+
+    describe file("#{server_directory}/test_openvpn_server/easy-rsa/revoked/vpnclientb") do
+      it { is_expected.to be_file }
+    end
+
     describe file("#{server_directory}/test_openvpn_server/easy-rsa/keys") do
       it { is_expected.to be_directory }
     end

diff --git a/spec/defines/openvpn_revoke_spec.rb b/spec/defines/openvpn_revoke_spec.rb
@@ -29,7 +29,7 @@
 
       it {
         is_expected.to contain_exec('revoke certificate for test_client in context of test_server').with(
-          'command' => ". ./vars && ./revoke-full test_client; echo \"exit $?\" | grep -qE '(error 23|exit (0|2))' && touch revoked/test_client"
+          'command' => ". ./vars && ./revoke-full test_client; echo \"exit $?\" | grep -qE '(error 23|exit (0|2))'"
         )
       }
     end
@@ -60,7 +60,7 @@
 
       it {
         is_expected.to contain_exec('revoke certificate for test_client3 in context of test_server').with(
-          'command' => ". ./vars && ./easyrsa revoke --batch test_client3; echo \"exit $?\" | grep -qE '(error 23|exit (0|2))' && touch revoked/test_client3"
+          'command' => ". ./vars && ./easyrsa --batch revoke test_client3; echo \"exit $?\" | grep -qE '(error 23|exit (0|2))'"
         )
       }
     end