Import and Export for python bindings
Closed, ResolvedPublic

Description

It would be nice if the python Context object had import and export functions.
I've thus attached a patch based on this one submitted a while ago to gnupg-devel by Tobias Mueller:
https://lists.gnupg.org/pipermail/gnupg-devel/2017-August/033031.html

It's pretty much exactly the same, I just updated export to use the new read helper function.

Commit message from the original patch:

python: Add more python import and export functions.

  • lang/python/gpg/core.py (Context): Added import_ and export
  • lang/python/tests/t-export.py: New test for export function
  • lang/python/tests/t-import.py: New test for import function
  • lang/python/tests/t-op-export.py: Renamed from t-export.py
  • lang/python/tests/t-op-import.py: Renamed from t-import.py

    --

gpgme's op_import and op_export functions return an error code rather
than a result. In Python, exceptions are thrown if any of those
functions returned an error code. This change gives a more natural
import and export function which return the result object or throw on
error.

Signed-off-by: Tobias Mueller <muelli at cryptobitch.de>

tookmund created this task.May 29 2018, 5:08 PM
tookmund updated the task description. (Show Details)May 29 2018, 9:36 PM
tookmund updated the task description. (Show Details)
BenM claimed this task.Jun 2 2018, 3:26 AM
BenM added a subscriber: BenM.

Actually op_import and op_export do work, but they're the underlying SWIG bindings, not the more pythonic layer Justus added a couple of years ago. I'd been planning on fixing that this month (part of the work is in one of the ben/howto-update branches), but not merged with master until it could be documented since there's something potentially hazardous in there (exporting secret keys).

Anyway, I'll have a look at the patch and see how alike our thinking is.

BenM added a comment.Jun 2 2018, 4:00 AM

Okay, the import is pretty much a match for what I have tucked away elsewhere, to that will probably get merged as is, more or less.

The export, however, won't be. Firstly, the eclectic modes are in there, but they're buried quite deeply. Secondly, there are a number of them with significant potential results (including secret key exports) and I want to separate those two into different functions (so users don't accidentally export the secret key when they didn't absolutely mean it.

There are two ways to address that sort of thing: either we expect them to memorise a series of obscure modes which even some developers find it troublesome to isolate, or we separate out the potentially dangerous stuff to avoid someone having a little catastrophe down the line. I'm opting for the latter. ;)

That makes sense. If you don't have any other patches floating around for this, would you mind if I took a crack at rewriting export?

Something like a set of methods, export_public and export_private ?

I'm not sure if it would need to still take an option for the other modes, maybe we just expect those that want more control over what's exported (minimal, etc) to use the op_export function?

BenM added a comment.EditedJun 4 2018, 8:45 PM

Not for export, there's a few traps in there, but if you want to take a second swing at import, I'd probably accept that instead.

I'd expect to see the function named key_import, then returning the result produced by either self.op_import_result() or by an error message. Ideally we want to document what sort of things will be found in op_import_result() under a variety of circumstances, but we can go through that later if you need to.

Edit: we can skip checking the content to confirm it's bytes, there's enough docs throughout the rest of the bindings indicating it should be and developers should be doing that themselves. We do, however, still need a check that will catch relevant errors and a check that won't die horribly if no key data is found.

werner triaged this task as Normal priority.Jun 6 2018, 5:57 PM

Apologies for the delay, been working on GSoC stuff.
Here's what I've got as of right now:

I've tested throwing some random data at GPGME and it seems like it handles it just fine, setting considered to 0 in the result.
I added an ValueError to be raised if no keys are considered, since that's probably not what the user wanted.
I looked at the currently existing GPGMEErrors but ValueError seemed like the most appropriate option.
I also added a test for it, but I'm not really sure how to run the tests so I've haven't actually tested the test.
I did build GPGME and manually test it, however, and it seems to work.

BenM added a comment.Jun 17 2018, 6:43 AM

Not to worry, we've all been pretty busy of late.

Okay, so I'm accepting the patch, but I'll be wrapping it in something
else shortly thereafter.

My additions, however, are the etremely boring (but necessary) part:
trying to catch at least the most common errors which may be matched
up to existing status codes in GPGME. It should match, but it is not
necessary for most developers to know what they are by heart or by
rote; part of the purpose of the pythonic layer is to remove that
complexity from direct access.

BenM closed this task as Resolved.Jun 17 2018, 8:10 AM

Patch committed to master in commit 5a80e755008bbb3f4c7f91ffccd38f26cd8b3960

The two subsequent commits are the one I mentioned above (nested try/except statements) and followed by a major PEP8 compliance overhaul of core.py.

Thanks for the patch and welcome to the weird and wonderful world of FOSS. :)

tookmund added a comment.EditedJun 18 2018, 7:42 PM

On 06/17/2018 02:10 AM, BenM (Ben McGinnes) wrote:

The two subsequent commits are the one I mentioned above (nested try/except
statements) and followed by a major PEP8 compliance overhaul of core.py.

Thanks for the patch and welcome to the weird and wonderful world of FOSS. :)

Thank you for fixing it up to use proper status codes.
Glad I could help improve GPGME, even in a very small way.