search

nextcloud / end_to_end_encryption

:closed_lock_with_key: Server API to support End-to-End Encryption
https://apps.nextcloud.com/apps/end_to_end_encryption
GNU Affero General Public License v3.0
276 stars 34 forks source link

Version 21 Compatibility #224

Closed sunjam closed 3 years ago

sunjam commented 3 years ago

Please confirm e2e compatibility for version 21. Thanks!

eibex commented 3 years ago

Any ETA on this? Especially considering the confusion with the latest update https://github.com/nextcloud/server/issues/26469

bcutter commented 3 years ago

Even the app (v1.6.2) can be enabled/activated, E2EE won´t work. E. g. desktop client throws

13:44:35||Encrypted/Test123.txt|8|1|1618062225||0||2|Server hat "409 Conflict" auf "PUT https://server.name/dav/files/Username/Encrypted/Test123.txt" geantwortet (Sabre\DAV\Exception\Conflict)|409|0|0|xxxxxxxx-c6cd-4484-xxxx-1591e96axxxx|

when trying to sync a file. So there definitely needs some adjustments to be done within the E2EE app to work properly again with NC v21.

bcutter commented 3 years ago

Managed to get it working. Enable app, restart desktop clients. I also changed maximum version in app setting temporarily, not sure if this is necessary.

eibex commented 3 years ago

@bcutter Do files get synced properly and do new files get encrypted properly too?

bcutter commented 3 years ago

Yes for both. Latest v3.2.0 desktop client and latest mobile apps in use.

robolange commented 3 years ago

@bcutter Do you have a full list of changes you made to the source code? Was it only the maximum version number? Did you make changes against the latest master or against a v1.6.2 snapshot? It'd be nice to get a PR with minimal changes.

szaimen commented 3 years ago

Maybe @tobiasKaminsky can answer to this issue since he is the co-maintainer of this app, afaik.

eibex commented 3 years ago

@robolange I simply clicked on enable untested app without modifying anything and everything is working just fine. Make sure you don't have PHP8 though.

robolange commented 3 years ago

@eibex Interesting... I'm running the nextcloud docker compose, freshly pulled today, nextcloud:fpm-alpine which is based on php:7.4-fpm-alpine3.13 (at least according to https://github.com/nextcloud/docker/blob/master/21.0/fpm-alpine/Dockerfile) and I still get the "An error occured during the request. Unable to proceed." message. Annoyingly, this is the only thing in the docker compose logs related to this error:

app_1  | 172.18.0.4 -  17/Apr/2021:16:20:37 +0000 "POST /index.php" 500
web_1  | 172.18.0.1 - - [17/Apr/2021:16:20:37 +0000] "POST /settings/apps/enable HTTP/2.0" 500 4006 "https://my.domain/settings/apps" "Mozilla/5.0 (X11; Linux x86_64; rv:87.0) Gecko/20100101 Firefox/87.0" "-"

