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

x/sys/windows: TestWinVerifyTrust failures on windows-arm64-10 since 2021-10-31 #49266

Closed
bcmills opened this issue Nov 1, 2021 · 14 comments
Closed
Labels
arch-arm64 Builders x/build issues (builders, bots, dashboards) FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done. okay-after-beta1 Used by release team to mark a release-blocker issue as okay to resolve either before or after beta1 OS-Windows release-blocker
Milestone

Comments

@bcmills
Copy link
Contributor

bcmills commented Nov 1, 2021

--- FAIL: TestWinVerifyTrust (0.31s)
    syscall_windows_test.go:654: ntoskrnl.exe did not verify: A required certificate is not within its validity period when verifying against the current system clock or the timestamp in the signed file.
FAIL
FAIL	golang.org/x/sys/windows	1.800s

greplogs --dashboard -md -l -e 'FAIL: TestWinVerifyTrust'

2021-11-01T13:12:37-611d5d6-4056934/windows-arm64-10
2021-11-01T13:08:16-611d5d6-732db40/windows-arm64-10
2021-11-01T02:47:30-611d5d6-fde4cc2/windows-arm64-10
2021-10-31T18:39:05-611d5d6-89c5270/windows-arm64-10
2021-10-31T18:13:09-611d5d6-fd09e88/windows-arm64-10
2021-10-31T17:52:41-611d5d6-3fa9ee4/windows-arm64-10
2021-10-31T15:36:50-611d5d6-243c5ae/windows-arm64-10

@gopherbot gopherbot added this to the Unreleased milestone Nov 1, 2021
@bcmills bcmills added arch-arm64 NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. OS-Windows release-blocker labels Nov 1, 2021
@bcmills bcmills modified the milestones: Unreleased, Go1.18 Nov 1, 2021
@bcmills
Copy link
Contributor Author

bcmills commented Nov 1, 2021

It isn't clear to me why this started on Oct. 31; it does not appear to have been masked by #49217, but the only failures so far are at the most recent commit of x/sys (which fixed that issue, which was itself Windows-specific).

This test itself was added in CL 331010 for #46906 (CC @zx2c4).

This could be due to a bad builder configuration (CC @golang/release).

@zx2c4
Copy link
Contributor

zx2c4 commented Nov 1, 2021

