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/synapse: doc: suggest LoadCredential before nixops/sops #260212

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

martinetd
Copy link
Member

Description of changes

This is a bit selfish, but that's what I was looking for in the .md when I wanted to quickly add a user without adding the password in the store on a local machine. Happy to reword or try another way of describing it.


nixops and similar are great, but require an upfront cost that is no longer necessary now systemd can read root-only files for restricted services through LoadCredential

Suggest the native method first in the service readme for simplicity


Note: it'd make more sense to set $CREDENTIALS_DIRECTORY/matrix-shared-secret instead of hardcoding /run/credentials/<service>, but that doesn't pass types.path check. The service name is made explicit when setting LoadCredential anyway, so this constant should be safe to use.

Things done

Cc @Ma27 @fadenb @mguentner @Ralith @sumnerevans @NickCao @dali99 (matrix team)

nixops and similar are great, but require an upfront cost that is no
longer necessary now systemd can read root-only files for restricted
services through `LoadCredential`
Suggest the native method first in the service readme for simplicity
@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 Oct 10, 2023
@NickCao
Copy link
Member

NickCao commented Oct 10, 2023

Tip: you can use /$CREDENTIALS_DIRECTORY/matrix-shared-secret to trick the check.

@Ma27
Copy link
Member

Ma27 commented Oct 10, 2023

Don't we have a section in the NixOS manual that explains how to deal with secrets? (cc @NixOS/documentation-team )

I'm pretty sure that this is by far not the only module where you have an issue like this and I think it's far better to have a central place where (1) the general issue is described (i.e. why that shouldn't be in the Nix store) and (2) how to deal with it (i.e. "deployment" tooling such as sops-nix/agenix) and mechanisms for services (i.e. LoadCredential, SuppplementaryGroups etc.). Then, all modules could just reference this section of the manual and we don't need to duplicate this information everywhere (and risking that it is incomplete - as demonstrated by @martinetd in this case).

If such a section exists, we should just link it here and remove the rest. Otherwise, I'd accept this patch as temporary improvement, but I'd really appreciate it if the docs team could add that to their backlog if possible :)

@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 Oct 10, 2023
@jian-lin
Copy link
Contributor

Don't we have a section in the NixOS manual that explains how to deal with secrets?

not yet #142282

@martinetd martinetd force-pushed the synapse_loadcredential branch from 9583153 to aee351d Compare October 10, 2023 20:31
@martinetd
Copy link
Member Author

Tip: you can use /$CREDENTIALS_DIRECTORY/matrix-shared-secret to trick the check.

Oh, that's nice I've updated it to that!
I don't suppose you have another trick to hide the systemd service name by accessing it through services.matrix-synapse.something instead of sytemd.services.matrix-synapse ?

If such a section exists, we should just link it here and remove the rest.

Agreed. There was something so I just built on it, but if there's something common it'd be better to share. Looking for 'LoadCredential" in all *.md files only found ths and a mention in nixos/modules/security/acme/default.md so I can confirm there doesn't seem to be anything yet.

@pennae
Copy link
Contributor

pennae commented Oct 10, 2023

Tip: you can use /$CREDENTIALS_DIRECTORY/matrix-shared-secret to trick the check.

you can also inject arbitraty synapse command line arguments by adding spaces to an extraConfigFiles entry. this is a bug, not a feature

@martinetd
Copy link
Member Author

Actually just tested that and the command is escaped, so that doesn't work -- systemd tries to run --config-path "/\$CREDENTIALS_DIRECTORY/secretfile" which.. isn't anywhere to be found.

Reverting the commit

@martinetd martinetd force-pushed the synapse_loadcredential branch from aee351d to 9583153 Compare October 10, 2023 20:39
@ju1m
Copy link
Contributor

ju1m commented Oct 10, 2023

Actually just tested that and the command is escaped, so that doesn't work -- systemd tries to run --config-path "/\$CREDENTIALS_DIRECTORY/secretfile" which.. isn't anywhere to be found.

In ExecStart= the env vars substitution is done by systemd (not bash), so it has to be written as: /${CREDENTIALS_DIRECTORY}/secretfile.
I've been bitten by this before:

Minor gotcha, $CREDENTIALS_DIRECTORY must indeed actually be used with ${} (as in ${CREDENTIALS_DIRECTORY}/mycred from the doc example) when part of a word, since contrary to bash, a / is not a separator for variable names.

This behavior is mentionned in the systemd.service manual:

Basic environment variable substitution is supported. Use "${FOO}" as part of a word, or as a word of its own, on the command line, in which case it will be erased and replaced by the exact value of the environment variable (if any) including all whitespace it contains, always resulting in exactly a single argument. Use "$FOO" as a separate word on the command line, in which case it will be replaced by the value of the environment variable split at whitespace, resulting in zero or more arguments. For this type of expansion, quotes are respected when splitting into words, and afterwards removed.

@ju1m
Copy link
Contributor

ju1m commented Oct 10, 2023

Hmm, by reading the code, I think the path /run/credentials/matrix-synapse.service/matrix-shared-secret should remain hardcoded instead of using ${CREDENTIALS_DIRECTORY}, because it is used in the shell script matrix-synapse-register_new_matrix_user, hence outside of matrix-synapse.service, hence without the right CREDENTIALS_DIRECTORY set.

@wegank wegank added the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Jul 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md 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
Development

Successfully merging this pull request may close these issues.

7 participants