Skip to content

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

Merged
merged 3 commits into from
Sep 5, 2023

Conversation

spikecurtis
Copy link
Contributor

@spikecurtis spikecurtis commented Sep 5, 2023

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.

Signed-off-by: Spike Curtis <spike@coder.com>
Copy link
Member

@johnstcn johnstcn left a 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.

@mafredri
Copy link
Member

mafredri commented Sep 5, 2023

Re: debug symbols, could we strip them after we’ve verified the build?

@spikecurtis
Copy link
Contributor Author

Re: debug symbols, could we strip them after we’ve verified the build?

We could, but then customers wouldn't be able to verify.

@spikecurtis
Copy link
Contributor Author

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.

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.

@mafredri
Copy link
Member

mafredri commented Sep 5, 2023

We could, but then customers wouldn't be able to verify.

I was thinking we would add ldflags+=(-X "'github.com/coder/coder/v2/buildinfo.boringcrypto=true'") and output this in coder version [--output json].

We could also archive the non-stripped binaries for potential verification and/or use where symbols maybe be needed.

@mafredri
Copy link
Member

mafredri commented Sep 5, 2023

I was thinking we would add ldflags+=(-X "'github.com/coder/coder/v2/buildinfo.boringcrypto=true'") and output this in coder version [--output json].

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>
@spikecurtis spikecurtis requested review from mafredri and removed request for kylecarbs September 5, 2023 12:51
@spikecurtis
Copy link
Contributor Author

Thanks @mafredri --- added to BuildInfo

Copy link
Member

@mafredri mafredri left a 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 \
Copy link
Member

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?

Copy link
Contributor Author

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>
@spikecurtis spikecurtis enabled auto-merge (squash) September 5, 2023 13:04
@spikecurtis spikecurtis merged commit 79cd604 into main Sep 5, 2023
@spikecurtis spikecurtis deleted the spike/9087-build-with-boringcrypto branch September 5, 2023 13:12
@github-actions github-actions bot locked and limited conversation to collaborators Sep 5, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Build Coder with boringcrypto
4 participants