Fix client cert revoke error with easyrsa 3.0

[chloesoe]

In easyrsa 3.0 (used in CentOS) the command has changed. Now there is
only a single binary to run the scripts. Further the generation of CRL
also has changed; now a new crl.pem file is created in keys/crl.pem
which overrides the symlink there. So the revocation check did not work
anymore, because the crl.pem in the base directory was not checked when
a client connected.

Resolves: VSHNOPS-1537

Pull Request (PR) description

This Pull Request (PR) fixes the following issues

[Dan33l]

Hi @chloesoe can you patch acceptance tests to tackle this issue and ensure that revoke process works ?

[Dan33l]

hum ... it looks that we have an other PR about same topic #332

[bastelfreak]

@chloesoe can you take a look at the failing travis jobs?

---

Edit: One travis job run into a timeout, I restarted it.

[chloesoe]

Hi @chloesoe can you patch acceptance tests to tackle this issue and ensure that revoke process works ?

@Dan33l: As I'm new in Puppet I was not able to write working acceptance tests in a reasonable time. I pushed my work I did so far. Perhaps I will have couple of hours in a month to try to get it running.
With commit b848cb9 I got a step further so a revoke test could possibly work, because so we don't get a deprecation warning and tests start (but failing yet).

[andrekeller]

@chloesoe a3c5458 should not be fixed in this PR, the proposed solution has the same issues as #312 (making key files world readable which was not the case before). PR should not address multiple issues.

[chloesoe]

@andrekeller That sounds right, but acceptance tests for revoke are failing immediately without this change. And this PR only will be approved if there are acceptance tests. So should I create a separate PR for a3c5458, wait for merging, and then pull and rebase this PR so this change is applied?

[Dan33l]

Thank you for the job already done @chloesoe .

Please have a look to inline comments.

[Dan33l]

For the moment the test is failing because the revoke process is not idempotent. So you did a good test. It cached idempotency issue. Great !

It is not idempotent because during the second run some modifications are done:

 Info: Applying configuration version '1557908446'
  Notice: /Stage[main]/Main/Openvpn::Server[test_openvpn_server]/Openvpn::Ca[test_openvpn_server]/File[/etc/openvpn/test_openvpn_server/easy-rsa/revoked/vpnclientb]/group: group changed 'root' to 'nogroup'
  Notice: /Stage[main]/Main/Openvpn::Server[test_openvpn_server]/Openvpn::Ca[test_openvpn_server]/File[/etc/openvpn/test_openvpn_server/easy-rsa/revoked/vpnclientb]/mode: mode changed '0644' to '0750'

And so the puppet code of revoke needs an update to become idempotent.

[andrekeller]

The permission change comes from here: https://github.com/voxpupuli/puppet-openvpn/blob/master/manifests/ca.pp#L75
I would argue this is wrong in the first place. Why does this need to be executable? I'd propose to fix this to be root / 640.

You would then need to ensure that the revoked file is created with the correct permission, you could amend the touch (https://github.com/voxpupuli/puppet-openvpn/blob/master/manifests/revoke.pp#L29) with a chmod to ensure the proper permissions. (You could also replace this with install install -b -m 640 /dev/null revoked/${name} however I'm not sure how portable the install command is.

[andrekeller]

@andrekeller That sounds right, but acceptance tests for revoke are failing immediately without this change. And this PR only will be approved if there are acceptance tests. So should I create a separate PR for a3c5458, wait for merging, and then pull and rebase this PR so this change is applied?

I'm not a maintainer, but I would drop this commit from this PR and wait for #312 to be merged into master. If you feel you can help with #312 even better. But it is definitely not in scope of this PR.

[erik-frontify]

This will renew the CRL on every puppet run if there is a "revoke". Can be fixed like that:

@@ -32,14 +32,16 @@ define openvpn::revoke (
         cwd      => "${etc_directory}/openvpn/${server}/easy-rsa",
         creates  => "${etc_directory}/openvpn/${server}/easy-rsa/revoked/${name}",
         provider => 'shell',
+        notify  => Exec["renew crl.pem for ${name}",],
       }
       # `easyrsa gen-crl` does not work, since it will create the crl.pem
       # to keys/crl.pem which is a symlinked to crl.pem in the servers etc
       # directory
       exec { "renew crl.pem for ${name}":
-        command  => ". ./vars && EASYRSA_REQ_CN='' EASYRSA_REQ_OU='' openssl ca -gencrl -out ../crl.pem -config ./openssl.cnf",
-        cwd      => "${openvpn::etc_directory}/openvpn/${server}/easy-rsa",
-        provider => 'shell',
+        command     => ". ./vars && EASYRSA_REQ_CN='' EASYRSA_REQ_OU='' openssl ca -gencrl -out ../crl.pem -config ./openssl.cnf",
+        cwd         => "${openvpn::etc_directory}/openvpn/${server}/easy-rsa",
+        provider    => 'shell',
+        refreshonly => true,
       }
     }
     '2.0': {

[vox-pupuli-tasks]

Dear @chloesoe, thanks for the PR!

This is pccibot, your friendly Vox Pupuli GitHub Bot. I noticed that your pull request contains merge conflict. Can you please rebase?

You can find my sourcecode at voxpupuli/vox-pupuli-tasks

[vox-pupuli-tasks]

Dear @chloesoe, thanks for the PR!

This is pccibot, your friendly Vox Pupuli GitHub Bot. I noticed that your pull request contains merge conflict. Can you please rebase?

You can find my sourcecode at voxpupuli/vox-pupuli-tasks

[Rubueno]

@bastelfreak This can be closed.

[vox-pupuli-tasks]

Dear @chloesoe, thanks for the PR!

This is pccibot, your friendly Vox Pupuli GitHub Bot. I noticed that your pull request contains merge conflict. Can you please rebase?

You can find my sourcecode at voxpupuli/vox-pupuli-tasks

[vox-pupuli-tasks]

Dear @chloesoe, thanks for the PR!

This is pccibot, your friendly Vox Pupuli GitHub Bot. I noticed that your pull request contains merge conflict. Can you please rebase?

You can find my sourcecode at voxpupuli/vox-pupuli-tasks