-
Notifications
You must be signed in to change notification settings - Fork 887
feat: add boringcrypto builds for linux #9528
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
Conversation
Signed-off-by: Spike Curtis <spike@coder.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need to build with boringcrypto, or just provide the option to build with boringcrypto? It's a shame that we need to leave the debugging symbols in.
Re: debug symbols, could we strip them after we’ve verified the build? |
We could, but then customers wouldn't be able to verify. |
We need to be able to ship boringcrypto builds, and I don't think we should ship variants for a single OS/arch: i.e. if you run on Linux amd64 you get boringcrypto. |
I was thinking we would add We could also archive the non-stripped binaries for potential verification and/or use where symbols maybe be needed. |
Actually, on second thought, we could utilize build tags here: //go:build boringcrypto
package buildinfo
import "crypto/boring"
var (
boringcrypto = boring.Enabled()
) With this, I assume we may not even need to verify via symbols 🤔 (but we could, just to be safe). |
Signed-off-by: Spike Curtis <spike@coder.com>
Thanks @mafredri --- added to BuildInfo |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking nice!
Edit: Looks like TestVersion/JSON_output
needs updating.
goexp="boringcrypto" | ||
fi | ||
|
||
GOEXPERIMENT="$goexp" CGO_ENABLED="$cgo" GOOS="$os" GOARCH="$arch" GOARM="$arm_version" go build \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does -boringcrypto
also need to be added to go build
args?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nope, I tested coder version
on a binary I built. GOEXPERIMENT=boringcrypto
is enough.
Signed-off-by: Spike Curtis <spike@coder.com>
fixes #9087
Adds support for building with Boring Crypto on Linux, when not cross-compiling.
It's maybe a little surprising for us to build different versions of Coder binaries depending on where you build. I chose to do it this way so that everyone gets boring crypto if available on their platform, so that we're not having people develop against non-boring-crypto and then shipping a different build that we haven't been testing with.
It also leaves debug symbols in boring crypto binaries, since it is hard to verify we are building with boring crypto without these. This increases binary sizes by about 5MiB for the linux amd64 slim binary (39 -> 44) and 8MiB (184 -> 192) for the fat linux amd64 binary.