add age.secrets.*.{action,service} #87

Radvendii · 2021-12-29T17:38:48Z

I don't have the brainspace to test this right now, but I figured I would put it up in case other people were inspired to test before I did.

Things that need testing:

Does this even evaluate in current form?
Does it correctly restart services when secrets change
Does it not restart services when secrets don't change (or when some secrets change but other don't
If you change a secret and it's action atomically, which action gets performed? The old one or the new one? (I think this should be the new one)

Implements #84

winterqt · 2022-01-08T20:08:21Z

Does it not restart services when secrets don't change (or when some secrets change but other don't

I believe this is false because of #89.

If you change a secret and it's action atomically, which action gets performed? The old one or the new one? (I think this should be the new one)

As far as I can tell, it would be the old action, since the path condition fires before the units get reloaded.

Radvendii · 2022-01-12T20:52:31Z

@winterqt thank you! After looking at your comment and thinking about it for a bit, I've completely reworked this PR.

It now hijacks NixOS's mechanism for restarting services. If you specify a service, this is simple. it just adds the information about the secret to the service file so that if any of it changes, the service gets reloaded.

If you specify an action, it creates a dummy service that just executes that action, and similarly causes it to be reloaded when the secret changes.

A few design decisions I'm not sure about:

Do we even need action? Do people use this for anything besides restarting services? Someone might want to reload a service. In theory they can set systemd.services.<service>.reloadIfChanged, but maybe they want it to restart if they actually change the service itself?
Should we change service to services and make it a list? There are sometimes multiple services that need to be restarted. This would be needed if we remove action, but maybe not if we don't.
Is there a better name for the service than agenix-${name}-action?

winterqt · 2022-01-12T21:18:43Z

This is really nicely done, congrats. Do you mind if I PR additions to the tests to account for this functionality to your branch, so this all lands in one PR?

modules/age.nix

Radvendii · 2022-01-13T14:58:40Z

Not at all, go for it.

Radvendii · 2022-01-13T15:42:58Z

Well, I thought of yet another rewrite. Advantages of this one include:

Simpler (not mucking with nix systemd magic)
compares the decrypted secrets, so if you rekey this won't run the action

We could just add something like this to the activation script for each secret:

    if ! [ -e ${secretType.path} ] || \
       ! ${pkgs.diffutils}/bin/diff -q $TMP_FILE ${secretType.path} >/dev/null || \
       [ "$(stat -c '%a' "$TMP_FILE")" -eq "$(stat -c '%a' "${secretType.path}")" ]
    then
      echo "Updated secret ${secretType.name} performing action..."
      ${secretType.action};
    fi

This isn't working on my machine though, because the file at secretType.path doesn't exist while this is running for some reason. Something I've noticed on my machine is that /run/agenix.d doesn't actually store any older generations of secrets, only the current one. Maybe that's the problem? It's getting deleted somewhere? Can't figure out where though.

winterqt · 2022-01-13T17:40:57Z

@ryantm The changes to the integration test to include this functionality changes some reformatting using nixpkgs-fmt, plus some changes to make it work on recent Nixpkgs versions.

Would you prefer that I split the fixes and formatting changes from the additions, or don't format at all, or are you fine with all of this (implementation, fixing the existing test, and testing of this functionality) being in one commit?

Radvendii · 2022-01-18T13:40:28Z

Oh, and another advantage of that last rewrite is that it would perform the action the first time the secret is created / the action was added, not just when it gets changed. The disadvantage, of course, is that I still don't know how to get it to work at all.

ryantm · 2022-01-18T15:57:01Z

The changes to the integration test

Maybe a bunch of things just changed because I don't see any changes to the integration tests now? Generally, I'd prefer to keep formatting changes in a separate PR, so the individual ones are easier to review.

Radvendii · 2022-01-18T16:14:03Z

I don't think @winterqt committed the integration test yet.

Radvendii · 2022-01-18T16:14:51Z

Oh, and for that you should @winterqt you should know I changed it to have services which takes a list rather than just a single service. I found that it would be helpful in practice.

Radvendii · 2022-01-23T18:08:31Z

@winterqt any progress on this? I'm happy to work on the integration test but I don't want to duplicate effort in case you just forgot to push.

winterqt · 2022-01-23T18:13:30Z

@Radvendii I opened Radvendii#1 a few days ago. I assumed you would be notified, but I just noticed that GitHub doesn't enable notifications on forks for their creators by default, like they do for normal repos.

Radvendii · 2022-01-24T14:36:35Z

Whoops! Also my notification settings are a bit scuffed right now, which might be why.

Radvendii · 2022-01-25T14:49:10Z

Okay, this is no longer WIP. I think it's good to go

Radvendii · 2022-02-04T11:50:29Z

Bump? @ryantm

infinisil · 2022-03-24T23:56:53Z

modules/age.nix

+      services = mkOption {
+        type = types.listOf types.str;
+        default = [];
+        description = "The systemd services that uses this secret. Will be restarted when the secret changes.";
+        example = "[ wireguard-wg0 ]";
+      };


Some feedback regarding this:

What about wanting to only reload a unit instead of restarting it? There's reloadTriggers as well. Maybe there should be a way to make it reload instead of restart.

I'd think this option should be named units or dependentUnits instead (or with above proposal maybe dependentUnitsToRestart), and the strings it should accept are of the form "wireguard-wg0.service", but also "my.target". restart/reloadTriggers exist for all unit types, not just services. This then also matches the wantedBy and co. NixOS options. Note that you can probably use utils.systemdUtils.unitNameType for the type of this option then.

The example should be declared like example = literalExpression ''[ "wireguard-wg0" ]'' (or adding the .service suffix with the above suggestion)

I'm glad you weighed in. I don't have strong opinions about any of this, and the only reason I didn't do it the way you're describing is because I'm not that familiar with systemd units. Feel free to make these changes.

Oh. You don't have access to my repository.* I guess make a PR against my fork and I'll accept it? I would just make the changes myself but I don't think I understand them well enough to do it right on my first try.

*I stop using git for a week and totally forget how anything works XD

infinisil · 2022-03-25T00:29:27Z

modules/age.nix

+                  # We execute the action on reload so that it doesn't happen at
+                  # startup. The only disadvantage is that it won't trigger the
+                  # first time the service is created.
+                  reload = action;
+                  reloadIfChanged = true;


I think it makes more sense to also trigger at startup. This service is then essentially a secret post-processing-after-loading script.

I'm not sure what you mean by "post-processing-after-loading script". This script doesn't do any post-processing, it just restarts the service. If I understand correctly, having it trigger at startup would mean that the dependent service (e.g. wireguard) would start up twice every time you booted, without any good reason.

infinisil · 2022-04-27T11:35:04Z

Check out #84 (comment) for a potential alternative

Radvendii · 2022-06-01T11:24:12Z

I've overhauled this to use /run/activation-{restart,reload}-list and to check whether the actual decrypted secrets have changed.

h/t @EHfive

represents an action to perform or systemd service to restart when the secret changes

Co-authored-by: Winter <[email protected]>

compare the decrypted secrets, and make use of /run/nixos/activation-{restart,reload}-list

Radvendii · 2022-06-02T19:56:33Z

ping @ryantm I think this PR is in an even better state and is ready to merge again (from my end)

ryantm

Thank you. This is looking pretty good overall to me. I left some comments/questions.

modules/age.nix

test/integration.nix

modules/age.nix

Radvendii · 2022-06-08T21:06:21Z

This is very bizzare. nix flake check works on my machine and it's on the same commit. TFW flakes not reproducible.

Radvendii · 2022-06-13T12:54:54Z

I fixed the behaviour in some edge cases (including the one we discussed), and added a bunch more tests to make sure it's behaving right in those edge cases. There are even more cases that could be added (e.g. changing more than one thing at the same time), but that becomes very big very fast. let me know if you'd like me to add those tests.

Radvendii · 2022-07-20T15:35:35Z

ping @ryantm hopefully it's ready now.

Radvendii · 2022-11-02T12:25:51Z

@ryantm

winterqt · 2023-08-07T23:48:26Z

ping @ryantm

winterqt · 2023-08-07T23:51:15Z

also ping @cole-h :)

Radvendii force-pushed the main branch from 2a2c708 to 4b9a185 Compare January 12, 2022 20:45

Radvendii force-pushed the main branch from 4b9a185 to 72d63d3 Compare January 12, 2022 20:58

winterqt suggested changes Jan 12, 2022

View reviewed changes

modules/age.nix Outdated Show resolved Hide resolved

modules/age.nix Outdated Show resolved Hide resolved

Radvendii mentioned this pull request Jan 18, 2022

WIP: Templates #96

Draft

Radvendii changed the title ~~WIP: add age.secrets.*.{action,service}~~ add age.secrets.*.{action,service} Jan 25, 2022

ryantm added the merge conflict label Mar 9, 2022

infinisil reviewed Mar 25, 2022

View reviewed changes

Radvendii force-pushed the main branch from 00c9fe3 to 90d3345 Compare June 1, 2022 11:20

