Merge many outstanding PRs, add a bunch of type signatures, improve code quality #450
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
(these were introduced accidentally when KOLANICH removed the six module in a23dcce)
Commit a23dcce removed the six dependency. Remove the other references in the docs and CI.
These tests are pulled from the OpenPGP Interoperability Test Suite, in particular from those tests tagged "forward-compat": https://tests.sequoia-pgp.org/?q=forward-compat These failures make it difficult to use PGPy in an OpenPGP ecosystem that can evolve, because PGPy will complain about OpenPGP objects that contain something it doesn't understand, even though the rest of the message is otherwise comprehensible and usable. See Justus Winter's message to [email protected] describing his concerns: https://mailarchive.ietf.org/arch/msg/openpgp/QUiEKx3PQeJOXnkcvvnuHpv739M I've selected specific examples that PGPy is known to currently fail with.
pgpy.types.Exportable was renamed to Armorable before version 0.3.0 was released (in d00010e, back in 2014), which indicates that this test program has not been run in over 8 years. Given that the asyncio stuff has also changed in python since then, it is simpler to just drop this file than try to fix it.
This reduces the number of SKIPPED and XFAIL messages from the test suite. Given where the cleanup is happening, also make the remaining XFAIL messages a little clearer.
This is not a signature subpacket. Rather, it is a signature type.
dkg
changed the title
|
I'm continuing to work on this series, including more code cleanup without introducing any significant API changes, bu it's not ready to merge yet. I'll remove the "WIP" label when it's done. |
It works in the basic mode, but we still need to handle the args for encrypt/decrypt.
- add enums for the flags passed into the sop interface
- make member functions of StatelessOpenPGP well-typed
- adjust docstrings so that help(sop) provides useful guidance
- handle sessionkey and timestamp parsing in sop.py
- handle all indirect access directly in sop.py
- complete strict typing ("mypy --strict sop.py" passes)
(the earlier arrangement of the comment was unclear, logic has not changed)
Python's cryptography module is just going to use the default backend. There was never really any affordance in the PGPy API to select a different backend anyway, so we can just drop it as a simplification.
…-refresh There should be no substantive change here, just making the text easier to read
This means we also adjust the IssuerFingerprint and IntendedRecipients subpackets to just expose a fingerprint.
We leave one use of hashlib in the tests, but all cryptographic digests in the running code get pulled from the cryptography module.
we keep PGPSignature.make_onepass to avoid an API change, but the logic of how to generate an OPS belongs in the Signature packet itself anyway.
This gives clearer semantics to any reader, and puts the mappings of magic numbers all in pgpy/constants.py instead of scattered throughout the codebase. It also improves type annotation coverage.
this was introduced in 12d085a and it's not clear why it needs to be there. __typeid__ is typically an Optional[int] (or Optional[IntEnum]), so i can't tell what this check would prevent
IntendedRecipient and IssuerFingerprint subpackets have the same wire formats, just different semantics. Consolidate the codebase here. Note that a freshly-created subpacket like this will emit an all-zeros fingerprint if you ask it for one, but will refuse to render itself into a wireformat.
placing subpackets in ascending order of subpacket id makes PGPy-generated certificates and signatures less distinguishable from other implementations.
This permits generation of packets that advertise support that PGPy can't provide so it's dangerous to use.
If we're signing specific subpacket content, there's no reason to include the same subpacket in the unhashed area; and if the content disagrees, there's no reason why any implementation should prefer the unhashed data over the hashed data.
This sets the stage for newer pubkey algorithms that don't use the MPI wire format construct.
Short IDs are only 32-bits long. They are cheap to brute force, while still being unintelligible and difficult to transcribe correctly for humans.
https://datatracker.ietf.org/doc/html/rfc4880#section-3.7.2.1 says: > These are followed by an Initial Vector of the same length as the > block size of the cipher for the decryption of the secret values, if > they are encrypted, and then the secret-key values themselves. But the test used a uniform IV of length 8 -- this corrects the test.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR collects many of the outstanding bugfix/cleanup PRs, plus #440 ("SOP" CLI functionality), and then also includes a bunch of type signature annotations. I offer this collection of changes all together because of some sequencing issues (e.g. collections.abc cleanup interacting with the PGPSignatures additions), and there were a few minor merge conflicts that i've resolved so no one else will have to.
A longer-term goal is to be able to deploy a typechecker to offer an additional layer of static analysis on the PGPy codebase, but the module is still far from clean when run through
mypy. This is just a start.The next bit of work i have queued after this, inspired by the typechecking cleanup, will offer some subtle but hopefully not dangerous API changes -- ones that i think shouldn't be a problem for most consumers of PGPy, but i want them to be vetted separately from this, so i'm offering this as a sort of checkpoint on the way there.