From 2491e6f92f5b562cbd6f7f931df630cb106f6688 Mon Sep 17 00:00:00 2001 From: Daniel Kahn Gillmor Date: Wed, 28 Nov 2018 01:22:13 -0500 Subject: [PATCH 1/6] python: simplify Context.decrypt() In the course of trying to address https://dev.gnupg.org/T4271, i discovered that gpg.Context.decrypt() has a bit of superfluous code. This changeset is intended to simplify the code without making any functional changes. Signed-off-by: Daniel Kahn Gillmor --- lang/python/src/core.py | 29 ++--------------------------- 1 file changed, 2 insertions(+), 27 deletions(-) diff --git a/lang/python/src/core.py b/lang/python/src/core.py index 6e925925..f7e843f1 100644 --- a/lang/python/src/core.py +++ b/lang/python/src/core.py @@ -386,13 +386,9 @@ class Context(GpgmeWrapper): if verify is False: verify = True sink_result = True - else: - pass elif isinstance(verify, list) is True: if len(verify) > 0: verify_sigs = True - else: - pass else: verify = True self.op_decrypt_verify(ciphertext, plaintext) @@ -447,29 +443,8 @@ class Context(GpgmeWrapper): if not ok: missing.append(key) if missing: - try: - raise errors.MissingSignatures(verify_result, missing, - results=results) - except errors.MissingSignatures as e: - raise e - # mse = e - # mserr = "gpg.errors.MissingSignatures:" - # print(mserr, miss_e, "\n") - # # The full details can then be found in mse.results, - # # mse.result, mse.missing if necessary. - # mse_list = [] - # msp = "Missing signatures from: \n".format() - # print(msp) - # for key in mse.missing: - # mse_list.append(key.fpr) - # msl = [] - # msl.append(key.fpr) - # for user in key.uids: - # msl.append(user.name) - # msl.append(user.email) - # # msl.append(user.uid) - # print(" ".join(msl)) - # raise mse + raise errors.MissingSignatures(verify_result, missing, + results=results) return results From 49af6d76e55f348c7b3cece756d6ac643d17ee68 Mon Sep 17 00:00:00 2001 From: Daniel Kahn Gillmor Date: Wed, 28 Nov 2018 01:47:20 -0500 Subject: [PATCH 2/6] python: clarify documentation for verify argument for Context.decrypt() It's easy to miss that verify can take a list of keys. Make it more obvious to the average python dev who reads docstrings. Signed-off-by: Daniel Kahn Gillmor --- lang/python/src/core.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/lang/python/src/core.py b/lang/python/src/core.py index f7e843f1..0b7b7e08 100644 --- a/lang/python/src/core.py +++ b/lang/python/src/core.py @@ -352,7 +352,8 @@ class Context(GpgmeWrapper): Keyword arguments: sink -- write result to sink instead of returning it passphrase -- for symmetric decryption - verify -- check signatures (default True) + verify -- check signatures (boolean or iterable of keys, + see above) (default True) Returns: plaintext -- the decrypted data (or None if sink is given) From b8fa76a30c02afc3d7f6aad0a59bb613d1b711fc Mon Sep 17 00:00:00 2001 From: Daniel Kahn Gillmor Date: Wed, 28 Nov 2018 01:51:24 -0500 Subject: [PATCH 3/6] python: gpg.Context.decrypt verify_sigs and sink_result are bools Both of these function-internal variables are never used for anything other than a binary state. Implement them as the booleans they are. Otherwise, casual readers of the code might think that they're supposed to represent something other than a flag (e.g. "verify_sigs" could mean "the signatures to verify", and "sink_result" could mean "the place where we sink the result"). Signed-Off-By: Daniel Kahn Gillmor --- lang/python/src/core.py | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/lang/python/src/core.py b/lang/python/src/core.py index 0b7b7e08..87cfd6bf 100644 --- a/lang/python/src/core.py +++ b/lang/python/src/core.py @@ -367,8 +367,8 @@ class Context(GpgmeWrapper): GPGMEError -- as signaled by the underlying library """ - sink_result = None - verify_sigs = None + sink_result = False + verify_sigs = False plaintext = sink if sink else Data() if passphrase is not None: @@ -397,7 +397,7 @@ class Context(GpgmeWrapper): self.op_decrypt(ciphertext, plaintext) except errors.GPGMEError as e: result = self.op_decrypt_result() - if verify is not None and sink_result is None: + if verify is not None and not sink_result: verify_result = self.op_verify_result() else: verify_result = None @@ -412,7 +412,7 @@ class Context(GpgmeWrapper): result = self.op_decrypt_result() - if verify is not None and sink_result is None: + if verify is not None and not sink_result: verify_result = self.op_verify_result() else: verify_result = None @@ -428,7 +428,7 @@ class Context(GpgmeWrapper): for s in verify_result.signatures): raise errors.BadSignatures(verify_result, results=results) - if verify_sigs is not None: + if verify_sigs: missing = [] for key in verify: ok = False From 5d8b4f74891af22379899ccee9e8ee849144eee3 Mon Sep 17 00:00:00 2001 From: Daniel Kahn Gillmor Date: Tue, 4 Dec 2018 20:27:15 +0300 Subject: [PATCH 4/6] python: Clarify the meaning of ctx.decrypt(verify=[]) * lang/python/src/core.py (Context.decrypt): docstring clarification of what it means to pass an empty list to the verify argument. Signed-off-by: Daniel Kahn Gillmor --- lang/python/src/core.py | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/lang/python/src/core.py b/lang/python/src/core.py index 87cfd6bf..3bd6f717 100644 --- a/lang/python/src/core.py +++ b/lang/python/src/core.py @@ -342,7 +342,10 @@ class Context(GpgmeWrapper): Decrypt the given ciphertext and verify any signatures. If VERIFY is an iterable of keys, the ciphertext must be signed - by all those keys, otherwise an error is raised. + by all those keys, otherwise an error is raised. Note: if + VERIFY is an empty iterable, that is treated the same as + passing verify=True (that is, do verify signatures, but no + specific keys are required). If the ciphertext is symmetrically encrypted using a passphrase, that passphrase can be given as parameter, using a From 878a0ad01265dba5b06429276bdcc5c21fedb6f5 Mon Sep 17 00:00:00 2001 From: Daniel Kahn Gillmor Date: Tue, 4 Dec 2018 20:29:40 +0300 Subject: [PATCH 5/6] python: ctx.decrypt() has problematic error handling * lang/python/src/core.py (Context.decrypt): document odd error-handling behavior as a potential problem to be addressed. Signed-off-by: Daniel Kahn Gillmor --- lang/python/src/core.py | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/lang/python/src/core.py b/lang/python/src/core.py index 3bd6f717..f3202eb1 100644 --- a/lang/python/src/core.py +++ b/lang/python/src/core.py @@ -427,6 +427,10 @@ class Context(GpgmeWrapper): results=results) if verify: + # FIXME: should we really throw BadSignature, even if + # we've encountered some good signatures? as above, once + # we hit this error, there is no way to accept it and + # continue to process the remaining signatures. if any(s.status != errors.NO_ERROR for s in verify_result.signatures): raise errors.BadSignatures(verify_result, results=results) From 65c28da4e49a8778607fdcf6f51a840166616d9f Mon Sep 17 00:00:00 2001 From: Daniel Kahn Gillmor Date: Tue, 4 Dec 2018 20:44:35 +0300 Subject: [PATCH 6/6] python: overhaul logic of Context.decrypt() * lang/python/src/core.py (Context.decrypt): simplify and clarify the logic behind handling verify=False. * lang/python/tests/t-decrypt.py: ensure that we test verify=False -- The function-internal variables were pretty unclear to the reader, and the logic caused pretty nasty breakage when verify=False. GnuPG-Bug-Id: 4271 Signed-off-by: Daniel Kahn Gillmor --- lang/python/src/core.py | 68 +++++++++++++++++----------------- lang/python/tests/t-decrypt.py | 2 +- 2 files changed, 36 insertions(+), 34 deletions(-) diff --git a/lang/python/src/core.py b/lang/python/src/core.py index f3202eb1..c096ee73 100644 --- a/lang/python/src/core.py +++ b/lang/python/src/core.py @@ -370,8 +370,8 @@ class Context(GpgmeWrapper): GPGMEError -- as signaled by the underlying library """ - sink_result = False - verify_sigs = False + do_sig_verification = False + required_keys = None plaintext = sink if sink else Data() if passphrase is not None: @@ -385,22 +385,25 @@ class Context(GpgmeWrapper): self.set_passphrase_cb(passphrase_cb) try: - if verify is not None: - if isinstance(verify, bool) is True: - if verify is False: - verify = True - sink_result = True - elif isinstance(verify, list) is True: - if len(verify) > 0: - verify_sigs = True - else: - verify = True + if isinstance(verify, bool): + do_sig_verification = verify + elif verify is None: + warnings.warn( + "ctx.decrypt called with verify=None, should be bool or iterable (treating as False).", + category=DeprecationWarning) + do_sig_verification = False + else: + # we hope this is an iterable: + required_keys = verify + do_sig_verification = True + + if do_sig_verification: self.op_decrypt_verify(ciphertext, plaintext) else: self.op_decrypt(ciphertext, plaintext) except errors.GPGMEError as e: result = self.op_decrypt_result() - if verify is not None and not sink_result: + if do_sig_verification: verify_result = self.op_verify_result() else: verify_result = None @@ -415,7 +418,7 @@ class Context(GpgmeWrapper): result = self.op_decrypt_result() - if verify is not None and not sink_result: + if do_sig_verification: verify_result = self.op_verify_result() else: verify_result = None @@ -426,7 +429,7 @@ class Context(GpgmeWrapper): raise errors.UnsupportedAlgorithm(result.unsupported_algorithm, results=results) - if verify: + if do_sig_verification: # FIXME: should we really throw BadSignature, even if # we've encountered some good signatures? as above, once # we hit this error, there is no way to accept it and @@ -434,25 +437,24 @@ class Context(GpgmeWrapper): if any(s.status != errors.NO_ERROR for s in verify_result.signatures): raise errors.BadSignatures(verify_result, results=results) - - if verify_sigs: - missing = [] - for key in verify: - ok = False - for subkey in key.subkeys: - for sig in verify_result.signatures: - if sig.summary & constants.SIGSUM_VALID == 0: - continue - if subkey.can_sign and subkey.fpr == sig.fpr: - ok = True + if required_keys is not None: + missing = [] + for key in required_keys: + ok = False + for subkey in key.subkeys: + for sig in verify_result.signatures: + if sig.summary & constants.SIGSUM_VALID == 0: + continue + if subkey.can_sign and subkey.fpr == sig.fpr: + ok = True break - if ok: - break - if not ok: - missing.append(key) - if missing: - raise errors.MissingSignatures(verify_result, missing, - results=results) + if ok: + break + if not ok: + missing.append(key) + if missing: + raise errors.MissingSignatures(verify_result, missing, + results=results) return results diff --git a/lang/python/tests/t-decrypt.py b/lang/python/tests/t-decrypt.py index 44743da7..c72b51ab 100755 --- a/lang/python/tests/t-decrypt.py +++ b/lang/python/tests/t-decrypt.py @@ -38,7 +38,7 @@ support.print_data(sink) # Idiomatic interface. with gpg.Context() as c: - plaintext, _, _ = c.decrypt(open(support.make_filename("cipher-1.asc"))) + plaintext, _, _ = c.decrypt(open(support.make_filename("cipher-1.asc")), verify=False) assert len(plaintext) > 0 assert plaintext.find(b'Wenn Sie dies lesen k') >= 0, \ 'Plaintext not found'