bitwarden_rs: init at 1.8.0 #55413
bitwarden_rs: init at 1.8.0 #55413
Conversation
nixos/modules/misc/ids.nix
Outdated
| @@ -638,6 +639,7 @@ | |||
| rss2email = 312; | |||
| cockroachdb = 313; | |||
| zoneminder = 314; | |||
| bitwarden_rs = 315; | |||
There was a problem hiding this comment.
would a systemd dynamic user do the trick ?
That is, do other services need access to files owned by this user ?
There was a problem hiding this comment.
I did not know about systemd dynamic users. I just read about them and I guess that I could change the configuration to use it, since indeed only bitwarden_rs would need access, but I think it best to keep the user and group options available in case of dataFolder is changed from its default location.
There was a problem hiding this comment.
In any case, static uid/gid's aren't needed, not assigning them in the module makes NixOS choose one on its own, which will be constant across the lifetime of a NixOS installation.
There was a problem hiding this comment.
Ah, I did not know this, so it would be safe to remove uid/gid in extraUsers/extraGroups and remove the entries in ids.nix?
There was a problem hiding this comment.
I have updated the PR to no longer use static uid/gid's.
| @@ -0,0 +1,477 @@ | |||
| { config, lib, pkgs, ... }: | |||
|
|
|||
| with import <nixpkgs/lib>; | |||
There was a problem hiding this comment.
Whoops, I seem to not to have undone that test case. It was to make sure that it would work with the standard lib rather than my modified lib.
|
Would it be acceptable to make the user non configurable, a dynamic systemd user, and paths would likewise be non-configurable and all under /var/lib/foo or /var/cache/foo as managed by systemd ? |
|
My counter suggestion would be to make the user a dynamic systemd user by default, when the default |
|
@symphorien I forced pushed the changes I proposed in response to your feedback, do you agree with the changes made? |
| SyslogIdentifier = "backup-bitwarden_rs"; | ||
| User = cfg.backupUser; | ||
| Group = cfg.backupGroup; | ||
| ExecStart = "${pkgs.bash}/bin/bash ${./backup.sh}"; |
There was a problem hiding this comment.
this service needs also to be a dynamic user when the main one is, otherwise it won't have access to /var/lib/bitwarden_rs
|
Looks reasonable, thanks |
|
1.8.0 has been released for a while now and compiles for me. I haven't gotten around to deploying it yet, though. |
|
@ajs124 Thank you for reminding me, now that NixOS supports rustc > 1.32 its possible to start using the latest release of bitwarden. I have updated everything and added the newly introduced options to the module. I have only tested it starting correctly and made sure the web interface was reachable, but I have not tested it thoroughly. |
| Whether to disable the use of an admin token. | ||
| ''; | ||
| }; | ||
| } |
There was a problem hiding this comment.
Oh boy, this list of options is a bit too big for my taste. This is exactly the kind of thing I'm hoping to prevent with NixOS/rfcs#42.
Oh and actually these options guarded on version numbers are very bad. I think this might lead to infinite recursion if people add an overlay for bitwarden_rs.
There was a problem hiding this comment.
I know (roughly) your position on this, but since I already went through the trouble of mapping all options per version, my position on it for now is, as long as there is no consensus on whether to accept the RFC, I am not going to completely redo this module.
There was a problem hiding this comment.
Then I guess we can agree to disagree, because I don't feel comfortable merging a module like this, I don't want anybody to have to maintain such code in the future, adding new options on every new release.
There was a problem hiding this comment.
Fair enough. How about I remove the module from the PR for now, but at least already merge the related packages?
There was a problem hiding this comment.
BTW, I do agree with the general idea behind the RFC. I had the same issues with the module system when I first learned about NixOS. So with regards to the RFC I am in favor of your idea (although I have not read all the details yet), but AFAIK explicit options + extraConfig is still how modules are expected to be arranged, making me uncomfortable to arrange it according to the RFC, since I have yet to see a module arranged like that and the discussion about it seems to be ongoing.
There was a problem hiding this comment.
I'd be fine with merging the package additions.
The approach in the RFC has been implemented in at least 2 NixOS modules already (one of them by me), see https://github.com/Infinisil/rfcs/blob/config-option/rfcs/0042-config-option.md#previous-implementations. I'm trying to get it along as fast as possible, but it's hard because most people don't always have time to review it. I'm holding back from merging PR's that go too strongly against the RFC for the time being, because once introduced, options can't really be removed.
There was a problem hiding this comment.
Given that people would have to write their own services to use the packages, I thought it would be best to at least try to see how much effort changing it would be. Does the modified module match with what you intended with the RFC? I am not confident about the enableWebVault, enableWebsocket, and dataFolder options, whether they should be options or part of the config option.
There was a problem hiding this comment.
Oh wow, very nice! Other than a few nitpicks this looks great!
| @@ -0,0 +1,17 @@ | |||
| #!/usr/bin/env bash | |||
|
|
|||
| # Based on: https://github.com/dani-garcia/bitwarden_rs#backing-up-your-vault | |||
There was a problem hiding this comment.
That link appears to be outdated, it's here now: https://github.com/dani-garcia/bitwarden_rs/wiki/Backing-up-your-vault
|
|
||
| dataFolder = mkOption { | ||
| type = str; | ||
| default = "/var/lib/bitwarden_rs"; |
There was a problem hiding this comment.
I end up questioning everything after all :). Do you think you need this option? Because I personally never used any such options for configuring the data directory.
If you do need it, rename it to dataDir, which is the more conventional name for such options.
There was a problem hiding this comment.
I would also ask you to question if the flexibility of a dataDir is really required or not. Having this option really works against systemd and adds complication for something the user can take care of with symlinks if really necessary.
See #60019 (comment) for more discussion.
There was a problem hiding this comment.
Actually, this option is part of bitwarden_rs's configuration, which I happen to reuse for the service. What trick is there again to work around needing a value from an attribute set to define a default of that attribute set, since that is what made me choose to put them outside the config option.
This produces infinite recursion (I get why, just do not remember how to work around it properly):
{
services.bitwarden_rs.config.webVaultFolder = mkIf cfg.config.webVaultEnabled (mkDefault "${pkgs.bitwarden_rs-vault}/share/bitwarden_rs/vault");
}Right now I worked around it with an apply.
{
apply = config: config // optionalAttrs (config.webVaultEnabled && !(config ? webVaultFolder)) {
webVaultFolder = "${pkgs.bitwarden_rs-vault}/share/bitwarden_rs/vault";
};
}@aanderse So you would suggest to not allow the user to configure the dataFolder of bitwarden_rs but always set it to /var/lib/bitwarden_rs and have the service always use StateDirectory = "bitwarden_rs"?
There was a problem hiding this comment.
@msteen Yes, for a service like this I suggest hard coding the data directory as that seems to work for almost every other Linux distribution out there. If the user has a really exotic use case then symlinks should more than suffice.
There was a problem hiding this comment.
Thanks for the confirmation. I have added an assertion to make clear to users that this is now the case, so that changes to config.dataFolder are not silently ignored.
|
Thanks for the review! I have updated the module accordingly. Although I am not completely convinced yet whether reducing the options to the bare minimum is the best way to go, since although they can still be easily changed, it might not be as easy for those relatively new to NixOS modules, I do like how it simplifies the module a lot. |
| therefore the names are converted from camel case (e.g. disable2FARemember) | ||
| to upper case snake case (e.g. DISABLE_2FA_REMEMBER). | ||
| In this conversion digits (0-9) are handled just like upper case characters, | ||
| so foo2 would be converted to FOO_2. |
There was a problem hiding this comment.
Does the conversion also handle the case of the keys already being environment variables? If so that could also be mentioned here.
| that is passed to bitwarden_rs's systemd service. | ||
|
|
||
| The available configuration options can be found in the environment template file: | ||
| https://github.com/dani-garcia/bitwarden_rs/blob/1.8.0/.env.template |
There was a problem hiding this comment.
Needs a <link xlink:href="https://example.com"/>
| apply = config: let cfg = { | ||
| dataFolder = "/var/lib/bitwarden_rs"; | ||
| webVaultEnabled = true; | ||
| } // config; in cfg // optionalAttrs (cfg.webVaultEnabled && !(cfg ? webVaultFolder)) { |
There was a problem hiding this comment.
This sure is a bit ugly, I'll give it a go to simplify this.
There was a problem hiding this comment.
I tried some other things, but this was the only way I got it to work, so if you have some better way of doing this, I very much welcome it.
There was a problem hiding this comment.
Does it matter that webVaultFolder is set even when it's not used? Because if it doesn't matter (aka the program just ignores it), you could unconditionally set it.
There was a problem hiding this comment.
In that case this is what I'd do:
diff --git a/nixos/modules/services/security/bitwarden_rs/default.nix b/nixos/modules/services/security/bitwarden_rs/default.nix
index d41b6e0dbd1..0b9d6037cf1 100644
--- a/nixos/modules/services/security/bitwarden_rs/default.nix
+++ b/nixos/modules/services/security/bitwarden_rs/default.nix
@@ -60,22 +60,13 @@ in {
The available configuration options can be found in the environment template file:
https://github.com/dani-garcia/bitwarden_rs/blob/1.8.0/.env.template
'';
- apply = config: let cfg = {
- dataFolder = "/var/lib/bitwarden_rs";
- webVaultEnabled = true;
- } // config; in cfg // optionalAttrs (cfg.webVaultEnabled && !(cfg ? webVaultFolder)) {
- webVaultFolder = "${pkgs.bitwarden_rs-vault}/share/bitwarden_rs/vault";
- };
};
};
config = mkIf cfg.enable {
- assertions = singleton {
- assertion = cfg.config.dataFolder == "/var/lib/bitwarden_rs";
- message = ''
- Due to the way bitwarden_rs's systemd service has been setup it is not possible to change the data folder
- to something other than the default location (/var/lib/bitwarden_rs).
- '';
+ services.bitwarden_rs.config = {
+ dataFolder = "/var/lib/bitwarden_rs";
+ webVaultFolder = "${pkgs.bitwarden_rs-vault}/share/bitwarden_rs/vault";
};
users.users.bitwarden_rs = { inherit group; };I don't think the assertion is necessary, because the user would have to use mkForce to set it, at which point they should realized that they need to change the module to have their special value work.
There was a problem hiding this comment.
The downside of always setting webVaultFolder is that it would cause it to be installed even when not being used.
There was a problem hiding this comment.
Alright I think apply can indeed not be gotten rid of, but I'd still put whatever you can in the config section, then the module system can handle the merging:
diff --git a/nixos/modules/services/security/bitwarden_rs/default.nix b/nixos/modules/services/security/bitwarden_rs/default.nix
index c0882711cd6..df64f38631d 100644
--- a/nixos/modules/services/security/bitwarden_rs/default.nix
+++ b/nixos/modules/services/security/bitwarden_rs/default.nix
@@ -69,24 +69,17 @@ in {
The available configuration options can be found in
<link xlink:href="https://github.com/dani-garcia/bitwarden_rs/blob/1.8.0/.env.template">the environment template file</link>.
'';
- apply = config: let cfg = {
- dataFolder = "/var/lib/bitwarden_rs";
- webVaultEnabled = true;
- } // config // {
- inherit (cfg) domain;
- }; in cfg // optionalAttrs (cfg.webVaultEnabled && !(cfg ? webVaultFolder)) {
+ apply = config: optionalAttrs config.webVaultEnabled {
webVaultFolder = "${pkgs.bitwarden_rs-vault}/share/bitwarden_rs/vault";
- };
+ } // config;
};
};
config = mkIf cfg.enable {
- assertions = singleton {
- assertion = cfg.config.dataFolder == "/var/lib/bitwarden_rs";
- message = ''
- Due to the way bitwarden_rs's systemd service has been setup it is not possible to change the data folder
- to something other than the default location (/var/lib/bitwarden_rs).
- '';
+ services.bitwarden_rs.config = {
+ dataFolder = "/var/lib/bitwarden_rs";
+ webVaultEnabled = mkDefault true;
+ domain = mkDefault cfg.domain;
};
users.users.bitwarden_rs = { inherit group; };I also made the apply a bit simpler by switching the order of the arguments to // (then webVaultFolder from config overrides the default one). Feel free to keep the assertion in if you want, I personally don't think it's needed.
| example = literalExample '' | ||
| { | ||
| signupsAllowed = true; | ||
| domain = https://example.com; |
There was a problem hiding this comment.
The domain option here would work really well as a separate option.
|
Also while trying this out, I noticed that you forgot to add your module to And as a last thing, can you split this PR into multiple commits, one commit per package, and another one for the module? |
|
I have made the changes you requested (except for the apply code, not sure how to improve upon it). Instead of splitting it up in 3 commits, I think it better to have both packages in one commit, since they might be 2 packages, but that is just structural, I consider |
|
Yeah a single commit for both packages sounds good. Is this correct that the package fails at runtime or is just of no use at runtime when you don't set a domain? If it works without a domain, then it would make sense to have domain by of type |
|
It's not mandatory (I tested it just now, it works without it, but according to the documentation some features will no longer work properly), so I have chosen not to make it a separate option, since I do not see why this particular config option should be considered special compared to the others, some of which are just as important. I still use the |
| The available configuration options can be found in | ||
| <link xlink:href="https://github.com/dani-garcia/bitwarden_rs/blob/1.8.0/.env.template">the environment template file</link>. | ||
| ''; | ||
| apply = config: config // optionalAttrs (config.webVaultEnabled && !(config ? webVaultFolder)) { |
There was a problem hiding this comment.
Oh, I suggested to flip this around like optionalAttrs config.webVaultEnabled { ... } // config. Is there a reason this didn't work? I'm pretty sure it should.
There was a problem hiding this comment.
Agreed, that would be better, less code and it better conveys that it is a default. I have updated it.
|
Awesome, thanks! |
|
Please do not ask for making a single config option as long as RFC 42 has not been accepted! |
|
@globin I didn't ask for it, I only mentioned the RFC and said that I wouldn't be comfortable with merging a module with so many options. And it's not like the RFC does something totally new. NixOS modules have always been implemented how it makes the most sense, and sometimes that has been such a In any case, and this is such a nice thing about the RFC, more options can always be added in a backwards-compatible way later. Not so if I would've merged this module with 50 options. |
|
Just wanted to pitch in and say that I am happy that @infinisil did not accept the PR as it was initially, because although I am not sure yet whether this is the best approach, it would have been painful to keep the module in sync with the updates, while in its current form the module is unlikely to need any changes. Maybe only after major releases, but the changelog is easy to check and so automatic updates can be passed without much trouble. I had some troubles with the default ACME SSL certificates module, so I made one for dehydrated (not yet made PR for it, still testing it) in a similar style as I ended making the one for bitwarden_rs, since they both are configured in a similar style (file with environment variables). |
|
This is exactly the issue I have with the RFC as mentioned there, too. This kind of module definition shifts the whole maintenance burden to the user; instead of us doing the update once at the point when we update the package, each and every user has to start reading the documentation for every downstream update to check for incompatible settings. That is the point why we want to have rigidly defined options with proper types and documentation. Then users get notified of changes during evaluation and not by the service failing to start. Imagine being the administrator of a range of different servers running a lot of diverse software. We can help them a lot by allowing them to rely on us checking for breaking changes while updating the software and not have them need to check everything, every time they deploy everything. Also we have nice documentation which allows users to oftentimes not even need to go to each upstream documentation, but for smaller services have everything nicely documented in one central place, be it Therefore in my opinion we should be adding each and every possible option to our modules and not starting to expand our use of catch-all options. I agree that it's fine to prefer that approach rather than not adding a module at all if it is to much work initially, but specifically in this case the work was already done and has just gotten chucked out. If it is to much burden on evaluation time and resources, we should not focus on getting rid of options but improving evaluation performance, most probably by only including the modules needed etc. Also there can always be maintainer scripts, generating the options, and that still is superior due to us having documentation and types and the user noticing removal and changes of options before something breaks! |
|
@globin The RFC doesn't disallow making things backwards-compatible, in fact I wrote a section on that: https://github.com/Infinisil/rfcs/blob/config-option/rfcs/0042-config-option.md#backwards-compatibility-for-configuration-settings. And in any case, modules that have options that depend on the package version are bad because the NixOS manual is rather static. You can't have options show up/disappear when you change a package version. So this means even when you e.g. have an older version of the package supporting older settings, they won't show up in the NixOS manual.
This is what happens when people do this: https://nixos.org/nixos/options.html#i2pd. And then look at how much maintenance this requires: https://github.com/NixOS/nixpkgs/commits/master/nixos/modules/services/networking/i2pd.nix (this is a great example actually). This includes:
I don't wanna know how many such cases were missed during the updates to this module, but I bet it's not zero. And i2pd is a more popular module that has some people using it. With less popular modules, maintenance becomes near-impossible. Even worse with packages that often change their config. I certainly don't have the time to maintain our 1000 or however many services we have to always have all options be the most up to date (or even working across multiple versions). Sure it would be nice to have all options of every service ever neatly documented in a single uniform place, but that's just not possible with the small amount of people that work on nixpkgs. What we can do is keep a small set of most important options up-to-date and make sure those are working well. See also the comment I just posted in the RFC: NixOS/rfcs#42 (comment) |
I made this more explicit in PR #176903 |
Motivation for this change
I wanted to use a password manager that I could access anywhere and that would allow me to share passwords with others, but I only wanted to use something that I could host myself, which the Rust implementation of the Bitwarden server API nicely supports. I have had multiple requests on the IRC to make this PR, which I now finally am able to do after managing to package the Bitwarden Web Vault after multiple failed attempts.
This PR is using an outdated version of
bitwarden_rs, the version I have been using on my server for several months, because the newer versions require a version of the Rust compiler > 1.31, but since I do not know how long it will take for a newer version to be ready, and 19.03 is around the corner, I thought it best to submit it under this outdated version that I at least know works for sure. I made the module so that it defines the options for all versions between 1.4 and 1.7 (not yet released), so that its easy to updatebitwarden_rswhen it can be updated.Things done
sandboxinnix.confon non-NixOS)nix-shell -p nox --run "nox-review wip"./result/bin/)nix path-info -Sbefore and after)