smimesign icon indicating copy to clipboard operation
smimesign copied to clipboard

Root certs are not used on Windows

Open Kurlee opened this issue 4 years ago • 12 comments

The implementation of the verify command fails to read from the system root store on Windows. This was likely done due to issues with the x509 library which fails to parse the root store as detailed in Issue #16736 of the golang crypto library. Windows instead was defaulting to the gocertifi library, which prevents users from adding or removing trusted roots, and verifying signatures against those roots.

In the golang crypto issue referenced before, there is a workaround which enables us to parse the root store and verify signatures against these roots. I have adopted this fix to this project.

All tests passing locally.

Thanks to @ndouthit for help in troubleshooting this issue.

Kurlee avatar Jul 27 '20 11:07 Kurlee

I see these tests failing on OSX, I will address this by first attempting to read the store using x509.SystemCertPool() call to basically test if I am on Windows or OSX and proceed from there

Kurlee avatar Jul 27 '20 11:07 Kurlee

Apologies for the bad initial request. Testing was passing locally, but failed in travis due to OSX throwing errors with the windows specific syscall methods. Tests are now passing with the go 1.12 and 1.x configurations and failing on master. This is consistent with the state of the project before this fix.

Kurlee avatar Jul 27 '20 13:07 Kurlee

Thanks for opening the PR. We have one other PR that is also failing CI for similar reasons (I think). Let us take a look at that and circle back.

ptoomey3 avatar Jul 27 '20 14:07 ptoomey3

It isn't just the other PR that is failing CI, the whole project failed for the go: master configuration since the latest commit 7e96b8b53e6fd45c9f9435ccd57ca102dba7eb06.

Kurlee avatar Jul 27 '20 14:07 Kurlee

@ptoomey3, the failing Travis tests are not due to any issue with the changes contained in this PR. Unit tests are passing on the 1.12 and 1.14 golang configurations in Travis. I opened an issue to address the failing Travis tests here, and I'll happily help troubleshoot this issue.

Respectfully request that you ignore the current state of Travis testing to address the issue with Windows Cert store. The results of these tests match the latest commit to this project.

Kurlee avatar Jul 28 '20 13:07 Kurlee

Thanks for opening the issue. Someone on our team plans to take a look this later this week.

ptoomey3 avatar Jul 28 '20 14:07 ptoomey3

As soon as https://github.com/github/smimesign/pull/69 gets pulled in, I'll update this branch so CI will be passing. Can I get an estimate since getting the Windows root store fix is important to my project? @ptoomey3

Kurlee avatar Aug 10 '20 12:08 Kurlee

We will chat about it on our team call today.

ptoomey3 avatar Aug 10 '20 13:08 ptoomey3

Thank you @lgarron! All unit tests passing

Kurlee avatar Aug 19 '20 08:08 Kurlee

Can I get a quick update on this? It's been a few months

Kurlee avatar Dec 10 '20 12:12 Kurlee

Thanks for the review @vcsjones. I have addressed your concern about unreliable enumeration of the windows root store in the following way. I used gocertifi to initialize the CertPool and then add the system certificates to this pool. The 'x509.CertPool.AddCert' function contains logic to prevent a duplicate cert from being added to the pool, so de-duplication is not required.

I hope this addresses your concerns sufficiently. There is still a chance that trusted certificates that are not in the Mozilla trusted roots bundle will not be enumerated and verification will fail, but there is now a baseline of trusted certificates which will always populate.

Kurlee avatar Dec 21 '20 17:12 Kurlee

Been a few months. Can I get a checkup on this?

Kurlee avatar Mar 13 '21 01:03 Kurlee