smimesign
smimesign copied to clipboard
Root certs are not used on Windows
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.
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
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.
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.
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.
@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.
Thanks for opening the issue. Someone on our team plans to take a look this later this week.
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
We will chat about it on our team call today.
Thank you @lgarron! All unit tests passing
Can I get a quick update on this? It's been a few months
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.
Been a few months. Can I get a checkup on this?