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/mysql: add authentication option #68353

Closed
wants to merge 1 commit into from

Conversation

Scriptkiddi
Copy link
Contributor

Add an authentication option to be able to create mysql users that do
not authenticate using the unix_socket plugin, since this requires the
username to exist on the host system.
(https://mariadb.com/kb/en/library/authentication-plugin-unix-socket/)

Motivation for this change
Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS)
  • 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 nix-review --run "nix-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.
Notify maintainers

cc @

Add an authentication option to be able to create mysql users that do
not authenticate using the unix_socket plugin, since this requires the
username to exist on the host system.
(https://mariadb.com/kb/en/library/authentication-plugin-unix-socket/)
@aanderse
Copy link
Member

I was under the impression that mysql hashes are not safe.

@Scriptkiddi
Copy link
Contributor Author

Hm good point how should we deal with that? I could move the hashes to a file or add a warning. I want this option because I want to be able to add users that do not require a unix account on the host

@lheckemann lheckemann added this to the 20.03 milestone Sep 10, 2019
@aanderse
Copy link
Member

@Scriptkiddi I'm planning some work on the mysql and postgresql modules for 20.03, partially motivated by my own needs and partially motivated by those of @talyz in #66274. I was jotting down a few ideas this morning after reading through your PR so how about we let this PR sit on the back burner a few days and I'll get back to you?

@aanderse aanderse self-assigned this Sep 10, 2019
@aanderse
Copy link
Member

@Scriptkiddi @florianjacob @talyz @ryantm very rough first pass at an addition to the mysql interface which could be mirrored with postgresql: https://github.com/NixOS/nixpkgs/compare/master...aanderse:sql?expand=1

Example usage:

services.mysql.ensureDatabases = [
  "db_a"
  "db_b"
  { name = "db_c"; }
  { name = "db_d"; owner = "timmy@localhost"; }
  { name = "db_e"; owner = "'aaron'@'%'"; }
];
services.mysql.ensureUsers = [
  # creates timmy@localhost who requires password to authenticate
  { name = "timmy";
    passwordFile = pkgs.writeText "password" "thisismypassword";
    ensurePermissions = { ... usual stuff here ... };
  }

  # creates a user who has access to all databases and WITH GRANT OPTION
  { name = "ryantm";
    isSuperUser = true;
  }

  # remote user
  { name = "aaron";
    host = "%";
    hashedPasswordFile = pkgs.writeText "hashed-password" "*ABCDEF HASHED PASSWORD";
    ensurePermissions = { ... usual stuff here ... };
  }

  # demonstrating backwards compatibility
  { name = "moodle";
    ensurePermissions = { ... usual stuff here ... };
  }
];

Note this is entirely backwards compatible with existing mysql interface.

@talyz
Copy link
Contributor

talyz commented Sep 12, 2019

@aanderse Using types.either to ensure backwards compatibility was my first approach for the postgresql work I did. Unfortunately, it doesn't work very well with the documentation generation - the submodule options will be missing in the manual.

@aanderse
Copy link
Member

@aanderse Using types.either to ensure backwards compatibility was my first approach for the postgresql work I did. Unfortunately, it doesn't work very well with the documentation generation - the submodule options will be missing in the manual.

@talyz sounds like a bug. I wonder if @infinisil has any thoughts on that, as I've seen him use it a fair amount.

@infinisil
Copy link
Member

types.coercedTo should work.

@aanderse
Copy link
Member

aanderse commented Oct 15, 2019

types.coercedTo should work.

Thanks @infinisil, that looks better.

I'm still hoping to hear more opinions about whether this sort of change is something we actually want in nixos. Clearly @Scriptkiddi wants this change, as they opened this PR. I think I'm in favor of this change. Is anyone against these types of changes to nixos?

This might be a good time to ping @danbst, @aszlig, and @thoughtpolice as the idea is that these changes would make their way into the postgresql module as well. What do you think about this type of functionality in nixos? See the implementation and then an example for usage, but the basic idea is deeper database and database user provisioning than already exists in nixos.

Copy link
Member

@grahamc grahamc left a comment

Choose a reason for hiding this comment

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

As a rule I'm against options which add "best effort", imperative options to Nix. Deleting a user from these settings doesn't delete the user from the system, and I think that is something we want to avoid in NixOS.

A good hint that the option either doesn't belong or doesn't function properly is having the word ensure in the name.

@aanderse
Copy link
Member

@grahamc I understand your points and agree. While I think this PR provides value, maybe the value comes at too high a cost, and doesn't fit quite right into nixos?

I know @talyz was also interested in this functionality, so I'll ask @talyz and @Scriptkiddi if they have anything to add at this point?

@talyz
Copy link
Contributor

talyz commented Oct 20, 2019

@grahamc @aanderse I agree that it's definitely not ideal, but find it a necessary evil. The alternatives I can think of seem worse to me:

  • all modules are responsible for provisioning their own database
  • manual intervention needed
  • dropping the modules requiring a database

Am I missing a better alternative?

Also, note that this is not actually about ensureDatabases or ensureUsers, since they already exist - it's about extending their functionality. The current alternative is to implement the necessary features locally in the module that needs it, like we did for GitLab.

@aanderse aanderse mentioned this pull request Oct 21, 2019
20 tasks
@ghost
Copy link

ghost commented Oct 21, 2019

I am joining the discussion, because I needed to extend the postgresql module to run a script after the creation of a database specified in ensureDatabases. I implemented this option in https://github.com/petabyteboy/nixpkgs/commit/d86208eb4ee23ca1f345c96b04bb0f7ce3a5d0bf and was made aware of this discussion by @aanderse

  • manual intervention needed

Fine for me, this was my initial approach, but others did request that the database is created automatically and the schema is loaded into the database after initial creation, which required this change

  • dropping the modules requiring a database

...

  • all modules are responsible for provisioning their own database

I definitely prefer less duplication of code in the modules

=> Maybe we can make it an internal option so that it is not directly exposed to users?

@alyssais
Copy link
Member

I think manual intervention is probably okay, as long as its well documented. It does feel like it would detract somewhat from NixOS’s value proposition of “write all your configuration in configuration.nix” if a bunch of commands are needed to set up initial state as well, though.

@alyssais
Copy link
Member

alyssais commented Oct 22, 2019 via email

@Scriptkiddi
Copy link
Contributor Author

As a rule I'm against options which add "best effort", imperative options to Nix. Deleting a user from these settings doesn't delete the user from the system, and I think that is something we want to avoid in NixOS.

A good hint that the option either doesn't belong or doesn't function properly is having the word ensure in the name.

Since I'm new to NixOs, couldnt we just modify the ensure script to delete users?

@flokli
Copy link
Contributor

flokli commented Oct 23, 2019

@Scriptkiddi we can't distinguish users that were configured via NixOS previously from users created by an admin, and don't want to delete these by accident, I assume.

@Scriptkiddi
Copy link
Contributor Author

You could add a not delete list or something like it to, protect users that are added another way

@talyz
Copy link
Contributor

talyz commented Oct 23, 2019

To phrase this another way, what's the difference between a package
creating its initial state on startup itself, and us doing it? What
makes one okay and not the other?

Exactly. My solution to this would be to reimplement the missing functionality in my configuration to not have to do it manually (since I'm deploying systems with NixOps) and I'm sure others would do the same. We'd end up with the same thing, but worse.

@aanderse
Copy link
Member

I found out this isn't the first PR to discuss in depth user provisioning for the mysql option. #6963

I'm still hoping @danbst and @aszlig will chime in with their opinions and reasoning on those opinions. @davidak, @copumpkin, and @edolstra may also have opinions on the topic of in depth user creation for the mysql module that they would be interested in sharing too.

@talyz
Copy link
Contributor

talyz commented Oct 31, 2019

I have a WIP for making ensureDatabases and ensureUsers declarative for the PostgreSQL module: #72365. Could you, and anyone else interested in the topic, have a look and see if this is something you'd like to see, @grahamc?

@florianjacob
Copy link
Contributor

florianjacob commented Nov 18, 2019

Regarding ensure* options: As the author of all ensure* options currently in master except the postgresql ones, I'm obviously thinking it's a good idea that helps users. I don't see why every module author and user should code the same functions again and again to create their databases, users, printers and whatever. Yes, those all are inherently stateful and probably never can be done fully declarative due to the impossibility of avoiding manual, stateful user intervention in some cases. But they just work, they provide you with automated printer setup in your fresh nixos install, they don't delete manually configured printers and generally require manual intervention on destructive operations, i.e. they will intentionally never delete your database, under any circumstance. That's why they just ensure, and do exactly that, and are helpful.

As this was one of my first big NixOS contributions, I chose the defensive prefix ensure for those kind of options, though many other options of this kind exist in NixOS, e.g. many service startup scripts that essentially only ensure the data directories are in a somewhat correct state and permissions, but obviously don't delete superfluous files or access permissions, or config.users.users would actually be config.users.ensureUsers if not set as mutable, which is the current default. Today I'm not sure anymore whether the ensure prefixes really make sense and whether the prefixes should be everywhere on such options as a warning, or nowhere.

If the community decides though that such options don't belong in NixOS and all users and module authors should go back and write their own bash scripts for that, I will accept that and stop proposung such options.

Regarding this specific PR: Seems like a good way to handle passwords for remote users (can't see why one should use that for anything that runs on the same host though) if a secure password hashing is used and / or the hash is stored in a non-accessible location. The default unsalted SHA1 hashing really isn't much better than plaintext passwords.
For more secure password hashing, state of the art seems to be to use authentication plugins for both MariaDB and MySQL, bud sadly they don't have a common secure denominator.

MariaDB offers https://mariadb.com/kb/en/authentication-plugin-ed25519/ and MySQL offers authentication-plugin-sha256 (why are both not using or at least offering actual password hashing functions? No idea…)

@@ -153,6 +153,20 @@ in
Name of the user to ensure.
'';
};
authentication_option = mkOption {
type = types.str;
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd suggest a more specific type:

with types; enum ["socket" "password"]

@@ -406,7 +420,7 @@ in

${concatMapStrings (user:
''
( echo "CREATE USER IF NOT EXISTS '${user.name}'@'localhost' IDENTIFIED WITH ${if isMariaDB then "unix_socket" else "auth_socket"};"
( echo "CREATE USER IF NOT EXISTS '${user.name}'@'localhost' ${optionalString (user.authentication_option == "password") ''IDENTIFIED BY PASSWORD '${user.password_hash}' ''} ${optionalString (user.authentication_option == "socket") ''IDENTIFIED WITH ${if isMariaDB then "unix_socket" else "auth_socket"} ''};"
Copy link
Contributor

Choose a reason for hiding this comment

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

A few issues:

  • line too long, should fit 100 columns
  • what happens when password is changed? As I understand it, user already exists so his password will silently remain the same. This is bad. Please do run ALTER USER to change password hash when it was changed in NixOS config
  • same for authentication_option

@Scriptkiddi
Copy link
Contributor Author

How are the chances of getting this merged? I'm willing to put in the work, but I don't want it to sit there open like the postgresql pr

@aanderse
Copy link
Member

aanderse commented Jan 3, 2020

@Scriptkiddi sorry for the delay on this. We're scheduling a call to discuss the best way forward. More details to come.

@Scriptkiddi
Copy link
Contributor Author

@aanderse Hey :), any news?

@aanderse
Copy link
Member

@Scriptkiddi yes! @grahamc, @flokli, and myself had a chat about this last week. We all agreed that declarative, reproducible database provisioning in the NixOS module system is a good thing.

Currently we have declarative provisioning but we decided we want to take this a step further and make it reproducible, so that the mysql and postgresql modules can become true to the spirit of NixOS.

To that end we have decided a program/script needs to be written to handle user and database provisioning, in the same way that a program/script exists to manage system user accounts in NixOS. The program/script would take a json file (describing what database users and databases are desired) as input and then compare that against the existing database users and databases that mysql (or postgresql) already have and create the users required to achieve this result.

A couple notes...

  • we want to add services.mysql.mutableUsers and services.postgresql.mutableUsers options which provides similar to functionality as users.mutableUsers
  • while we want to delete database users which aren't declared in configuration.nix when mutableUsers is set to false, we never want to delete databases when they are not in configuration.nix, similar to how we never delete users home directories in the same scenario
  • the program to handle user and database provisioning would ideally be written in rust
  • we are looking for people to hack on this with us, especially the (ideally rust) program/script to create and delete database users, and create databases as described by configuration.nix
  • it would be fantastic if we could have this ready for the 20.03 release... which is a tight timeline, but hey let's try our best right?

I'm really hoping a few people from this thread will be interested in hacking on this. @Scriptkiddi, @talyz, @florianjacob ... maybe @petabyteboy - any interest? rust skills in demand 😄

PS. @talyz since this is a fairly breaking change it is a great time to design module options from ground up! So all the goodness (postgresql extensions) that you were hoping for will happen with this change. I look forward to your contribution on this. Thanks for #75688 as a bash implementation.

@disassembler disassembler modified the milestones: 20.03, 20.09 Feb 5, 2020
@stale
Copy link

stale bot commented Aug 3, 2020

Hello, I'm a bot and I thank you in the name of the community for your contributions.

Nixpkgs is a busy repository, and unfortunately sometimes PRs get left behind for too long. Nevertheless, we'd like to help committers reach the PRs that are still important. This PR has had no activity for 180 days, and so I marked it as stale, but you can rest assured it will never be closed by a non-human.

If this is still important to you and you'd like to remove the stale label, we ask that you leave a comment. Your comment can be as simple as "still important to me". But there's a bit more you can do:

If you received an approval by an unprivileged maintainer and you are just waiting for a merge, you can @ mention someone with merge permissions and ask them to help. You might be able to find someone relevant by using Git blame on the relevant files, or via GitHub's web interface. You can see if someone's a member of the nixpkgs-committers team, by hovering with the mouse over their username on the web interface, or by searching them directly on the list.

If your PR wasn't reviewed at all, it might help to find someone who's perhaps a user of the package or module you are changing, or alternatively, ask once more for a review by the maintainer of the package/module this is about. If you don't know any, you can use Git blame on the relevant files, or GitHub's web interface to find someone who touched the relevant files in the past.

If your PR has had reviews and nevertheless got stale, make sure you've responded to all of the reviewer's requests / questions. Usually when PR authors show responsibility and dedication, reviewers (privileged or not) show dedication as well. If you've pushed a change, it's possible the reviewer wasn't notified about your push via email, so you can always officially request them for a review, or just @ mention them and say you've addressed their comments.

Lastly, you can always ask for help at our Discourse Forum, or more specifically, at this thread or at #nixos' IRC channel.

@stale stale bot added the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Aug 3, 2020
@aanderse
Copy link
Member

aanderse commented Aug 3, 2020

Closing in favor of #94048.

@aanderse aanderse closed this Aug 3, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet