nixos/nextcloud: automatically import system certificates

[provokateurin]

By default Nextcloud manages its own certificates and ignores the system certificates. Usually this is fine, but users will get confused why the additional certificates from security.pki.certificateFiles are not trusted by Nextcloud. Calling the import command multiple times will just update the existing bundle file, so any certificates removed from the system certificates will also be removed from Nextcloud.

I'm not sure if this will properly work when the bundle is updated, but I also didn't want to write security.pki.caBundle into a separate file just to tell Nextcloud to import that instead of the /etc one.
Maybe there are also some reservations against doing this automatic import, but I think it's fine since most applications just use the system certificates and Nextcloud is quite different because it manages its certificates completely itself. So this change makes Nextcloud behave more like most applications that don't have any special certificate handling.

This is the logic called by the import command: https://github.com/nextcloud/server/blob/ae16a287583a485e4350e242a09fe4c294801b46/lib/private/Security/CertificateManager.php#L151-L170

Things done

• Built on platform(s)
  • x86_64-linux
  • aarch64-linux
  • x86_64-darwin
  • aarch64-darwin
• For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
  • sandbox = relaxed
  • sandbox = true
• Tested, as applicable:
  • NixOS test(s) (look inside nixos/tests)
  • and/or package tests
  • or, for functions and "core" functionality, tests in lib/tests or pkgs/test
  • made sure NixOS tests are linked to the relevant packages
• Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
• Tested basic functionality of all binary files (usually in ./result/bin/)
• 25.05 Release Notes (or backporting 24.11 and 25.05 Release notes)
  • (Package updates) Added a release notes entry if the change is major or breaking
  • (Module updates) Added a release notes entry if the change is significant
  • (Module addition) Added a release notes entry if adding a new NixOS module
• Fits CONTRIBUTING.md.

---

Add a 👍 reaction to pull requests you find important.

[Ma27]

Hmm, this only adds stuff from the system-wide ca bundle, right?

Is there a particular reason Nextcloud does this on its own? Is it an option to somehow make it use the global store only? Otherwise, the settings from security.pki.caCertificateBlacklist wouldn't be propagated to Nextcloud.

Also, I think we need something like restartTriggers = [ config.environment.etc."ssl/certs/ca-certificates.crt".source ] in here to make sure changes to only the ca bundle actually trigger a change here.

[provokateurin]

Yes you can only add certificates or remove added ones, but not remove certificates trusted by Nextcloud itself.

I'm not sure why Nextcloud manages this on its own, but I'll have a look at the code and commit history for some clues.

[Ma27]

Yes you can only add certificates or remove added ones, but not remove certificates trusted by Nextcloud itself.