Think you could grab \windows\system32\ntoskrnl.exe from the affected builder? (I would do it myself but I don't have gomote access.) I'd be curious to see if I can repro locally and to see if there's something interesting about the timestamp in the signature or a lack of a timestamp and suchlike.

@dmitshur
Copy link
Contributor

dmitshur commented Nov 3, 2021

@zx2c4 Sure, I can get that exe. Will update this in a bit.

Edit: Shared over email.

@dmitshur
Copy link
Contributor

dmitshur commented Nov 3, 2021

@toothrot mentioned potentially needing to update the Windows 10 version that we use on this builder, and I'm starting to realize this failure might be directly related to that. Will discuss with Alex after he's back.

@dmitshur dmitshur added the Builders x/build issues (builders, bots, dashboards) label Nov 3, 2021
@zx2c4
Copy link
Contributor

zx2c4 commented Nov 3, 2021

Thanks for sending me the file. It looks like this is an ntoskrnl.exe from an arm64 insiders build. Apparently test builds don't get their binaries timestamped, so when the CA for this file expired -- 10/31/2021 -- so did the file. Ordinarily there's a counter signature from a timestamp server, with that server's CA being valid for a very long time, so that the actual file CA expiring isn't an issue. But I suppose they don't bother doing these for insider build files, which makes sense. You might consider moving from the insider track to the stable track.

@dmitshur dmitshur added NeedsFix The path to resolution is known, but the work has not been done. and removed NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. labels Nov 5, 2021
@toothrot toothrot self-assigned this Nov 10, 2021
@cagedmantis
Copy link
Contributor

Perhaps the best path forward is skipping the test.

@zx2c4
Copy link
Contributor

zx2c4 commented Nov 10, 2021

I can just sign a file with my EV cert, with a timestamp valid until 2031, and put it in the data directory. This shouldn't be a problem.

However, at the moment I'm a bit loathe to work further on the Go Windows runtime. I spent months working on unbitrotting the arm32 port, successfully did that, turned things over to the Google team to handle the builder, and that's gone totally neglected, and so arm32 support will now bitrot again. At some point I asked for gomote access to fix the race detector on Windows, and that then went nowhere (#44579). More recently, I've tried to make some large additions to x/sys/windows, (CL299009, CL359054), and can't get a +2. The whole thing puts a sour taste in my mouth and makes me wonder why I spend time working on this. If trying to do things here is an uphill battle, I'm not so much interested.

@toothrot
Copy link
Contributor

Hey @zx2c4. I want you to know that we hear you, and we're doing the most we can.

I was never able to get the ARM32 builder to build correctly, and I apologize for that. I'm not sure how to move it forward, as I couldn't get much support on how to correct the issue given that Windows ARM32 support outside of Go seems to be relatively unsupported from any avenue that I could reach. We're also constrained by the hardware we're using to host Windows VMs, and there was a blocking bug that prevented me from making progress. This would all be much easier if there were any Windows ARM VMs available anywhere on the planet.

Secondly, we did mention that gomote access is on hold in #44579. That's still true. Our team has only just recently gotten the bandwidth to correct issues with gomote that prevent us from granting any new access. Even still, due to constraints completely outside of my control, we will be unable to grant access to windows-arm64 builders even after our work is completed. I don't see that changing in the near future, and it's unrelated to any kind of technical problem.

I plan on taking some time out of my weekend in order to update the Windows ARM builders. I unfortunately can't help review the x/sys changes, as it is outside my realm of expertise.

@zx2c4
Copy link
Contributor

zx2c4 commented Nov 10, 2021

Thanks for your message. I really appreciate it.

With regards to the ARM32 VM -- it should work on an M1 using the normal arm64 build of windows. Just make sure you're running the latest stable (22000) rather than an insider build.

@bcmills
Copy link
Contributor Author

bcmills commented Nov 17, 2021

This test is also now failing on windows-386-2012 with a different failure mode (see #49651).

I think we should skip it until it can be made more robust.

@gopherbot
Copy link

Change https://golang.org/cl/364794 mentions this issue: windows: skip TestWinVerifyTrust

gopherbot pushed a commit to golang/sys that referenced this issue Nov 17, 2021
This test is failing (with different failure modes) on two different
builders. Adding a skip until it can be made more robust.

For golang/go#49651
For golang/go#49266
Updates golang/go#46906

Change-Id: I0fdd0e6f729c37e234b62b65abc53003eb8834f0
Reviewed-on: https://go-review.googlesource.com/c/sys/+/364794
Trust: Bryan C. Mills <bcmills@google.com>
Trust: Patrik Nyblom <pnyb@google.com>
Run-TryBot: Bryan C. Mills <bcmills@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Patrik Nyblom <pnyb@google.com>
Reviewed-by: Dmitri Shuralyov <dmitshur@golang.org>
@toothrot toothrot added the okay-after-beta1 Used by release team to mark a release-blocker issue as okay to resolve either before or after beta1 label Nov 19, 2021
@toothrot
Copy link
Contributor

FYI: In #47019 (comment), I just mentioned that we're working on updating the windows builder images.

@gopherbot
Copy link

Change https://golang.org/cl/366655 mentions this issue: windows: re-eenable TestWinVerifyTrust with newly signed file

@gopherbot
Copy link

Change https://golang.org/cl/366655 mentions this issue: windows: re-enable TestWinVerifyTrust with newly signed file

@golang golang locked and limited conversation to collaborators Jun 23, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
arch-arm64 Builders x/build issues (builders, bots, dashboards) FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done. okay-after-beta1 Used by release team to mark a release-blocker issue as okay to resolve either before or after beta1 OS-Windows release-blocker
Projects
Archived in project
Development

No branches or pull requests

6 participants