-
-
Notifications
You must be signed in to change notification settings - Fork 14.2k
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
nixos/biboumi: init #94917
nixos/biboumi: init #94917
Conversation
Did you contacted upstream regarding the systemd hardening? I just had a quick look to their repo, looks like they're providing several Dockerfile but no contrib systemd service. It could be nice to upstream this set of hardening options and see with upstream whether this is going to break any runtime assumption they have. To be clear: this would be a nice to have but won't block this PR. Reviewing a module not having any tests is always a bit tricky. Not having any tests will also make it harder for you to maintain the module on the long run. I do understand though that this module will require both a XMPP server and a IRC one to test... Do you think you could leverage the prosody test and mock the IRC part? |
@NinjaTrappeur
|
Upstream here, just to notify that I noted the use of a systemd-socket for the 113 port here: https://lab.louiz.org/louiz/biboumi/-/issues/3441 I don’t know when this will be done, if at all. But at least it will not be forgotten. |
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.
Thanks a lot for your work on this module. I tested this on my infra and did not faced any issue so far. The systemd service is nicely hardened, kudos!
Thetypes.attrs
-based configuration seems a bit loose to me. Having to deploy the whole configuration just to read you're missing a mandatory attribute in the service logs is a long and tedious process. I think we should leverage either the NixOS module system either the software itself to validate as much configuration as possible at build time.
It seems like biboumi lacks such a -t
config test flag. It's a shame. I opened a PR upstream adding this: https://lab.louiz.org/louiz/biboumi/-/merge_requests/56. I think we can move forward with this PR without this static test though.
In the meantime, we probably could add either add some module options/assertions for the mandatory attributes (hostname, password).
Once that being done, I think we'll be ready to merge this.
@NinjaTrappeur Thanks for the review, I've moved the RFC0042 |
3fbeeda
to
dc10874
Compare
I have a build failure around the hostname default value now, looks like we should either explicitely point to I'll have a deeper look to this tonight. |
@NinjaTrappeur Fixed, thanks! |
We're still having a problem. The ofborg error is coming from the default hostname. diff --git a/nixos/modules/services/networking/biboumi.nix b/nixos/modules/services/networking/biboumi.nix
index 0c90d46cabf..9d713ba1ebf 100644
--- a/nixos/modules/services/networking/biboumi.nix
+++ b/nixos/modules/services/networking/biboumi.nix
@@ -58,7 +58,6 @@ in
};
options.hostname = mkOption {
type = types.str;
- default = "biboumi.${config.networking.domain}";
example = "biboumi.example.org";
description = ''
The hostname served by the XMPP gateway. Fixes it. I now have another strange doc-related error when deploying this on top of the current
I tried removing all the links & derivations mention in the options defaults & doc without any effect. This is the first time I'm working with In the meantime, feel free to pull your hair as well on this one :P |
Alright, I found the issue: it comes from the NixOS's system ca bundle is pointing to So overall, once we apply the following patch, we'll be good to go (manually tested on my personal infra). diff --git a/nixos/modules/services/networking/biboumi.nix b/nixos/modules/services/networking/biboumi.nix
index 0c90d46cabf..ac37e03446f 100644
--- a/nixos/modules/services/networking/biboumi.nix
+++ b/nixos/modules/services/networking/biboumi.nix
@@ -42,7 +42,7 @@ in
};
options.ca_file = mkOption {
type = types.path;
- default = "" + etc."ssl/certs/ca-certificates.crt".source;
+ default = "/etc/ssl/certs/ca-certificates.crt";
description = ''
Specifies which file should be used as the list of trusted CA
when negociating a TLS session.
@@ -58,7 +58,6 @@ in
};
options.hostname = mkOption {
type = types.str;
- default = "biboumi.${config.networking.domain}";
example = "biboumi.example.org";
description = ''
The hostname served by the XMPP gateway. We'll probably want to add some rudimentary testing in a subsequent PR. As we can see with these eval failures, there are a lot of moving parts here, it might turn into a pain to maintain. |
@NinjaTrappeur Thanks again for the debugging and sorry for not having seen the problems earlier: I'm still a bit submerged and lost in ofborg's mails and WebUI, and the "This failed status will be cleared when ofborg finishes eval." mislead me into believing it would somehow succeed. |
The CI is still red: we have a trailing space line 55. Everything should be green once this space gets removed. Note: you probably want to configure your editor to remove those when saving a file.
on emacs. |
@NinjaTrappeur Thanks again. And while we're at it, the following should do under vim:
|
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.
LGTM
Motivation for this change
To use Biboumi, an XMPP to IRC gateway: https://biboumi.louiz.org/
Things done
biboumi
to its latest release: 8.5services.biboumi
systemd-analyze security biboumi
returnOK
services.biboumi.settings
.Note that
✗ PrivateUsers=
is due to the need forAmbientCapabilities=CAP_NET_BIND_SERVICE
(See https://bugs.archlinux.org/task/65921 ) . If Biboumi's upstream were to support a systemd's socket for the identd port,PrivateUsers=true
would likely be doable.sandbox
innix.conf
on non-NixOS linux)nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
./result/bin/
)nix path-info -S
before and after)