wordpress: replace the dbPassword option with dbPasswordFile by basvandijk · Pull Request #24146 · NixOS/nixpkgs

basvandijk · 2017-03-21T00:07:46Z

We shouldn't force users to store passwords in the world-readable Nix store so this commit replaces the dbPassword option with dbPasswordFile which should be set to a file that contains the wordpress DB password. Users can then provision this file however they want, either by storing it unsafely in the Nix store using pkgs.writeTextFile or safely using something like nixops key management.

It would be great if this can also be cherry-picked on release-17.03.

mention-bot · 2017-03-21T00:07:47Z

@basvandijk, thanks for your PR! By analyzing the history of the files in this pull request, we identified @qknight, @grahamc and @ehmry to be potential reviewers.

globin · 2017-03-21T00:10:37Z

I'd prefer to have both options.

basvandijk · 2017-03-21T00:23:26Z

@globin I prefer to make users conscious about the safety of their keys. An option like dbPassword kind of hides the fact that the key is stored in the world-readable Nix store. Of course documentation can help but not everybody reads that.

The dbPasswordFile option forces users to think where they are going to store the key.

Of course the old unsafe behaviour is easy to recreate using pkgs.writeTextFile as I do in the test.

basvandijk · 2017-03-21T02:15:14Z

@globin I changed my mind and followed your preference. dbPasswordFile now defaults to a file in the Nix store with the value of dbPassword. I added documentation to warn users about the security issues.

The nice thing is that this is also backwards compatible.

LnL7 · 2017-03-21T19:09:32Z

It's possible to add warnings when using certain options.

RonnyPfannschmidt · 2017-03-22T14:57:18Z

btw, as is, this one is a breaking change

basvandijk · 2017-03-22T14:59:28Z

@RonnyPfannschmidt what breaks because of this change?

RonnyPfannschmidt · 2017-03-22T15:08:26Z

@basvandijk any user that has this already configured afterwards has a broken configuration, because the old thing is gone, there is not the slightest hint of a transition help either

basvandijk · 2017-03-22T15:11:29Z

@RonnyPfannschmidt the old thing isn't gone. The change should be backwards compatible.

RonnyPfannschmidt · 2017-03-22T15:14:03Z

indeed, i am sorry, i missed the pkgs.writeTextFile bit

LnL7 · 2017-03-22T17:31:35Z

nixos/modules/services/web-servers/apache-httpd/wordpress.nix

I'm not sure, but I think more complex defaults like this are usually moved to the implementation with a mkDefault

Correct, otherwise the manual is rebuilt when dbPassword changes in your config.

Also, where's the config attrset for this module?

@globin note that wordpress is not mentioned in the manual.

@LnL7 wordpress.nix is not a traditional NixOS module and doesn't have a config attribute. It should be used as:

{ services.httpd.extraSubservices = [ { serviceType = "wordpress"; dbName = "..."; themes = []; plugins = []; } ]; }

So there's no need and no place to use mkDefault.

I see 😕. https://github.com/NixOS/nixpkgs/blob/master/nixos/modules/services/web-servers/apache-httpd/per-server-options.nix#L145

We shouldn't force users to store passwords in the world-readable Nix store.

basvandijk · 2017-03-22T22:38:10Z

Similarly to other web-apps I added an example to the dbPasswordFile option: "/run/keys/wordpress-dbpassword".

Is there anything that prevents this being mergeable?

qknight · 2017-03-24T16:23:04Z

love this concept, we should not stop at password(s) though. wordpress also contains other key materials as AUTH_KEY, AUTH_SALT, ... shown here:

 https://api.wordpress.org/secret-key/1.1/salt/

same is true for owncloud/nextcloud and so on.

basvandijk · 2017-03-24T17:47:44Z

@qknight thanks and I agree that AUTH_KEY and so forth should not be placed in the Nix store as well. However at the moment the wordpress module doesn't set these parameters so (apart from the dbPassword option) users aren't currently being forced to do something unsafe.

basvandijk · 2017-03-28T21:14:52Z

@qknight thanks for the merge! Can this also be cherry-picked on release-17.03? I would like to use this in production.

…4146) We shouldn't force users to store passwords in the world-readable Nix store.

basvandijk · 2017-04-29T13:05:48Z

Hi @qknight, it would be great if you can cherry pick this on release-17.03.

qknight · 2017-05-05T09:04:50Z

@basvandijk added it to release-17.03 just now. please check if it works for you.

basvandijk force-pushed the wordpress-dbPasswordFile branch from b5f1ffc to bc52e9b Compare March 21, 2017 02:12

basvandijk force-pushed the wordpress-dbPasswordFile branch from bc52e9b to cc191ef Compare March 21, 2017 02:16

fpletz added the 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS label Mar 22, 2017

fpletz approved these changes Mar 22, 2017

View reviewed changes

LnL7 approved these changes Mar 22, 2017

View reviewed changes

basvandijk force-pushed the wordpress-dbPasswordFile branch from cc191ef to 29f5d7f Compare March 22, 2017 22:30

wordpress: replace the dbPassword option with dbPasswordFile

d6c2c3c

We shouldn't force users to store passwords in the world-readable Nix store.

basvandijk force-pushed the wordpress-dbPasswordFile branch from 29f5d7f to d6c2c3c Compare March 22, 2017 22:33

basvandijk mentioned this pull request Mar 24, 2017

Provide options for storing secrets outside the Nix store #24288

Open

75 tasks

qknight merged commit 6f2eca1 into NixOS:master Mar 28, 2017

krofek pushed a commit to krofek/nixpkgs that referenced this pull request Mar 30, 2017

wordpress: replace the dbPassword option with dbPasswordFile (NixOS#2…

33825a2

…4146) We shouldn't force users to store passwords in the world-readable Nix store.

Uh oh!

Conversation

basvandijk commented Mar 21, 2017

Uh oh!

mention-bot commented Mar 21, 2017

Uh oh!

globin commented Mar 21, 2017

Uh oh!

basvandijk commented Mar 21, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

basvandijk commented Mar 21, 2017

Uh oh!

LnL7 commented Mar 21, 2017

Uh oh!

RonnyPfannschmidt commented Mar 22, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

basvandijk commented Mar 22, 2017

Uh oh!

RonnyPfannschmidt commented Mar 22, 2017

Uh oh!

basvandijk commented Mar 22, 2017

Uh oh!

RonnyPfannschmidt commented Mar 22, 2017

Uh oh!

LnL7 Mar 22, 2017

Choose a reason for hiding this comment

Uh oh!

globin Mar 22, 2017

Choose a reason for hiding this comment

Uh oh!

LnL7 Mar 22, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

basvandijk Mar 22, 2017

Choose a reason for hiding this comment

Uh oh!

LnL7 Mar 22, 2017

Choose a reason for hiding this comment

Uh oh!

basvandijk commented Mar 22, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

qknight commented Mar 24, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

basvandijk commented Mar 24, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

basvandijk commented Mar 28, 2017

Uh oh!

basvandijk commented Apr 29, 2017

Uh oh!

qknight commented May 5, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

basvandijk commented Mar 21, 2017 •

edited

Loading

RonnyPfannschmidt commented Mar 22, 2017 •

edited

Loading

LnL7 Mar 22, 2017 •

edited

Loading

basvandijk commented Mar 22, 2017 •

edited

Loading

qknight commented Mar 24, 2017 •

edited

Loading

basvandijk commented Mar 24, 2017 •

edited

Loading