Unclear TODO: "Self-sigs verification is not yet working because self-sigs are not parsed!!!"

[dkg]

in b8c28c5, @KOLANICH introduced PGPKey.self_verified, which contains the warning mentioned in the subject here:

    @property
    def self_verified(self):
        warnings.warn("TODO: Self-sigs verification is not yet working because self-sigs are not parsed!!!")
        return SecurityIssues.OK
        
        if self._self_verified is None:
            self._do_self_signatures_verification()
        
        return self._self_verified

This is pretty odd. The caching part of the function, which appears to actually call the expected verification, is effectively dead code because of the early return SecurityIssues.OK.

I recommend dropping the first two lines of this function.

[KOLANICH]

It is dead code because if we allow it execution, then it won't work and cause breakage. So for now we just print a warning in a hope someone finishes it. The present dead code is mostly a memo on what we are going to do.

[dkg]

What specifically won't work? the codepaths exist that should populate the SecurityIssues (e.g. PGPKey.self_verify and PGPKey._do_self_signatures_verification, but when i remove the lines in question it does appear that the primary key appears to yield SecurityIssues.NoSelfSignature for test_gen_key. That's odd, because there is indeed a self-signature, afaict, made by PGPKey.add_uid during test_gen_key.

"hope someone finishes it" doesn't sound like a great plan, especially if there are no open bug reports about what is broken. I'll try to remove it and make sure the test suites pass, but if you have any specific guidance about what you think specifically needs to be fixed, i'd appreciate it.

[dkg]

OK, i did a bit more debugging, which i've posted to my diagnose-457 branch, and it is indeed broken.

There are at least three ways that i think these verifications are broken:

• OpenPGP keys that use ECDSA or ECDH with anything but Ed25519 and Curve25519 are considered "unsafe" -- but none of the implemented NIST or Brainpool curves are known to be bad. I don't think that PGPy should be raising concerns about them. But it currently does, returning SecurityIssues.InsecureCurve.
• PGPKey.self_signatures does not yield any self-signatures associated with the user ID, which means that during simple OpenPGP key generation (PGPKey.new followed by PGPKey.add_uid) we end up with SecurityIssues.NoSelfSignature.
• When you add a direct key self-signature (e.g. key |= key.certify(key, usage=KeyFlags.Certify|KeyFlags.Sign)), there is a recursive function call when invoking key.self_verify():

pgpy/pgp.py:2373: in self_verify
    if not self.verify(self, s):
pgpy/pgp.py:2478: in verify
    subkey_issues = self.check_soundness(self_verifying)
pgpy/pgp.py:2410: in check_soundness
    return self.check_management(self_verifying) | self.check_primitives()
pgpy/pgp.py:2399: in check_management
    res = self.self_verified
pgpy/pgp.py:2390: in self_verified
    self._do_self_signatures_verification()
pgpy/pgp.py:2382: in _do_self_signatures_verification
    self._self_verified = self.self_verify()
pgpy/pgp.py:2373: in self_verify
    if not self.verify(self, s):
E   RecursionError: maximum recursion depth exceeded
!!! Recursion detected (same locals & position)

In the diagnose-457 branch i mentioned above, i've added tests/test_20_selfsigs.py which triggers this last failure.

So, we can't remove that code because there's a recursive loop.

[KOLANICH]

anything but Ed25519 and Curve25519 are considered "unsafe" -- but none of the implemented NIST or Brainpool curves are known to be bad.

I am not a professional cryptographer, so I just whitelisted those of the supported curves that had a green tick for them on https://safecurves.cr.yp.to/ . I have no opinion about the validity of the criteria listed there, I'm neither a mathematician nor a cryptographer.

[KOLANICH]

PGPKey.self_signatures does not yield any self-signatures associated with the user ID, which means that during simple OpenPGP key generation (PGPKey.new followed by PGPKey.add_uid) we end up with SecurityIssues.NoSelfSignature.

self_signatures are not available even in keys generated by gpg. It seems that they are just not parsed.

[KOLANICH]

When you add a direct key self-signature (e.g. key |= key.certify(key, usage=KeyFlags.Certify|KeyFlags.Sign)), there is a recursive function call when invoking key.self_verify()

Thanks for catching this. I haven't tested that case. My use case was centered around keys and signatures generated by gpg and PGPy used only for verification. When I saw the self-sigs were not parsed, since I had no time to spend on digging and debugging the parser, I just disabled the code path verifying the self-sigs.

[KOLANICH]

It is likely that self_verifying argument was intended to break theninfinite recursion.

[dkg]

There are a lot of similar-sounding user-facing functions here, and it's hard to imagine a user knowing when they should call one or the other. In PGPKey alone, i'm seeing (ignoring the ones that are prefixed with an _, which usually indicates an internal interface):

• is_considered_insecure()
• self_verify()
• self_verified (a property, which is just a cache of self_verify())
• check_primitives()
• check_management()
• check_soundness()

Seems to me like there should just be a single user-facing function or property that indicates that the key itself is not inherently broken. What "broken" means is of course different depending on how the user might want to use a given key (e.g. encrypt, decrypt, sign, or verify), but we can defer to the KeyAction decorator to take care of the specific usages.

These functions were all added between 0.5.4 and 0.6.0. So i'm proposing that we remove them all, and leave only a single property like PGPKey.security_issues as a user-facing function (there may be _-prefixed functions that are used to produce that property, of course)

[KOLANICH]

I apologize for the bad quality of my PRs, they really don't represent a finished and stable set of functionality and I have never expected them to be merged without a proper review. I think that the functions structure the code and document its behavior, and also provide the points to override the behaviour in subclasses if anyone needs it, so I think that it is preferred to just keep them just prefixed. My vision is that the checks should be done automatically when either keys are imported or used, and calling the certain methods is when a user wants to check certain properties of keys without checking the rest. But feel free to do everything you consider as right, including just reverting my changes completely.