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

easier revocation checks #225

Open
anarcat opened this issue Nov 8, 2017 · 5 comments
Open

easier revocation checks #225

anarcat opened this issue Nov 8, 2017 · 5 comments
Milestone

Comments

@anarcat
Copy link
Contributor

anarcat commented Nov 8, 2017

There is a neat is_expired property in the PGPKey class. We should also have a is_revoked property to easily check if a key is revoked. That property should:

The latter may require this to be a real function, not a property, because we may have to pass public key material to verify the revocation signature, in case of third-party revocations. With RFCbis, the full fingerprint is attached to signatures, so we may be able to check against that, but that seems flaky as well: that field is just indicative...

So I'm not sure this can be a property. Maybe this could be an indicative is_revoked property that needs to be checked with an actual verify using actual public key material.

@anarcat anarcat changed the title easy revocation checks easier revocation checks Nov 8, 2017
@anarcat
Copy link
Contributor Author

anarcat commented Nov 8, 2017

so this can partly be done right now, provided that you have the key material of the delegated revokers.

for keyclass, algo, fingerprint in somekey.revocation_keys():
  if fingerprint == somekey.fingerprint:
    revoker = somekey
  else:
    revoker = PGPkey.from_thin_air(fingerprint) # obviously that part is missing
  revoker.verify(somekey, somekey.revocation_signatures())

the challenge here, obviously is to verify revocation on somekey when the revoker key isn't the key itself. we then need to fetch the key elsewhere, something that cannot be done at all in PGPy right now.

@anarcat
Copy link
Contributor Author

anarcat commented Nov 8, 2017

looking at this comment it seems the above code misses some key part of the RFC, namely that the keyclass and algo fields match the signature as well, something that verify doesn't support right now. so maybe there's room for a dedicated revoker.revoked(somekey) interface that would cover for those situations.

@Commod0re Commod0re added this to the 0.5.0 milestone Nov 8, 2017
anarcat added a commit to anarcat/PGPy that referenced this issue Nov 8, 2017
It may not be obvious for users of the API that we can get the list of
revoker keys for a given key from the (private) _signature list, so
add a convenient accessor.

This is not really useful right now because it will raise a
NotImplementedError if any such signature is found, but will become
actually quite useful once SecurityInnovation#198 lands.

This is part of the process to make revocation checks easier in SecurityInnovation#225.
@anarcat
Copy link
Contributor Author

anarcat commented Nov 8, 2017

and obviously, this doesn't actually work:

for keyclass, algo, fingerprint in somekey.revocation_keys():

so I made #227 to fix that.

@anarcat
Copy link
Contributor Author

anarcat commented Nov 8, 2017

so here's how i implemented this so far:

https://0xacab.org/monkeysphere/monkeypy/commit/0bf0d9f30b419bdccbb1d4942665124294730a88#6768269555ca74f15c1cdba28cf1d4a0b31feb67_43_69

instead of relying on a blind is_revoked property (from #223 - obviously too naive), we expand the check as discussed to cover revoker delegations and so on. in short, this would look like:

def is_revoked(subkey, primary=None):
  revoker = primary or subkey
  for sig in key.revocation_signatures(): # needs #227
    if sig.signer != revoker.fingerprint.keyid:
      continue
    verified = revoker.verify(key, sig)
    if verified:
      return verified
  return False

... and that's just for self-signatures. for revoker delegation, we'll probably need to turn that into a set of revokers and dynamically load the public key material for those folks in my program.

anarcat added a commit to anarcat/PGPy that referenced this issue Nov 8, 2017
It may not be obvious for users of the API that we can get the list of
revoker keys for a given key from the (private) _signature list, so
add a convenient accessor.

This is not really useful right now because it will raise a
NotImplementedError if any such signature is found, but will become
actually quite useful once SecurityInnovation#198 lands.

This is part of the process to make revocation checks easier in SecurityInnovation#225.
@anarcat
Copy link
Contributor Author

anarcat commented Nov 8, 2017

and yes, this could be part of the PGPKey API, which would be, as @Commod0re suggested, "did key A revoke key B" semantics:

def revoked(self, key):
    for sig in revocation_signatures(key):  # needs #227
        if sig.signer not in {self.fingerprint.keyid} | set(self.subkeys):
            continue
        logging.debug('checking revocation signature %r', sig)
        verified = self.verify(key, sig)
        if verified:
            return verified
    return False

(Maybe SignatureVerification elements could be aggregated here somhow and be returned in one shot.)

Then this would be used like this:

revoker = primary or subkey
if revoker.revoked(subkey):
  return True
revokers = list(subkey.revocation_keys())
for revoker in [PGPKey.from_keyserver(x) for x in revokers]:
  if revoker.revoked(subkey):
    return True

logic like this is now defined in openpgp2ssh here:

https://0xacab.org/monkeysphere/monkeypy/blob/dcd2fd4b34404b5be43716927cf3b075df5274f9/openpgp2ssh.py#L80

how does that look?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants