Redsocks #22356
Redsocks #22356
Conversation
|
@Ekleog, thanks for your PR! By analyzing the history of the files in this pull request, we identified @edolstra, @bjornfor and @offlinehacker to be potential reviewers. |
| src = fetchFromGitHub { | ||
| owner = "darkk"; | ||
| repo = "redsocks"; | ||
| rev = "release-0.5"; |
There was a problem hiding this comment.
Consider factoring out the version so that it can be shared between name and rev.
| ''; | ||
| }; | ||
|
|
||
| user = mkOption { |
There was a problem hiding this comment.
Why does this need to be configurable?
There was a problem hiding this comment.
log strictly-speaking doesn't need to be configurable, but I thought it may come in handy to people doing centralized log management to be able to directly send logs to syslog, instead of to journalctl via stderr. As it didn't add any complexity (redsocks also has this option), I decided to add it.
Do you think it would be better left out?
There was a problem hiding this comment.
I was thinking of the user/group options; unless there's a realistic use case for using anything other than the default, please consider whether they need to be configurable.
There was a problem hiding this comment.
Oh indeed, the email github sent me started with the line defining log, so I thought that was what you were reacting about.
I just pushed a new commit adding a redsocks user (in order to avoid horizontal privilege escalation among nobody:nogroup daemons) and making this user the user for redsocks while removing the option, is this OK with you?
|
|
||
| ##### implementation | ||
| config = | ||
| let redsocks_blocks = concatMapStrings (block: |
There was a problem hiding this comment.
A small stylistic note: You can have more than one binding per let; as it is, having so many levels of strictly unnecessary lets is a bit unusual and so makes the code a bit harder to follow.
There was a problem hiding this comment.
I think it's better in latest push -f, thanks!
|
Just wanted to check current status, as I think the issues mentioned are now fixed? |
|
Would you mind squashing into two commits, one the package and one for the module? |
redsocks module: use separate user for redsocks daemon
|
Not at all, here it is! |
|
Thank you |
|
Thanks for your feedback! 😄 |
Motivation for this change
This change adds package redsocks and a module for it, allowing to do transparent proxifying of applications via iptables.
Things done
(nix.useSandbox on NixOS,
or option
build-use-sandboxinnix.confon non-NixOS)
nix-shell -p nox --run "nox-review wip"./result/bin/)Hope this helps!