use /map: linker flag to avoid running a binary to capture the hash#3133
use /map: linker flag to avoid running a binary to capture the hash#3133yoavwizstein wants to merge 5 commits intoaws:mainfrom
Conversation
There was a problem hiding this comment.
Thanks for this PR! The approach of using the /MAP: linker flag to avoid running a binary during build aligns with how Linux and macOS already work.
The previous capture_hash approach had a nice side-effect: by running fips_empty_main.exe and observing the integrity check fail (with the placeholder hash), it implicitly proved the integrity check machinery was wired up and functional. With the new static-analysis approach, that implicit verification goes away.
To compensate, I've pushed a commit on top of yours that adds FIPS integrity validation to the Windows CI. It does a couple of things:
-
Extends
break-hash.gowith PE support -- this existing tool could only corrupt ELF binaries. It now accepts a-mapflag and uses the same map file +debug/peapproach your PR adds toinject_hash.goto locate and corrupt the FIPS module in a Windows DLL. -
Added a
fips_build_and_testsubroutine torun_windows_tests.bat-- after the normal build and test pass, it runs a negative test: corruptscrypto.dllviabreak-hash.go, runstest_fips.exe, and verifies the process aborts. This proves the integrity check actually runs on DLL load and can detect tampering.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3133 +/- ##
=======================================
Coverage 77.97% 77.97%
=======================================
Files 689 689
Lines 122630 122630
Branches 17078 17077 -1
=======================================
+ Hits 95615 95626 +11
+ Misses 26117 26105 -12
- Partials 898 899 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Haven't looked at the full implementation yet. When inspecting some of the failing CI dimensions, removal of |
c37175b to
ac27524
Compare
|
@justsmth yea i've noticed your changes, thanks for helping here! |
b4c989a to
ff4483b
Compare
Issues:
Resolves #3021
Description of changes:
I've done some rebasing after @justsmth merged #3013 (it was easier for me that way)
Call-outs:
Point out areas that need special attention or support during the review process. Discuss architecture or design changes.
Testing:
Ran some of the local tests, it seems to pass! though i would like to see if ci works, i believe the full testing matrix should pass :)
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license and the ISC license.