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

Remove all the imported pubkeys from keyring #90

Merged
merged 1 commit into from
Feb 24, 2020

Conversation

hanazuki
Copy link
Contributor

In case multiple GPG public keys are given, the current implementation only removes the first key after use and leaves the others, which will be used to verify subsequent downloads (insecure). This patch makes sure to remove all of them.

In case multiple GPG public keys are given, the current implementation
only removes the first key after use and leaves the others, which will
be used to verify subsequent downloads (insecure).
This patch makes sure to remove all of them.
@hanazuki
Copy link
Contributor Author

It might be more robust to every time throw away our used keyring and create a new one.

@flavorjones flavorjones requested a review from larskanis January 16, 2020 13:43
@flavorjones
Copy link
Owner

Thank you for submitting this, and apologies for my slow response!

@larskanis you wrote the keyring code in a91d840, can you take a look at this? We don't have test coverage on it and there seems to be some portability concerns (based on the commit message) that I don't want to break.

Copy link
Collaborator

@larskanis larskanis left a comment

Choose a reason for hiding this comment

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

LGTM!

@larskanis
Copy link
Collaborator

It might be more robust to every time throw away our used keyring and create a new one.

The portability issues mentioned in a91d840 are the reason why the keyring isn't deleted as whole file. But gpg --status-fd is made for batch processing, so that this interface shouldn't change, so that I think our implementation is reasonable safe. It was just buggy in that way that it assumed only one key.

@flavorjones
Copy link
Owner

Thanks for reviewing, @larskanis. And thanks, @hanazuki, for the PR! I'm merging now and will release a new version shortly.

@flavorjones flavorjones merged commit a2cfe25 into flavorjones:master Feb 24, 2020
flavorjones added a commit that referenced this pull request Feb 24, 2020
@flavorjones
Copy link
Owner

v2.5.0 has been released with this change. Thank you again!

@hanazuki
Copy link
Contributor Author

@flavorjones Thank you!
@larskanis Thank you for your review and advice.

@hanazuki hanazuki deleted the multiple-pubkeys branch February 25, 2020 02:28
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