Add expiration to master keys & encryption subkey

[trishankatdatadog]

• Add expiration to master keys and expiration subkeys
• Check for expiration of master key when returning encryption/signing subkeys
• Find newest instead of first unexpired signing subkey

Signed-off-by: Trishank Karthik Kuppusamy trishank.kuppusamy@datadoghq.com

[trishankatdatadog]

I suppose there could be a slight difference in the creation and expiration time of the master key and encryption subkey now, but it should not matter in practice.

[twiss]

Hey @trishankatdatadog, thanks for the PR!

Typically, key expiry is set on the primary key only. Of course subkeys can expire separately, but most typically, the subkeys expire when the primary key expires. (This makes it easier to extend the lifetime of the entire key, since you just have to re-sign one signature.)
Unfortunately, this is not how ProtonMail/crypto currently behaves; subkeys' lifetime currently extends beyond the lifetime of the primary key, but this is a bug. (See ProtonMail/gopenpgp#80, which has this as its root cause.) So probably EncryptionKey should check whether the primary key is expired before looking at the subkeys. (And similarly for SigningKey.) And then, personally I would leave the subkey generation code as it was, to avoid setting an expiration there and also to avoid the (admittedly minor) potential issue of different creation dates.

[trishankatdatadog]

@twiss, a few comments:

1. So I fixed the code to return no signing/encryption keys when the primary UID has expired. However, I also added a test to show that this fails when a direct signature on the primary key itself has expired. AFAICT, we do not handle such signature packets at all. How should we handle this test?

2. I brought back the old code to directly add the encryption subkey. I still find it a little inconsistent to not add expiry on this subkey, but checking the expiration of the primary UID does mitigate this.

3. I just noticed that we get the newest unexpired encryption subkey, but the first unexpired signing subkey. What is the rationale for this? The latter can lead to unintuitive behavior from an end-user's perspective.

[trishankatdatadog]

ping 🙂

[twiss]

Hey @trishankatdatadog, sorry for the delay, and thanks for the ping :)

1. IMO, I wouldn't worry about this for now. This is an uncommon way of expiring a key. So I would just leave out this test for now.
2. Yeah, I understand. This is standard for OpenPGP keys, though
3. Yeah, good point. It should return the newest in both cases indeed. Feel free to fix it here or in a separate PR, if you feel like :)

[twiss]

@trishankatdatadog Just for clarity, and as a gentle ping, I'm happy to merge this without the failing test 😊

[trishankatdatadog]

@trishankatdatadog Just for clarity, and as a gentle ping, I'm happy to merge this without the failing test 😊

Oh, hey, sorry, lemme get around to this, was busy with other projects, thanks!

[twiss]

Np, thanks! 🙏

[trishankatdatadog]

@twiss Reverted the bad test, let me get around to returning the first newest unexpired signing subkey ASAP in this PR?

[twiss]

@trishankatdatadog Yeah, of course, thanks! 🙏

[trishankatdatadog]

Working on testing finding the newest instead of the first unexpired signing subkey...

[trishankatdatadog]

@twiss Done, please take a careful look. In particular, I found that I had to add this line to prevent keys from being considered immediately expired, but the corresponding line seems to have no effect on whether signatures are considered immediately expired (they are not).

[twiss]

@trishankatdatadog Thanks! Yeah, makes sense. That's probably because SigExpired is not called as often, but yes this change seems correct. I've left two very minor nitpicks, but other than that looks good to me 👍

[trishankatdatadog]

@trishankatdatadog Thanks! Yeah, makes sense. That's probably because SigExpired is not called as often, but yes this change seems correct. I've left two very minor nitpicks, but other than that looks good to me 👍

Done, please take a final look 👀

[twiss]

Looks good, thanks a lot! 👍