Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

crypto/x509: Update test case in verify_test #40604

Open
SparrowLii opened this issue Aug 6, 2020 · 10 comments
Open

crypto/x509: Update test case in verify_test #40604

SparrowLii opened this issue Aug 6, 2020 · 10 comments
Assignees
Milestone

Comments

@SparrowLii
Copy link
Contributor

@SparrowLii SparrowLii commented Aug 6, 2020

What version of Go are you using (go version)?

go version go1.14.6

Does this issue reproduce with the latest release?

Yes

What operating system and processor architecture are you using (go env)?

windows/amd64

What did you do?

I just run the test instruction in my desktop (go test crypto/x509) .

What did you expect to see?

get the right certificate chain in the test.

What did you see instead?

There was an error followed:

--- FAIL: TestSystemVerify (0.03s)
--- FAIL: TestSystemVerify/SHA-384 (0.00s)
verify_test.go:584: no expected chain matched BR/Moip Pagamentos S.A./MOIP,SSL Blindado EV/api.moip.com.br -> GB/COMODO CA Limited//COMODO RSA Extended Validation Secure Server CA -> GB/COMODO CA Limited//COMODO RSA Certification Authority -> GB/Comodo CA Limited//AAA Certificate Services

The expected chain in the verify_test is:
expectedChains: [][]string{
{
"api.moip.com.br",
"COMODO RSA Extended Validation Secure Server CA",
"COMODO RSA Certification Authority",
"AddTrust External CA Root",
},
},

I'm wondering if this is a wrong of mine alone or we should update the test case either.

@SparrowLii SparrowLii changed the title crypto/x509: Update AddTrust External CA Root in verify_test crypto/x509: Update test case in verify_test Aug 6, 2020
@gopherbot
Copy link

@gopherbot gopherbot commented Aug 6, 2020

Change https://golang.org/cl/247117 mentions this issue: crypto/x509: update test case for windows

@KCoen
Copy link

@KCoen KCoen commented Aug 11, 2020

The root certificate used for this test "AddTrust External CA Root" Expired on 2020-05-30, the test tries to override the time, but this is ignored for root certificates on windows.

I presume this test would succeed on windows if the root certificate was valid.

@hyangah
Copy link
Contributor

@hyangah hyangah commented Aug 13, 2020

cc @golang/osp-team how can we check whether the builders run these tests and if so, why they didn't fail?

@SparrowLii
Copy link
Contributor Author

@SparrowLii SparrowLii commented Aug 13, 2020

@hyangah I tested crypto/x509 in several different desktops and found that only those PCs which installed Windows recently failed. By the way, I has updated my go version to 1.15.
I guess it dose be because "AddTrust External CA Root" Expired, as @KCoen said.

@dmitshur
Copy link
Member

@dmitshur dmitshur commented Aug 13, 2020

how can we check whether the builders run these tests

@hyangah It will be possible to click on the "ok" text at build.golang.org to see successful build logs after #34119 and #34744 are resolved. It's already implemented in my local prototype, which I need to finish. Please subscribe to those issues for updates.

Until then, it needs to be accessed manually. See here for the build log from a recent windows-amd64-2016 builder result. It contains:

windows-amd64-2016 at c2e73fb446bffd02c651e51c6641cc90fd065b70

:: Running C:\workdir\go\src\make.bat with args [...]

[...]
ok  	crypto/x509	1.434s
[...]

if so, why they didn't fail?

The most likely explanation is there is some difference in environment between the builder (its environment variables, Windows image, etc.) and the machine used by the original issue reporter.

Our latest Windows builder images are from 2016 (Windows Server 2016), so based on @SparrowLii's comment, it's possible this would be caught on a builder with a newer Windows image. /cc @andybons @toothrot @cagedmantis @FiloSottile

@alexbrainman
Copy link
Member

@alexbrainman alexbrainman commented Aug 13, 2020

This happens on my computer too. My computer is standard Windows 10. It gets updated automatically every few month without me even noticing. So this would be the same for most Windows 10 users.

I also googled for no expected chain matched BR/Moip Pagamentos, and I found related issues #7824 and #11730. I did not investigate, if they are related or not.

I don't run all.bat regularly. But I am making runtime change, so I run all.bat and noticed this issue.

Let me know, if you need me do bisect this issue.

Thank you.

Alex

@FiloSottile FiloSottile added this to the Go1.16 milestone Sep 22, 2020
@FiloSottile FiloSottile added the Testing label Sep 22, 2020
@FiloSottile FiloSottile self-assigned this Sep 22, 2020
@rolandshoemaker
Copy link
Member

@rolandshoemaker rolandshoemaker commented Sep 22, 2020

Certainly possible that if the builders are using a static Windows image that they aren't pulling changes to the authroot list which may cause the root store to be in a different state than on actual machines?

The CertGetCertificateChain docs are slightly vague about how pTime (which is what we use VerifyOptions.CurrentTime for) is used for root validation:

Note that the time does not affect trust list, revocation, or root store checking. ... Trust in a particular certificate being a trusted root is based on the current state of the root store and not the state of the root store at a time passed in by this parameter.

It could be that CertGetCertificateChain won't pass back a expired root even if it is still in the authroot list and pTime is set to a time when it would be valid...

@KCoen
Copy link

@KCoen KCoen commented Sep 24, 2020

So on machines with both "AddTrust External CA Root" and "AAA Certificate Services" root certs installed, windows will currently only return the AAA Cert causing the test to fail.

The systemLax flag on the test only lets the test pass if the system fails to return any chain.

I found that you can still query the chain leading up to the other root cert by calling CertGetCertificateChain with CERT_CHAIN_RETURN_LOWER_QUALITY_CONTEXTS

I've submitted 'changes' for that here https://go-review.googlesource.com/c/go/+/257257

Eventually machines might not have the original root that the test is trying to find and it'll break again, it might just be a better idea to split out the tests that are ran against a system certificate store, because testing the entire chain against a preset is just gonna keep breaking every couple months on a couple machines

@FiloSottile
Copy link
Member

@FiloSottile FiloSottile commented Oct 5, 2020

@rolandshoemaker you seemed to have looked into this, so I'll leave it to you, but let me know if you have any issue with it.

@gopherbot
Copy link

@gopherbot gopherbot commented Oct 14, 2020

Change https://golang.org/cl/257257 mentions this issue: crypto/x509: return additional chains from Verify on Windows

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
9 participants
You can’t perform that action at this time.