nextcloud25: use openssl 1.1 as a PHP extension to fix RC4 encryption #198470

RaitoBezarius · 2022-10-29T14:22:48Z

Description of changes

NextCloud encryption is RC4-based, when moving to NextCloud 25 and OpenSSL3, NC cannot decrypt the files anymore, see #197726.

This adds a openssl-legacy PHP extension that can be used for these scenarios where OpenSSL 1.1 is easier to use.

Another solution is to provide a custom openssl.cnf file to re-enable RC4 encryption to OpenSSL 3, I will open such a PR soon, but it will might be a bit uglier than this one.

cc @mweinelt @Ma27 @AkechiShiro

Things done

drupol

LGTM but I'll let the seniors approve it.

ajs124

I hate this, but we probably don't really have another choice. Or rather, I don't see how using openssl_3 and enabling legacy ciphers is better. Let's just hope upstream manages to fix this sometime soon.

What we could do is make this an option and add it to the release notes. That's probably not a better solution either.

I'd love to get some more opinions from some nixos nextcloud and php people.

RaitoBezarius · 2022-10-29T22:45:43Z

I hate this, but we probably don't really have another choice. Or rather, I don't see how using openssl_3 and enabling legacy ciphers is better. Let's just hope upstream manages to fix this sometime soon.

What we could do is make this an option and add it to the release notes. That's probably not a better solution either.

I'd love to get some more opinions from some nixos nextcloud and php people.

We can get this behind a flag to be fair and let people enable it when they need it plus letting people who really want server side encryption complain.

Ma27

👎 from me. I'm pretty sure that this is not the only occurrence in the codebase where PHP's openssl extension is used. If that's the case I don't want to disable OpenSSL 3 for everyone using Nextcloud (just because some people have that security theatre enabled).

I'm fine with merging that, but only if the behavior is hidden behind a flag that's called e.g. enableBrokenCiphersForSSE (and yes, the option's name should reflect the fact that you're using broken ciphers IMHO).

RaitoBezarius · 2022-11-06T17:30:38Z

-1 from me. I'm pretty sure that this is not the only occurrence in the codebase where PHP's openssl extension is used. If that's the case I don't want to disable OpenSSL 3 for everyone using Nextcloud (just because some people have that security theatre enabled).

I'm fine with merging that, but only if the behavior is hidden behind a flag that's called e.g. enableBrokenCiphersForSSE (and yes, the option's name should reflect the fact that you're using broken ciphers IMHO).

Totally agreed and asked for some opinions in Matrix chat beforehand regarding this, and I was open to this precise solution. :) (just took the easiest way to not do useless work).

This is now done, I may have been a bit aggressive with the message, let me know your thoughts on them.
Plus, I added a test for this reusing basic.nix and I explicitly advise to disable SSE.

RaitoBezarius · 2022-11-06T17:31:13Z

@mweinelt do you think I should add more notes in the release notes?

drupol

Beautiful!

Question for my own information: Shouldn't enableBrokenCiphersForSSE be a global flag in nixpkgs ?

mweinelt · 2022-11-06T17:36:19Z

@mweinelt do you think I should add more notes in the release notes?

cc @Mic92 @winterqt

RaitoBezarius · 2022-11-06T17:37:17Z

Beautiful!

Question for my own information: Shouldn't enableBrokenCiphersForSSE be a global flag in nixpkgs ?

We enable the OpenSSL 1.x replacement only for this very NixOS module, therefore, it does not prevent anyone from using a NextCloud using OpenSSL 3.x locally, does not prevent anyone from replacing our module with a smarter module for anything I cannot even imagine, etc.

I think very local replacements like this one are ideal, esp. to let advanced users be able to do whatever they want and keep composability.

If a user want to port our mechanism to patch, I provide for a fake OpenSSL PHP Extension called openssl-legacy linked against OpenSSL 1.x and can be reused in any NixOS module or PHP environment.

nixos/modules/services/web-apps/nextcloud.nix

Ma27 · 2022-11-09T13:00:06Z

OK, found another small thing to be fixed before merging, please don't do that right now.

I guess to be safe, I should implement a VM test for that.

* s/NextCloud/Nextcloud/g * `enableBrokenCiphersForSSE` should be enabled by default for any NixOS installation from before 22.11 to make sure existing installations don't run into the issue. Not the other way round. * Update release notes to reflect on that. * Improve wording of the warning a bit: explain which option to change to get rid of it. * Ensure that basic tests w/o `enableBrokenCiphersForSSE` run with OpenSSL 3.

Ma27 · 2022-11-10T12:56:05Z

Rebased onto master and pushed another fix (see commit message for all the details).

Considering that this now touches PHP stuff and I contributed quite a bunch of stuff in here, I'd like to have more reviewers.
cc @ajs124 @drupol @RaitoBezarius @yayayayaka

nixos/modules/services/web-apps/nextcloud.nix

Upon testing the change itself I realized that it doesn't build properly because * the `pname` of a php extension is `php-<name>`, not `<name>`. * calling the extension `openssl-legacy` resulted in PHP trying to compile `ext/openssl-legacy` which broke since it doesn't exist: source root is php-8.1.12 setting SOURCE_DATE_EPOCH to timestamp 1666719000 of file php-8.1.12/win32/wsyslog.c patching sources cdToExtensionRootPhase /nix/store/48mnkga4kh84xyiqwzx8v7iv090i7z66-stdenv-linux/setup: line 1399: cd: ext/openssl-legacy: No such file or directory I didn't encounter that one before because I was mostly interested in having a sane behavior for everyone not using this "feature" and the documentation around this. My findings about the behavior with turning openssl1.1 on/off are still valid because I tested this on `master` with manually replacing `openssl` by `openssl_1_1` in `php-packages.nix`. To work around the issue I had to slightly modify the extension build-system for PHP: * The attribute `extensionName` is now relevant to determine the output paths (e.g. `lib/openssl.so`). This is not a behavioral change for existing extensions because then `extensionName==name`. However when specifying `extName` in `php-packages.nix` this value is overridden and it is made sure that the extension called `extName` NOT `name` (i.e. `openssl` vs `openssl-legacy`) is built and installed. The `name` still has to be kept to keep the legacy openssl available as `php.extensions.openssl-legacy`. Additionally I implemented a small VM test to check the behavior with server-side encryption: * For `stateVersion` below 22.11, OpenSSL 1.1 is used (in `basic.nix` it's checked that OpenSSL 3 is used). With that the "default" behavior of the module is checked. * It is ensured that the PHP interpreter for Nextcloud's php-fpm actually loads the correct openssl extension. * It is tested that (encrypted) files remain usable when (temporarily) installing OpenSSL3 (of course then they're not decryptable, but on a rollback that should still be possible). Finally, a few more documentation changes: * I also mentioned the issue in `nextcloud.xml` to make sure the issue is at least mentioned in the manual section about Nextcloud. Not too much detail here, but the relevant option `enableBrokenCiphersForSSE` is referenced. * I fixed a few minor wording issues to also give the full context (we're talking about Nextcloud; we're talking about the PHP extension **only**; please check if you really need this even though it's enabled by default). This is because I felt that sometimes it might be hard to understand what's going on when e.g. an eval-warning appears without telling where exactly it comes from.

Ma27 · 2022-11-11T13:46:09Z

@ajs124 fixed :)

etu

As for the changes to the php packages I think this looks fine

Ma27 · 2022-11-14T10:09:46Z

cc @NixOS/release-engineers @NixOS/nixos-release-managers this should really land in 22.11. Would be cool to get a few more reviewers here first, though.

cab404

Wow, that's some thorough testing.

ajs124 · 2022-11-21T12:48:24Z

pkgs/top-level/php-packages.nix

