Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Revoke command missing on easy-rsa 3.0 #331 #332

Closed
wants to merge 2 commits into from
Closed

Revoke command missing on easy-rsa 3.0 #331 #332

wants to merge 2 commits into from

Conversation

jasonsattler
Copy link

#331

This block of code works for me.

 each($revoked) |$name| {
    exec { "revoke certificate for ${name} in context of ${server}":
      command  => ". ./vars && echo yes | ./easyrsa revoke ${name} 2>&1 | grep -E 'Already revoked|was successful|not a valid certificate' && ./easyrsa gen-crl && /bin/cp -f keys/crl.pem ../crl.pem && touch revoked/${name}",
      cwd      => "/etc/openvpn/${server}/easy-rsa",
      creates  => "/etc/openvpn/${server}/easy-rsa/revoked/${name}",
      provider => 'shell',
      notify   => Service["openvpn@${server}"]
    }
  }

Pull Request (PR) description

This Pull Request (PR) fixes the following issues

manifests/revoke.pp Outdated Show resolved Hide resolved
@Dan33l
Copy link
Member

Dan33l commented Mar 6, 2019

Thank you @jasonsattler for this PR. I added an inline comment.

Are you able to update acceptance tests by revoking certificate of the client produced by tests ?

@jasonsattler
Copy link
Author

New to ruby. Also still setting up my env. I am little stuck on how to add the notify to the tests. Any help or direction would be great. Commented it out of the puppet manifest for now.

Jason Sattler added 2 commits March 6, 2019 20:42
#331

This block of code works for me.
```
 each($revoked) |$name| {
    exec { "revoke certificate for ${name} in context of ${server}":
      command  => ". ./vars && echo yes | ./easyrsa revoke ${name} 2>&1 | grep -E 'Already revoked|was successful|not a valid certificate' && ./easyrsa gen-crl && /bin/cp -f keys/crl.pem ../crl.pem && touch revoked/${name}",
      cwd      => "/etc/openvpn/${server}/easy-rsa",
      creates  => "/etc/openvpn/${server}/easy-rsa/revoked/${name}",
      provider => 'shell',
      notify   => Service["openvpn@${server}"]
    }
  }
```

fix lint issue

fix validation

fix comma

Adding to revoke spec

Update test and revoke.pp
@jasonsattler
Copy link
Author

Found that I had the wrong name in the notify.

@Dan33l
Copy link
Member

Dan33l commented Mar 7, 2019

First step acceptance are here https://github.com/voxpupuli/puppet-openvpn/blob/master/spec/acceptance/openvpn_spec.rb.

Second step what is the puppet code used to revoke a client certificate ?
Here is the client defined during the tests : https://github.com/voxpupuli/puppet-openvpn/blob/master/spec/acceptance/openvpn_spec.rb#L53

3rd step is to run this code two times like here https://github.com/voxpupuli/puppet-openvpn/blob/master/spec/acceptance/openvpn_spec.rb#L60

4rd check the result

If you want to try you can join slack or IRC for asking questions on #voxpupuli
Or gives the puppet code of 2sd step and i'll code acceptance test.

@jasonsattler
Copy link
Author

Thank you for the direction. I will try to find some time today to look at that. Last night was able to get the notify back in and get the following test to work.

bundle exec rake spec SPEC=spec/defines/openvpn_revoke_spec.rb

Looks like this one is failing. Which I have not had time to look at yet.

bundle exec rake spec SPEC=spec/classes/openvpn_init_spec.rb

output:

 1) openvpn on archlinux-4-x86_64 should compile into a catalogue without dependency cycles
     Failure/Error: it { is_expected.to compile.with_all_deps }
     
       dependency cycles found: (Concat_file[/etc/openvpn/uster/download-configs/uster-client2.ovpn] => Concat[/etc/openvpn/uster/download-configs/uster-client2.ovpn] => Openvpn::Client[uster-client2] => Openvpn::Revoke[uster-client2] => Exec[revoke certificate for uster-client2 in context of uster] => Service[openvpn@uster] => Openvpn::Server[uster] => Openvpn::Revoke[uster-client2])
       ; (Concat_file[/etc/openvpn/winterthur/download-configs/winti-client2.ovpn] => Concat[/etc/openvpn/winterthur/download-configs/winti-client2.ovpn] => Openvpn::Client[winti-client2] => Openvpn::Revoke[winti-client2] => Exec[revoke certificate for winti-client2 in context of winterthur] => Service[openvpn@winterthur] => Openvpn::Server[winterthur] => Openvpn::Revoke[winti-client2])
     # ./spec/classes/openvpn_init_spec.rb:8:in `block (4 levels) in <top (required)>'

@bastelfreak bastelfreak added bug Something isn't working tests-fail labels Mar 7, 2019
case $openvpn::easyrsa_version {
'2.0': {
exec { "revoke certificate for ${name} in context of ${server}":
command => ". ./vars && ./revoke-full ${name}; echo \"exit $?\" | grep -qE '(error 23|exit (0|2))' && touch revoked/${name}",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A bit annoying that this is so ugly to automate :(

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree 100%!!!!!! I was not very happy to find that.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is why acceptance tests are mandatory IMO. This will permit to check if it is not too much fragile.

}

exec { "revoke certificate for ${name} in context of ${server}":
command => ". ./vars && echo yes | ./easyrsa revoke ${name} 2>&1 | grep -E 'Already revoked|was successful|not a valid certificate' && ./easyrsa gen-crl && /bin/cp -f keys/crl.pem ../crl.pem && touch revoked/${name}",

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

May I ask, why you are using 'echo yes' instead of the --batch switch on easyrsa?

I just hacked together a 'revoke-full' temp fix and found this to be simpler.
./easyrsa --batch revoke ${name}

Cheers,
Jan

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No reason, just missed that flag.

@Dan33l
Copy link
Member

Dan33l commented May 9, 2019

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

@jasonsattler
Copy link
Author

Sorry I have not gotten back to this. Been busy rolling a new service out to production. If the other PR has test feel free to close this one.

@Dan33l
Copy link
Member

Dan33l commented May 15, 2019

So i close this PR.
Thank you guys.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working skip-changelog tests-fail
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants