Skip to content
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.

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

nixos/roundcube: security improvements #77532

Merged
merged 3 commits into from Jan 22, 2020
Merged

nixos/roundcube: security improvements #77532

merged 3 commits into from Jan 22, 2020

Conversation

@symphorien
Copy link
Contributor

@symphorien symphorien commented Jan 11, 2020

Motivation for this change
  • database password is world readable in the store
  • php update script is ran as root
Things done
  • If the database is local, use postgres peer authentication. Otherwise, use a password file.
  • Leave database initialisation to postgresql.ensure*.
  • Leave /var/lib/roundcube creation to systemd.
  • Run php upgrade script as unpriviledged user.
  • initialize the des_key (the installer does it, but nixos bypasses the installer...)
  • En passant, fix warning about mime types database

This should be backward compatible:

  • des_key is only used for cookies apparently. So changing it only logs users out.
  • changing the default user: /var/lib/roundcube is chowned by systemd
  • database owner does not change, and can still log in with peer auth even if it previously used a password

The nixos test still passes, with no modification.

  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.

cc @Ma27 @globin @Vskilet

Copy link
Contributor

@aanderse aanderse left a comment

Fantastic work! Thanks for doing this.

I wonder if @Ma27 has an existing roundcube instance that could be used for testing... 🤔

@aanderse
Copy link
Contributor

@aanderse aanderse commented Jan 12, 2020

Thanks @symphorien! Let's see what @Ma27 has to say and go from there. Looking over the code you get the 👍 from me, though. Great work!

@Vskilet
Copy link
Contributor

@Vskilet Vskilet commented Jan 13, 2020

Hoo thanks @symphorien ! I'll test now !

Copy link
Contributor

@Vskilet Vskilet left a comment

I tested my old config, the new one with the passwordfile and the peer authentication : it all works !
Great work 👍

@Ma27
Copy link
Member

@Ma27 Ma27 commented Jan 13, 2020

I wonder if @Ma27 has an existing roundcube instance that could be used for testing...

I do :)
If I have sufficient time, I can test this tomorrow :)

Copy link
Member

@Ma27 Ma27 left a comment

Thanks a lot for improving this module! Unfortunately I fonud some issues that should be addressed first :)

des_key is only used for cookies apparently. So changing it only logs users out.

Not necessarily: I observed on my personal instance that I didn't get logged out, but I couldn't connect to the mailservers (and had to hit "Logout" manually).

nixos/modules/services/mail/roundcube.nix Outdated Show resolved Hide resolved
nixos/modules/services/mail/roundcube.nix Outdated Show resolved Hide resolved
nixos/modules/services/mail/roundcube.nix Outdated Show resolved Hide resolved
If the database is local, use postgres peer authentication.
Otherwise, use a password file.

Leave database initialisation to postgresql.ensure*.
Leave /var/lib/roundcube creation to systemd.
Run php upgrade script as unpriviledged user.
@symphorien symphorien force-pushed the symphorien:roundcube branch from 27402e0 to b9c8225 Jan 18, 2020
@symphorien
Copy link
Contributor Author

@symphorien symphorien commented Jan 18, 2020

Thanks for your review. I fixed the syntax errors you found.

I don't know what to do about the des_key problem. Any idea?

symphorien added 2 commits Jan 5, 2020
The php installer creates a random one, but we bypass it, so we have
to create one ourselves.

This should be backward compatible as encryption is used for session
cookies only: users at the time of the upgrade will be logged out but
nothing more.

https://github.com/roundcube/roundcubemail/blob/259b7fa0650fea9320b38cb17c4e80497acae7a3/config/config.inc.php.sample#L73
fixes this warning:
WARNING: Mimetype to file extension mapping doesn't work properly!
@symphorien symphorien force-pushed the symphorien:roundcube branch from b9c8225 to b5d692e Jan 18, 2020
@symphorien
Copy link
Contributor Author

@symphorien symphorien commented Jan 18, 2020

On freenode, I was advised to empty the table called session. This is implemented in the current version of the PR. Would you mind testing another time, @Ma27 ?
Users are logged out when /var/lib/roundcube/des_key is created, so you should remove this file beforehand.

@Ma27
Ma27 approved these changes Jan 22, 2020
Copy link
Member

@Ma27 Ma27 left a comment

Deployed to my roundcube instance and confirmed that truncating the session table if no des-key exists works fine 👍

The changes seem fine to me and since there are two more 👍, this should be good to go, so thanks a lot!

@Ma27 Ma27 merged commit 2d9e51a into NixOS:master Jan 22, 2020
13 checks passed
13 checks passed
Evaluation Performance Report Evaluator Performance Report
Details
grahamcofborg-eval ^.^!
Details
grahamcofborg-eval-check-maintainers matching changed paths to changed attrs...
Details
grahamcofborg-eval-check-meta config.nix: checkMeta = true
Details
grahamcofborg-eval-darwin nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A darwin-tested
Details
grahamcofborg-eval-nixos nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./nixos/release-combined.nix -A tested
Details
grahamcofborg-eval-nixos-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./nixos/release.nix -A manual
Details
grahamcofborg-eval-nixos-options nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./nixos/release.nix -A options
Details
grahamcofborg-eval-nixpkgs-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A manual
Details
grahamcofborg-eval-nixpkgs-tarball nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A tarball
Details
grahamcofborg-eval-nixpkgs-unstable-jobset nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A unstable
Details
grahamcofborg-eval-package-list nix-env -qa --json --file .
Details
grahamcofborg-eval-package-list-no-aliases nix-env -qa --json --file . --arg config { allowAliases = false; }
Details
@symphorien symphorien deleted the symphorien:roundcube branch Feb 5, 2020
symphorien added a commit to symphorien/nixpkgs that referenced this pull request Feb 5, 2020
Ma27 added a commit that referenced this pull request Feb 6, 2020
nixos/roundcube: add release notes for #77532
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.