-
Notifications
You must be signed in to change notification settings - Fork 102
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
Support os umask #152
Support os umask #152
Conversation
add a comment to indicate that the file is managed by puppet
Add 'MANAGED BY PUPPET' header
Fix missing Requirements
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's good practise to base a PR on the current master, so you do not get changes in there that have not yet been merged or should be part of another PR.
manifests/init.pp
Outdated
@@ -214,5 +216,8 @@ | |||
password_hash => $grub_password_hash, | |||
boot_without_password => $boot_without_password, | |||
} | |||
class { 'os_hardening::umask': |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I kind of feel like this should not be a separate module but part of login_defs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
login_defs class is clearly made for managing /etc/login.defs, I don't see why it should manage umask, unless I set umask value in login.defs file which is not the case
README.md
Outdated
@@ -12,6 +12,7 @@ This Puppet module provides secure configuration of your base OS with hardening. | |||
|
|||
* Puppet OpenSource or Enterprise | |||
* [Module stdlib](https://forge.puppet.com/puppetlabs/stdlib) | |||
* [Module sysctl](https://forge.puppet.com/thias/sysctl) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should not be part of this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, please rebase
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes of course, sorry i'm new with git stuff, will try to fix this out.
files/limits.conf
Outdated
@@ -1,2 +1,3 @@ | |||
# MANAGED BY PUPPET |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should also not be part of this PR.
files/profile.conf
Outdated
@@ -1,2 +1,3 @@ | |||
# MANAGED BY PUPPET |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nor should this be part of this PR.
Create license file
Is the rebase correct at this stage ? |
manifests/umask.pp
Outdated
|
||
} | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please remove a few of these empty lines.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please run a pdk validate
on your code, there are some code style errors ...
templates/umask.sh.erb
Outdated
@@ -0,0 +1,2 @@ | |||
# MANAGED BY PUPPET | |||
umask <%= @system_umask %> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing newline?
manifests/umask.pp
Outdated
class os_hardening::umask ( | ||
$system_umask = undef, | ||
|
||
){ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add a whitespace between these brackets
Will look into this again when I'm back from my holiday (2 weeks) ... |
@hdep I have now tested this PR against my distributions (CentOS 7.5, OpenSUSE 15.0, Ubuntu 16.04.5), and on all systems I get this error message on login (and the umask is not changed):
I have set the parameter like this: I'm at loss here ... can you test this also on another distribution? |
I had an issue with my text editor which switch end of line from LF to CR/LF. |
Ha, right, I didn't see this ... and now it works on all distros!
|
The EOL for umask.p and init.pp is set to LF on my side there is nothing to change. |
As discussed in #147 here is a PR which do the following :
I test it on Debian 9 and works as expected so far.
Let me know if I need to change something.