-
-
Notifications
You must be signed in to change notification settings - Fork 16.6k
nixos/amule: fix and improve #403310
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
base: master
Are you sure you want to change the base?
nixos/amule: fix and improve #403310
Conversation
47daf5f
to
4d1aff2
Compare
ba23761
to
fe31927
Compare
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: https://discourse.nixos.org/t/prs-ready-for-review-december/1711/29 |
Requesting a review from @ymarkus since he created the module currently present in nixpkgs. Sorry for the noise but since there are no maintainers I don't know who to ping. EDIT: apparently I cannot request a review from @snicket2100 and @someone-stole-my-name, probably it's because they are not in the organization |
Hey, I forgot #354517 never made its way in. Thanks for stepping up and taking care of this :) There was another pending batch of changes focusing mostly on config (like this PR), perhaps you could borrow some ideas from it (eg the full amuled{
config,
lib,
pkgs,
utils,
...
}:
with lib;
let
cfg = config.services.amule;
amuledConfigFile = builtins.toString (
pkgs.writeTextFile {
name = "amule.conf";
text = lib.generators.toINI { } amuledMergedConfig;
}
);
amuledMergedConfig = attrsets.recursiveUpdate amuledDefaultConfig cfg.config;
amuledDefaultConfig = {
eMule = {
AppVersion = "GIT";
Nick = "http://www.aMule.org";
QueueSizePref = 50;
MaxUpload = 0;
MaxDownload = 0;
SlotAllocation = 2;
Port = 4662;
UDPPort = 4672;
UDPEnable = 1;
Address = "";
Autoconnect = 1;
MaxSourcesPerFile = 300;
MaxConnections = 500;
MaxConnectionsPerFiveSeconds = 20;
RemoveDeadServer = 1;
DeadServerRetry = 3;
ServerKeepAliveTimeout = 0;
Reconnect = 1;
Scoresystem = 1;
Serverlist = 0;
AddServerListFromServer = 0;
AddServerListFromClient = 0;
SafeServerConnect = 0;
AutoConnectStaticOnly = 0;
UPnPEnabled = 0;
UPnPTCPPort = 50000;
SmartIdCheck = 1;
ConnectToKad = 1;
ConnectToED2K = 1;
TempDir = "${cfg.dataDir}/temp";
IncomingDir = "${cfg.dataDir}/incoming";
ICH = 1;
AICHTrust = 0;
CheckDiskspace = 1;
MinFreeDiskSpace = 1;
AddNewFilesPaused = 0;
PreviewPrio = 0;
ManualHighPrio = 0;
StartNextFile = 0;
StartNextFileSameCat = 0;
StartNextFileAlpha = 0;
FileBufferSizePref = 16;
DAPPref = 1;
UAPPref = 1;
AllocateFullFile = 0;
OSDirectory = cfg.dataDir;
OnlineSignature = 0;
OnlineSignatureUpdate = 5;
EnableTrayIcon = 0;
MinToTray = 0;
Notifications = 0;
ConfirmExit = 1;
StartupMinimized = 0;
"3DDepth" = 10;
ToolTipDelay = 1;
ShowOverhead = 0;
ShowInfoOnCatTabs = 1;
VerticalToolbar = 0;
GeoIPEnabled = 1;
ShowVersionOnTitle = 0;
VideoPlayer = "";
StatGraphsInterval = 3;
statsInterval = 30;
DownloadCapacity = 300;
UploadCapacity = 100;
StatsAverageMinutes = 5;
VariousStatisticsMaxValue = 100;
SeeShare = 2;
FilterLanIPs = 1;
ParanoidFiltering = 1;
IPFilterAutoLoad = 1;
IPFilterURL = "";
FilterLevel = 127;
IPFilterSystem = 0;
FilterMessages = 1;
FilterAllMessages = 0;
MessagesFromFriendsOnly = 0;
MessageFromValidSourcesOnly = 1;
FilterWordMessages = 0;
MessageFilter = "";
ShowMessagesInLog = 1;
FilterComments = 0;
CommentFilter = "";
ShareHiddenFiles = 0;
AutoSortDownloads = 0;
NewVersionCheck = 1;
AdvancedSpamFilter = 1;
MessageUseCaptchas = 1;
Language = "";
SplitterbarPosition = 75;
YourHostname = "";
DateTimeFormat = "%A, %x, %X";
AllcatType = 0;
ShowAllNotCats = 0;
SmartIdState = 0;
DropSlowSources = 0;
KadNodesUrl = "http://upd.emule-security.org/nodes.dat";
Ed2kServersUrl = "http://upd.emule-security.org/server.met";
ShowRatesOnTitle = 0;
GeoLiteCountryUpdateUrl = "http://geolite.maxmind.com/download/geoip/database/GeoLiteCountry/GeoIP.dat.gz";
StatsServerName = "Shorty's ED2K stats";
StatsServerURL = "http://ed2k.shortypower.dyndns.org/?hash=";
CreateSparseFiles = 1;
};
Browser = {
OpenPageInTab = 1;
CustomBrowserString = "";
};
Proxy = {
ProxyEnableProxy = 0;
ProxyType = 0;
ProxyName = "";
ProxyPort = 1080;
ProxyEnablePassword = 0;
ProxyUser = "";
ProxyPassword = "";
};
ExternalConnect = {
UseSrcSeeds = 0;
AcceptExternalConnections = 1;
ECAddress = "127.0.0.1";
ECPort = 4712;
ECPassword = null;
UPnPECEnabled = 0;
ShowProgressBar = 1;
ShowPercent = 1;
UseSecIdent = 1;
IpFilterClients = 1;
IpFilterServers = 1;
TransmitOnlyUploadingClients = 0;
};
WebServer = {
Enabled = if cfg.enableWeb then 1 else 0;
Password = null;
PasswordLow = "";
Port = 4711;
WebUPnPTCPPort = 50001;
UPnPWebServerEnabled = 0;
UseGzip = 1;
UseLowRightsUser = 0;
PageRefreshTime = 60;
Template = "default";
Path = "amuleweb";
};
GUI = {
HideOnClose = 0;
};
Razor_Preferences = {
FastED2KLinksHandler = 1;
};
SkinGUIOptions = {
Skin = "";
};
Statistics = {
MaxClientVersions = 0;
};
Obfuscation = {
IsClientCryptLayerSupported = 1;
IsCryptLayerRequested = 1;
IsClientCryptLayerRequired = 0;
CryptoPaddingLenght = 254;
CryptoKadUDPKey = null;
};
PowerManagement = {
PreventSleepWhileDownloading = 0;
};
UserEvents = { };
"UserEvents/DownloadCompleted" = {
CoreEnabled = 0;
CoreCommand = "";
GUIEnabled = 0;
GUICommand = "";
};
"UserEvents/NewChatSession" = {
CoreEnabled = 0;
CoreCommand = "";
GUIEnabled = 0;
GUICommand = "";
};
"UserEvents/OutOfDiskSpace" = {
CoreEnabled = 0;
CoreCommand = "";
GUIEnabled = 0;
GUICommand = "";
};
"UserEvents/ErrorOnCompletion" = {
CoreEnabled = 0;
CoreCommand = "";
GUIEnabled = 0;
GUICommand = "";
};
HTTPDownload = {
URL_1 = "https://shortypower.org/server.met";
URL_2 = "http://upd.emule-security.org/server.met";
};
};
in
{
options = {
services.amule = {
enable = mkOption {
type = types.bool;
default = false;
description = ''
Whether to run the AMule daemon.
'';
};
enableWeb = mkOption {
type = types.bool;
default = false;
description = ''
Whether to run the AMule web interface.
'';
};
config = mkOption {
type = types.attrs;
default = amuledDefaultConfig;
description = ''
The configuration for the AMule daemon. At a minimum, you need to
provide: ExternalConnect.ECPassword, and Obfuscation.CryptoKadUDPKey.
'';
};
dataDir = mkOption {
type = types.str;
default = "/var/lib/amule/";
description = ''
The directory holding the configuration, and by default incoming and
temporary files.
'';
};
user = mkOption {
type = types.str;
default = "amule";
description = ''
The user the AMule daemon should run as.
'';
};
group = mkOption {
type = types.str;
default = "amule";
description = ''
The group the AMule daemon should run as.
'';
};
package = mkPackageOption pkgs "amule-daemon" { };
};
};
config = mkIf cfg.enable {
assertions = [
{
assertion = amuledMergedConfig.ExternalConnect.ECPassword != null;
message = ''
<option>services.amule.config.ExternalConnect.ECPassword</option> needs to be set.
'';
}
{
assertion = amuledMergedConfig.Obfuscation.CryptoKadUDPKey != null;
message = ''
<option>services.amule.config.Obfuscation.CryptoKadUDPKey</option> needs to be set.
You can generate a random unsigned 32bit integer using "od -An -N4 -tu4 < /dev/urandom | tr -d ' '"
'';
}
{
assertion = cfg.enableWeb -> amuledMergedConfig.WebServer.Password != null;
message = ''
<option>services.amule.config.WebServer.Password</option> needs to be set if the web interface is enabled.
'';
}
];
systemd.tmpfiles.settings."10-amuled".${cfg.dataDir}.d = {
inherit (cfg) user group;
mode = "0700";
};
users.users = mkIf (cfg.user == "amule") {
amule = {
inherit (cfg) group;
home = cfg.dataDir;
isNormalUser = false;
isSystemUser = true;
uid = config.ids.uids.amule;
};
};
users.groups = mkIf (cfg.group == "amule") {
amule = {
gid = config.ids.gids.amule;
};
};
systemd.services.amuled = {
description = "AMule daemon";
wantedBy = [ "multi-user.target" ];
after = [ "network.target" ];
environment.HOME = cfg.dataDir;
restartTriggers = [ amuledConfigFile ];
# aMule requires write permissions to the config. A link to the store,
# bind mount or the likes causes a crash, and if it's possible aMule
# recreating the config as a normal file.
preStart = ''
rm -f ${cfg.dataDir}/amule.conf
cat ${amuledConfigFile} > ${cfg.dataDir}/amule.conf
'';
serviceConfig = {
User = "${cfg.user}";
Group = "${cfg.group}";
WorkingDirectory = cfg.dataDir;
ExecStart = utils.escapeSystemdExecArgs (
[
(lib.getExe' cfg.package "amuled")
"--config-dir=${config.services.amule.dataDir}"
]
++ optionals cfg.enableWeb [ "--use-amuleweb=${pkgs.amule-web}/bin/amuleweb" ]
);
# TODO: this failed
Restart = "on-failure";
RestartSec = "5s";
# Hardening
NoNewPrivileges = true;
PrivateTmp = true;
PrivateDevices = true;
DevicePolicy = "closed";
ProtectSystem = "strict";
ReadWritePaths = cfg.dataDir;
ProtectHome = "tmpfs";
ProtectControlGroups = true;
ProtectKernelModules = true;
ProtectKernelTunables = true;
RestrictAddressFamilies = "AF_INET";
RestrictNamespaces = true;
RestrictRealtime = true;
RestrictSUIDSGID = true;
LockPersonality = true;
};
};
};
} |
@someone-stole-my-name Cool! Thanks for the quick answer, I had no idea you've already worked on that, I will definitely steal from your module. However there is one thing that I can't understand in your implementation i.e. why you force the user into setting I've just edited my module, now there is an assertion that forces the user into not setting that key. Then, when aMule is first started I noticed that it edits the configuration file populating that value and creates the Does this make sense to you? |
iirc the idea was to force the user to do what amule would do for him if the value was unset (src/Preferences.cpp#L1225). No idea what's the relationship between |
Probably you are right, they are not related. However what's the point of forcing the user into generating that value manually and then set the options when aMule already dynamically generates it? Honestly I prefer leaving aMule generate it freeing the user from the burden of manually set it. EDIT: I believe I was wrong, probably I didn't really delete the configuration file and restarted the service, I've just tried again and that value in the configuration file is empty. However I still think that that value may be automatically generated in the pre-start script. |
It's just a matter of taste I suppose 😅 I wanted config file to look exactly as specified by the user without hidden magic underneath. Your change forces a new |
14288f0
to
b38b954
Compare
966199f
to
c6cf19b
Compare
Sorry, I'm a bit surprised as I can't remember ever having anything to do with this, so I can't help. |
I changed my mind several times, I did as you but then I've realized that I'm not very confident that
Don't worry about that and thank you for answering! |
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: https://discourse.nixos.org/t/prs-ready-for-review/3032/5455 |
ccce0ec
to
2eafe4b
Compare
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 has slipped through the cracks; forgive me. Please rebase to address the merge conflicts.
Please also give the option descriptions one final pass to fix missing punctuation if needed.
SmartIdState = 1; | ||
DropSlowSources = 0; | ||
KadNodesUrl = "http://upd.emule-security.org/nodes.dat"; | ||
Ed2kServersUrl = "http://upd.emule-security.org/server.met"; | ||
ShowRatesOnTitle = 0; | ||
GeoLiteCountryUpdateUrl = "http://geolite.maxmind.com/download/geoip/database/GeoLiteCountry/GeoIP.dat.gz"; | ||
StatsServerName = "Shorty's ED2K stats"; | ||
StatsServerURL = "http://ed2k.shortypower.dyndns.org/?hash="; | ||
CreateSparseFiles = 1; | ||
}; | ||
Browser = { | ||
OpenPageInTab = 1; | ||
CustomBrowserString = ""; | ||
}; | ||
Proxy = { | ||
ProxyEnableProxy = 0; | ||
ProxyType = 0; | ||
ProxyName = ""; | ||
ProxyPort = 1080; | ||
ProxyEnablePassword = 0; | ||
ProxyUser = ""; | ||
ProxyPassword = ""; | ||
}; | ||
ExternalConnect = { | ||
UseSrcSeeds = 0; | ||
AcceptExternalConnections = 1; | ||
ECAddress = ""; | ||
ECPort = null; | ||
ECPassword = ""; | ||
UPnPECEnabled = 0; | ||
ShowProgressBar = 1; | ||
ShowPercent = 1; | ||
UseSecIdent = 1; | ||
IpFilterClients = 1; | ||
IpFilterServers = 1; | ||
TransmitOnlyUploadingClients = 0; | ||
}; | ||
WebServer = { | ||
Enabled = null; | ||
Password = ""; | ||
PasswordLow = ""; | ||
Port = null; | ||
WebUPnPTCPPort = 50001; | ||
UPnPWebServerEnabled = 0; | ||
UseGzip = 1; | ||
UseLowRightsUser = 0; | ||
PageRefreshTime = 120; | ||
Template = "default"; | ||
Path = "amuleweb"; | ||
}; | ||
GUI = { | ||
HideOnClose = 0; | ||
}; | ||
Razor_Preferences = { | ||
FastED2KLinksHandler = 1; | ||
}; | ||
SkinGUIOptions = { | ||
Skin = ""; | ||
}; | ||
Statistics = { | ||
MaxClientVersions = 0; | ||
}; | ||
Obfuscation = { | ||
IsClientCryptLayerSupported = 1; | ||
IsCryptLayerRequested = 1; | ||
IsClientCryptLayerRequired = 0; | ||
CryptoPaddingLenght = 254; | ||
CryptoKadUDPKey = ""; | ||
}; | ||
PowerManagement = { | ||
PreventSleepWhileDownloading = 0; | ||
}; | ||
"UserEvents/DownloadCompleted" = { | ||
CoreEnabled = 0; | ||
CoreCommand = ""; | ||
GUIEnabled = 0; | ||
GUICommand = ""; | ||
}; | ||
"UserEvents/NewChatSession" = { | ||
CoreEnabled = 0; | ||
CoreCommand = ""; | ||
GUIEnabled = 0; | ||
GUICommand = ""; | ||
}; | ||
"UserEvents/OutOfDiskSpace" = { | ||
CoreEnabled = 0; | ||
CoreCommand = ""; | ||
GUIEnabled = 0; | ||
GUICommand = ""; | ||
}; | ||
"UserEvents/ErrorOnCompletion" = { | ||
CoreEnabled = 0; | ||
CoreCommand = ""; | ||
GUIEnabled = 0; | ||
GUICommand = ""; | ||
}; | ||
HTTPDownload = { | ||
URL_1 = ""; | ||
}; | ||
}; |
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 actually need to access the value of every single one of these options? Does the final settings file need everything here?
The user the AMule daemon should run as. | ||
Directory where aMule moves completed downloads. | ||
Files in this directory are automatically shared. | ||
Ensure the aMule service has write permissions |
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.
Ensure the aMule service has write permissions |
The module is already making sure of this, so no need to bother the user.
OSDirectory = mkOption { | ||
type = types.path; | ||
default = cfg.dataDir; | ||
defaultText = literalExpression "\${config.services.amule.dataDir}"; | ||
description = "On-disk state directory, probably you don't want to change this"; | ||
}; |
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.
OSDirectory = mkOption { | |
type = types.path; | |
default = cfg.dataDir; | |
defaultText = literalExpression "\${config.services.amule.dataDir}"; | |
description = "On-disk state directory, probably you don't want to change this"; | |
}; | |
OSDirectory = mkOption { | |
type = types.path; | |
default = cfg.dataDir; | |
defaultText = literalExpression "\${config.services.amule.dataDir}"; | |
description = "On-disk state directory, probably you don't want to change this."; | |
internal = true; | |
}; |
Options that shouldn't be changed should be marked as internal
.
Enabled = lib.mkOption { | ||
type = types.enum [ | ||
0 | ||
1 | ||
]; | ||
default = 0; | ||
description = "Set to 1 to enable the web server"; | ||
}; |
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.
If aMule makes heavy use of integer booleans, then it might be worth it to create a helper function that converted our true
/false
booleans into integer booleans, for ergonomics.
openPeerPorts = mkEnableOption "open the peer port(s) in the firewall"; | ||
|
||
openExternalConnectPort = mkEnableOption "open the external connect port"; | ||
|
||
openWebServerPort = mkEnableOption "open the web server port"; |
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.
openPeerPorts = mkEnableOption "open the peer port(s) in the firewall"; | |
openExternalConnectPort = mkEnableOption "open the external connect port"; | |
openWebServerPort = mkEnableOption "open the web server port"; | |
openFirewall = mkEnableOption "opening the aMule ports in the firewall"; |
Any reason why this can't be combined into one openFirewall
setting? Enabling it will open the aMule ports, and if the webserver is also enabled, then it'll open the webserver port as well.
users.groups = lib.mkIf (cfg.user == null) [ | ||
{ | ||
name = "amule"; | ||
gid = config.ids.gids.amule; |
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.
Are we no longer using the amule GID?
User = cfg.user; | ||
Group = cfg.group; |
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.
Since there's only one service, can we use systemd's DynamicUser feature here?
Things done
settings
optionamule
,amule-gui
,amule-web
andamule-daemon
) and the NixOS modulemainProgram
to all the packages aboveThis PR should supersede both #226502 and #354517
nix.conf
? (See Nix manual)sandbox = relaxed
sandbox = true
nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD"
. Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/
)Add a 👍 reaction to pull requests you find important.