+        {
+          name = "openssl-legacy";
+          extName = "openssl";
+          buildInputs = [ openssl_1_1 ];


With #202126, this can probably be replaced with openssl_legacy. Doesn't make a big difference, but might be useful, depending on how long we need to keep this around.

AkechiShiro · 2023-02-24T22:02:09Z

A fix for the root issue is coming up in Nextcloud 26 : nextcloud/server#32003 (comment)

Tagging maintainers of Nextcloud so that they be notified about this coming change :
@schneefux @bachp @globin @fpletz

I guess this fix needs a tweak so that it can only be used on pkgs.nextcloud25 and lower, once pkgs.nextcloud defaults to Nextcloud 26.

Should I be trying my hand at making a PR to adapt the Nextcloud module to take into account that fix should be allowed to run only on NC25 and lower versions. And ignore the fix for NC26 by using the function versionOlder and cfg.package.version ?

I'd guess some testing would be needed to make sure nothing breaks down on NC26, there are only beta version of the server, so testing will have to wait and will be possible when NC26 stable or a release candidate is out.

tl;dr :
Nextcloud 26 will not depend on OpenSSL for RC4 server side encryption (AFAIK downgrading OpenSSL will not be necessary anymore for server side encryption to be working), see these pending PR for Nextcloud 26 for more information (both PRs were merged in Nextcloud 26 beta 5) : nextcloud/server#36173 (comment) nextcloud/server#35916

Ma27 · 2023-02-24T22:24:44Z

A fix for the root issue is coming up in Nextcloud 26

I wouldn't call it a fix for the root issue though, rather working around the root cause ;-)

Should I be trying my hand at making a PR to adapt the Nextcloud module to take into account that fix should be allowed to run only on NC25 and lower versions

We don't have nc26 yet, so I guess, no action needed here until the release is out. Thanks for the heads-up, I'll adapt the module then accordingly.

Of course, if you're faster when it's out, I won't stop you from filing a PR.

chayleaf · 2023-04-02T13:14:49Z

nc26 is released now (with this patch nextcloud/server#36173 merged)

RaitoBezarius · 2023-04-02T13:58:35Z

We already adapted NC26 IIRC. We still need our workaround for NC25 as long as it is a supported version.

chayleaf · 2023-04-02T19:38:07Z

I think it still uses openssl-legacy with NC26, at least I couldn't find anything for that in

nixpkgs/nixos/modules/services/web-apps/nextcloud.nix

Line 21 in 2fdee63

    
           ++ (if cfg.enableBrokenCiphersForSSE then [ cfg.phpPackage.extensions.openssl-legacy ] else [ cfg.phpPackage.extensions.openssl ])

it should probably be an assertion (assert NC < 26 or !enableBrokenCiphersForSSE)

nh2 · 2023-07-02T19:16:06Z

I want to upgrade from NixOS 22.11 to 23.05 with Nextcloud installed.

I get the output:

error: Package ‘openssl-1.1.1u’ in /nix/store/51p9r89zh93piqkmr18h4m7mkf10a1sg-nixos-23.05/nixos/pkgs/development/libraries/openssl/default.nix:210 is marked as insecure, refusing to evaluate.


       Known issues:
        - OpenSSL 1.1 is reaching its end of life on 2023/09/11 and cannot be supported through the NixOS 23.05 release cycle. https://www.openssl.org/blog/blog/2023/03/28/1.1.1-EOL/

Unfortunately there is no mention at all of Nextcloud, even though it seems to be the problem.

I only found it with an invocation from #210452 (comment)

cd /etc/nixos
NIXPKGS_ALLOW_INSECURE=1 nix-instantiate --strict "<nixpkgs/nixos>" -A system

which prints:

trace: warning: You're using PHP's openssl extension built against OpenSSL 1.1 for Nextcloud.
This is only necessary if you're using Nextcloud's server-side encryption.
Please keep in mind that it's using the broken RC4 cipher.

If you don't use that feature, you can switch to OpenSSL 3 and get
rid of this warning by declaring

  services.nextcloud.enableBrokenCiphersForSSE = false;

If you need to use server-side encryption you can ignore this warning.
Otherwise you'd have to disable server-side encryption first in order
to be able to safely disable this option and get rid of this warning.
See <https://docs.nextcloud.com/server/latest/admin_manual/configuration_files/encryption_configuration.html#disabling-encryption> on how to achieve this.

For more context, here is the implementing pull request: https://github.com/NixOS/nixpkgs/pull/198470

It would be good if nixos-rebuild boot could be convinced to output a better error message / produce this warning, vs leaving the user in the dark on which package depends on old OpenSSL.

Ma27 · 2023-07-02T20:18:24Z

This isn't a problem with Nextcloud's packaging, but with how warnings currently work in nixpkgs (the problem appears with every dependant of openssl 1.1.1 on 23.05 and later), this might be subject to change with https://github.com/NixOS/rfcs/pull/127/files though. In other words, this is IMHO the wrong place to report the issue because it's not a problem with this package and this is effectively necro-bumping an old PR (this wasn't a problem when this change was merged).

In other words, please file a new issue for that :)

RaitoBezarius requested review from jtojnar, aanderse, etu, globin, Ma27 and talyz as code owners October 29, 2022 14:22

github-actions bot added 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: module (update) This PR changes an existing module in `nixos/` labels Oct 29, 2022

ofborg bot added the 8.has: package (new) This PR adds a new package label Oct 29, 2022

ofborg bot requested a review from drupol October 29, 2022 14:32

ofborg bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1-10 labels Oct 29, 2022

mweinelt requested a review from ajs124 October 29, 2022 14:45

drupol reviewed Oct 29, 2022

View reviewed changes

ajs124 approved these changes Oct 29, 2022

View reviewed changes

Ma27 requested changes Oct 30, 2022

View reviewed changes

RaitoBezarius force-pushed the nc25-openssl branch from ae0d256 to fb174fc Compare November 6, 2022 17:29

RaitoBezarius requested a review from Ma27 November 6, 2022 17:30

drupol reviewed Nov 6, 2022

View reviewed changes

ofborg bot requested a review from drupol November 6, 2022 17:40

Ma27 reviewed Nov 6, 2022

View reviewed changes

nixos/modules/services/web-apps/nextcloud.nix Outdated Show resolved Hide resolved

RaitoBezarius force-pushed the nc25-openssl branch from fb174fc to 3d833c5 Compare November 6, 2022 17:47

Ma27 reviewed Nov 6, 2022

View reviewed changes

nixos/modules/services/web-apps/nextcloud.nix Outdated Show resolved Hide resolved

RaitoBezarius commented Nov 8, 2022

View reviewed changes

nixos/modules/services/web-apps/nextcloud.nix Show resolved Hide resolved

Ma27 marked this pull request as draft November 9, 2022 13:35

Ma27 self-assigned this Nov 9, 2022

RaitoBezarius and others added 3 commits November 10, 2022 12:17

nextcloud25: use openssl 1.1 as a PHP extension to fix RC4 encryption

7eefaeb

nextcloud25: enable by default broken ciphers for NixOS ≤ 22.11

394d4de

Ma27 force-pushed the nc25-openssl branch from accc2e0 to 576730c Compare November 10, 2022 12:54

Ma27 marked this pull request as ready for review November 10, 2022 12:54

ajs124 reviewed Nov 11, 2022

View reviewed changes

nixos/modules/services/web-apps/nextcloud.nix Outdated Show resolved Hide resolved

nixos/modules/services/web-apps/nextcloud.nix Outdated Show resolved Hide resolved

Ma27 force-pushed the nc25-openssl branch from 576730c to 35b146c Compare November 11, 2022 13:45

etu approved these changes Nov 12, 2022

View reviewed changes

Ma27 requested a review from ajs124 November 12, 2022 22:19

cab404 approved these changes Nov 14, 2022

View reviewed changes

Ma27 merged commit 2580440 into NixOS:master Nov 20, 2022

mweinelt mentioned this pull request Nov 20, 2022

Updating OpenSSL breaks Nextcloud server 23/24/25 side encryption due to RC4 deprecation #197726

Closed

ajs124 reviewed Nov 21, 2022

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

nextcloud25: use openssl 1.1 as a PHP extension to fix RC4 encryption #198470

nextcloud25: use openssl 1.1 as a PHP extension to fix RC4 encryption #198470

RaitoBezarius commented Oct 29, 2022 •

edited

Loading

drupol left a comment

ajs124 left a comment

RaitoBezarius commented Oct 29, 2022

Ma27 left a comment

RaitoBezarius commented Nov 6, 2022

RaitoBezarius commented Nov 6, 2022

drupol left a comment

mweinelt commented Nov 6, 2022

RaitoBezarius commented Nov 6, 2022

Ma27 commented Nov 9, 2022

Ma27 commented Nov 10, 2022

Ma27 commented Nov 11, 2022

etu left a comment

Ma27 commented Nov 14, 2022

cab404 left a comment

ajs124 Nov 21, 2022

AkechiShiro commented Feb 24, 2023 •

edited

Loading

Ma27 commented Feb 24, 2023

chayleaf commented Apr 2, 2023 •

edited

Loading

RaitoBezarius commented Apr 2, 2023

chayleaf commented Apr 2, 2023 •

edited

Loading

nh2 commented Jul 2, 2023

Ma27 commented Jul 2, 2023

nextcloud25: use openssl 1.1 as a PHP extension to fix RC4 encryption #198470

nextcloud25: use openssl 1.1 as a PHP extension to fix RC4 encryption #198470

Conversation

RaitoBezarius commented Oct 29, 2022 • edited Loading

Description of changes

Things done

drupol left a comment

Choose a reason for hiding this comment

ajs124 left a comment

Choose a reason for hiding this comment

RaitoBezarius commented Oct 29, 2022

Ma27 left a comment

Choose a reason for hiding this comment

RaitoBezarius commented Nov 6, 2022

RaitoBezarius commented Nov 6, 2022

drupol left a comment

Choose a reason for hiding this comment

mweinelt commented Nov 6, 2022

RaitoBezarius commented Nov 6, 2022

Ma27 commented Nov 9, 2022

Ma27 commented Nov 10, 2022

Ma27 commented Nov 11, 2022

etu left a comment

Choose a reason for hiding this comment

Ma27 commented Nov 14, 2022

cab404 left a comment

Choose a reason for hiding this comment

ajs124 Nov 21, 2022

Choose a reason for hiding this comment

AkechiShiro commented Feb 24, 2023 • edited Loading

Ma27 commented Feb 24, 2023

chayleaf commented Apr 2, 2023 • edited Loading

RaitoBezarius commented Apr 2, 2023

chayleaf commented Apr 2, 2023 • edited Loading

nh2 commented Jul 2, 2023

Ma27 commented Jul 2, 2023

RaitoBezarius commented Oct 29, 2022 •

edited

Loading

AkechiShiro commented Feb 24, 2023 •

edited

Loading

chayleaf commented Apr 2, 2023 •

edited

Loading

chayleaf commented Apr 2, 2023 •

edited

Loading