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

Closed
SparrowLii opened this issue Aug 6, 2020 · 13 comments
Closed

crypto/x509: Update test case in verify_test #40604

SparrowLii opened this issue Aug 6, 2020 · 13 comments
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. Testing An issue that has been verified to require only test changes, not just a test failure.
Milestone

Comments

@SparrowLii
Copy link
Contributor

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
@ALTree ALTree added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Aug 6, 2020
@gopherbot
Copy link

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

@KCoen
Copy link

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 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 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
Contributor

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

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 An issue that has been verified to require only test changes, not just a test failure. label Sep 22, 2020
@FiloSottile FiloSottile self-assigned this Sep 22, 2020
@rolandshoemaker
Copy link
Member

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 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
Contributor

@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

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

@egroj97
Copy link

egroj97 commented Jan 1, 2021

Hello, tried to install from source, but when testing, I get the same error:

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

I'm building from branch: go1.15.6, the error is still showing because it hasn't been added to an official release?

Edit: I'm trying to compile from x86-64 Windows 10 Build 19042.685 using MinGW-W64 GCC 8.1.0 x86-64

@FiloSottile
Copy link
Contributor

Correct, this is fixed in Go 1.16 but not backported to Go 1.15. If you are just trying to build Go, you can ignore the failure and use make.bash. I'm not sure what our policy on fixing tests in old branches is. It would be ok to just skip this test. (The actual fix in Go 1.16 is too large to backport.) /cc @golang/release

@dmitshur
Copy link
Contributor

dmitshur commented Jan 5, 2021

We skip or fix tests in old branches when the test failure is understood and it interferes with the release process, but it's not a goal to always fix all old tests. If this isn't causing much trouble, it seems fine to leave to Go 1.16 where it's fixed.

@golang golang locked and limited conversation to collaborators Jan 5, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. Testing An issue that has been verified to require only test changes, not just a test failure.
Projects
None yet
Development

No branches or pull requests

10 participants