improve API for signatures and subkeys #451

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 openpgp@ietf.org 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.

…iased)

This is not a signature subpacket. Rather, it is a signature type.

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)

Signed-off-by: Daniel Kahn Gillmor <dkg@fifthhorseman.net>

By making all arguments to the functions keyword arguments, we can use **kwargs to receive any extended options. Signed-off-by: Daniel Kahn Gillmor <dkg@fifthhorseman.net>

This reflects the changes to the subcommand names and additional arguments from draft-dkg-openpgp-stateless-cli-01

This implements simple signatures inside encryption for sopgpy

This should enable tests of signature verification concurrent with decryption. We do this by refactoring out the signature verification and relying on PGPY to know how to verify an "inline" message.

…xt as text

This works around SecurityInnovation#388

there have been a bunch of changes (including implementations of previously-missing options) This just acknowledges those changes.

… locked secret key

…igning)

See discussion at https://gitlab.com/sequoia-pgp/sequoia-sop/-/issues/17

sigsubj objects have an "issues" bitfield, which follows the "Anna Karenina principle" instead of "verified" boolean.

as of 0.6.0, from_blob() methods will return non-functioning objects rather than raising an error directly.

… far.

…version

… and OpenSSL

"nested" semantically probably is meant to mean "another OPS packet follows". But the byte on the wire is defined as 0 means another OPS packet follows. Furthermore, the flags are set in the wrong way: before this commit, the code produced a series of OPS packets where the first packet had a different value for the flag than all subsequent ones. What we want is where all the flags *except* the last OPS packet (corresponding to the *first* Sig packet) are 0. See https://gitlab.com/sequoia-pgp/openpgp-interoperability-test-suite/-/issues/84

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

…nknown

…en discovered

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

If someone actually wants to use IDEA, CAST5, or Blowfish, we want to see the CryptographyDeprecationWarning objects emitted by cryptography.hazmat.primitives.ciphers. But if we're just looking up a non-deprecated cipher we should just return it directly. This avoids the following kinds of warnings: pgpy/pgpy/constants.py:191: CryptographyDeprecationWarning: IDEA has been deprecated bs = {SymmetricKeyAlgorithm.IDEA: algorithms.IDEA, pgpy/pgpy/constants.py:193: CryptographyDeprecationWarning: CAST5 has been deprecated SymmetricKeyAlgorithm.CAST5: algorithms.CAST5, pgpy/pgpy/constants.py:194: CryptographyDeprecationWarning: Blowfish has been deprecated SymmetricKeyAlgorithm.Blowfish: algorithms.Blowfish,

…suite Once CAST5 is deprecated, a deprecation warning might arise in addition to the warnings about already-protected secret keys might not be the first warning. Instead, we have the test look through the warnings and validate the content of any matching warning.

