这是indexloc提供的服务,不要输入任何密码
Skip to content

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.

@ryantm
Copy link
Owner

ryantm commented Aug 4, 2025

Feel free to reopen if we add a regression test.

@ryantm ryantm closed this Aug 4, 2025
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

6 participants