Switch user and drop privileges - #13
Conversation
Sorry, something went wrong.
dominikschubert
left a comment
There was a problem hiding this comment.
Minor nits, otherwise LGTM! 🚀
Sorry, something went wrong.
| func DropPrivileges(userToSwitchTo string) { | ||
| // Lookup user and group IDs for the user we want to switch to. | ||
| userInfo, err := user.Lookup(userToSwitchTo) | ||
| if err != nil { | ||
| log.Errorln("Error looking up user:", userToSwitchTo, err) | ||
| } |
There was a problem hiding this comment.
| func DropPrivileges(userToSwitchTo string) { | |
| // Lookup user and group IDs for the user we want to switch to. | |
| userInfo, err := user.Lookup(userToSwitchTo) | |
| if err != nil { | |
| log.Errorln("Error looking up user:", userToSwitchTo, err) | |
| } | |
| func DropPrivileges(userToSwitchTo string) error { | |
| // Lookup user and group IDs for the user we want to switch to. | |
| userInfo, err := user.Lookup(userToSwitchTo) | |
| if err != nil { | |
| log.Errorln("Error looking up user:", userToSwitchTo, err) | |
| return err | |
| } | |
| ... | |
| return nil |
This is more of a general feedback for the methods in this file. It would generally be better to "fail" early and bubble up the error when receiving an error we don't plan to recover from.
Sorry, something went wrong.
There was a problem hiding this comment.
done for methods without a return value. Should we return a tuple for all methods with return values?
Sorry, something went wrong.
There was a problem hiding this comment.
switched to warn logging for recoverable errors for now
Sorry, something went wrong.
dfangl
left a comment
There was a problem hiding this comment.
Only problem I have is the location of the privilege dropping, which should, in my opinion, be after binding port 53 and extracting the archives.
Sorry, something went wrong.
dfangl
left a comment
There was a problem hiding this comment.
LGTM now. We should however merge the other PR first, and might want to rebase this one to the localstack branch.
Sorry, something went wrong.
Make `GetenvWithDefault` behave like `os.environ.get` in Python
As @dfangl pointed out: Binding port 53 might require root permissions for binding port < 1024 depending on the Docker version moby/moby#41030
Using warning level for recoverable warnings rather than breaking errors.
dominikschubert
left a comment
There was a problem hiding this comment.
LGTM 👍
Sorry, something went wrong.
Depends on #12
Switch to sbx_user1051 user for runtime parity and drop root privileges.
Limitations