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

add -d/--decrypt option to decrypt a file to stdout #158

Merged
merged 1 commit into from
Feb 23, 2023

Conversation

whentze
Copy link
Contributor

@whentze whentze commented Feb 21, 2023

See #154 (comment) for some discussion.

The implementation is not super pretty, but works. Feel free to pick on the style.
I also replaced one of the EDITOR=cat invocations in the integration test with usage of this option.

Speaking of which, that test takes forever to run for me, it seems to wait for 5 minutes for sshd to time out. Is that expected? Maybe something related to missing entropy?

@ryantm
Copy link
Owner

ryantm commented Feb 21, 2023

Speaking of which, that test takes forever to run for me, it seems to wait for 5 minutes for sshd to time out. Is that expected? Maybe something related to missing entropy?

$ time nix flake check

real  0m13.916s
user  0m10.053s
sys 0m2.026s

Copy link
Owner

@ryantm ryantm left a comment

Choose a reason for hiding this comment

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

looks reasonable to me, thanks!

Copy link
Owner

@ryantm ryantm left a comment

Choose a reason for hiding this comment

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

There is a subtle problem with this! It leaves the CLEARTEXT_DIR around after the program exits. We should fix this because it is a security issue in my opinion.

@n8henrie
Copy link
Collaborator

@ryantm I have been wondering / worrying about this possibility (leaving behind a decrypted temp file) as complexity grows.

What would you think about adding a trap "rm -f \"$TEMPFILE\"" EXIT or equivalent right after the creation of the tempfile, as a little extra insurance?

@ryantm
Copy link
Owner

ryantm commented Feb 21, 2023

We should add tests for it.

@whentze
Copy link
Contributor Author

whentze commented Feb 21, 2023

Oh damn, good catch! Will fix.

@n8henrie
Copy link
Collaborator

This PR adds a bash-style [[ test, as opposed to the POSIX-style that has been exclusively used so far.

@ryantm Do you have a strong preference for keeping the POSIX-style? I don't imagine nix will move away from bash anytime soon, and going to the former is a little easier to use in some cases and would quiet another shellcheck warning (that I silenced in #160, I don't think this lint is enabled by default).

@ryantm
Copy link
Owner

ryantm commented Feb 21, 2023

I'm okay with using Bash style tests if needed.

@whentze
Copy link
Contributor Author

whentze commented Feb 22, 2023

@n8henrie I didn't add the bash-style tests, only moved them around. They're already on main: https://github.com/ryantm/agenix/blob/main/pkgs/agenix.sh#L120

@whentze
Copy link
Contributor Author

whentze commented Feb 22, 2023

We should add tests for it.

Done, and as you said, the secret indeed stays around. Next up, I will make that test pass :)

@whentze
Copy link
Contributor Author

whentze commented Feb 22, 2023

Done, and the test passes now.

@ryantm ryantm force-pushed the decrypt-only branch 2 times, most recently from a5ec292 to 97c82fc Compare February 23, 2023 03:19
Copy link
Owner

@ryantm ryantm left a comment

Choose a reason for hiding this comment

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

looks good now! I rebased it on main, squashed it down to one commit, copied the CLI help output to the readme, changed the capitalization of the help output slightly

@ryantm ryantm merged commit c2a71c8 into ryantm:main Feb 23, 2023
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