-
Notifications
You must be signed in to change notification settings - Fork 22
overhaul multi-device recipients synchronization #54
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
base: main
Are you sure you want to change the base?
Conversation
320b29b to
9737526
Compare
c3d1b5b to
8528801
Compare
| pw_add() { | ||
| if yn "generate a password?"; then | ||
| pass=$(rand_chars "${PA_LENGTH:-50}" "${PA_PATTERN:-A-Za-z0-9-_}") || | ||
| pass=$(rand_chars "${PA_LENGTH:-50}" "${PA_PATTERN:-A-Za-z0-9-_}") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
enabling pipefail broke this because of SIGPIPE in rand_chars, and because there was no pipefail the errors weren't handled anyway, so it was a bug either way
|
@biox hey jes I'm pretty satisfied with the changes in this PR at this point and I'm curious to see what you think and whether we should go through with it at all. it would be my largest contribution yet, with various tweaks here and there concerning git synchronization and key management, and I think it would greatly improve the experience of setup with multiple computers. I have already used this branch to transfer my actual store to the new PC, and it boiled down to: pa git remote add, pa git pull, pa git push on the new computer, then pa git pull and pa git push on the old computer, and finally pa git pull on the new computer again, and I was ready to go. each computer got its own identity, recipients synchronization and reencryption happened automatically. I'm not sure it gets simpler than this. my main concern here is that rekeying depends on pipefail. without it pa overwrites all passwords with encrypted empty data if decryption fails, and it's expected to fail after cloning the store to a system without suitable identity. I couldn't come up with a better way to prevent it. I could assign the decrypted data to a variable, handle fails, then pass it to age for encryption through heredoc as with pa add, but it seems hacky and pipefail is a much more natural solution here. I don't consider pipefail usable until shellcheck supports it (#32), but it should be resolved soon after the new debian release which is expected to come out next week. |
pa
Outdated
|
|
||
| glob "$command" 'g*' && { | ||
| git "$@" | ||
| git "$@" && change_keys |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not strictly neccessary, but I figured it would be neat to have a consistent usable store after just one git operation. for example, it would allow to use pa-urn to archive the store after pa git pulling without an intermediate command just for rekeying, and the other change_keys is there for non-git synchronization
| # Assign this diff driver to all passwords. | ||
| printf '%s\n' '*.age diff=age' >.gitattributes | ||
| printf '%s\n' "*.age diff=age | ||
| .gitattributes merge=union |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so that repos initialized after the change wouldn't conflict on merge with repos initialized before it
pa
Outdated
| git config user.name >/dev/null || git config user.name pa | ||
| git config user.email >/dev/null || git config user.email "" | ||
| git config user.email >/dev/null || | ||
| git config user.email "${EMAIL:-$(id -u -n)@$(uname -n)}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- username@host would be more useful for git logs with multiple hosts
- I found out that user.name is not required to git, it can be inferred from user.email
pa
Outdated
| cp "$recipients_file" "$recipients_sync" | ||
|
|
||
| # Purge git repository to prevent leaks for unwanted recipients. | ||
| [ -d .git ] && find .git ! -name config -type f -exec rm -f {} + |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
justification: #50 (comment)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
one use case that was handled better with pa-rekey that would be a bit more involved without it is changing the identity. by default, if you run pa-rekey it generates a new key pair and reencrypts all passwords for it, useful for example in case the identity is leaked. in my view integrating a contrib as part of automatic flow is a big win, and it's inconvenient to maintain two tools for key managament (particularly because of stuff like #53), so I would think about how to incorporate that use case into our new rekey logic as another issue
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
rewrote pa-rekey to reflect this
3ce4738 to
ffdc1ce
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
repurposed to only changing the identity, utilizing the added rekeying logic in pa
fae3f83 to
7d84b89
Compare
55fef4e to
a27defa
Compare
b86c94a to
af1e5c9
Compare
6fc8408 to
8eb82ba
Compare
994c125 to
b82999b
Compare
|
@arcxio - sorry for the silence here, i've been watching your updates & waiting for a moment of stasis before diving into review (i'm moving houses rn and haven't had a lick of spare time recently). are you satisfied with the current state of this PR? if so, i'll give it a full review sometime in the next few days. |
97d1c1a to
1d3bb0c
Compare
|
@biox your review will be appreciated any time! there's no rush really, I'm still not sure this can be merged at least until dash will get a release with pipefail, and that won't happen earlier than the end of this year. it's just been my on and off side project, which got bloated over time as I looked for more things to improve with regards to key management. that being said, the main bulk (change_recipients function) I'd say is pretty stable, it was extensively tested and I don't have any ideas on how to significantly improve upon it. as I mentioned it's been really useful for my own store and dramatically improved UX in my view, admittedly at substantial complexity cost - and I'm curious to see if you'd say I made worthwhile tradeoffs here. I've just rebased on main and will try to abstain from refactoring attempts for now, it feels OK to me at the moment. |
|
|
||
| dependencies | ||
| - age | ||
| - age-keygen |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
technically age-keygen is optional now as it can be substituted with plugins below, but since it's expected to be delivered with age I don't think it matters (and it's always used as a default fallback)
1d3bb0c to
168ea4d
Compare
168ea4d to
11ca949
Compare
11ca949 to
92f2c53
Compare
|
hi @arcxio! i'm finally done moving, so i should be able to commit more time to pa again. just wondering what status is here - ready for a review? or should i wait for the aforementioned dash release? |
hey @biox! it's ready. dash release already happened, so pipefail should be available just about everywhere now |
QOL update for multiple computers using the same password store.
recipients are additionally tracked in the password store and can be indexed in a git repository (resolves #50)
rekey is seamlessly integrated and is triggered automatically when recipients change
contrib/pa-rekey is repurposed to rotate identities with a better UX (resolves #53)
identity and recipients files are independently created
Apple's Secure Enclave and TPM support