Warn and fix umask values that are too permissive #686
Conversation
azdagron
requested review from
amartinezfayo,
evan2645,
MarcosDY,
martincapello and
walmav
as code owners
cmd/spire-agent/cli/run/run.go
Outdated
| @@ -30,6 +30,8 @@ const ( | |||
| defaultDataDir = "." | |||
| defaultLogLevel = "INFO" | |||
| defaultUmask = 0077 | |||
There was a problem hiding this comment.
Hmm I would have expected this to go away? What role does it play now that we have a minimum?
There was a problem hiding this comment.
The default provides our default umask which filters out group and everyone perms. You can change the umask to be slightly more permissive, but not beyond the minimum.
There was a problem hiding this comment.
Umask can be changed by the operator too right? Does it make sense to do away with the configurable altogether and take whatever we get so long as it's minimum or better? Just feels a little simpler (and more traditional) than what we have now
There was a problem hiding this comment.
I've pushed a commit that moves in this direction. If the configurable is set, a warning is issued. If the configurable is does not include the minimum bits required, a warning is issued, and it is upgraded before being set. Otherwise, the current umask is checked, and if it does not have the minimum required bits, a warning is issued and the umask is upgraded.
Signed-off-by: Andrew Harding <azdagron@gmail.com>
- moved umask setting out of agent/server packages into CLI - umask setting via config is deprecated (warning logged) - umask must meet minimum requirements whether set via config or not. In either case, a warning is logged and the umask is upgraded. Signed-off-by: Andrew Harding <azdagron@gmail.com>
Signed-off-by: Andrew Harding <azdagron@gmail.com>
pkg/common/cli/umask.go
Outdated
| desiredUmask |= minimumUmask | ||
| log.Warnf("Desired umask %#04o is too permissive; setting umask %#04o.", badUmask, desiredUmask) | ||
| } else { | ||
| log.Warnf("Setting umask %#04o.", desiredUmask) |
There was a problem hiding this comment.
Maybe we can get away without this one? Or perhaps it could be a debug message?
There was a problem hiding this comment.
I'd be fine making it debug.
There was a problem hiding this comment.
Actually, i just consolidated it into the warning message for deprecation. I think it turned out pretty clean.
Signed-off-by: Andrew Harding <azdagron@gmail.com>
If a umask value is provided that does not meet minimum requirements, log a warning and upgrade the umask to the minimum requirement.
The minimum requirement is defined as 0027 (masks out group write and everyone read/write/execute bits).
Fixes #664