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

Added re-signing of existing signed files. #108

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

Conversation

hlein
Copy link

@hlein hlein commented Oct 12, 2019

When editing an existing file, check if the source file was signed,
and if so, enable signing when we save it.

When writing out a file, moved the "should we sign" check so that
it always happens. When saving an existing file, b:GPGOptions
exists already, so g:GPGPreferSign was not being checked previously.

This partially addresses
#34

If we wanted to support selecting between different private keys to
sign with, then checking which key had signed a file and looking
for a corresponding private key in our keyring could be done in
the --list-packets check. But that is another future issue & PR.

When editing an existing file, check if the source file was signed,
and if so, enable signing when we save it.

When writing out a file, moved the "should we sign" check so that
it always happens.  When saving an existing file, b:GPGOptions
exists already, so g:GPGPreferSign was not being checked previously.

This partially addresses
jamessan#34

If we wanted to support selecting between different private keys to
sign with, then checking _which_ key had signed a file and looking
for a corresponding private key in our keyring could be done in
the --list-packets check.  But that is another future issue & PR.
Copy link
Owner

@jamessan jamessan left a comment

Choose a reason for hiding this comment

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

Thanks for looking into this! The general idea looks right, but I think there are some improvements that can be made.

let output = s:GPGSystem(cmd)

if (matchstr(output, "^:\(onepass_sig\|signature\) packet:") >= 0)
let g:GPGPreferSign = 1
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
let g:GPGPreferSign = 1
call s:GPGDebug(1, 'this file is signed')
let b:GPGOptions += ['sign']

Copy link
Owner

Choose a reason for hiding this comment

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

This would ensure that we only change the option for this buffer, instead of overriding the user's choice for all buffers just because this one file is signed.

Copy link
Author

Choose a reason for hiding this comment

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

Ah, per-buffer sure sounds better than my method.

I think at one or two places I wasn't sure if it was safe to edit b:GPGOptions yet, because gnupg might be invoked for something else prior to encrypt-to-save (i.e. if another gpg -d were done, including --sign would be a usage error).

If you are sure it's safe/appropriate to do there - then +1 for sure!

Copy link
Owner

Choose a reason for hiding this comment

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

I think at one or two places I wasn't sure if it was safe to edit b:GPGOptions yet, because gnupg might be invoked for something else prior to encrypt-to-save

b:GPGOptions is only used as part of the command-line for the encryption phase, so it's safe.

call s:GPGDebug(1, "no options set, so using default options: " . string(b:GPGOptions))
endif

" check if we should sign this file
Copy link
Owner

Choose a reason for hiding this comment

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

This change isn't necessary with the above suggestion.

Copy link
Author

Choose a reason for hiding this comment

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

Well...

As a user, I want a way to say that any file I edit should get signed when I save it, even if the original was not signed. When I read of the g:GPGPreferSign option, that's initially what I expected it to do. Perhaps there should be a g:GPGAlwaysSign that behaves that way? (In fact I think I started to add a g:GPGAlwaysSign, then thought it was redundant. But maybe not?)

Copy link
Owner

Choose a reason for hiding this comment

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

As a user, I want a way to say that any file I edit should get signed when I save it, even if the original was not signed.

Then you probably want to add something like this to your vimrc:

autocmd User GnuPG if index(b:GPGOptions, 'sign') == -1 | call add(b:GPGOptions, 'sign') | endif

Copy link
Author

Choose a reason for hiding this comment

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

Then you probably want to add something like this to your vimrc:

Oh, nice, I will try that! (And sorry this has veered into stupid user questions ;)

@jamessan
Copy link
Owner

Do you intend to update the PR (and preferable GPG sign the commit) or do you want me to just make the final changes?

@hlein
Copy link
Author

hlein commented Oct 26, 2019

Do you intend to update the PR (and preferable GPG sign the commit) or do you want me to just make the final changes?

Oh, please go ahead and make the changes if you have the time. I was muddling my way through the code, you will do it right the first time ;)

Thanks!


" determine if the file was signed; if so, we will too
let cmd = { 'level': 3 }
let cmd.args = '--list-packets ' . s:shellescape(filename, { 'cygpath': 1 })
Copy link
Owner

Choose a reason for hiding this comment

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

I just noticed that the man page describes this option as such (emphasis added):

List only the sequence of packets. This command is only useful for debugging. When used with option --verbose the actual MPI values are dumped and not only their lengths. Note that the output of this command may change with new releases.

It'd be nice if there was a more stable way to get this information.

Copy link
Author

Choose a reason for hiding this comment

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

Ugh, yes that is unfortunate.

Hm, often "use --status-fd" is recommended. So as a quick test:

$ gpg --status-fd 3 -d test.pgp 3>&1 1>/dev/null 2>/dev/null | awk '/^\[GNUPG:\] VALIDSIG /{print $3}'
96063BF9B5934CBCE31AA3846200F6E3781E3DD7

I don't know how well that redirection would play with s:GPGSystem, might need a separate helper for it.

Copy link
Owner

Choose a reason for hiding this comment

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

This sounds like more motivation for me to finally move to using jobs/channels, loopback pinentry, etc. so I can get more fine-grained information out of gpg, as well as supporting nvim.

@jamessan jamessan changed the base branch from master to main November 8, 2020 02:52
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.

2 participants