nixos/cryptpad: init, cryptpad: init at 2024.6.0#251687
Conversation
There was a problem hiding this comment.
This is a bit ugly and fragile -- for example enabling DynamicUsers makes the directory unreadable to nginx. Might be work-aroundable with BindReadOnlyPaths in the service instead... but it works for default config, which will do for now
There was a problem hiding this comment.
Let's do small steps: let's get this somewhere and let's do follow up to stabilize further hardening, etc.
There was a problem hiding this comment.
left a comment in the code here but not quite sure what to do about ACME. Checking if acceptTerms seems safe enough to do the right thing, but it might be a bit surprising... jitsi for example just sets enableACME to mkDefault true so this is slightly better?
There was a problem hiding this comment.
That sounds good to me: if someone generally accepts ACME's terms, we might as well secure their cryptpad install by default.
There was a problem hiding this comment.
Not really, no, I'm not aware of module doing this by default, some people may accept terms and did not set the DNS record yet, so please, just let the user enable ACME explicitly.
There was a problem hiding this comment.
I wanted to avoid making the user write their domain multiple times (once in cryptpad.config.httpUnsafeOrigin/httpSafeOrigin and once here) -- would it make sense to expose nginx.virtualhosts.<domain> as cryptpad.nginxVhost/nginxSandboxVhost or something similar?
a34deef to
f7fe4dc
Compare
There was a problem hiding this comment.
That sounds good to me: if someone generally accepts ACME's terms, we might as well secure their cryptpad install by default.
There was a problem hiding this comment.
This TODO should be removed.
There was a problem hiding this comment.
huh it ended up in a weird place somehow, sorry.
I'd rather keep it or something similar somewhere: right now if someone changes the paths in services.cryptpad.config.blockPath for example the nginx config we have here won't be correct -- we'd need to include a default value in our generated file and use it in the generated nginx config to have something robust.
I don't think it's really useful to do, so I didn't, but it's something to keep in mind if it comes up.
I'll move this comment to the config freeform when this discussion is closed
There was a problem hiding this comment.
I don't see a services.cryptpad.config.blockPath, so I don't quite understand the issue you are describing...
There was a problem hiding this comment.
Right, so, "blockPath" is a setting in the config/config.example.js of the cryptpad repo: one can change this through the services.cryptpad.config freeform if they want to... But it wouldn't work, because we hardcoded /var/lib/cryptpad/block in the nginx config.
(It's not in the default values because it's not really needed -- we use the implicit defaults in cryptpad itself for the nodejs service)
Point being, I don't think it's obvious that the nginx config we use depends on the default values, so we probably ought to keep it as a comment somewhere, until someone comes along and parametrizes the config (in case of blockPath it's just a single word to change in the config, so it's not that difficult per se, but there's a dozen fooPath variables in the cryptpad config so doing everything would be a bit of work for little value in my opinion)
I'll move the comment to the end of the freeform and reword it a bit to make it more clear
There was a problem hiding this comment.
Couldn't the module set services.nginx.enable = true; if cfg.nginx.enable?
There was a problem hiding this comment.
In my nixos config I also have a few other settings like recommendedTlsSettings and others; I'm not sure how much we'd want to include in the cryptpad module or not? e.g. is nginx safe without these recommended settings? (and if not why do these default to false!)
Looking for nginx.enable in nixos/modules we have both kinds of services -- sourcehut for example has an assert like this, and nextcloud has services.nginx.enable = mkDefault true; some have a straight = true without mkDefault, some use it without checking... Some also set their own 'nginx' option to config.services.nginx.enable.
My personal preference is that one 'config.services.foo' shouldn't enable not obviously related services automatically, so either this assert or defaulting services.cryptpad.nginx to services.nginx.enable, but I don't have any strong feeling either way so happy to set this here instead as you suggest. (read: will do if you confirm)
There was a problem hiding this comment.
IMHO, the way we do it in other modules is to have a portal from cfg.nginx inside a virtual host fragment of configuration.
This way, you can do things like services.cryptpad.nginx.locations."/".xyz, etc. and act upon the configuration from there.
It makes a lot of sense because then you can extend the NixOS module with any web server as long as you have the internal domain knowledge.
Conclusion: It should not enable NGINX, it should not assert NGINX is enabled. As it is in other modules.
There was a problem hiding this comment.
Do you have a typical module that does this?
I did a quick look around before writing this and there really is a bit of everything, I didn't see any common pattern. If you're aware of one, pointing me to a "typical" (or ideal) module would be great!
Thanks for the review! |
|
TL;DR: it works! I upgraded my instance from 4.11 to what this PR provides (5.4). Instance is running fine, and documents were conserved. |
|
Another thing to note: here's the report of the checkup reportThis instance is running CryptPad 5.4.0. Page content was served by You appear to be using a firefox/gecko browser on UNIX to view this page. 52 / 57 tests passed. Details found below This instance does not provide a valid |
| Test number | 14 |
| Returned value |
This instance's encrypted support ticket functionality has not been enabled. This can make it difficult for its users to safely report issues that concern sensitive information. This can be configured via the admin panel's Support tab.
| Test number | 15 |
| Returned value |
This instance has not been configured to support web administration. This can be enabled by adding a registered user's public signing key to the adminKeys array in cryptpad/config/config.js. Changes to cryptpad/config/config.js will require a server restart in order for /api/config to be updated.
| Test number | 16 |
| Returned value | false |
No terms of service were specified. This link will be included in the home page footer and 'About CryptPad' menu. It's advised that you either provide one or disable registration. See the relevant documentation about how to customize CryptPad's terms value.
| Test number | 37 |
| Returned value |
No privacy policy was specified. This link will be included in the home page footer and 'About CryptPad' menu. It's advised that you either provide one or disable registration. See the relevant documentation about how to customize CryptPad's privacy value.
| Test number | 39 |
| Returned value |
There was a problem hiding this comment.
TL;DR: it works!
Great!
I also had to update a few documents but I was logged in so it was transparent to me, it's good to know that anonymous users can't do it -- I'll write it in the release notes
Another thing to note: here's the report of the /checkup endpoint of my instance:
sounds coherent with what I have.
My instance has registration disabled so it's not showing the privacy policy etc, but I have an error on a check that only works with registration enabled + the same warnings about support tab etc.
Support tab requires some setup on the server I haven't looked at, that'll probably require some adjustment if someone wants it to work.
I'm a bit out of time for today but to recap:
- I'll adjust the few comments I replied about just now
- What do you think about nginx? I'd tend to keep the assert but make services.cryptpad.nginx default to services.nginx.enabled?
Anything else?
I'll find some time to update this tomorrow
There was a problem hiding this comment.
Right, so, "blockPath" is a setting in the config/config.example.js of the cryptpad repo: one can change this through the services.cryptpad.config freeform if they want to... But it wouldn't work, because we hardcoded /var/lib/cryptpad/block in the nginx config.
(It's not in the default values because it's not really needed -- we use the implicit defaults in cryptpad itself for the nodejs service)
Point being, I don't think it's obvious that the nginx config we use depends on the default values, so we probably ought to keep it as a comment somewhere, until someone comes along and parametrizes the config (in case of blockPath it's just a single word to change in the config, so it's not that difficult per se, but there's a dozen fooPath variables in the cryptpad config so doing everything would be a bit of work for little value in my opinion)
I'll move the comment to the end of the freeform and reword it a bit to make it more clear
RaitoBezarius
left a comment
There was a problem hiding this comment.
NGINX configuration is an impeding maintenance disaster IMHO. We ought to separate CryptPad specific needs (and abstract over them in the right way, including changes inside the NGINX module, I am a maintainer of it and can help towards that) and stuff that a user can enable: HSTS, OSCP stapling, TLS hardening, etc.
|
Overall, an awesome news, but there's some work to do in the NixOS module to get its complexity down IMHO :). Adopting RFC42 could help drastically here, cutting down NGINX complexity and pushing it on the user might be a good idea here too. Thank you for doing this and I am available (let's say: starting the 6th) to help you land this. |
martinetd
left a comment
There was a problem hiding this comment.
Thanks a lot for your review and offer to help! guidance much appreciated, especially for the nginx bit.
Adopting RFC42 could help drastically here, cutting down NGINX complexity and pushing it on the user might be a good idea here too.
I thought RFC42 was pretty much what I did with cryptpad.config being a freeform -- I'll need to look it up again. Or was this about nginx's config?
I left a bigger comment about nginx on the SSL params things so let's continue that discussion here to keep it threaded
There was a problem hiding this comment.
Do you have a typical module that does this?
I did a quick look around before writing this and there really is a bit of everything, I didn't see any common pattern. If you're aware of one, pointing me to a "typical" (or ideal) module would be great!
There was a problem hiding this comment.
I wanted to avoid making the user write their domain multiple times (once in cryptpad.config.httpUnsafeOrigin/httpSafeOrigin and once here) -- would it make sense to expose nginx.virtualhosts.<domain> as cryptpad.nginxVhost/nginxSandboxVhost or something similar?
|
Update from cryptpad/cryptpad#1213 : they're dropping their complex nginx config in favor of one that just forwards everything to the node process. I feel it's a bit of a shame, but that'll definitely be much easier to write "properly", so I'll update this PR with that as soon as I have some time (likely next weekend). |
|
Sorry for the delay, I had mostly forgotten about this and have been keeping myself busy otherwise so might not have time to finish shortly... Regarding my last comment, I was waiting for feedback from upstream regarding the default config but they're really really bad at replying to github so never got any answer; they reverted the "simpler nginx config" shortly after committing it and now there's both configs in their documentation. With that in mind, what do you think @RaitoBezarius ? Cheers. |
Sounds good for now for me. (unfortunately always too many things to do…) |
|
Thanks for the effort to bring Cryptpad back to NixOS, @martinetd! I made a feeble attempt last year but I couldn't even get the package to build with my limited Nix skills at the time. I've met one of the Cryptpad developers last month at NGI Forum 2023 in Brussels and they said that they indeed updated their build system, nice! So I read up on the PR here and saw the discussion on the Nginx configuration bit. By chance I happen to have just done a clean Cryptpad install on a Debian server last week and I remember seeing the notes about Nginx in their documentation. There are indeed 2 ways to put Cryptpad behind Nginx now. From the Cryptpad documentation:
I went with 1. (up to 3000 concurrent users) on the Debian server because it was for a relatively small setup. But to be honest that's what I often need Cryptpad for. I'd say large scale Cryptad setups (+3000 concurrent users) are probably less common than smaller scale setups. So I would recommend to go forward with the simple Nginx configuration and get this PR moving so that Cryptpad gets back into NixOS. Then future fixup PRs can add the more complicated Nginx configuration. All that said. Can I do anything to help? I'm testing out this PR on an instance of my own. What can we do to move this forward? |
|
I've hacked on this for a couple of hours, but it's getting pretty late here and I'm tired. I'd love to help out and polish this PR a bit more. I'm not sure what the best way to collaborate on this is. I think I've been able to clean up some of the code here with the simplified Nginx config, but I wonder what the best way to share is. Because I don't think there's a way I can add commits to this PR. Maybe I have to open a PR at the source at https://github.com/martinetd/nixpkgs/tree/cryptpad? Or should I just dump code suggestions in a review? What do you think? I'm definitely not done with my incantation so I could continue to work on it over the next days / weeks and then come back here when I feel it looks presentable. Then you can have a look and decide what to do with it? |
|
Some findings:
From what I understand in https://github.com/NixOS/rfcs/blob/45b76f20add8d7e0e1d0bc0eed9357b067c899a6/rfcs/0042-config-option.md it seems like the idea is to have as few settings available as possible. I like that idea. There's not much Cryptpad needs to get going, so fewer options better. I wouldn't go as far as to create a webserver config abstraction to support other webservers. I generally prefer to stick to using what upstream supports. |
|
Sorry for the delay in updating this; I think I've covered everything:
I've also updated my own instance to this PR and was able to access old data, create new files etc fine as expected. the checkup only fails HSTS but we don't have any nginx config that sets it so I'll let that fail. So hopefully this is now ready! |
7822f37 to
4f943fa
Compare
|
I got nerd-snipped into adding some systemd hardening options to the service (then drifted into #328648 because SocketBindDeny=all didn't work...); shouldn't change anything user-facing but I've kept that as a separate commit to easily revert just in case. (At least no problem on my server, and I've also confirmed disabling blockDailyCheck also still works (EDIT: it connected but didn't validate TLS.. probably fixed now but the thing doesn't log errors so /shrug) even if I'm not sure who would do that... fun fact, if you disable it in the nixosTests then cryptpad tries to connect to the accounting server before eth0 is up so that telemetry request never goes anywhere unless you restart the service later (or make the service depend on having a default route); I'll pretend I didn't see that) Really won't touch this anymore now. |
|
(sorry to systemd folks, pushed the bpf fix by mistake here and removed it but I assume the harm is done and you got cc'd :/) |
drupol
left a comment
There was a problem hiding this comment.
Diff LGTM, nice work, I'm glad to see it back in nixpkgs !
Resolved: NGINX configs guarded by configureNginx, RFC42 adopted
drupol
left a comment
There was a problem hiding this comment.
Do you mind rewording your commit log messages accordingly?
For example, there's no need to mention that it has been added back.
Just use the regular commit message as if it was the first time it is added.
Something like:
nixos/cryptpad: initcryptpad: init at 2024.6.0
This re-adds the cryptpad package that was removed due to bower packaging being insane. upstream improved that, so add the package back. Link: cryptpad/cryptpad#295 Co-authored-by: Pol Dellaiera <pol.dellaiera@protonmail.com>
This is a full rewrite independent of the previously removed cryptpad module, managing cryptpad's config in RFC0042 along with a shiny test. Upstream cryptpad provides two nginx configs, with many optimizations and complex settings; this uses the easier variant for now but improvements (e.g. serving blocks and js files directly through nginx) should be possible with a bit of work and care about http headers. the /checkup page of cryptpad passes all tests except HSTS, we don't seem to have any nginx config with HSTS enabled in nixpkgs so leave this as is for now. Co-authored-by: Pol Dellaiera <pol.dellaiera@protonmail.com> Co-authored-by: Michael Smith <shmitty@protonmail.com>
cryptpad is not directly exposed to the network, but has plenty that can be hardened more properly, so fix that.
|
Fixed the subject line; I think it still makes sense to mention it's a re-add in the details but that probably doesn't bother anyone if I ramble a bit there. I'm not sure if you meant I should also squash the hardening patch; I'd also prefer to keep it separate in case the hardening causes problems for anyone as I haven't left my instance running with these hardened settings for long enough to be confident all corner cases work properly and it could be helpful to just revert it, but will squash if you think it's better. |
|
Thank you! Opinionated thought: I don't believe mentioning that it has been removed then added again in the commit log message is relevant. However, mentioning it in the commit log description and PR makes sense. |
|
Hmm, I guess that if the file name stays the same doing a git log by path would easily find the removal commit again, but in this case for the package the path isn't the same so one wouldn't stumble on the old package by chance so one wouldn't notice it's a re-add and try to compare with the old package (although to be fair it's different enough that it probably wouldn't be of any help) For the module git log would yield the removal and old commits, at which point I think being explicit that it's a full rewrite that has nothing to do with the old version is also helpful. Anyway, in general I'd rather be more verbose than not enough in commit messages -- if what was in commit messages wasn't enough one can always find the PR, but the first place I look is commit messages and if that has enough information I'll be more than happy to not have to open firefox. |
|
No worries, we are just discussing, and it's totally fine to not agree on everything. The current state of things in this PR is good, I wouldn't change anything. |
Things done
cryptpad removed its dependency on bower and can be sanely packaged again.
Add the package and module back.
The old module almost didn't do anything and there was no test so this really is a full rewrite and should be considered as such.
I have a few remarks/questions I'll post inline
cc people who were involved in this or earlier versions of the modules:
@Pamplemousse @DavHau @xeji @blitz @zimbatm
Link: cryptpad/cryptpad#295
fixes #217231
nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/)