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 keyserver-options parser and expose ca-cert-file for recv-keys #166

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

dmwilcox
Copy link

Hi! I noticed I couldn't get hkps working without --keyserver-options so I tacked it through. Please take a look and let me know what I can do to better make the code fit with the rest of the codebase and be committed upstream.

Thank you for such a lovely library, cheers!

ps: I've thought it would be good to warn about trying to use gpg v1 with '--keyserver-options' (which doesn't seem to work) or using 'ca-cert-file', and others deprecated and relegated to dirmngr in gpg v2.1. Suggestions on how to handle each would be much appreciated.

@dmwilcox
Copy link
Author

Hello, any thoughts on if we could get this in the mainline? And if it needs any additional work. Thank you!

@isislovecruft isislovecruft self-requested a review July 25, 2018 23:40
@isislovecruft
Copy link
Owner

Hi @dmwilcox! Thanks for the patches! I'm sorry it's taken me so long to get to them.

Copy link
Owner

@isislovecruft isislovecruft left a comment

Choose a reason for hiding this comment

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

Look good to me. I can fix up the tiny complaints I had.

:rtype: :obj:`str` or :obj:`None`
:returns: A string of the keyserver option or None.
"""
def _is_valid_file(option_value):
Copy link
Owner

Choose a reason for hiding this comment

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

You probably want to use gnupg._util._is_file() here.

Copy link
Author

Choose a reason for hiding this comment

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

Fixing.

def _check_keyserver_option(ks_option):
"""Check that the provided keyserver option is valid and safe.

:param str ks_option: A valid argument to --keyserver-option.
Copy link
Owner

Choose a reason for hiding this comment

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

It looks like you renamed the parameter to option_value?

Copy link
Owner

Choose a reason for hiding this comment

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

Oh, nope, I'm sorry. I appear to be used to C and Rust where the docstrings go above the function they document. Carry on!

Copy link
Author

Choose a reason for hiding this comment

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

Thanks, no worries! I've solidly moved into Go these days so who knows what I'm going to write in during this merge :)

gnupg/gnupg.py Outdated
>>> ca_cert = '/home/user/hkps.pool.sks-keyservers.netCA.pem'
>>> gpg.recv_keys('6F682D87',
keyserver=ssl_keyserver,
keyserver_certs=ca_cert)
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 we'll need >>> on these two lines to be able to run them as a doctest.

Copy link
Author

Choose a reason for hiding this comment

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

Adding.

@dmwilcox
Copy link
Author

@isislovecruft

It looks like I messed up Python3 support somewhere along the way -- I'll take a more thorough look tomorrow and report back. Also if we were to write tests for these gpg2 features we'd need gpg2 in Travis -- which I haven't used or looked into yet so that's something to think about.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants