PKCS#7 CMS signing and verification #526

jonwltn · 2024-12-11T18:39:49Z

This is a very rough, initial draft at adding PKCS#7 CMS signing and verification to KSE. There are lots of TODOs for proper error messaging and missing functionality.

I added a comment to #456 showing sample screenshots. This implementation only supports signing/verification, and I piggybacked off the existing KeyEntry Sign menu item, but if encryption and decryption are going to be added, it might make sense to add new top-level menu item for PKCS#7 CMS operations.

Things left to do:

Fix counter signing when using a timestamp server
Re-enable signature verification code
Establish trust to certs in the current or cacerts keystores during verification
Review all UI strings for clarity and accuracy
Clean up the code

…pe and digest.

Prompt for a file when the content for a detached signature cannot be automatically identified.

… stamp.

kaikramer

It's still a draft, but I thought it might be helpful to give feedback as early as possible.

You are doing well, but feel free to ask in the comments section if there are things that you would like to discuss.

kse/src/main/java/org/kse/crypto/signing/CmsSigner.java

kse/src/main/resources/org/kse/gui/actions/resources.properties

kse/src/main/java/org/kse/crypto/signing/KseSignerInformation.java

kse/src/main/java/org/kse/gui/actions/VerifySignatureAction.java

kse/src/main/java/org/kse/crypto/signing/CmsSigner.java

jonwltn · 2024-12-28T17:53:52Z

Thank you for the review comments. They came at a good time. The commits that you reviewed are structurally nailed down, and I don't anticipate any major changes to the design. I'm mostly cleaning up the code and making certain that all my TODOs are addressed.

Here is a screenshot of the view signature form:

There are two ASN.1 buttons. The top one displays the ASN.1 for the selected signer info. The bottom one displays the ASN.1 for the entire PKCS#7 structure. I can either rename the button to "Signer ASN.1" or just remove the button entirely.

kse/src/main/java/org/kse/crypto/signing/KseSignerInformation.java

kaikramer · 2024-12-31T14:48:22Z

There are two ASN.1 buttons. The top one displays the ASN.1 for the selected signer info. The bottom one displays the ASN.1 for the entire PKCS#7 structure. I can either rename the button to "Signer ASN.1" or just remove the button entirely.

I like having the separate "Signer ASN.1" button because PKCS#7 structures can be quite confusing and being able to filter out the relevant parts is very convenient.

…with Ed448 signature algorithm.

jonwltn · 2025-01-07T18:22:51Z

I want to always use the cacerts and Windows trusted root certs for verifying the Time Stamp Token certs since TSAs should be using trusted certs. I want to directly use AuthorityCertificates rather than the helper methods in AuthorityCertificatesAction, but the action super class handles the loading of cacerts. Thoughts?

For verifying signature trust, I am going to build a cert chain to a self-signed cert, if possible since the signature might not have all the certs if it's malformed, then verify the trust of the root of the chain. I think I can use X509CertUtil.establishTrust to build my chain by giving it the list of signature certs (which are not trusted). Then I can take the root from that chain and use X509CertUtil.establishTrust again to try to find a matching cert using the currently loaded keystore.

kaikramer · 2025-01-07T22:30:04Z

I want to always use the cacerts and Windows trusted root certs for verifying the Time Stamp Token certs since TSAs should be using trusted certs. I want to directly use AuthorityCertificates rather than the helper methods in AuthorityCertificatesAction, but the action super class handles the loading of cacerts. Thoughts?

I agree with you that it's better to verify the timestamps regardless of the user settings. Even for the rare cases when a private TSA was used, a trust anchor is probably available in either cacerts or the OS truststore.

However, I'm not sure if I fully understand the question, because the action super class (you mean KeyStoreExplorerAction, right?) does not really provide much for loading cacerts - apart from the preferences with the file location but you can get the preferences always via PreferencesManager.getPreferences().

If you are referring to AuthorityCertificatesAction.loadCaCertificatesKeyStore(), which consists mostly of password input and error handling and that would be annoying to replicate in VerifySignatureAction and therefore you want to move it into its super class? If that is your suggestion, then I am not very happy about it, but I can live with it.

For verifying signature trust, I am going to build a cert chain to a self-signed cert, if possible since the signature might not have all the certs if it's malformed, then verify the trust of the root of the chain. I think I can use X509CertUtil.establishTrust to build my chain by giving it the list of signature certs (which are not trusted). Then I can take the root from that chain and use X509CertUtil.establishTrust again to try to find a matching cert using the currently loaded keystore.

If I understood you correctly then this should work just fine. Just keep in mind that you'll have to provide a matching self-signed trust anchor somewhere, otherwise establishTrust() will just return null. X509CertUtil.establishTrust() with the list parameter is currently private, but you can make it public if necessary.

Some possible sources to consider or take inspiration from:

The JDK class sun/security/pkcs/SignerInfo.java contains a getCertificateChain() method
BC might have something similar
There is also the RFC 5280 "Certificate Path Validation" algorithm (https://datatracker.ietf.org/doc/html/rfc5280#section-6), see VerifyCertificateAction

…ntent is not available.

jonwltn · 2025-01-10T05:53:40Z

I did something wrong when rebasing my branch with the latest changes from main. Now this pull request includes all the files from the last 13 commits. GitHub recommends discarding all my commits, which I'm good with doing since I have another branch with all my changes.

jonwltn · 2025-01-10T06:00:48Z

Ok. This is ready to go. I have one last TODO regarding icons for the signer/counter signer when viewing a signature. Right now, there are no icons in the signer tree view.

What should I do about non-English translations? I've only provided English resource strings in this pull request.

jonwltn · 2025-01-10T06:06:16Z

kse/src/main/java/org/kse/gui/actions/AuthorityCertificatesAction.java

@@ -95,7 +95,7 @@ protected KeyStore getWindowsTrustedRootCertificates() throws CryptoException {
        return windowsTrustedRootCertificates;
    }

-    private KeyStore loadCaCertificatesKeyStore() {
+    protected KeyStore loadCaCertificatesKeyStore() {


I made this method protected so that I could use it in the VerifySignatureAction subclass. This seemed like the best compromise.

jonwltn · 2025-01-10T06:07:50Z

kse/src/main/java/org/kse/gui/actions/VerifySignatureAction.java

+        return certs;
+    }
+
+    private Store<X509CertificateHolder> getTrustedCertsNoPrefs()


This method and the following one are essentially duplicates of the super class methods, but they don't check for the user prefs. These certs are used for verifying the TSA timestamp token certs.

jonwltn · 2025-01-10T06:10:34Z

kse/src/main/java/org/kse/gui/actions/VerifySignatureAction.java

+        }
+    }
+
+    private Collection<X509Certificate> extractCertificates(KeyStore keystore) throws CryptoException {


This extract method is essentially the same as the one in X509CertUtil, but it also reads the certs in chain since the current keystore might have the verifying cert linked to a private key (e.g. signing user wanting to see if the signature if valid).

For other users that didn't sign the file checking for the cert chain is obviously unnecessary.

jonwltn · 2025-01-10T06:11:17Z

kse/src/main/java/org/kse/gui/dialogs/SignerTreeCellRend.java

+
+            cell.setText(signer.getShortName());
+
+            // TODO JW Is an icon for signer tree cell renderer desired?


The last TODO. If icons are something for the future, then I'll just remove the TODO and the commented out code.

jonwltn added 30 commits December 10, 2024 11:27

Initial buttons and menu items for verify signature action.

ca39bf6

Moved appbundler dependency spec to better location.

c581926

Added verify signature file chooser.

74adc62

Added basic signed data/signature verification.

c299030

Hooked up view signature dialog showing only the issuer DN.

46b49c7

Initial population of the form content.

4280243

Initial update to resource files for DViewSignature. Added content ty…

e163bb9

…pe and digest.

Cleaned up view signature dialog and resources. Displayed subject name.

eb73723

Added view certificates button.

ce1f382

Added counter signers button.

806d0f6

Fixed warning about PEMParser not being closed.

0499a55

Fixed potential NPE when a signer is not selected.

ec9b50f

Populated signature algorithm. Verified the signature.

8c7a12b

Signed attribute table can be null.

2694b65

Handle PEM encoded signatures.

3bc633a

Hooked up the PEM button.

85273e2

Hooked up the ANS1 button.

4c3e444

Refactoring and cleanup.

b216d6e

Removed embedded CertHolder to X509Certificate conversion loigc.

90fadb2

Work with both attached and detached signatures.

2a92e6c

Prompt for a file when the content for a detached signature cannot be automatically identified.

Added sign file menu item and dialogs.

c63a671

Hooked up the digital signature code.

b6e5e9a

Added time stamping and viewing of the time stamp signer.

f01e9d4

Added counter signing -- not fully complete (see TODOs).

cdfbfda

Added file chooser for detached content for counter signing.

9413e1b

Clean up. Added icon files for signing. Enabled PEM output.

3322d04

Remove signature output PEM encoding TODO.

7e984c3

Fixed RSAPSS algorithm lookup TODO.

83e7a3c

Fixed DSignFile layout.

ccbbf79

Fixed TODO for CmsSigner exception. Parameterized DigestType for time…

28545d4

… stamp.

jonwltn added 3 commits December 24, 2024 21:40

Cleaned up warnings. Fixed selection of first tree node.

80ccb7e

Added TODO about supported digest types for time stamp servers.

aaeff1d

Remove content type resource strings. Updated TODOs.T

8666cdc

kaikramer reviewed Dec 26, 2024

View reviewed changes

jonwltn commented Dec 29, 2024

View reviewed changes

kse/src/main/java/org/kse/crypto/signing/KseSignerInformation.java Outdated Show resolved Hide resolved

jonwltn added 4 commits January 1, 2025 12:23

Removed unusd resource strings.

2b33253

Removed more unused resource strings. Resolved some TODOs. Fixed TSA …

7cbd87a

…with Ed448 signature algorithm.

Use the explicit provider if one is present for signing.

0189dfc

Resolved more TODOs. Addressed comments from the PR.

7b26dc8

jonwltn added 2 commits January 7, 2025 12:03

Removed feature idea TODOs. Removed unused parameters in DSignFile.

cb684ce

Fixed TODOs in DViewSignature.

801e5df

jonwltn added 4 commits January 7, 2025 14:44

Verify cert trust during signature verification.

508bb70

Use cacerts for verify timestamp cert trust.

38700bd

Resolved some more TODOs. Displayed "Not Verified" when the signed co…

c72b9dc

…ntent is not available.

Removed last remaining TODOs in VerifySignatureAction.

3dc3c8e

jonwltn closed this Jan 10, 2025

jonwltn force-pushed the verify branch from 42ce6e9 to 6c8ecab Compare January 10, 2025 05:53

Merge branch 'verify-tmp' into verify

83589ec

jonwltn reopened this Jan 10, 2025

jonwltn marked this pull request as ready for review January 10, 2025 06:03

jonwltn commented Jan 10, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

PKCS#7 CMS signing and verification #526

PKCS#7 CMS signing and verification #526

jonwltn commented Dec 11, 2024

kaikramer left a comment

jonwltn commented Dec 28, 2024

kaikramer commented Dec 31, 2024

jonwltn commented Jan 7, 2025 •

edited

Loading

kaikramer commented Jan 7, 2025 •

edited

Loading

jonwltn commented Jan 10, 2025

jonwltn commented Jan 10, 2025 •

edited

Loading

jonwltn Jan 10, 2025

jonwltn Jan 10, 2025

jonwltn Jan 10, 2025

jonwltn Jan 10, 2025


		cell.setText(signer.getShortName());

		// TODO JW Is an icon for signer tree cell renderer desired?

PKCS#7 CMS signing and verification #526

Are you sure you want to change the base?

PKCS#7 CMS signing and verification #526

Conversation

jonwltn commented Dec 11, 2024

kaikramer left a comment

Choose a reason for hiding this comment

jonwltn commented Dec 28, 2024

kaikramer commented Dec 31, 2024

jonwltn commented Jan 7, 2025 • edited Loading

kaikramer commented Jan 7, 2025 • edited Loading

jonwltn commented Jan 10, 2025

jonwltn commented Jan 10, 2025 • edited Loading

jonwltn Jan 10, 2025

Choose a reason for hiding this comment

jonwltn Jan 10, 2025

Choose a reason for hiding this comment

jonwltn Jan 10, 2025

Choose a reason for hiding this comment

jonwltn Jan 10, 2025

Choose a reason for hiding this comment

jonwltn commented Jan 7, 2025 •

edited

Loading

kaikramer commented Jan 7, 2025 •

edited

Loading

jonwltn commented Jan 10, 2025 •

edited

Loading