Skip to content

Conversation

@felixscheinost
Copy link
Contributor

@felixscheinost felixscheinost commented Oct 25, 2024

Currently rekey re-encrypts all files.

For my personal use-case, agenix would ideally only files that require rekeying, i.e. files where the identities changed. But I don’t think there’s an (easy) way to achieve that with age currently, as there’s no way to get the current recipients from an encrypted file?

This change would allow the user to manually specifiy that only secrets that contain a given identity should be rekeyed.

In my use-case this is handy as when I add a new server I want all secrets that are shared between servers (where the new identity was added) to be rekeyed, but I don’t want all secrets that are personal to different servers to also be rekeyed.

The syntax currently isn't great, but works for me now. Am open to implement improvements here.

Example:

agenix --rekey 'ssh-ed25519 AA... root@some-host'

Are you in general open to this feature? This PR is more of a draft but "works".

Currently rekey re-encrypts all files.

For my personal use-case, agenix would ideally only files that require rekeying, i.e. files where the identities changed.
But I don’t think there’s an (easy) way to achieve that with `age` currently, as there’s no way to get the current recipients from an encrypted file?

This change would allow the user to manually specifiy that only secrets that contain a given identity should be rekeyed.

In my use-case this is handy as when I add a new server I want all secrets that are shared between servers (where the new identity was added) to be rekeyed, but I don’t want all secrets that are personal to different servers to also be rekeyed.
# shellcheck disable=SC2016
echo '-e, --edit FILE edits FILE using $EDITOR'
echo '-r, --rekey re-encrypts all secrets with specified recipients'
echo '-r, --rekey [PUBLIC_KEY] re-encrypts all secrets with specified recipients'
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure if this makes sense here.

It makes parsing harder e.g. --rekey -v is currently broken.

Choose a reason for hiding this comment

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

If you choose to take the other changes from this review:

Suggested change
echo '-r, --rekey [PUBLIC_KEY] re-encrypts all secrets with specified recipients'
echo '-r, --rekey [PUBLIC_KEY...] re-encrypts all secrets with specified recipients'

However that extends the length beyond the current space available so the entire thing needs fresh spacing.

;;
-r|--rekey)
shift
if test $# -gt 0; then
Copy link
Contributor Author

Choose a reason for hiding this comment

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

TODO: Fix e.g. --rekey -v

Choose a reason for hiding this comment

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

I could just keep the REKEY=1 below and not add anything here, instead handling the arguments in the * case.

Basically:

  -r|--rekey)
    shift
    REKEY=1
    ;;
# […]
  -*)
    show_help
    exit 1
  *)
    if test 1 = "$REKEY"; then
      REKEY_PUBLIC_KEY+=( "$1" )
      shift
    else
      show_help
      exit 1
    fi
    ;;

FILTER_EXPRESSION="true";
fi

RULES_EXPRESSION=$(cat <<EOF
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Does using a here document make sense here? I tried fitting it all in the single line but that isn't pretty.

Copy link

@benaryorg benaryorg left a comment

Choose a reason for hiding this comment

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

I see one issue with the PR as it is right now; it adds no value over running env EDITOR=: agenix --edit $identity, unless I'm somehow mistaken.

I do think providing some benefit would be nice, hence allowing multiple arguments seems like a solid choice.

test $# -eq 0 && (show_help && exit 1)

REKEY=0
REKEY_PUBLIC_KEY=

Choose a reason for hiding this comment

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

Since the shebang is already bash it makes perfect sense to use an array here to be able to provide multiple keys IMHO.

Suggested change
REKEY_PUBLIC_KEY=
REKEY_PUBLIC_KEY=()

nit (outside the scope of this PR tho): these are variables, not environment variables, they shouldn't be uppercase

;;
-r|--rekey)
shift
if test $# -gt 0; then

Choose a reason for hiding this comment

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

I could just keep the REKEY=1 below and not add anything here, instead handling the arguments in the * case.

Basically:

  -r|--rekey)
    shift
    REKEY=1
    ;;
# […]
  -*)
    show_help
    exit 1
  *)
    if test 1 = "$REKEY"; then
      REKEY_PUBLIC_KEY+=( "$1" )
      shift
    else
      show_help
      exit 1
    fi
    ;;

Comment on lines +197 to +212
if test ! -z "$REKEY_PUBLIC_KEY"; then
FILTER_EXPRESSION="builtins.elem \"$REKEY_PUBLIC_KEY\" rules.\${file}.publicKeys";
else
FILTER_EXPRESSION="true";
fi

RULES_EXPRESSION=$(cat <<EOF
let
rules = import $RULES;
filter = file: $FILTER_EXPRESSION;
in
builtins.filter filter (builtins.attrNames rules)
EOF
)

FILES=$( (@nixInstantiate@ --json --eval -E "$RULES_EXPRESSION" | @jqBin@ -r .[]) || exit 1)
Copy link

@benaryorg benaryorg Mar 28, 2025

Choose a reason for hiding this comment

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

With the other changes from my review this should use a loop over the keys.
Besides, using wordsplit for iteration is bad, this should use a loop (which is outside the code changes of this PR but not outside the scope IMHO). Otherwise having a space in any filename is a recipe for chaos.

Suggested change
if test ! -z "$REKEY_PUBLIC_KEY"; then
FILTER_EXPRESSION="builtins.elem \"$REKEY_PUBLIC_KEY\" rules.\${file}.publicKeys";
else
FILTER_EXPRESSION="true";
fi
RULES_EXPRESSION=$(cat <<EOF
let
rules = import $RULES;
filter = file: $FILTER_EXPRESSION;
in
builtins.filter filter (builtins.attrNames rules)
EOF
)
FILES=$( (@nixInstantiate@ --json --eval -E "$RULES_EXPRESSION" | @jqBin@ -r .[]) || exit 1)
if test "${#REKEY_PUBLIC_KEY[@]}" -gt 0; then
FILTER_EXPRESSION="let keys = [ $(printf \"%s\"\\n "${REKEY_PUBLIC_KEY[@]}" | paste -sd " ") ]; in builtins.any (key: builtins.elem key keys) rules.\${file}.publicKeys;
else
FILTER_EXPRESSION="true";
fi
RULES_EXPRESSION=$(cat <<EOF
let
rules = import $RULES;
filter = file: $FILTER_EXPRESSION;
in
builtins.filter filter (builtins.attrNames rules)
EOF
)
FILES=()
while read -r file; do
FILES+=( "$file" )
done < <(( @nixInstantiate@ --json --eval -E "$RULES_EXPRESSION" | @jqBin@ -r .[]) || exit 1)

Not sure if the extra parens around the instantiate are needed give that pipefail is set.

nit (outside the scope of this PR): someone should clean up the indents in this file, choose: either two or four spaces, or ideally if you want people to be able to use their preferred width (and also aid accessibility for screen readers and such) use hard tabs instead, but choose one of those

# shellcheck disable=SC2016
echo '-e, --edit FILE edits FILE using $EDITOR'
echo '-r, --rekey re-encrypts all secrets with specified recipients'
echo '-r, --rekey [PUBLIC_KEY] re-encrypts all secrets with specified recipients'

Choose a reason for hiding this comment

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

If you choose to take the other changes from this review:

Suggested change
echo '-r, --rekey [PUBLIC_KEY] re-encrypts all secrets with specified recipients'
echo '-r, --rekey [PUBLIC_KEY...] re-encrypts all secrets with specified recipients'

However that extends the length beyond the current space available so the entire thing needs fresh spacing.

@felixscheinost
Copy link
Contributor Author

I see one issue with the PR as it is right now; it adds no value over running env EDITOR=: agenix --edit $identity, unless I'm somehow mistaken.

@benaryorg I must be misunderstanding what you mean because the command you suggested would only update a single file, $identity, defined in secrets.nix, right?

If I substitute $identity for one of the identities actually used in "secrets.nix" I get, as expected, an error:

error: attribute '"ssh-ed25519 [snip]"' missing

       at «string»:1:39:

            1| (let rules = import ./secrets.nix; in rules."ssh-ed25519 [snip]".publicKeys)

This change allows one to rekey all secrets where a given identity is included. I use this when adding a new host that shares a couple of secrets with other hosts. I don't want to need to manually look through which secrets need to be rekeyed and I also don't want to rekey ALL my secrets.

I recently came across https://github.com/oddlama/agenix-rekey, which might also provide this feature? I am planning to look into it.

@benaryorg
Copy link

Addendum to my review:

I had a missing [@] in a place (line 198, comment is edited now).
Due to a limitation of GitHub's review feature I can't seem to comment on lines outside the diff though, because line 200 or so also needs to be changed from for FILE in $FILES to for FILE in "${FILES[@]}".
There may be other bits around the changed code which may need minor adaption.


@benaryorg I must be misunderstanding what you mean because the command you suggested would only update a single file, $identity, defined in secrets.nix, right?

Oooohh, damn. Yes, you are 100% right, passing the identity doesn't do much.
Somehow I mixed up identities with files halfway through, sorry about that.

I recently came across https://github.com/oddlama/agenix-rekey, which might also provide this feature? I am planning to look into it.

It would still be nice to have rekeying in agenix directly.
I for instance do not use agenix-rekey, and instead have secrets.nix import my regular infra repo and calculates the secrets, their paths, and the identities from the configuration itself (i.e. I only add agenix secrets in nix and create/rekey the file).
Today I did however replace one machine with another and ran into the issue of having to rekey a bunch of files, and stumbled upon this PR, and it would absolutely be a great fit in agenix itself.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants