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

Improve forward compatibility #442

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

Conversation

dkg
Copy link
Contributor

@dkg dkg commented Jun 14, 2023

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.


This series should make all the new tests pass, which should also improve the score of PGPy on the interop test suite.

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.
@dkg
Copy link
Contributor Author

dkg commented Jun 14, 2023

These changes also include some (hopefully positive) side effects:

  • remove the dependency on pyasn1, since arbitrary ASN.1 parsing isn't needed -- if the ECC curve is unknown, the point is unknown as well.
  • clean up dependency on the cryptography library, closing import pgpy is failing with an error - cryptography.utils has no attribute register_interface #402 (see e015068)
  • introduce the beginnings of some type annotations
  • add a PGPSignatures object which can be used to handle a detached OpenPGP signature object that contains more than one signature, by analogy with an inline-signed message that has more than one signature.

@dkg dkg force-pushed the compat-tests branch 2 times, most recently from b61a02f to 4915ce0 Compare June 16, 2023 22:23
dkg added 12 commits June 28, 2023 15:25
a PGPMessage object can contain more than one signature.  Detached signatures should
also be able to handle having more than one signature.

https://www.ietf.org/archive/id/draft-ietf-openpgp-crypto-refresh-09.html#name-detached-signatures says:

> These detached signatures are simply one or more Signature packets
> stored separately from the data for which they are a signature.

A PGPSignatures object makes the most sense to represent such a thing.

Closes: SecurityInnovation#197
The cryptography module supports encoding and decoding these signatures
directly, so no need for pyasn1 or the custom ASN1 decoder to translate
between the OpenPGP format and the RFC 3279 format.
OpenSSL 1.0.2 is ancient at this point -- Brainpool is part of the standard
distribution.  At any rate, we need 1.1.0 for X25519 and 1.1.1 for Ed25519.

And python's cryptography module has supported Brainpool since version 2.2
(also ancient).

Registering subclasses with the cryptography module is complicated across
versions (see pyca/cryptography#7234 which removed
register_interface), but we don't need any of that functionality as long as
we depend on non-ancient modules.

At the same time, we don't need pyasn1 any longer if we just treat the OID
as a bytestring label.

As this also drops all the shenanigans around
cryptography.utils.register_interface, we can also say it
Closes: SecurityInnovation#402
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.

1 participant