No stack trace or any other useful message :-(

szaimen commented 3 years ago

and I still get the "An error occured during the request. Unable to proceed."

And you are sure that this is related to the E2EE app?

robolange commented 3 years ago

When I click "Enable" for the "End-to-End Encryption" app, that error message appears and the app is not enabled. It would seem reasonable to assume so...

I'm don't know much about PHP. Is there something I can do to enable debugging info or otherwise get more info?

szaimen commented 3 years ago

Actually this is still most likely because of the incompatible release. On NC22 it will be possible to force the installation of any app (also incompatible ones) at least via OCC. But this is currently not yet possible. See https://github.com/nextcloud/server/pull/26252

robolange commented 3 years ago

I don't think that this is merely an issue of needing to bump up the max-version variable. I'm going to publish, in excruciating detail, my experimental setup in the hopes that others can replicate it exactly.

Okay, new experimental setup: I removed all docker containers, removed all container images, removed all docker volumes. Totally clean container infrastructure.

I forked and modified end_to_end_encryption, changing only the version number and nextcloud max-version number. My fork can be found here: https://github.com/robolange/end_to_end_encryption/tree/nc21-1.6.2.1-test

I built the tarball for that app locally from that fork using make.

I used the docker-compose from here, making the necessary changes for domain name, etc: https://github.com/nextcloud/docker/tree/master/.examples/docker-compose/with-nginx-proxy/postgres/fpm

This pulls in nextcloud:fpm-alpine, which is currently an alias for nextcloud:21.0.1-fpm-alpine, which is built on top of php:7.4-fpm-alpine3.13. Because I've cleaned my volumes, bringing this up results in a totally new environment. I created an admin account, allowed it to install the recommended apps, enabled the default encryption module, and enabled server side encryption.

Then, from my host system, I went into the docker volume as root and untarred the customized end_to_end_encryption module into the custom_apps directory. (I then recursively chowned it to the same uid and gid as the rest of the nextcloud files.)

To be extra safe, I shut down the nextcloud docker-compose services and brought them back up. Refreshing in my admin account in the browser, I saw that "End-to-End Encryption" with my custom verson number (1.6.2.1) had been added to "Your Apps", with the options Remove and Enable given. I clicked Enable...

And I got the error message "An error occured during the request. Unable to proceed." Also, in the docker-compose logs, I saw:

app_1  | 172.18.0.4 -  17/Apr/2021:17:54:49 +0000 "POST /index.php" 500
web_1  | 172.18.0.1 - - [17/Apr/2021:17:54:50 +0000] "POST /settings/apps/enable HTTP/2.0" 500 4006 "https://my.domain/settings/apps" "Mozilla/5.0 (X11; Linux x86_64; rv:87.0) Gecko/20100101 Firefox/87.0" "-"

So now I'm going to try to figure out how to make php give me more detailed logging, so I can see exactly why it is giving a 500 error. If anyone has any advice on that, I'd appreciate it. But from this, it doesn't seem that this is merely an issue with version numbers, nor does it appear to be (only) an issue with PHP8, as I am using PHP7.4.

szaimen commented 3 years ago

Is anything in the Nextcloud logs when you try to enable the app and it fails? I suppose so...

enabled the default encryption module, and enabled server side encryption.

I am unsure if E2EE actually works with server side encryption...

robolange commented 3 years ago

I would find it odd if the two were mutually exclusive. Even when you have end-to-end encryption on some folders, you'll probably not want it on all folders, so server-side would be useful sometimes.

Anyway, I'll try starting over clean again, but not enabling ~end-to-end~ server-side encryption, and see if that makes a difference.

szaimen commented 3 years ago

but not enabling end-to-end encryption

You mean server side encryption?

robolange commented 3 years ago

He, yeah, I edited my response to clarify.

szaimen commented 3 years ago

BTW maybe you find the following article about Nextclouds different encryption features interesting, too: https://github.com/nextcloud/desktop/pull/3136/files?short_path=d1779f0#diff-d1779f0654cfd5a551be7075492c99be67747a883b70df71a3cf90789f634500

robolange commented 3 years ago

Disclaimer: I don't really know PHP, so I don't understand why the original code worked for some people and not others.

While reading the nextcloud.log (entries to which are not send to stdout for docker-compose, but which are written into the file $DATA/nextcloud.log) I found an issue with a migration in end_to_end_encryption. In lib/Migration/Version1005Date20200312102456.php if you change the reference to Doctrine\DBAL\Types\Type to Doctrine\DBAL\Types\Types and then change the subsequent references to Type:: to Types::, then the module is able to be installed and appears to be working. (I believe this is safe and okay because looking at https://github.com/nextcloud/server/blob/master/core/Migrations/Version21000Date20210309185126.php for example that is what is used.)

My changes are at: https://github.com/robolange/end_to_end_encryption/tree/nc21-1.6.2.1-test

I based these changes off the v1.6.2 tag with the intent that they should be minimal changes to the current production version to get this working. This will not apply cleanly to master. I noticed that master already claims to only support nextcloud v22, so I think we should produce a bugfix for the v.1.6.2 branch to support nextcloud v21.

I am ready to make a PR, but please let me know what branch I should PR into.

szaimen commented 3 years ago

You are completely correct about the DBAL Types interface. It was reported as breaking change in NC21 in many repositories by @ChristophWurst e.g. here: https://github.com/nextcloud/nextcloudpi/issues/1239. I wonder why he didn't create an issue in this repository...

Concerning the correct branch, I think it is fine if you base your changes on the latest master branch and create a PR with your changes.

robolange commented 3 years ago

Hmm, it has actually been corrected in master. However, master is set to min-version 22. I've changed that locally and test it, and it seems to work, so I'm making a small PR against master to decrement the min-version to 21.

ChristophWurst commented 3 years ago

I wonder why he didn't create an issue in this repository...

I occasionally make mistakes. Apparently my github search query didn't show usages in this repo.

szaimen commented 3 years ago

I occasionally make mistakes. Apparently my github search query didn't show usages in this repo.

All good! I didn't wanna make any accusation!

robolange commented 3 years ago

This issue (the DBAL Types change) was fixed in master in January. Unfortunately, there has been no formal release since then, which I assume is what triggers a new version to be pushed into the nextcloud app store. Then in February, the min-version in master got bumped up to v22, so even if a release had happened, I guess it wouldn't show up in the app store for v21 servers. i'm hoping that my patch (https://github.com/nextcloud/end_to_end_encryption/pull/230) can get accepted, and a new release cut, which should restore compatibility with v21 servers.

nickvergessen commented 3 years ago

DBAL stuff I fixed with #218 before it was branched off, so that is fixed in stable21 as well.

PHP8 testing is added in #232 and seems to pass it was backported to stable21 in #234

Is someone willing to give a package a quick test before it's being put into the appstore? end_to_end_encryption-1.7.0-test.tar.gz

brtptrs commented 3 years ago

Installation on NC21.0.1 works! php7.4 Have not tested the functionality yet

robolange commented 3 years ago

I too can confirm that the stable21 branch works on NC21.0.1 (also PHP 7.4), both with and without server-side encryption enabled. The only minor suggestion I would make is to update the version tag in the Makefile so that the tarball it generates has the correct version number, but that doesn't affect the functionality of the package.

nickvergessen commented 3 years ago

make appstore version=1.7.0 fixes the "number" issue.

nickvergessen commented 3 years ago

Release is in the appstore now https://apps.nextcloud.com/apps/end_to_end_encryption

szaimen commented 3 years ago

@nickvergessen it seems like I wasn't fast enough 😅 Is my testing still needed then?