Use public key fingerprint as S/MIME Certificate id #3570

[rrrooommmaaa]

This PR uses public key fingerprint for S/MIME Certificate id,
postfixed with "-X509" internally.

close #3570
close #3559

---

Tests (delete all except exactly one):

• Tests added or updated

---

To be filled by reviewers

I have reviewed that this PR... (tick whichever items you personally focused on during this review):

• addresses the issue it closes (if any)
• code is readable and understandable
• is accompanied with tests, or tests are not needed
• is free of vulnerabilities
• is documented clearly and usefully, or doesn't need documentation

[rrrooommmaaa]

I guess we need to add migration correcting the fingerprints applied by PR #3445?

[tomholub]

Looks great, thanks! I'm conflicted on whether this one needs a migration or not - S/MIME is a minor usecase for now. But to be prudent, it would be better to migrate (can be done in single step - users won't have many S/MIME keys). Also, migration errors can be ignored.

[tomholub]

Or maybe, we could enumerate exactly what the side effects would be if we completely skipped migration. If the side effects are an inconvenience to user, but not a security problem, then we may skip. If security problem is implied, then we should migrate.

[rrrooommmaaa]

Or maybe, we could enumerate exactly what the side effects would be if we completely skipped migration. If the side effects are an inconvenience to user, but not a security problem, then we may skip. If security problem is implied, then we should migrate.

Side effects... not too many, but... I'm trying to imagine some situations.
For example, we can load the certificate (Key) by email, and then some function may want to re-load it by longid and unexpectedly doesn't find it.
On update, another copy of the pubkey record (under different fingerprint) will be added and linked -- this may affect revocation by CRL in future.

[tomholub]

I'll let you decide - I don't want to stop you from doing migration now and then causing you headaches later when we bump into it. We can afford to do it properly now and be sure.

This would look like we also have longid collisions (between same-key OpenPGP and S/MIME certs)? There is no such thing as longids in S/MIME, unless I'm wrong. When an S/MIME message is encrypted or signed, does the encoded data contain information about which key it's encrypted for, and which key signed it? If so, how is this information encoded in s/mime? This is really what we need longids for in OpenPGP, else we would have dropped them. This information is encoded in OpenPGP as longids of the appropriate keys in the encrypted message packets.

[rrrooommmaaa]

When an S/MIME message is encrypted or signed, does the encoded data contain information about which key it's encrypted for, and which key signed it?

Yes, looking at https://tools.ietf.org/html/rfc2315 it turns out, that the pair <Issuer Name, SerialNumber> is used to identify the certificate for signature verification, so we may want to store it in longid or, perhaps, in fingerprint field? It may be quite long indeed.

6.7 IssuerAndSerialNumber

   The IssuerAndSerialNumber type identifies a certificate (and thereby
   an entity and a public key) by the distinguished name of the
   certificate issuer and an issuer-specific certificate serial number.

As I can see, forge doesn't support verification yet?

    verify: function() {
      throw new Error('PKCS#7 signature verification not yet implemented.');
    },

[rrrooommmaaa]

Should we check the certification chain when importing a certificate? If not, this IssuerAndSerialNumber seems not very reliable to me, because user may generate a bunch of certificates without caring too much about these fields. We can hash this data to longids and assume they may not be unique, what do you think about it?

[tomholub]

Should we check the certification chain when importing a certificate? If not, this IssuerAndSerialNumber seems not very reliable to me, because user may generate a bunch of certificates without caring too much about these fields. We can hash this data to longids and assume they may not be unique, what do you think about it?

I had the same thought. We don't currently verify the certificate chain, and the extension is not well set up for it either. Eventually, we probably will.

As I can see, forge doesn't support verification yet?

Also interesting - we'd have to implement it in the future.

We can hash this data to longids and assume they may not be unique, what do you think about it?

We could indeed do that. It wouldn't even need to be hashed - nothing stops us from storing 123ABC.. for OpenPGP key and issuer~serial for s/mime, provided that we check that neither issuer nor serial contains ~.

As you say, that would mean when we search by such longid, we may get back more than one result. Once we actually can handle verifying s/mime message signatures, we could refuse to verify and show gray color signature with text "conflicting s/mime certificates found". After that, once we implement verifying the chain, we can choose whichever first key that has valid chain.

---

Therefore, right now a issuer~serial (or a hash of that if you feel there's a benefit - I think maybe there isn't - it doesn't have to look like OpenPGP longid) would do. Can be duplicate. And when we pull back more than one OpenPGP key by longid from local contacts (theoretically possible - there were longid collisions discovered) we could show the gray warning when verifying signature.

Then we just need to put an appropriate comments next to longid property to talk about what we store there for S/MIME.

[rrrooommmaaa]

Therefore, right now a issuer~serial (or a hash of that if you feel there's a benefit - I think maybe there isn't - it doesn't have to look like OpenPGP longid) would do.

Issuer Name consists of attributes, that is, pairs of OID -> Value.
Example of parsed Issuer by forge library:

0:{type: '2.5.4.6', value: 'DE', valueTagClass: 19, name: 'countryName', shortName: 'C'}
1:{type: '2.5.4.8', value: 'Franconia', valueTagClass: 12, name: 'stateOrProvinceName', shortName: 'ST'}
2:{type: '2.5.4.7', value: 'Ansbach', valueTagClass: 12, name: 'localityName', shortName: 'L'}
3:{type: '2.5.4.10', value: 'Stefan Siegl', valueTagClass: 12, name: 'organizationName', shortName: 'O'}
4:{type: '2.5.4.11', value: 'Geierlein', valueTagClass: 12, name: 'organizationalUnitName', shortName: 'OU'}
5:{type: '2.5.4.3', value: 'Geierlein DEV', valueTagClass: 12, name: 'commonName', shortName: 'CN'}
6:{type: '1.2.840.113549.1.9.1', value: 'stesie@brokenpipe.de', valueTagClass: 22, name: 'emailAddress', shortName: 'E'}

Interestingly, the order of the fields is important for forge's findRecipient, otherwise match won't happen.
So, to preserve all this data (of different types -- see valueTagClass), we can, I think, use asn1.toDer() function
and then base64-encode the DER-formatted data. If it's too long, hash it.
Maybe, some projects have already implemented something like this, I can search the github.

[tomholub]

Understood, and agreed. Please see what others do, so that our approach is not too exotic :)

[rrrooommmaaa]

I added a test to actually search for a certificate based on the data from PKCS#7 message (IssuerAndSerialNumber) and it succeeded. However, in some parts of the app it is assumed that "longid" can be derived from the "fingerprint" and we're currently using a different scheme for X.509 certificates. So we either have to refactor the code (especially KeyInfo (?)) or... move X.509 "longid" to "fingerprint", possibly pre- or post-fixed by some other data.
The certificate used in the test generated 200+ characters of Base64 encoded "longid"

[rrrooommmaaa]

For example, we can have <rsa public key fingerprint>-X509-<issuer and serial number> as fingerprint (aka id) for certificates, hence X509-<issuer and serial number> can go to longid.
if public key fingerprint can't be calculated, just -X509-<issuer and serial number> as fingerprint.
Or we can shorten the keys by using hash(issuer and serial number).
What do you think?

[tomholub]

However, in some parts of the app it is assumed that "longid" can be derived from the "fingerprint" and we're currently using a different scheme for X.509 certificates.

I hope there would not be too many of such places. I think they could be refactored away? (always derive longid from whole key instead of from fingerprint) If they cannot be refactored, then we could do something like what you said.

[rrrooommmaaa]

Am I getting it right, that X.509 certificates with private keys (PFX) will also be stored in AcctStore?
I added support for them as well as added type to KeyInfoWithOptionalPp structure (should we rename it?) for easier handling.
The only 2 places where fingerprintToLongid is used are:

1. PgpBlockViewSignatureModule -- is it supposed to be used for OpenPGP keys only?
2. PassphraseStore -- there are "todo"s for refactoring anyway

[tomholub]

Am I getting it right, that X.509 certificates with private keys (PFX) will also be stored in AcctStore?

Yes, soon we will also store these

I added support for them as well as added type to KeyInfoWithOptionalPp structure (should we rename it?) for easier handling.

What would you rename it to?

The only 2 places where fingerprintToLongid is used are:
PgpBlockViewSignatureModule -- is it supposed to be used for OpenPGP keys only?

It's ok if for now it only supports OpenPGP, for now. Once we start supporting verifying S/MIME signatures, we'll have to fix it.

PassphraseStore -- there are "todo"s for refactoring anyway

Ah - I guess here we used to go by longid, then we changed to flexible fingerprint OR longid, with the plan to eventually stop supporting going by longid, which we could indeed do soon.

[rrrooommmaaa]

What would you rename it to?

ExtendedKeyInfo -- the point is -- this structure may have additional fields not needed to be stored.

[tomholub]

Got it, can rename

[tomholub]

Looks good!

[tomholub]

A small nitpick - maybe this migration updateX509FingerprintsAndLongids should be done after moveContactsToEmailsAndPubkeys is already migrated? (switch line order)

[rrrooommmaaa]

The contacts migration moveContactsToEmailsAndPubkeys sets correct fingerprints and longids, we only need to fix contacts migrated by the previous version

[tomholub]

This is interesting - could we instead filter when pulling pubkeys from storage? Eg SELECT FROM pubkey WHERE type = 'x509'. Or we don't store this info?

[rrrooommmaaa]

We don't store this info yet, only in ids (fingerprints) -- postfixed with "-X509", and in longids -- prefixed with "X509-"

[tomholub]

I'll merge this anyway - please let me know your thoughts for the above comments afterwards.