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

Fixing Windows issues #206

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

Conversation

instantname
Copy link
Contributor

Here are a few fixes that make python-gnupg work on Windows.

The main change is about paths: the strategy in this PR is to escape the paths as late as possible in the code.

According to my tests, this solves #199 and #204 on Python 2 & 3.

Below is a summary of the tests results. On Windows, since the GPG class could not be instantiated before, the tests are necessarily better... On Debian, some tests are already failing on the master branch and many even make the process hang (example with "test_gnupg.py crypt" in note [11] of attached file). When removing the ones that make the tests hang, I saw no notable difference between the tests on the master branch and this pull request.

Tests summary (see attached log for the numbered notes):

Python 2.7.13, GnuPG 1.4.22, this pull request (ea96545):

  • parsers: OK
  • encodings: OK
  • basic: OK
  • genkey: OK
  • sign: 2 fails out of 11 [1]
  • crypt: 7 fails, 1 error out of 18 [2]
  • listkeys: OK
  • keyrings: OK
  • recvkeys: OK
  • revokekey: 2 errors out of 2 [3]
  • expiration: hangs [4]
  • signing: OK

Python 3.6, GnuPG 1.4.22, this pull request (ea96545):

Debian Jessie, Python 2.7, master (705d7f4):

  • parsers: 1 fail out of 6 (similar to [5])
  • encodings: OK
  • basic: OK
  • genkey: 7 hangs out of 10
  • sign: 7 hangs out of 11
  • crypt: 7 hangs, 7 fail out of 18 ([10], pretty close to [2])
  • listkeys: 1 hangs out of 1
  • keyrings: 1 hangs out of 7
  • recvkeys: 1 hangs out of 1
  • revokekey: 2 errors out of 2 (similar to [3])
  • expiration: 4 hangs out of 4
  • signing: 4 hangs out of 4

Debian Jessie, Python 2.7, this pull request (ea96545), excluding the tests that hanged above:

  • parsers: OK
  • encodings: OK
  • basic: OK
  • genkey: OK (excludes 7 tests)
  • sign: OK (excludes 7 tests)
  • crypt: 7 fail out of 11 (excludes 7 tests, similar to [10])
  • listkeys: OK (all tests excluded)
  • keyrings: OK (1 test excluded)
  • recvkeys: OK (all tests excluded)
  • revokekey: 2 errors out of 2 (similar to [3])
  • expiration: OK (all tests excluded)
  • signing: OK (all tests excluded)

pr1_log.txt

Only use _fix_unsafe() at the very last possible time.
Python 2 -> 3 broke path discovery.
@isislovecruft
Copy link
Owner

Hi @instantname! Thanks for the patches, and my apologies for taking so long to get to this.

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.

I would prefer it if we could instead find a way to use a shell input sanitisation method which doesn't break things on Windows. Maybe checkout what the shlex module does on python3 a Windows machine? (Not sure if that'll tell you anything useful, it's just an idea.)

@@ -419,28 +418,27 @@ def _homedir_setter(self, directory):
% _util._conf)
directory = _util._conf

hd = _parsers._fix_unsafe(directory)
Copy link
Owner

Choose a reason for hiding this comment

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

I'd strongly prefer to fix unsafe strings as early as possible to prevent potential problems while working with them later. (For example, this is part of what makes this library not vulnerable to SigSpoof.)

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