Skip to content

Comments

load, encrypt and decrypt private key for S/MIME#3860

Merged
tomholub merged 24 commits intomasterfrom
issue-3529-smime-private-key
Aug 24, 2021
Merged

load, encrypt and decrypt private key for S/MIME#3860
tomholub merged 24 commits intomasterfrom
issue-3529-smime-private-key

Conversation

@rrrooommmaaa
Copy link
Contributor

@rrrooommmaaa rrrooommmaaa commented Jul 28, 2021

This PR does these changes:

  1. As forge.pkcs12.pkcs12FromAsn1(asn, password) throws an exception on invalid password 'PKCS#12 MAC could not be verified. Invalid password?', it's not possible to extract the certificate too, so I changed the internal storage format to those of a typical PEM file containing PKCS#8 (possibly encrypted) private key e.g. -----BEGIN ENCRYPTED PRIVATE KEY----- as well as certificate(s).
    Thus, as we encounter an encrypted private key block, we load it into memory unchanged (in string form), to be later converted to a usable PrivateKey object with forge.pki.decryptRsaPrivateKey()
    The current implementation takes the first certificate in PEM file and assumes that this is the certificate to match the private key.
    Is this correct? Can certificates in PEM file be shuffled, that is can a CA certificate come first, followed by the user certificate?
    To make the app more robust, should we:
  • parse all the certificates to determine the lower-level one?
  • when decrypting the private key, make sure it matches the certificate we previously selected?
  1. Refactoring -- Certificate.privateKey is no longer used (as it's never filled in forge library) so perhaps having this field in types is an error. The benefit is that our own field may be of string (encrypted) or object (ready-to-use) type, allowing the same manipulations as OpenPGP keys.

close #3529


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
Copy link
Contributor Author

@tomholub I added the description on the PR. Can you comment on the general approach as well as the proposed privateKey-certificate matching logic?

@tomholub
Copy link
Collaborator

tomholub commented Aug 15, 2021

Is this correct? Can certificates in PEM file be shuffled, that is can a CA certificate come first, followed by the user certificate?

I don't know. For pkcs12 just about any order is possible, and for pkcs8, I'd also say that it could be shuffled, although there may be a convention to follow certain order (hard to say).

To make the app more robust, should we:

  • parse all the certificates to determine the lower-level one?
  • when decrypting the private key, make sure it matches the certificate we previously selected?

Sounds to me like we should probably do both of these, as you say, for robustness. (unless they are mutually exclusive? I'm not entirely sure what "determine the lower-level one" means)

@tomholub
Copy link
Collaborator

parse all the certificates to determine the lower-level one?

Ok now I get what you mean - you mean the lowest level certificate in the trust chain - the "user certificate" if you will as opposed to the CA certificates.

In the future, it may be good to retain the whole certificate chain, but that is out of scope for now - too much hassle at this stage, I think. Alternatively, instead of keeping whole chain, maybe we could validate the whole chain before we import the key, then forget it (also out of scope now).

Therefore, your approach to throw CAs away and preserve the leaf certificate sounds good to me.

@rrrooommmaaa
Copy link
Contributor Author

Yes, the lower-level certificate is the one in the chain that signs no other certificates.
These measures are not mutually exclusive. In theory, the certificate in the file may not match the private key, we can only find it out after decryption.

Copy link
Collaborator

@tomholub tomholub left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Progressing nicely 👍

@rrrooommmaaa rrrooommmaaa changed the title load private key for S/MIME load, encrypt and decrypt private key for S/MIME Aug 16, 2021
@rrrooommmaaa rrrooommmaaa marked this pull request as ready for review August 24, 2021 11:39
@rrrooommmaaa rrrooommmaaa requested a review from tomholub August 24, 2021 11:46
tomholub
tomholub previously approved these changes Aug 24, 2021
Copy link
Collaborator

@tomholub tomholub left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good!

@tomholub tomholub enabled auto-merge (squash) August 24, 2021 12:14
@tomholub
Copy link
Collaborator

I'll attempt to fix the gmail tests here.

Copy link
Collaborator

@tomholub tomholub left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did various gmail tests cleanup and disabled most Gmail tests due to #3929

@tomholub tomholub merged commit cd9e75a into master Aug 24, 2021
@tomholub tomholub deleted the issue-3529-smime-private-key branch August 24, 2021 16:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

add support for encrypting pkcs12 s/mime key

2 participants