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

Allow password protected private keys. #14

Merged
merged 23 commits into from
Feb 9, 2025
Merged

Allow password protected private keys. #14

merged 23 commits into from
Feb 9, 2025

Conversation

RomeoV
Copy link
Contributor

@RomeoV RomeoV commented Aug 6, 2024

Currently, password protected private keys or identity files are not supported, because when age is run and prompts for the password, agenix.el currently can't forward the prompt to the user.
Note also that age doesn't support an ssh-agent.

I first tried an approach that catches the password prompt and forwards it to the user, but that was a big mess.
Instead I have now implemented the following logic.

In agenix--process-exit-code-and-output, first find the identity file from the args and check whether it exists, is readable, and then whether it is password protected.
If it is not password protected, proceed as before. However, if it is password protected, prompt the user for the password. (This prompt is therefore not coming from age but from this package.)

Then, copy the private key into a tmpfile and remove the password from the private key in this tmpfile.
Then pass this tmpfile as the identity file and remove it again.

@t4ccer
Copy link
Owner

t4ccer commented Aug 7, 2024

Thanks for contributing! It works fine with one key, but there are some edge cases that it does not handle correctly. If you have secret.age file encrypted with key1 only but your agenix-key-files is '(key2 key1) it'll prompt you for password for key2 only and then fail to decrypt secret.age as key1 is still not substituted for temp file without password.

How about moving handling password from agenix--process-exit-code-and-output to a place where we append --identity flags in agenix-decrypt-buffer and prompt for password there as we iterate through keys?

RomeoV added 2 commits August 7, 2024 11:32
Before we just appended all keys in `agenix-key-files`.
The problem is that when they are password protected, emacs has to
prompt for all the passwords. So instead we let the user pick which one
to use.
@RomeoV
Copy link
Contributor Author

RomeoV commented Aug 7, 2024

Thanks for your swift review. I looked a bit more into the logic before my change and agree with your suggestion.
I have undone the changes to agenix--process-exit-code-and-output and instead prompt the user now for which key to use.
Then the code checks whether we need a password and proceeds roughly as before.

One change is now that the user always gets prompted (whereas before the code could try out all private keys). I am not sure if there is an elegant way to circumvent this. Perhaps we could pass a flag or a package local variable whether to prompt (which would be necessary for password protected keys) or just to try all the keys (which would run into the problem you described above, but would still work fine if the first key that works is not protected).

But imo the workflow now is also very convenient, even if the user is prompted every time.

RomeoV added 3 commits August 7, 2024 13:15
This makes it consistent with `temp-identity-path`.
Now we should be handling the case of opening a fresh file better.
@RomeoV
Copy link
Contributor Author

RomeoV commented Aug 7, 2024

I'll push another couple of commits later that fixes some bugs.

@t4ccer
Copy link
Owner

t4ccer commented Aug 7, 2024

I'm not sure how to solve that ideally honestly. Having to specify a key file is a bit of UX hit for users that don't use passphases at all (not longer "transparent" after all), but also asking for passphase for all keys in agenix-key-files may not be perfect.

I thought about being a bit more clever about selecting key list but doesn't seem that trivial. age binary doesn't expose any utils to list public keys used to encrypt the file to select only relevant decrypting key. age file format is not that complex but maybe lets wait with parsing it with elisp (but that's an option).

Another option would be to continue passing all key files to age as we do now and somehow handle interactions with running process to allow user to just type the passphase directly there, maybe something using utils from comint.el but I have absolutely no clue how to even approach that.

RomeoV added 2 commits August 7, 2024 19:49
…ties

We move out the responsibility of removing the password from keys
outwards and prepate to only take cleartext paths.
@RomeoV
Copy link
Contributor Author

RomeoV commented Aug 7, 2024

I have refactored it now to this approach: If all key files are not password protected, just proceed as before. But if any of the key files are protected, then prompt the user and call all the required features.
This should maintain the current UX for users not using password protected keys, but enables using password protected files regardless.

I also tried to keep the code somewhat clean, but the complexity definitely did increase. Feel free to comment
on any of the elisp code, I am far from an elisp expert.

Another option would be to continue passing all key files to age as we do now and somehow handle interactions with running process to allow user to just type the passphase directly there, maybe something using utils from comint.el but I have absolutely no clue how to even approach that.

This was actually the approach I initially tried, see e.g. https://github.com/romeov/agenix.el/blob/password-try-one/agenix.el. But it turned out to be a huge mess, so I went with the current way, creating a temporary cleartext key and processing that one.

@pawsen
Copy link

pawsen commented Feb 4, 2025

Any luck this can be merged?

@t4ccer
Copy link
Owner

t4ccer commented Feb 8, 2025

Sorry, I'm quite busy recently. I'll take a look this weekend.

@t4ccer
Copy link
Owner

t4ccer commented Feb 9, 2025

Alright lets go with it. It feels a bit hacky but that's usual when wrapping a CLI program. I was experimenting a bit with native modules and depending on age-the-library to prompt for password input when age actually thinks it's necessary in #15 and https://github.com/t4ccer/elrage but distribution system for emacs packages with native modules is just not there yet. This patch in its current state does not introduce any friction for me (and others that do not have password protected key files) so that's good enough for me if the community sees a value in having that.

Thank you for the contribution 🎉

@t4ccer t4ccer merged commit a12ded8 into t4ccer:main Feb 9, 2025
12 checks passed
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.

3 participants