Radvendii and others added 5 commits June 1, 2022 13:45

add age.secrets.*.{action,service}

cd108c0

represents an action to perform or systemd service to restart when the secret changes

hashString (readFile ...) -> hashFile

76d9127

Co-authored-by: Winter <[email protected]>

clearer comment

0b898d9

Co-authored-by: Winter <[email protected]>

service -> services

2b8f414

Fix integration test

3d61ccc

winterqt and others added 3 commits June 1, 2022 13:49

Add update actions to integration test

4e139eb

fix nix flake check

5a98964

overhaul on change actions

34f637d

compare the decrypted secrets, and make use of /run/nixos/activation-{restart,reload}-list

Radvendii force-pushed the main branch from 90d3345 to 34f637d Compare June 1, 2022 17:53

upgrade nixpkgs to 22.05 so systemdUtils exists

5c721fa

ryantm requested changes Jun 2, 2022

View reviewed changes

modules/age.nix Outdated Show resolved Hide resolved

test/integration.nix Outdated Show resolved Hide resolved

modules/age.nix Outdated Show resolved Hide resolved

test {reload,restart}Units

a4b8a9a

fix case of path changes when symlink = false and expand testing

7a303a3

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add age.secrets.*.{action,service} #87

add age.secrets.*.{action,service} #87

Radvendii commented Dec 29, 2021 •

edited

Loading

winterqt commented Jan 8, 2022

Radvendii commented Jan 12, 2022 •

edited

Loading

winterqt commented Jan 12, 2022

Radvendii commented Jan 13, 2022

Radvendii commented Jan 13, 2022

winterqt commented Jan 13, 2022 •

edited

Loading

Radvendii commented Jan 18, 2022

ryantm commented Jan 18, 2022

Radvendii commented Jan 18, 2022

Radvendii commented Jan 18, 2022

Radvendii commented Jan 23, 2022

winterqt commented Jan 23, 2022

Radvendii commented Jan 24, 2022

Radvendii commented Jan 25, 2022

Radvendii commented Feb 4, 2022

infinisil Mar 24, 2022

Radvendii Mar 25, 2022

Radvendii Mar 25, 2022

infinisil Mar 25, 2022

Radvendii Mar 25, 2022

infinisil commented Apr 27, 2022

Radvendii commented Jun 1, 2022 •

edited

Loading

Radvendii commented Jun 2, 2022

ryantm left a comment

Radvendii commented Jun 8, 2022

Radvendii commented Jun 13, 2022

Radvendii commented Jul 20, 2022

Radvendii commented Nov 2, 2022

winterqt commented Aug 7, 2023

winterqt commented Aug 7, 2023

add age.secrets.*.{action,service} #87

Are you sure you want to change the base?

add age.secrets.*.{action,service} #87

Conversation

Radvendii commented Dec 29, 2021 • edited Loading

winterqt commented Jan 8, 2022

Radvendii commented Jan 12, 2022 • edited Loading

winterqt commented Jan 12, 2022

Radvendii commented Jan 13, 2022

Radvendii commented Jan 13, 2022

winterqt commented Jan 13, 2022 • edited Loading

Radvendii commented Jan 18, 2022

ryantm commented Jan 18, 2022

Radvendii commented Jan 18, 2022

Radvendii commented Jan 18, 2022

Radvendii commented Jan 23, 2022

winterqt commented Jan 23, 2022

Radvendii commented Jan 24, 2022

Radvendii commented Jan 25, 2022

Radvendii commented Feb 4, 2022

infinisil Mar 24, 2022

Choose a reason for hiding this comment

Radvendii Mar 25, 2022

Choose a reason for hiding this comment

Radvendii Mar 25, 2022

Choose a reason for hiding this comment

infinisil Mar 25, 2022

Choose a reason for hiding this comment

Radvendii Mar 25, 2022

Choose a reason for hiding this comment

infinisil commented Apr 27, 2022

Radvendii commented Jun 1, 2022 • edited Loading

Radvendii commented Jun 2, 2022

ryantm left a comment

Choose a reason for hiding this comment

Radvendii commented Jun 8, 2022

Radvendii commented Jun 13, 2022

Radvendii commented Jul 20, 2022

Radvendii commented Nov 2, 2022

winterqt commented Aug 7, 2023

winterqt commented Aug 7, 2023

Radvendii commented Dec 29, 2021 •

edited

Loading

Radvendii commented Jan 12, 2022 •

edited

Loading

winterqt commented Jan 13, 2022 •

edited

Loading

Radvendii commented Jun 1, 2022 •

edited

Loading