rnpgp / rnp

RNP: high performance C++ OpenPGP library used by Mozilla Thunderbird
https://www.rnpgp.org
Other
200 stars 55 forks source link

CLI: unexpected output on expired key+subkey. #1842

Closed ni4 closed 1 year ago

ni4 commented 2 years ago

Description

If key and subkeys are expired, currently CLI displays the following:

pub   2048/RSA 73641292304398ea 2015-02-01 [SC] [EXPIRED 2017-01-31]
      65eac3d3967887faedbc314873641292304398ea
uid           key-2015
sub   2048/RSA 6081e5cbbeeb3955 2015-02-01 [INVALID] [EXPIRES 2017-01-31]
      fe43feabb22c83b35a5ef7d76081e5cbbeeb3955

While correct would be to display [EXPIRED] for the subkey.

andrey-utkin commented 2 years ago

So I've been looking into it for a couple of days, I have understood what's going on to some extent, now I want to share what I see and get clarification about what exactly we need.

So formally and fundamentally, the key is valid when everything is good. Otherwise it's invalid. There is however a common desire to see a more nuanced status, especially around expiration and revocation. (However evidently often people expect to see green/alright status in not-strictly-alright cases! See P. S.)

I found no one specific rigid formal model on how a terse terminal UI should represent the key state, but I have my own guess of what @ni4 has in mind. I also think it's important to align with other implementations and public expectations.

My guess:

Is that close to what you have in mind @ni4 ?

What GnuPG does?

gpg shows the primary key as expired, and doesn't show the subkey at all. I'd say it's smart. I have never looked so closely into key validation, and I was overwhelmed by the amount of complexity involved: multiple types of self-signatures, their optionality, two types of expiration time (key e. time, signature e. time), revocations are a special kind of signature which cancels another signature...

How to implement that ("my guess" system)? Track the validity as a structure with multiple properties indicating specific failures/defects (because there's only one validity - the absense of defects). The properties should be reported upwards until the moment of presentation. When a complex entity is validated, aggregation of the validity structures should happen based on OR operation of the same fields, i.e. result.expired = part1.expired || part2.expired.

What happens in RNP currently is internally incoherent.

At CLI presentation level, around src/rnp/fficli.cpp:1242, we query key's properties "valid" (seemingly includes not expired and not revoked), "expired", "revoked".

In src/lib/types.h, in pgp_validity_t, we have bool expired and...

bool valid{};     /* item is valid by signature/key checks and calculations.
             Still may be revoked or expired. */

So at very least the terminology is a bit confusing at these different levels. What about bool good? Just like gpg says "Good signature" even in case of expired or revoked signing key.

pgp_key_t::validate_primary() does its work, resulting in validity.valid being set to false in this case, but also validity.expired being set to true.

Then pgp_key_t::validate_subkey() runs, resulting in an early return in this case:

pgp_key_t::validate_subkey(pgp_key_t *primary, const rnp::SecurityContext &ctx)
{
    /* consider subkey as valid on this level if it has valid primary key, has at least one
     * non-expired binding signature, and is not revoked. */
    validity_.reset();
    validity_.validated = true;
    if (!primary || !primary->valid()) {
    return;
    }

Notably, this results in validity_.expired being left as false, which trips the aforementioned CLI presentation level logic: "if it's not valid but not expired, INVALID it is".

P. S. Some public expectations about cryptographic validation outcomes. Particularly around Github/Gitlab commit signing.

Conservative and admirable:

https://security.stackexchange.com/questions/235164/subkey-rotation-and-revocation

PGP will only care whether a subkey is "valid" or "invalid". Expired or revoked keys will be equally treated as "invalid", and it's up to the UI to display the reason behind it ("expired" vs. "revoked").

Naive and eyebrow-raising:

https://github.com/isaacs/github/issues/1099#issuecomment-779077468

Title itself:

Revoked and expired GPG subkeys should keep have verified tag on old signature

GPG has a build in mechanism to manage this. When you revoke the subkey, you have to choose a reason for this action. Some of them explicitly state, that subkey has not been compromised. IMHO, subkeys, revoked in this way should remain verified.

It sounds like everyone pretty much agrees with the basic principle here though. If a key expires naturally, or is "archived" in some way to indicate that it was not known to be leaked or compromised, then commits made with that key during its valid time should continue to be treated as valid.

(+ many more in that thread)

https://news.ycombinator.com/item?id=25856812

IMHO old commits (before key was compromised/revoked) should be considered valid

https://gitlab.com/gitlab-org/gitlab/-/issues/255279

For example, if a commit is signed by a valid subkey, but later that subkey got revoked (or expires? didn't test), after uploading the public key again, the commit is then labelled with 'unverified'. But unless the signature is generated after the subkey's been revoked, IMO it should still be considered 'verified'. Perhaps we can also take the 'revocation reason' into consideration: if it was 'compromised' then sure, label those commits as 'unverified', since we don't know when exactly it was compromised; but if it's simply 'no longer being used', I think the existing commits/annotated tags/etc should still stay verified.

500 seat, Premium customer requesting this issue be resolved Customer is seeing expired GPG keys show up as unverified instead of expired.

ni4 commented 2 years ago

@andrey-utkin Actually, it's what issue heading says - in this particular case correctly would be to say that subkey is expired, not invalid. That's what this issue about :)

andrey-utkin commented 2 years ago

But I am not clear on the model which makes you ask for that. That's why I am asking for clarification.

ronaldtse commented 2 years ago

I think the question from @andrey-utkin to @ni4 is just:

My guess:

  • show the key's capabilities when it's perfectly valid
  • show "EXPIRED" if, except for the expiration, it's perfectly valid
  • show "REVOKED" if, except for the revocation, it's perfectly valid
  • what to show if revoked AND expired but otherwise valid? "REVOKED" because that feels like a stronger and more irrevocable reason.
  • show "INVALID" if it's not valid regardless of being expired or revoked (e.g. critical parts missing, computational integrity check failed)

Is that close to what you have in mind @ni4 ?

Is this our expectation?

If a key is revoked and expired, it should show "revoked and expired", no?

ni4 commented 2 years ago

But I am not clear on the model which makes you ask for that. That's why I am asking for clarification.

Actually it's quite simple - in this particular case we should set flag expired for the subkey as true, and we just need to add some checks for this.

ni4 commented 2 years ago

I think the question from @andrey-utkin to @ni4 is just: ... If a key is revoked and expired, it should show "revoked and expired", no?

By current CLI logic it would be [EXPIRED] [REVOKED]. Actually the issue is just about unset expired flag in the case when it should be set (please see the previous comment).