Skip to content

nixos/nextcloud: automatically import system certificates #405873

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.

Sign up for GitHub

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

Closed
provokateurin wants to merge 1 commit into from
Closed

nixos/nextcloud: automatically import system certificates #405873

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions nixos/modules/services/web-apps/nextcloud.nix
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1205,6 +1205,8 @@ in

${lib.getExe occ} upgrade

${lib.getExe occ} security:certificates:import /etc/ssl/certs/ca-certificates.crt
Copy link
Member

@Ma27 Ma27 May 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

@provokateurin provokateurin May 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@Ma27 Ma27 May 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

@provokateurin provokateurin May 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

@provokateurin provokateurin May 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@Ma27 Ma27 May 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

@provokateurin provokateurin May 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

❤️

Copy link
Member Author

@provokateurin provokateurin May 11, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

@provokateurin provokateurin May 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ?

Copy link
Member

@Ma27 Ma27 May 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


${lib.getExe occ} config:system:delete trusted_domains

${optionalString (cfg.extraAppsEnable && cfg.extraApps != { }) ''
Expand Down