imghdr is deprecated and will be removed in python 3.13 (see https://peps.python.org/pep-0594/#imghdr) The relevant code in imghdr is just: ``` def test_jpeg(h, f): """JPEG data with JFIF or Exif markers; and raw JPEG""" if h[6:10] in (b'JFIF', b'Exif'): return 'jpeg' elif h[:4] == b'\xff\xd8\xff\xdb': return 'jpeg' ``` So we transplant it directly

…ptography-deprecations', 'drop-unused-test', 'drop-imghdr', 'cleanup-registrations', 'fix-aliased-function', 'drop-mistaken-sbs' and 'fix-ops-nested-flag', remote-tracking branches 'ktdreyer/rm-six', 'KOLANICH/modernize' and 'janmojzis/master' into dkg/overhaul

This appears to have been introduced in 38a4f9f nearly 9 years ago, but it doesn't look like it ever worked. It's not clear to me why this index of keys by key ID is returned at all in these functions, but since September 2014 (v0.3.0) the index has always returned the key ID of the primary key anyway. There are a few things broken in this particular line: - the use of `~` if `fingerprint.keyid` doesn't work for some reason: that won't match anything. - and, None gets passed as the second object in the tuple, but the second object should be either True or False. Rather than try to salvage something that i don't understand in the first place, i figure it's better to drop it and acknowledge the status quo.

(see https://stackoverflow.com/questions/8205558/defining-boolness-of-a-class-in-python for discussion)

This was introduced (accidentally, i think) in 9ce9ad14ad553304b0f064a8b9e6a929b7527931

functools.singledispatch was made available in Python 3.4

This sets the stage to distinguish CFB from AEAD

using a keyword hash that only pulls out one particular keyword is a recipe for API abuse. better to declare the function arguments explicitly.

This keeps the types consistent for typechecking. Otherwise, we see errors complaining that decargs and encargs have different types depending on which codepath is followed.

it might be nicer to specify the allowed category of __flags__ in more details, but i do not know how to do that

…ble-length Tuples

Without this, mypy complains: error: First argument to namedtuple() should be "_reason_for_revocation", not "ReasonForRevocation" [name-match]

The return object from this function is a bit fuzzy, and might be better created as an explicit object, rather than a dict. But for now, at least keep the type definitions reasonable.

…itself being abstrct

(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.

…ut whose values can also be reached by KeyID

This allows subkey selection by Fingerprint as well as Key ID We use the FingerprintDict more efficiently, and we also define issuer_matches and signing_subkey for simplicity. PGPKey.decrypt now also can find the correct key/subkey even if the PKESK is identified by fingerprint.

…cket present This is a change in the API to more accurately reflect what is happening in a signature. It's possible to have a subpacket that contains the empty string, and this allows a consumer to distinguish between the two cases.

For features, keyserverprefs, and key_flags, we also now return a simple IntFlags object (not a set), and None when no corresponding subpacket is present.

This is useful because the S2KSpecifier will be reused in several different places during implementation of the crypto refresh, witho ut the usage octet, the IV, or the encryption algorithm identifier. This also paves the way for a simpler addition of other S2K types (including Argon2)

users shouldn't need to choose an algorithm, there should just be sane choices. We allow the user to provide arguments to avoid an API change.

This gives the user more control if they want to control how the encrypted blob works, while deferring to the S2KSpecifier interface. We copy the S2K Specifier upon reciept, so that if the s2k specifier changes later, it doesn't sneakily get changed here too.

enables creation of reproducible results, not for standard use.

- a selfsig *without* a preferences subpacket (no preference stated) is different from a selfsig with an empty preferences subpacket (a stated preference for the MTI algorithm) - given the complications around deciding between multiple selfsigs with different preferences, we might want to skip a selfsig with no preference stated, but terminate on a selfsig with a stated preference, even if it is empty - use a generator to walk the list of selfsigs that might have legitimately-stated preferences This includes a subtle API change (preferences properties can now return None as distinct from an empty list), but i think it's necessary to do the right thing here. Tests are adjusted to match the new behavior as well.

This makes it easier to generate deterministic test vectors from PGPy, but *SHOULD NOT* be used in regular operations.

without this, the first use of the S2K will internally generate a salt and then subsequent uses of it will reuse the salt, probably not a desired outcome.

This means cleaning up how we handle OnePassSignature generation as well. And, PGPUID.signers now returns Fingerprints instead of KeyIDs, so we fix up the tests too.

This moves code shared by v6 keys and v4 keys into common base classes We also adjust the inheritance tree for PrivSubKey: this should cause no functional change, but it makes the expanded API surface for PubSubKey more visible directly in PrivSubKey, which in turn will make type annotations clearer when subkeys are invoked.

If the session key in encrypt_sk is an int, we need to convert it manually to a bytes object.

Note that bytes_to_text and text_to_bytes no longer accept None as input. When they would accept None as input, that meant that their output could also be None which in turn made it harder to vet the type of the output when the input was clearly not None.

…r PrivKey packet In addition to the extra type safety, this also corrects an erroneous comment about them.

Note that we change types.Header._lenfmt from an int to a bool, and rename it _openpgp_format. this is in keeping with the crypto-refresh, which explicitly names the two headr formats "OpenPGP format" and "Legacy Format".

Note that you used to be able to use the or operator to append UserID and UserAttribute packets indefinitely, but only the first packet would take effect. all the other ones would be silently ignored. the current code is simpler, but instead ignores all the previous packets, and uses the last one.

This also avoids parsing and then re-assembling the user ID during __format__ operations, which in the worst case can actually modify the string returned.

…superclass

…ions in v6 keys, the wire format for the subpacket sections contains 4-octet length fields instead of 2. Make the rest of the code flexible enough to prepare for that (without changing it)

…hable)

…ritance This shouldn't be a problem because __copy__ will actually work correctly thanks to the multiple inheritance itself. There could be a better way to annotate this that i don't know about, but this should do.

While the term "Tag" is present in the spec, it's ambiguous and confusing, and the identifier is also referred to as the type of packet. See discussion over on https://gitlab.com/openpgp-wg/rfc4880bis/-/merge_requests/319

This means that we can also simplify parsing the String2Key (no need to pass along whether we need to parse out an iv).

This will become useful as newer versions of secret key packets have different wire formats for the key material.

This will become useful because when String2Key is placed on the wire in a Secret Key packet, it may be represented differently depending on the version of the secret key packet.

This prepares us for a simplified switch for v6 keys. Improve the type signatures for the superclass here, since pubkey() is a method, not a property.

…order

This lets us use forward-declared type annotations without manually noting them as strings.

I'm deeply skeptical about the sense or utility of this, but it is at least documenting the current behavior more adequately. The resulting messages (when signed) form this packet sequence: SIG PKESK SEIPD This is technically valid in the formal OpenPGP message grammar but it's unclear what it is supposed to mean.

(instead of sigsubj (with namedtuple)) This makes it easier to see type-annotations. Since the name of the type was never fully exported anyway, and it keeps all of its standard members, this isn't really an API change.

This had been broken with the introduction of PGPSignatures, but it should now work again.

newlines near the top of a CSF message can be handled more delicately. - if the Hash: header is missing, we want the trailing newline to appear - there's no need to have a non-capturing grouping places where the group is not used.

…ithm I don't expect new image encoding types to be defined. Indeed, the version itself could just go away and we could probably retcon UserAttribute 1 as JPEG (see https://gitlab.com/openpgp-wg/rfc4880bis/-/issues/162) with a weird 16-octet header. But it's better to have the code normalized here.

…tion, etc

See the rationale from the upcoming standard: https://www.ietf.org/archive/id/draft-ietf-openpgp-crypto-refresh-07.html#name-optional-checksum

The implementation will select a reasonable option for the indicated pubkey algorithm if you don't supply it.

This could be done only during the --profile draft-ietf-openpgp-crypto-refresh-10, but there's nothing in the earlier specifications that would indicate a problem or incompatibility to include such a signature.

…o base class

Move encrypt into fields.PubKey, and decrypt into fields.PrivKey. This is a simplifying change, and also prepares the codebase for the crypto-refresh, which has different subtle changes in the encoded pkesk, for different versions and different algorithms. The underlying cryptographic code gets pushed into fields.py (out of sight from higher-level functions) and lets us make more straightforward and explicit type definitions.

- if something is not implemented, just raise NotImplementedError directly, rather than trying to sometimes return a NotImplemented object. Returning a NotImplemented object complicates the type signature without providing much of an advantage in terms of workflow. - PGPSignature.target_signature is a property that was never implemented or used. We can just drop this property entirely, implementing it only when it can actually be implemented.

use search_pref_sigs() instead of walking each user ID

This leaves only one place to list the requirements

This will be important if a non-v4 signature gets embedded. It might end up being an opaque signature, if PGPy can't read the version, but it shouldn't try to force a SignatureV4 if it is not actually version 4. This change also isolates the parsing of the embedded signature so that it can only consume bytes within the enclosing subpacket.

This hasn't been necessary since fingerprints knew their own version, it was just a no-op.

This is what caught the problem of passing _version to addnew

Commits on Aug 19, 2023

note dependency on sop as optional

dkg committed Aug 19, 2023

Configuration menu

View commit details

Copy full SHA for f6cf609

Browse repository at this point

Copy the full SHA

f6cf609 View commit details

Browse the repository at this point in the history

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

improve API for signatures and subkeys #451

improve API for signatures and subkeys #451

Commits on Jun 13, 2023

Commits on Jun 14, 2023

Commits on Jun 15, 2023

Commits on Jun 16, 2023

Commits on Jun 28, 2023

Commits on Jul 11, 2023

Commits on Jul 13, 2023

Commits on Aug 11, 2023

Commits on Aug 18, 2023

Commits on Aug 19, 2023

Commits on Aug 23, 2023

improve API for signatures and subkeys #451

Are you sure you want to change the base?

improve API for signatures and subkeys #451

Commits on Jun 13, 2023

Commits on Jun 14, 2023

Commits on Jun 15, 2023

Commits on Jun 16, 2023

Commits on Jun 28, 2023

Commits on Jul 11, 2023

Commits on Jul 13, 2023

Commits on Aug 11, 2023

Commits on Aug 18, 2023

Commits on Aug 19, 2023

Commits on Aug 23, 2023