So, while I haven't looked at the code yet, I'm starting to consider patching this out.
Like, I kinda get that it may be useful to add CAs here (though it's kinda weird to bypass the system-wide store). But exclusive management sounds pretty undesirable. I don't want to wait for a Nextcloud release (and Nextcloud shouldn't have to do releases) just to get rid of honest Ahmed's CA in time[1]

[1] While I hope it's obviously hyperbolic given this request never made it, I think I have a point nonetheless given the sensitivity of CAs.

[provokateurin]

So I asked some colleagues and this is the information I was able to gather:

The feature was introduced in owncloud/core#10420 and used to be there in order for users to provide their own certificates for external storages. Nowadays this functionality seems to be lost as it doesn't exist anywhere in the frontend nor the backend, so it only manages Nextcloud wide certificates.
I don't think there are strict reasons why it has to be that way, but unfortunately this system exists and removing it will be an annoying breaking change, so upstream is very unlikely to accept such a change.

I'm starting to consider patching this out

I was thinking about that as well and it should be viable to write and maintain such a patch, but unfortunately patching Nextcloud will be a pain due to the builtin integrity check which will start to complain about the changes. We can probably work around this by disabling the integrity check with another patch or go with #354035 where we should be able to apply patches and still pass the integrity check. In the context of NixOS it should be fine to also remove the integrity check, since the nix store already has its own integrity check.

A simpler way that also retains the feature would be to replace https://github.com/nextcloud/server/blob/master/resources/config/ca-bundle.crt during the build process, but that would mean everyone who has custom certificates will rebuild the Nextcloud package. The problem with the integrity check would remain though.

I don't want to wait for a Nextcloud release (and Nextcloud shouldn't have to do releases) just to get rid of honest Ahmed's CA in time

This was also an argument I brought up, since it allows admins to react much quicker to untrusted certificates and doesn't require updating Nextcloud.

[provokateurin]

So I tested this:

      patchPhase = ''
        cp ${cacert}/etc/ssl/certs/ca-bundle.crt resources/config/ca-bundle.crt
      '';

It works just fine, but as expected the integrity check failed and this still wouldn't allow adding custom certificates, just the "normal" system certificates.

[Ma27]

but unfortunately patching Nextcloud will be a pain due to the builtin integrity check which will start to complain about the changes

May I ask if this check is actually relevant for the NixOS use-case?
AFAIU these kinds of checks (matomo also has it) are for use-cases where you have e.g. an rsync or FTP based deployment and there's a risk that some files don't get removed. Additionally, the store is read-only, so I'd be surprised if we'd run into bugs where the code gets modified by accident. Also (I hope I'm not misremembering), isn't this check only run on updates?

I'm asking because we had patches in the past already (e.g. 958914f).

this system exists and removing it will be an annoying breaking change, so upstream is very unlikely to accept such a change

Wouldn't it be an option to deprecate this, but give people (multiple) majors to switch?
Like, I do get that this would be a pain some older installations (though I'm wondering how many, this seems to require a more involved setup with an S3 using an internal CA?) and I think given your comment we both agree that CAs shouldn't be managed by an application unless there's a very good reason (and so far I fail to see it).

Anyways,

It works just fine, but as expected the integrity check failed and this still wouldn't allow adding custom certificates, just the "normal" system certificates.

This is the approach I'd go for if we don't get said config option upstream and removing the custom CA stuff ourselves turns out to be too expensive. With a small remark though: as you already mentioned, people may have a modified CA bundle. AFAIU all the CA bundle creation happens at build-time, so we'll probably need to expose the modified bundle in ca.nix and do cfg.package.override { cacert = config.security.pki.finalBundle; } then (the Nextcloud rebuild is cheap and it shouldn't even rebuild unless you have customizations, so it's OK in this case IMHO).

So, maybe a suggestion would be to

• patch the relevant code away in all majors we have (or patch in the correct CA bundle -- depending on what we'll agree on regarding the integrity check part)
• hope that this will make it into v32 (I don't know your exact backport policy, so I can't say if this has a chance of being backported), so we may be able to remove the patching when v31 gets out of support.

Does that make sense?

I wrote some code to add an option for this, let's see if it will be accepted: nextcloud/server#52749

Last but not least, thanks a lot for communicating this and also working on a solution. That's greatly appreciated! <3

[provokateurin]

May I ask if this check is actually relevant for the NixOS use-case?

You probably missed it, but I also said that the integrity check is really not necessary for NixOS, so we agree on this 🤝

isn't this check only run on updates?

No, it's only on the admin overview and even if it's red it doesn't break the installation. It's really just a warning for admins, but I'd dislike that we have this error shown in the admin overview while everything is actually fine.

I'm asking because we had patches in the past already

Uhm that's really odd... But it seems like the patch is gone now because it was reverted/fixed upstream?

Wouldn't it be an option to deprecate this, but give people (multiple) majors to switch?

Absolutely, but such changes are usually not that well received ("Why change it if it works?") and also require dedication to actually move forward once the deprecated phase is over. I'd expect this to be multiple years and I'm not sure that someone will then actually make the change. Since we rather need something now than in a few years this is really not a viable option (although it is the correct one).

AFAIU all the CA bundle creation happens at build-time, so we'll probably need to expose the modified bundle in ca.nix and do cfg.package.override { cacert = config.security.pki.finalBundle; } then (the Nextcloud rebuild is cheap and it shouldn't even rebuild unless you have customizations, so it's OK in this case IMHO).

That's exactly what I thought as well and I think that's the best solution if upstream isn't going to accept the proposed config option.

patch the relevant code away in all majors we have (or patch in the correct CA bundle -- depending on what we'll agree on regarding the integrity check part)

👍

I don't know your exact backport policy, so I can't say if this has a chance of being backported

Usually only fixes are backported to the currently maintained versions, but in theory I might be able to get this backported since it's a non-breaking change that isn't going to be noticable to anyone. It's mostly annoying to have features that only start working on a specific patch version in a major release, so I'd rather not do it to avoid confusion.

Does that make sense?

Yes, I think we're pretty much on the same page here!

Last but not least, thanks a lot for communicating this and also working on a solution. That's greatly appreciated! <3

❤️

[provokateurin]

Just found this in the code, so we don't have to patch out the integrity check and can easily disable it: https://github.com/nextcloud/server/blob/195dbad119c0567ae5b486efc1a4d0102844d3df/lib/private/IntegrityCheck/Checker.php#L73
The comment above has a strong warning, but I think NixOS clearly is such an exemption where it makes sense to disable it..

I'm going to give it a try now and open a new PR once I have something usable.

[provokateurin]

Done: #406252

Now having done this, I'm not even sure if we should also proceed with the config.php option as the patching approach should be enough for the use-case. What do you think @Ma27 ?

[Ma27]

You probably missed it, but I also said that the integrity check is really not necessary for NixOS, so we agree on this 🤝

Oopsie, yes indeed 🙈

I'm wondering if we should disable the integrity check by default here actually. Wdyt?

Uhm that's really odd... But it seems like the patch is gone now because it was reverted/fixed upstream?

Yes it was. I think I wasn't entirely clear on this: my point was not that the integrity check didn't complain (it did back then), but that there's a precedent for this approach.

Since we rather need something now than in a few years this is really not a viable option (although it is the correct one).

Agreed. This is mostly me hoping that we can eventually drop the customizations on our end in the long run.

[Ma27]

I think there's nothing left to discuss, so I'd close this, OK?

Will take a look at the other PR ~tomorrowish.