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

chown crl.pem to nobody on revoke #47

Closed
wants to merge 2 commits into from
Closed

chown crl.pem to nobody on revoke #47

wants to merge 2 commits into from

Conversation

tes5884
Copy link

@tes5884 tes5884 commented Mar 28, 2017

This is a patch for Issue #25.

Apologies if I didn't format things correctly, this is my first time doing a pull request.

Thanks

@@ -133,6 +133,7 @@ if [[ -e /etc/openvpn/server.conf ]]; then
rm -rf pki/issued/$CLIENT.crt
rm -rf /etc/openvpn/crl.pem
cp /etc/openvpn/easy-rsa/pki/crl.pem /etc/openvpn/crl.pem
chmod nobody:nobody /etc/openvpn/crl.pem
Copy link
Contributor

Choose a reason for hiding this comment

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

It would be better to use instead chmod nobody:$NOGROUP /etc/openvpn/crl.pem but

# Find out if the machine uses nogroup or nobody for the permissionless group
if grep -qs "^nogroup:" /etc/group; then
        NOGROUP=nogroup
else
       	NOGROUP=nobody
fi

from here should be moved on the top of the script

Copy link
Contributor

Choose a reason for hiding this comment

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

Moreover, I think it's chown, not chmod, doesn't it?

Copy link
Author

Choose a reason for hiding this comment

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

  1. Much better!
  2. I'm an idiot - the irony is on my local (not git) script I actually did use chown...

Excuse my newbness - do I change my patch, or do you just submit your own? how does this work?

Thanks!

Copy link
Contributor

Choose a reason for hiding this comment

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

You should change your patch or this owner of this repo can.
I'm not the owner so I can't push code on this repo.

So the best way is you change your patch :)

Thank you

@tes5884
Copy link
Author

tes5884 commented Mar 29, 2017

Did the change - hopefully I did it correctly :)

Thank you @Kcchouette for your assistance, and advice!

@tes5884 tes5884 changed the title chmod crl.pem to nobody on revoke chown crl.pem to nobody on revoke Mar 30, 2017
@robiiinos
Copy link
Contributor

I'm gonna try it!
Before your PR, I did a pretty same correction on my side to make it works.

@Kcchouette
Copy link
Contributor

Have you tried it, @Bashilor ?

@chocolateshirt
Copy link

@Kcchouette : I had done it and it proved

go to /etc/openvpn

then make chown nobody crl.pem

@angristan
Copy link
Owner

I tested using @tes5884 fork, but after the installation, crl.pem is still owned by root:root. I'm searching what is wrong

@angristan
Copy link
Owner

Oh, ok.
Why do you put chown nobody:$NOGROUP /etc/openvpn/crl.pem after revoking the client ? Shouldn't it be when installing OpenVPN ?

Copy link
Contributor

@Kcchouette Kcchouette left a comment

Choose a reason for hiding this comment

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

Indeed, you should replace that by chown nobody:$NOGROUP /etc/openvpn/crl.pem, shouldn't you?

@angristan
Copy link
Owner

What ?

@Kcchouette
Copy link
Contributor

Replace chmod 644 /etc/openvpn/crl.pem by chown nobody:$NOGROUP /etc/openvpn/crl.pem (or adding it after) here?

@angristan angristan closed this in 8c66c8e Jun 25, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants