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/vaultwarden: add more systemd service hardening; move state dir in 24.05 #292485

Closed
wants to merge 2 commits into from

Conversation

fsnkty
Copy link
Member

@fsnkty fsnkty commented Mar 1, 2024

Description of changes

added systemd service hardening to vaultwarden and backup-vaultwarden.
change state directory defaults to /var/lib/vaultwarden over /var/lib/bitwarden_rs when system.stateVersion is >= 24.05

removed changes.
ReadWritePaths = [ "/var/lib/bitwarden_rs" ]; I thought was a requirement when protectSystem is strict but I guess that's not the case when stateDirectory is used?
The following should have no affect
removed with, mostly to address the top level use of with lib; but also removed some pointless uses ( like on lists a single item long).
removed now pointless lib.mdDoc
applied the rfc style formatting.

Things done

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
    • sandbox = relaxed
    • sandbox = true
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 24.05 Release Notes (or backporting 23.05 and 23.11 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
  • Fits CONTRIBUTING.md.

Add a 👍 reaction to pull requests you find important.

@github-actions github-actions bot added 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: module (update) This PR changes an existing module in `nixos/` labels Mar 1, 2024
@fsnkty fsnkty force-pushed the nixos-vaultwarden-hardening branch 2 times, most recently from 2989f7d to 62a7521 Compare March 1, 2024 03:26
@fsnkty fsnkty changed the title Nixos vaultwarden hardening Nixos/vaultwarden: add ssytemd service hardening Mar 1, 2024
@ofborg ofborg bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild 10.rebuild-linux: 1-10 labels Mar 1, 2024
Copy link
Member

@SuperSandro2000 SuperSandro2000 left a comment

Choose a reason for hiding this comment

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

Please drop the reformatting. Also the lib.mdDoc changes while technically no longer needed, will be eventually be removed by a treewide change and only add noise to the PR and cause conflicts with other PRs open.

@SuperSandro2000
Copy link
Member

Please also include the backup hardening options from f7b63db#diff-8245e51bae8611eb979f6b30b0938c936f8d046396b7d3bb9c204b2cb7ad4b74

@SuperSandro2000 SuperSandro2000 changed the title Nixos/vaultwarden: add ssytemd service hardening Nixos/vaultwarden: add sytemd service hardening Mar 2, 2024
@fsnkty
Copy link
Member Author

fsnkty commented Mar 2, 2024

Please drop the reformatting. Also the lib.mdDoc changes while technically no longer needed, will be eventually be removed by a treewide change and only add noise to the PR and cause conflicts with other PRs open.

  1. Will do
  2. Yeah am now aware from another comment on a different PR, I had asked around about why there was seemingly no plan to treewide remove lib.mdDoc before hand so had just thrown it in if i was making a pr for something else. any suggestions on how i can better keep up with the plans for things like this? Will also remove this, thanks.

@fsnkty fsnkty force-pushed the nixos-vaultwarden-hardening branch from 62a7521 to 33b6464 Compare March 9, 2024 06:42
@fsnkty
Copy link
Member Author

fsnkty commented Mar 9, 2024

ah forgot to update the following commit replacing with oops!

@fsnkty fsnkty force-pushed the nixos-vaultwarden-hardening branch 2 times, most recently from f61673c to 42b3e61 Compare March 9, 2024 07:17
@SuperSandro2000
Copy link
Member

2. Yeah am now aware from another comment on a different PR, I had asked around about why there was seemingly no plan to treewide remove lib.mdDoc before hand so had just thrown it in if i was making a pr for something else. any suggestions on how i can better keep up with the plans for things like this? Will also remove this, thanks.

I don't know, it just made the PR pretty big and harder for me to review

@fsnkty fsnkty force-pushed the nixos-vaultwarden-hardening branch from 42b3e61 to 0e2d7e8 Compare March 12, 2024 07:21
@fsnkty
Copy link
Member Author

fsnkty commented Mar 12, 2024

to fixup

sighs you'd think I'd have figured how to rename a commit by now.

@fsnkty fsnkty force-pushed the nixos-vaultwarden-hardening branch from 0e2d7e8 to 44140c9 Compare March 12, 2024 07:23
@SuperSandro2000 SuperSandro2000 changed the title Nixos/vaultwarden: add sytemd service hardening nixos/vaultwarden: add sytemd service hardening Apr 3, 2024
@fsnkty fsnkty force-pushed the nixos-vaultwarden-hardening branch from 44140c9 to 52b8a9d Compare April 4, 2024 22:06
@fsnkty
Copy link
Member Author

fsnkty commented Apr 4, 2024

rebasing onto master. backup-vaultwarden's ReadOnlyDirectories had mkIf and hasPrefix from no where, adding lib. ? seems to be me misunderstanding git again.

for now ill make what i can source configEnv.DATA_FOLDER, I'm going to drop the move to vaultwarden over bitwarden_rs in the path in this commit as that seems like it should be done separately. the 2nd commit does this dependent on state version but i have low confidence in the quality of my solution, explanation of potential issues with it would be appreciated.

there should also be a release note for this breaking change right?

@fsnkty fsnkty changed the title nixos/vaultwarden: add sytemd service hardening nixos/vaultwarden: add more systemd service hardening move state dir in 24.05 Apr 4, 2024
@fsnkty fsnkty changed the title nixos/vaultwarden: add more systemd service hardening move state dir in 24.05 nixos/vaultwarden: add more systemd service hardening & move state dir in 24.05 Apr 4, 2024
@SuperSandro2000 SuperSandro2000 changed the title nixos/vaultwarden: add more systemd service hardening & move state dir in 24.05 nixos/vaultwarden: add more systemd service hardening & move state dir in 24.05; move to /var/lib/vaultwarden in 24.05 Apr 5, 2024
@SuperSandro2000 SuperSandro2000 changed the title nixos/vaultwarden: add more systemd service hardening & move state dir in 24.05; move to /var/lib/vaultwarden in 24.05 nixos/vaultwarden: add more systemd service hardening; move state dir in 24.05 Apr 5, 2024
@fsnkty fsnkty force-pushed the nixos-vaultwarden-hardening branch from 52b8a9d to f9c3a81 Compare April 5, 2024 22:47
@fsnkty
Copy link
Member Author

fsnkty commented Apr 7, 2024

been using this seemingly without issue since I made the push. systems on 24.05 (host "library" here
I don't use the included backup service however so confirmation from anyone using it would be great :)

@SuperSandro2000
Copy link
Member

I don't use the included backup service however so confirmation from anyone using it would be great :)

We can merge #292857 first to get a test.

@wegank wegank added the 12.approvals: 1 This PR was reviewed and approved by one reputable person label Apr 27, 2024
@fsnkty
Copy link
Member Author

fsnkty commented Jun 16, 2024

with #292857 being merged ( thanks sandro ) how do we go about getting this PR tested with it?

Copy link
Member

@leona-ya leona-ya left a comment

Choose a reason for hiding this comment

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

I rebased this PR on master for testing and ran nixosTests.vaultwarden.

It seems that every tests fails with

server # [   17.057818] vaultwarden[1060]: Error: Rocket.
server # [   17.059276] vaultwarden[1060]: [CAUSE] Bind(
server # [   17.059876] vaultwarden[1060]:     Os {
server # [   17.071526] vaultwarden[1060]:         code: 13,
server # [   17.072214] vaultwarden[1060]:         kind: PermissionDenied,
server # [   17.072977] vaultwarden[1060]:         message: "Permission denied",
server # [   17.073809] vaultwarden[1060]:     },

and additionally the vaultwarden.mysql test shows

mysql-start[970]: 2024-06-16  9:49:56 59 [Warning] Aborted connection 59 to db: 'bitwarden' user: 'bitwardenuser' host: 'localhost' (Got an error reading communication packets)

but unsure whether this is just because vaultwarden doesn't start at all.

in { DATA_FOLDER = "/var/lib/bitwarden_rs"; } // lib.optionalAttrs (!(configEnv ? WEB_VAULT_ENABLED) || configEnv.WEB_VAULT_ENABLED == "true") {
in {
DATA_FOLDER = (
if lib.versionAtLeast config.system.stateVersion "24.05"
Copy link
Member

Choose a reason for hiding this comment

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

This should by now be 24.11 probably.

@SuperSandro2000
Copy link
Member

My plan is to first merge hexa's PR and then sort out what's left over in this one.

@wegank wegank added the 2.status: merge conflict This PR has merge conflicts with the target branch label Jul 4, 2024
@SuperSandro2000
Copy link
Member

@fsnkty can you please rebase on master?

@fsnkty
Copy link
Member Author

fsnkty commented Sep 20, 2024

well that push was a disaster, evidently I am unable to rebase on master xP
I wont have the time or setup to work on this for the foreseeable future so its likely best you handle this however you see fit in another PR @SuperSandro2000
just to note I agree with @leona-ya's change request to change the minimum state version up a stage

@fsnkty fsnkty closed this Sep 20, 2024
@fsnkty fsnkty deleted the nixos-vaultwarden-hardening branch September 20, 2024 08:32
@ofborg ofborg bot removed the 2.status: merge conflict This PR has merge conflicts with the target branch label Sep 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: module (update) This PR changes an existing module in `nixos/` 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild 10.rebuild-linux: 1-10 12.approvals: 1 This PR was reviewed and approved by one reputable person
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants