Skip to content

Do not set DEFAULT_DECRYPT in function edit #292

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

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

Conversation

ck3d
Copy link

@ck3d ck3d commented Oct 13, 2024

Edit is called multiple times during rekey

fixes #272

@witchof0x20
Copy link

Can confirm this works.

Did
nix run github:ck3d/agenix/fix-272#agenix -- -v

since I was also running into this bug

@jhillyerd
Copy link

Also ran into this issue after upgrading to nixos 24.11. nix run github:ck3d/agenix/fix-272#agenix -- -r worked for me.

@jakubgs
Copy link

jakubgs commented May 14, 2025

Can someone please review this? @n8henrie is see you reviewed last merged PR. Would appreciate some feedback at least.

Copy link
Collaborator

@n8henrie n8henrie left a comment

Choose a reason for hiding this comment

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

Hi -- sorry this is brief. Can you explain why this is your recommended fix instead of testing for the -o option already being set?

@ck3d
Copy link
Author

ck3d commented May 18, 2025

The output is different for each edit call.

@jhillyerd
Copy link

@n8henrie the way I'd review this:

  1. It is a bug that the edit function changes DEFAULT_DECRYPT, it's meant to be used as a template by the decrypt function and really shouldn't be modified after being initialized.
  2. edit is called repeatedly by rekey, which is what manifested the bug

That's why it doesn't make sense to check for an existing -o

@jhillyerd
Copy link

@ck3d can you rebase this PR?

@n8henrie
Copy link
Collaborator

Thanks for elaborating.

I agree, looking at the whole context a bit more, it seems like the age tool is not really designed to work on more than one file at a time, so looping while modifying global variables is a setup for trouble.

I am super tempted to just refactor the script to a use local variables, readonly globals, and a more functional style.

This should also get a regression test (please), ideally in a separate commit that currently fails.

Thanks to all for input!

Edit is called multiple times during rekey

fixes ryantm#272
@ck3d
Copy link
Author

ck3d commented May 22, 2025

PR rebased and ready to merge

Copy link
Collaborator

@n8henrie n8henrie left a comment

Choose a reason for hiding this comment

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

Please add a regression test, ideally in a separate commit that currently fails, and is subsequently fixed by the PR.

@ck3d
Copy link
Author

ck3d commented May 22, 2025

I know how important tests are, but I can not spend time on writing a regression test.

@jhillyerd
Copy link

I will try writing a test this weekend.

@n8henrie
Copy link
Collaborator

I think we determined between #330 and #272 that this is only an issue if one has decided to pin to an outdated version of agenix or is using an alternate implementation like rage, so I think this can be closed. Will leave open for a little while longer to see if anyone on the thread is experiencing issues here on main without rage; if so, a regression test would be appreciated.

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.

Rekey loop adds multiple -o flags
5 participants