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.

Sign up for GitHub

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

Jump to bottom

nixos/dendrite: add an option loadCredential #164135

Merged
merged 1 commit into from
May 29, 2022
Merged

nixos/dendrite: add an option loadCredential #164135

merged 1 commit into from
May 29, 2022
+19 −3

Conversation

jian-lin
Copy link
Contributor

@jian-lin jian-lin commented Mar 14, 2022 •
edited
Loading

Description of changes

systemd-247 provides a mechanism called LoadCredential for secrets and
it is better than environment file. See the section of Environment=
in the manual of systemd.exec for more information.

Some options in config.yaml need values to be strings, which currently
can be used with environmentFile but not loadCredential. But it's
possible to use loadCredential for those options, e.g. we can
substitute their values in ExecStart, but not in ExecStartPre due to
1.

Things done
  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
  • 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/)
  • 22.05 Release Notes (or backporting 21.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
    • (Release notes changes) Ran nixos/doc/manual/md-to-db.sh to update generated release notes
  • Fits CONTRIBUTING.md.

Sorry, something went wrong.

@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 14, 2022
@jian-lin jian-lin mentioned this pull request Mar 14, 2022
Merged
13 tasks
@ofborg ofborg bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1-10 labels Mar 14, 2022
@mjlbach
Copy link
Contributor

mjlbach commented Mar 16, 2022

The change LGTM, I wanted to do this but there were issues at the time with systemd 247 (start reading here if interested in the context)

@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/prs-ready-for-review/3032/780

@jian-lin
Copy link
Contributor Author

/marvin opt-in
/status needs_reviewer

@marvin-mk2
Copy link

marvin-mk2 bot commented Mar 18, 2022

Hi! I'm an experimental bot. My goal is to guide this PR through its stages, hopefully ending with a merge. You can read up on the usage here.

@marvin-mk2 marvin-mk2 bot added marvin This PR was reviewed by Marvin, a discontinued bot: https://github.com/timokau/marvin-mk2 needs_reviewer (old Marvin label, do not use) labels Mar 18, 2022
@marvin-mk2 marvin-mk2 bot requested a review from SuperSandro2000 March 18, 2022 12:39
@marvin-mk2 marvin-mk2 bot added awaiting_reviewer (old Marvin label, do not use) and removed needs_reviewer (old Marvin label, do not use) labels Mar 18, 2022
@marvin-mk2
Copy link

marvin-mk2 bot commented Mar 21, 2022

Reminder: Please review!

This Pull Request is awaiting review. If you are the assigned reviewer, please have a look. Try to find another reviewer if necessary. If you can't, please say so. If the status is not accurate, please change it. If nothing happens, this PR will be should be put back in the needs_reviewer queue in one day.

Note: This feature is currently broken. The bot will not actually change the status. If you see this message multiple times, please request a status change manually.

@jian-lin
Copy link
Contributor Author

/status needs_reviewer

@marvin-mk2 marvin-mk2 bot added needs_reviewer (old Marvin label, do not use) and removed awaiting_reviewer (old Marvin label, do not use) labels Mar 22, 2022
@marvin-mk2 marvin-mk2 bot added awaiting_reviewer (old Marvin label, do not use) and removed needs_reviewer (old Marvin label, do not use) labels Mar 23, 2022
@marvin-mk2
Copy link

marvin-mk2 bot commented Mar 26, 2022

Reminder: Please review!

This Pull Request is awaiting review. If you are the assigned reviewer, please have a look. Try to find another reviewer if necessary. If you can't, please say so. If the status is not accurate, please change it. If nothing happens, this PR will be should be put back in the needs_reviewer queue in one day.

Note: This feature is currently broken. The bot will not actually change the status. If you see this message multiple times, please request a status change manually.

@cole-h cole-h removed marvin This PR was reviewed by Marvin, a discontinued bot: https://github.com/timokau/marvin-mk2 awaiting_reviewer (old Marvin label, do not use) labels Mar 26, 2022
@chuangzhu
Copy link
Contributor

I don't think deprecating environmentFile is a good idea. LoadCredential just loads a file to a secret directory so services with DynamicUser enabled can read it without permission issue. It cannot substitute secrets in the config file. The client_api.registration_shared_secret option cannot currently be passed in another file. If you want to connect a remote PostgreSQL database. you will need to pass the password in *.database.connection_string in a way like postgresql://user:password@hostname/database.

I agree with adding the loadCredential option, but I don't think it is a replacement for environmentFile.

@dotlambda
Copy link
Member

We should use the new option in the test.

@jian-lin
Copy link
Contributor Author

jian-lin commented Apr 8, 2022

I don't think deprecating environmentFile is a good idea. LoadCredential just loads a file to a secret directory so services with DynamicUser enabled can read it without permission issue. It cannot substitute secrets in the config file. The client_api.registration_shared_secret option cannot currently be passed in another file. If you want to connect a remote PostgreSQL database. you will need to pass the password in *.database.connection_string in a way like postgresql://user:password@hostname/database.

I agree with adding the loadCredential option, but I don't think it is a replacement for environmentFile.

You are right. Some options in the config.yaml need to be string instead of path. They cannot be substituted using our current module. I missed that.

Using LoadCredential for those options is still possible, e.g. we can substitute secrets in ExecStart phase, but not in ExecStartPre due to systemd/systemd#19604 . I personally don't use those options, so I'll leave it for someone to make another PR.

@jian-lin jian-lin force-pushed the dendrite-add-option-loadCredential branch from 2e80b1e to ebfe44d Compare April 8, 2022 08:03
@jian-lin jian-lin changed the title nixos/dendrite: add option loadCredential, deprecate environmentFile nixos/dendrite: add an option loadCredential Apr 8, 2022
@jian-lin jian-lin force-pushed the dendrite-add-option-loadCredential branch 2 times, most recently from db2cbc1 to f1b39fc Compare April 8, 2022 08:31
@jian-lin
Copy link
Contributor Author

jian-lin commented Apr 8, 2022

Thanks for the review. Changes have been made.

@jian-lin jian-lin requested a review from dotlambda April 8, 2022 10:16
@jian-lin jian-lin force-pushed the dendrite-add-option-loadCredential branch 2 times, most recently from 31dfbde to a1eb796 Compare April 8, 2022 18:17
@jian-lin
Copy link
Contributor Author

jian-lin commented Apr 9, 2022

Can we merge this since changes have been made as required? @dotlambda @SuperSandro2000

@jian-lin
Copy link
Contributor Author

I don't use dendrite anymore.

@jian-lin jian-lin closed this May 29, 2022
@dotlambda dotlambda reopened this May 29, 2022
@dotlambda
Copy link
Member

I missed this, sorry.

@dotlambda dotlambda requested a review from NickCao May 29, 2022 01:43
@jian-lin @dotlambda
nixos/dendrite: add an option loadCredential
7c9d130 
systemd-247 provides a mechanism called LoadCredential for secrets and
it is better than environment file. See the section of Environment=
in the manual of systemd.exec for more information.

Some options in config.yaml need values to be strings, which currently
can be used with environmentFile but not loadCredential. But it's
possible to use loadCredential for those options, e.g. we can
substitute their values in ExecStart, but not in ExecStartPre due to
[1].

[1]: systemd/systemd#19604
@dotlambda dotlambda force-pushed the dendrite-add-option-loadCredential branch from a1eb796 to 7c9d130 Compare May 29, 2022 01:47
dotlambda
dotlambda approved these changes May 29, 2022
View reviewed changes
Copy link
Member

@dotlambda dotlambda left a comment •
edited
Loading

Choose a reason for hiding this comment

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

I think we should still merge this because it's necessary for anyone who does use dendrite.
cc @tim-tx

@dotlambda dotlambda requested review from McSinyx and ratsclub May 29, 2022 01:49
@jian-lin
Copy link
Contributor Author

jian-lin commented May 29, 2022 •
edited
Loading

I think we should still merge this because it's necessary for anyone who does use dendrite.

Well, it's not necessary, just an improvement of UX.

Anyway, I will be happy if it gets merged.

@dotlambda
Copy link
Member

@ofborg test dendrite

@dotlambda dotlambda merged commit 37792e5 into NixOS:master May 29, 2022
@github-actions github-actions bot mentioned this pull request May 29, 2022
Merged
1 task
@github-actions
Copy link
Contributor

Successfully created backport PR #175356 for release-22.05.

@jian-lin jian-lin deleted the dendrite-add-option-loadCredential branch June 1, 2022 02:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dotlambda dotlambda

@NickCao NickCao

@SuperSandro2000 SuperSandro2000

@McSinyx McSinyx

@ratsclub ratsclub

Assignees
No one assigned
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 on Darwin 10.rebuild-linux: 1-10
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@jian-lin @mjlbach @nixos-discourse @chuangzhu @dotlambda @NickCao @cole-h