-
Notifications
You must be signed in to change notification settings - Fork 98
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
actions/config: ensure config file is created with mode 0644 #152
Conversation
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 looks good, and thanks for adding the test.
Just a few minor nits.
actions/config.go
Outdated
configFile, err := os.OpenFile(ConfigFileLocation, createFlags, configPermissions) | ||
unix.Umask(oldMask) |
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 initially though this should be a defer
in case os.OpenFile
panics, but now I realize it's OK, as we shouldn't open any additional files on panic.
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 decided to change it to just do the OpenFile() in a helper function that overrides umask.
actions/config_test.go
Outdated
t.Fatal(err) | ||
} | ||
ConfigFileLocation = tempFile.Name() | ||
os.Remove(ConfigFileLocation) |
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.
Do we need this os.Remove
and the defer os.Remove
below?
It seems like it might be better to just use ioutil.TempDir
and then cleanup everything in a single call.
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.
Good idea. I changed it to do that instead.
actions/config_test.go
Outdated
ConfigFileLocation = tempFile.Name() | ||
os.Remove(ConfigFileLocation) | ||
|
||
err = CreateConfigFile(time.Millisecond, false) |
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.
Combine this line with the line below, for style consistancy.
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.
Done
If the user has set a restrictive umask, e.g. 0077, then /etc/fscrypt.conf would be created without the world-readable bit set. Fix it by overriding the umask when creating the file. Resolves #151
@josephlr, any more comments on this pull request? |
If the user has set a restrictive umask, e.g. 0077, then
/etc/fscrypt.conf would be created without the world-readable bit set.
Fix it by overriding the umask when creating the file.
Resolves #151