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.

Sign up for GitHub

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

Jump to bottom

Add debug.BuildSetting entries corresponding to crypto backend usage #963

Open
dagood opened this issue Jun 26, 2023 · 4 comments · May be fixed by #972
Open

Add debug.BuildSetting entries corresponding to crypto backend usage #963

dagood opened this issue Jun 26, 2023 · 4 comments · May be fixed by #972
Labels
fips

Comments

@dagood
Copy link
Member

dagood commented Jun 26, 2023

go version -m example reports the go experiment and some other info that can most likely be used to figure out whether a crypto module was used to build example, but it isn't straightforward. I think we can fairly easily add this info in a more consistent way:

https://pkg.go.dev/runtime/debug#BuildSetting

diff --git a/src/cmd/go/internal/load/pkg.go b/src/cmd/go/internal/load/pkg.go
index c0e6265e29d065..fdeb82b0484ec6 100644
--- a/src/cmd/go/internal/load/pkg.go
+++ b/src/cmd/go/internal/load/pkg.go
@@ -2430,6 +2430,9 @@ func (p *Package) setBuildInfo(ctx context.Context, autoVCS bool) {
 	if key, val := cfg.GetArchEnv(); key != "" && val != "" {
 		appendSetting(key, val)
 	}
+	if ok := cfg.BuildContext.MatchTag("goexperiment.opensslcrypto"); ok {
+		appendSetting("goexperiment.opensslcrypto", "on")
+	}
 
 	// Add VCS status if all conditions are true:
 	//
diff --git a/src/go/build/build.go b/src/go/build/build.go
index 48adcfed5cf3cb..8f757a87c09257 100644
--- a/src/go/build/build.go
+++ b/src/go/build/build.go
@@ -2021,6 +2021,10 @@ func (ctxt *Context) matchTag(name string, allTags map[string]bool) bool {
 	return false
 }
 
+func (ctxt *Context) MatchTag(tag string) bool {
+	return ctxt.matchTag(tag, nil)
+}
+
 // goodOSArchFile returns false if the name contains a $GOOS or $GOARCH
 // suffix which does not match the current system.
 // The recognized name formats are:

This works as expected with go version -m, and other tools can probably be adapted to read the data directly from Go binaries if necessary (e.g. a Go toolset isn't present).
@dagood dagood added the fips label Jun 26, 2023
@dagood
Copy link
Member Author

dagood commented Jun 27, 2023 •
edited
Loading

Maybe this is also a nice way to do:

This would let it be reliably machine-readable without any parsing logic.

@dagood
Copy link
Member Author

dagood commented Jul 5, 2023

Hmm, there is already a boring/backend-specific mechanism that puts a byte sequence in the binary. We could use/extend this rather than using buildinfo:

https://github.com/golang/go/blob/master/src/crypto/internal/boring/sig/sig_amd64.s
https://github.com/rsc/goversion/blob/597212e462da05a7902d6cea0ec895a0d9b8b218/version/read.go#L149-L157

Those sig sequence has always been in boring releases (Go 1.8+), and buildinfo is more recent (Go 1.18+). I'm not sure if one is actually easier to read than another (while avoiding false positives), when considering how something like BinSkim might implement it.

I don't think we could use that as-is: we need to be able to distinguish OpenSSL/CNG usage from Boring usage. We would need to at least add one more sig for "a backend implemented by microsoft/go is in use".

Maybe a worse issue is that if crypto is not used at all, the sig won't be present. This could be a problem if the scanner only looks for "uses OpenSSL" rather than "uses OpenSSL or no crypto is used". Also: to keep the policy simple internally, we might want to enforce that a Go program is built such that it would use OpenSSL/CNG if it were to use crypto. This way the team building the Go app will know right away that they need to adjust the build, rather than (potentially) waiting until they add a dependency on crypto and only finding out then (potentially in an unclear context).

As far as human use goes, it's nice to see this in the buildinfo with a standard go version -m app call rather than needing something else that specifically handles the sig sequence.

@dagood dagood linked a pull request Jul 5, 2023 that will close this issue
Draft
@dagood
Copy link
Member Author

dagood commented Jul 21, 2023 •
edited
Loading

There's some conversation internally about whether we need to do this (for e.g. BinSkim), or if CodeQL already has enough info to detect backends. ("Proposal for new golang query to detect approved usage of crypto" email thread.)

But I'm realizing that IMO, we should do this regardless of what the internal scanning tools need. (In 1.22.)

/cc @microsoft/golang-compiler

CodeQL runs during compilation, and can't be used to investigate a compiled program. BinSkim is (in my experience) awkward to configure for quick sanity checks. Giving our users the ability to run the built-in tool go version -m app and see what it was actually compiled with is valuable in itself.

Some teams may also be motivated to build their own checks into their builds. It might be considered too late if BinSkim detects a problem (perhaps more too late if CodeQL ends up being the internal tool of choice--because the report wouldn't block a build as far as I know).

I think that even if this isn't used in an automated check, teams inside Microsoft that are rebuilding OSS repositories using upstream build scripts will want to make sure that the GOEXPERIMENT made it where it needed to go and had the intended effect. Examining the outputs--even just checking it once, manually--would be a lot easier with this functionality.

Along these lines: we should make sure requirefips is easy to check, too. It's different to get it into the right places as a simple build tag (using GOFLAGS?) vs. a GOEXPERIMENT.

@dagood
Copy link
Member Author

dagood commented Aug 8, 2023

Notes from sync:

  • The prototype (Add the active crypto backend to build info (go version -m app) #972) adds cryptobackend=openssl. We should probably prefix it like ms_cryptobackend=openssl in case upstream uses the same name for boring, or if we want to align this key with other FIPS-interested fork maintainers.
  • Directly including the Go toolset version (including revision) would also be helpful.
  • We can also provide a tool that verifies a given app is built by the Microsoft Go toolset, and if it uses the correct backend for our internal policies.
    • This could be patched into go version. That would make it easy to access just after building an app. This has maintenance overhead (branching and general patch conflict maintenance) and means that you need our toolset to run it, which may not be convenient for verifiers that run separately from the build.
    • We could release this as a standalone tool. It could also work when built by upstream Go, for simplicity, and we could provide prebuilt binaries. It could also be used as a library by other Go apps that want to incorporate this validation.
    • Or both: make it standalone, but also vendor it into our fork and add a flag to go version to run it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
fips
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Add the active crypto backend to build info (go version -m app) microsoft/go
1 participant
@dagood