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

all: move dev.boringcrypto into main branch behind GOEXPERIMENT #51940

Open
rsc opened this issue Mar 25, 2022 · 28 comments
Open

all: move dev.boringcrypto into main branch behind GOEXPERIMENT #51940

rsc opened this issue Mar 25, 2022 · 28 comments
Assignees
Labels
NeedsFix
Milestone

Comments

@rsc
Copy link
Contributor

@rsc rsc commented Mar 25, 2022

The dev.boringcrypto branch started out as a bit of an experiment, back in the Go 1.8 time frame. It is clearly here to stay as something that we maintain alongside the main distribution.

Maintaining a whole separate branch is cumbersome, requiring frequent conflict resolution during merges and being just generally painful.

It would be far less upkeep if we kept the boringcrypto code in the main branch behind a GOEXPERIMENT, same as we do for GOEXPERIMENT=fieldtrack. We should do that.

This bug is to track work toward that goal. Generally speaking it will require a little bit of rewriting of parts that we can't reasonably merge and then a bunch of build tags.

@rsc rsc added the NeedsFix label Mar 25, 2022
@rsc rsc added this to the Go1.19 milestone Mar 25, 2022
@rsc rsc self-assigned this Mar 25, 2022
@gopherbot
Copy link

@gopherbot gopherbot commented Mar 25, 2022

Change https://go.dev/cl/395815 mentions this issue: dashboard: add linux-amd64-boringcrypto builder

@gopherbot
Copy link

@gopherbot gopherbot commented Mar 25, 2022

Change https://go.dev/cl/395881 mentions this issue: [dev.boringcrypto] all: add boringcrypto build tags

@gopherbot
Copy link

@gopherbot gopherbot commented Mar 25, 2022

Change https://go.dev/cl/395879 mentions this issue: [dev.boringcrypto] make.bash: disable GOEXPERIMENT when using bootstrap toolchain

@gopherbot
Copy link

@gopherbot gopherbot commented Mar 25, 2022

Change https://go.dev/cl/395880 mentions this issue: [dev.boringcrypto] internal/goexperiment: add GOEXPERIMENT=boringcrypto

@gopherbot
Copy link

@gopherbot gopherbot commented Mar 25, 2022

Change https://go.dev/cl/395883 mentions this issue: [dev.boringcrypto] crypto/ecdsa, crypto/rsa: use boring.Cache

@gopherbot
Copy link

@gopherbot gopherbot commented Mar 25, 2022

Change https://go.dev/cl/395884 mentions this issue: [dev.boringcrypto] cmd/compile: remove the awful boringcrypto kludge

@gopherbot
Copy link

@gopherbot gopherbot commented Mar 25, 2022

Change https://go.dev/cl/395876 mentions this issue: [dev.boringcrypto] crypto/internal/boring: make SHA calls allocation-free

@gopherbot
Copy link

@gopherbot gopherbot commented Mar 25, 2022

Change https://go.dev/cl/395878 mentions this issue: [dev.boringcrypto] crypto/x509: rename VerifyOptions.IsBoring to AllowCert

@gopherbot
Copy link

@gopherbot gopherbot commented Mar 25, 2022

Change https://go.dev/cl/395882 mentions this issue: [dev.boringcrypto] crypto/internal/boring: add GC-aware cache

@gopherbot
Copy link

@gopherbot gopherbot commented Mar 25, 2022

Change https://go.dev/cl/395877 mentions this issue: [dev.boringcrypto] crypto/..., go/build: align deps test with standard rules

@qmuntal
Copy link
Contributor

@qmuntal qmuntal commented Mar 28, 2022

@rsc have you considered moving crypto/internal/boring to golang.org/x/crypto/boring and use it as a vendored package? This approach is showing good results in our out-of-tree OpenSSL port, reducing code bloat in the main repo and making it easier to maintain and backport.

gopherbot pushed a commit that referenced this issue Mar 30, 2022
…ap toolchain

When using Go 1.4 this doesn't matter, but when using Go 1.17,
the bootstrap toolchain will complain about unknown GOEXPERIMENT settings.
Clearly GOEXPERIMENT is for the toolchain being built, not the bootstrap.

For #51940.

Change-Id: Iff77204391a5a66f7eecab1c7036ebe77e1a4e82
Reviewed-on: https://go-review.googlesource.com/c/go/+/395879
Trust: Russ Cox <rsc@golang.org>
Run-TryBot: Russ Cox <rsc@golang.org>
Reviewed-by: Ian Lance Taylor <iant@golang.org>
TryBot-Result: Gopher Robot <gobot@golang.org>
@gopherbot
Copy link

@gopherbot gopherbot commented Apr 4, 2022

Change https://go.dev/cl/397894 mentions this issue: make.bash: disable GOEXPERIMENT when using bootstrap toolchain

@gopherbot
Copy link

@gopherbot gopherbot commented Apr 4, 2022

Change https://go.dev/cl/397895 mentions this issue: internal/goexperiment: add GOEXPERIMENT=boringcrypto

gopherbot pushed a commit that referenced this issue Apr 4, 2022
When using Go 1.4 this doesn't matter, but when using Go 1.17,
the bootstrap toolchain will complain about unknown GOEXPERIMENT settings.
Clearly GOEXPERIMENT is for the toolchain being built, not the bootstrap.

Already submitted as CL 395879 on the dev.boringcrypto branch,
but needed on master to set up GOEXPERIMENT=boringcrypto
builder ahead of merge.

For #51940.

Change-Id: Ib6a4099cca799b4d5df1974cdb5471adb0fd557d
Reviewed-on: https://go-review.googlesource.com/c/go/+/397894
Trust: Russ Cox <rsc@golang.org>
Run-TryBot: Russ Cox <rsc@golang.org>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Ian Lance Taylor <iant@golang.org>
gopherbot pushed a commit that referenced this issue Apr 4, 2022
Not hooked up to everything else yet.

Copy of CL 395880, for setting up GOEXPERIMENT=boringcrypto
builder ahead of merge.

For #51940.

Change-Id: If842761f77d07329d88748990b95f4b39c2f153a
Reviewed-on: https://go-review.googlesource.com/c/go/+/397895
Trust: Russ Cox <rsc@golang.org>
Run-TryBot: Russ Cox <rsc@golang.org>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Ian Lance Taylor <iant@golang.org>
gopherbot pushed a commit to golang/build that referenced this issue Apr 5, 2022
As of CL 397895, GOEXPERIMENT=boringcrypto is understood
on the master branch (and is a no-op). This CL adds a
linux-amd64-boringcrypto builder in advance of merging
actual boringcrypto code behind that GOEXPERIMENT flag.

For golang/go#51940.

Change-Id: I6611caf8f7a10f334e5343cadaf3b1c1e5bf4b2f
Reviewed-on: https://go-review.googlesource.com/c/build/+/395815
Trust: Russ Cox <rsc@golang.org>
Run-TryBot: Russ Cox <rsc@golang.org>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Dmitri Shuralyov <dmitshur@golang.org>
@gopherbot
Copy link

@gopherbot gopherbot commented Apr 26, 2022

Change https://go.dev/cl/402187 mentions this issue: [dev.boringcrypto] crypto/internal/boring: add GC-aware cache

@gopherbot
Copy link

@gopherbot gopherbot commented Apr 26, 2022

Change https://go.dev/cl/402182 mentions this issue: [dev.boringcrypto] all: add boringcrypto build tags

@gopherbot
Copy link

@gopherbot gopherbot commented Apr 26, 2022

Change https://go.dev/cl/402185 mentions this issue: [dev.boringcrypto] crypto/..., go/build: align deps test with standard rules

@gopherbot
Copy link

@gopherbot gopherbot commented Apr 26, 2022

Change https://go.dev/cl/402188 mentions this issue: [dev.boringcrypto] crypto/ecdsa, crypto/rsa: use boring.Cache

@gopherbot
Copy link

@gopherbot gopherbot commented Apr 26, 2022

Change https://go.dev/cl/402189 mentions this issue: [dev.boringcrypto] cmd/compile: remove the awful boringcrypto kludge

@gopherbot
Copy link

@gopherbot gopherbot commented Apr 26, 2022

Change https://go.dev/cl/402186 mentions this issue: [dev.boringcrypto] crypto/x509: remove VerifyOptions.IsBoring

@gopherbot
Copy link

@gopherbot gopherbot commented Apr 26, 2022

Change https://go.dev/cl/402184 mentions this issue: [dev.boringcrypto] crypto/internal/boring: make SHA calls allocation-free

@gopherbot
Copy link

@gopherbot gopherbot commented Apr 27, 2022

Change https://go.dev/cl/402596 mentions this issue: [dev.boringcrypto] cmd/go: pass dependency syso to cgo too

@gopherbot
Copy link

@gopherbot gopherbot commented Apr 27, 2022

Change https://go.dev/cl/395875 mentions this issue: [dev.boringcrypto] crypto/internal/boring: avoid allocation in big.Int conversion

@gopherbot
Copy link

@gopherbot gopherbot commented Apr 27, 2022

Change https://go.dev/cl/402595 mentions this issue: [dev.boringcrypto] cmd: use notsha256 instead of md5, sha1, sha256

@gopherbot
Copy link

@gopherbot gopherbot commented Apr 27, 2022

Change https://go.dev/cl/402594 mentions this issue: [dev.boringcrypto] cmd/internal/notsha256: add new package

@gopherbot
Copy link

@gopherbot gopherbot commented Apr 27, 2022

Change https://go.dev/cl/402597 mentions this issue: [dev.boringcrypto] cmd/dist: default to use of boringcrypto

gopherbot pushed a commit that referenced this issue Apr 29, 2022
Package notsha256 implements the NOTSHA256 hash,
defined as bitwise NOT of SHA-256.

It will be used from the Go compiler toolchain where an
arbitrary hash is needed and the code currently reaches
for MD5, SHA1, or SHA256. The problem with all of those
is that when we add GOEXPERIMENT=boringcrypto, the
bootstrap process will not converge if the compiler itself
depends on the boringcrypto cgo code.
Using notsha256 avoids boringcrypto.

It is possible that I don't fully understand the convergence
problem and that there is a way to make the compiler converge
when using cgo, but keeping cgo out of the compiler seems safest.
It also makes clear that (except for the hack in codesign)
the code using this package doesn't care which hash is used.

For #51940.

Change-Id: Ie7c661183eacf8413a9d2074c96cbb9361e125ef
Reviewed-on: https://go-review.googlesource.com/c/go/+/402594
Reviewed-by: Ian Lance Taylor <iant@google.com>
Run-TryBot: Russ Cox <rsc@golang.org>
TryBot-Result: Gopher Robot <gobot@golang.org>
gopherbot pushed a commit that referenced this issue Apr 29, 2022
When we add GOEXPERIMENT=boringcrypto, the bootstrap process
will not converge if the compiler itself depends on the boringcrypto
cgo-based implementations of sha1 and sha256.

Using notsha256 avoids boringcrypto and makes bootstrap converge.
Removing md5 is not strictly necessary but it seemed worthwhile to
be consistent.

For #51940.

Change-Id: Iba649507e0964d1a49a1d16e463dd23c4e348f14
Reviewed-on: https://go-review.googlesource.com/c/go/+/402595
Reviewed-by: Ian Lance Taylor <iant@google.com>
Run-TryBot: Russ Cox <rsc@golang.org>
TryBot-Result: Gopher Robot <gobot@golang.org>
gopherbot pushed a commit that referenced this issue Apr 29, 2022
Proposal #42477 asked for a way to apply conditional build tags
to syso files (which have no source code to hold //go:build lines).

We ended up suggesting that the standard answer should be to
put the syso in its own package and then import that package from
a source file that is itself conditionally compiled.

A followup comment on that issue pointed out a problem that I did
not understand until I tried to use this approach myself: the cgo
build fails by default, because the link step only uses syso files from
the current package. You have to override this explicitly by arranging
to pass a “ignore unresolved symbols” flag to the host linker.
Many users will not know how to do this.
(I don't know how to do this off the top of my head.)

If we want users to use this approach, we should make it work better.
This CL does that, by including the syso files from dependencies of
the current package in the link step.

For #51940.

Change-Id: I53a0371b2df17e39a000a645b7686daa6a98722d
Reviewed-on: https://go-review.googlesource.com/c/go/+/402596
Reviewed-by: Ian Lance Taylor <iant@google.com>
Run-TryBot: Russ Cox <rsc@golang.org>
TryBot-Result: Gopher Robot <gobot@golang.org>
gopherbot pushed a commit that referenced this issue Apr 29, 2022
A plain make.bash in this tree will produce a working,
standard Go toolchain, not a BoringCrypto-enabled one.

The BoringCrypto-enabled one will be created with:

	GOEXPERIMENT=boringcrypto ./make.bash

For #51940.

Change-Id: Ia9102ed993242eb1cb7f9b93eca97e81986a27b3
Reviewed-on: https://go-review.googlesource.com/c/go/+/395881
Run-TryBot: Russ Cox <rsc@golang.org>
Reviewed-by: Ian Lance Taylor <iant@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Ian Lance Taylor <iant@golang.org>
gopherbot pushed a commit that referenced this issue Apr 29, 2022
The dev.boringcrypto branch has historically forced use of boringcrypto
with no additional configuration flags. The previous CL undid that.
This CL redoes it, so that direct uses of dev.boringcrypto don't lapse
unexpectedly into not having boringcrypto enabled.

When dev.boringcrypto is merged into master, we will undo this change
as part of the merge, so that the only final difference between master
and dev.boringcrypto will be this CL.

For #51940.

Change-Id: I816593a0b30b4e71093a7da9451bae7807d7167e
Reviewed-on: https://go-review.googlesource.com/c/go/+/402597
Run-TryBot: Russ Cox <rsc@golang.org>
Reviewed-by: Ian Lance Taylor <iant@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
gopherbot pushed a commit that referenced this issue Apr 29, 2022
…t conversion

The conversion via byte slices is inefficient; we can convert via word slices
and avoid the copy entirely.

For #51940.

Change-Id: I06f747e0acffffae427d9706d43bdacf146c027d
Reviewed-on: https://go-review.googlesource.com/c/go/+/395875
Reviewed-by: Roland Shoemaker <roland@golang.org>
Run-TryBot: Russ Cox <rsc@golang.org>
TryBot-Result: Gopher Robot <gobot@golang.org>
gopherbot pushed a commit that referenced this issue Apr 29, 2022
…free

The standard Go implementations are allocation-free.
Making the BoringCrypto ones the same helps avoid
surprises, including in some of our own tests.

For #51940.

Change-Id: Ic9c5dc46f5e29ca85f571244be2b380ec2cf89c9
Reviewed-on: https://go-review.googlesource.com/c/go/+/395876
Reviewed-by: Roland Shoemaker <roland@golang.org>
Run-TryBot: Russ Cox <rsc@golang.org>
TryBot-Result: Gopher Robot <gobot@golang.org>
gopherbot pushed a commit that referenced this issue Apr 29, 2022
…d rules

One annoying difference between dev.boringcrypto and master is that
there is not a clear separation between low-level (math/big-free)
crypto and high-level crypto, because crypto/internal/boring imports
both encoding/asn1 and math/big.

This CL removes both those problematic imports and aligns the
dependency rules in the go/build test with the ones in the main
branch.

To remove encoding/asn1, the crypto/internal/boring APIs change to
accepting and returning encoded ASN.1, leaving crypto/ecdsa to do the
marshaling and unmarshaling, which it already contains code to do.

To remove math/big, the crypto/internal/boring package defines
type BigInt []uint, which is the same representation as a big.Int's
internal storage. The new package crypto/internal/boring/bbig provides
conversions between BigInt and *big.Int. The boring package can then
be in the low-level crypto set, and any package needing to use bignum
APIs (necessarily in the high-level crypto set) can import bbig to
convert.

To simplify everything we hide from the test the fact that
crypto/internal/boring imports cgo. Better to pretend it doesn't and
keep the prohibitions that other packages like crypto/aes must not use
cgo (outside of BoringCrypto).

	$ git diff origin/master src/go/build/deps_test.go
	diff --git a/src/go/build/deps_test.go b/src/go/build/deps_test.go
	index 6ce872e297..a63979cc93 100644
	--- a/src/go/build/deps_test.go
	+++ b/src/go/build/deps_test.go
	@@ -402,9 +402,13 @@ var depsRules = `
	 	NET, log
	 	< net/mail;

	+	NONE < crypto/internal/boring/sig;
	+	sync/atomic < crypto/internal/boring/fipstls;
	+	crypto/internal/boring/sig, crypto/internal/boring/fipstls < crypto/tls/fipsonly;
	+
	 	# CRYPTO is core crypto algorithms - no cgo, fmt, net.
	 	# Unfortunately, stuck with reflect via encoding/binary.
	-	encoding/binary, golang.org/x/sys/cpu, hash
	+	crypto/internal/boring/sig, encoding/binary, golang.org/x/sys/cpu, hash
	 	< crypto
	 	< crypto/subtle
	 	< crypto/internal/subtle
	@@ -413,6 +417,8 @@ var depsRules = `
	 	< crypto/ed25519/internal/edwards25519/field, golang.org/x/crypto/curve25519/internal/field
	 	< crypto/ed25519/internal/edwards25519
	 	< crypto/cipher
	+	< crypto/internal/boring
	+	< crypto/boring
	 	< crypto/aes, crypto/des, crypto/hmac, crypto/md5, crypto/rc4,
	 	  crypto/sha1, crypto/sha256, crypto/sha512
	 	< CRYPTO;
	@@ -421,6 +427,7 @@ var depsRules = `

	 	# CRYPTO-MATH is core bignum-based crypto - no cgo, net; fmt now ok.
	 	CRYPTO, FMT, math/big, embed
	+	< crypto/internal/boring/bbig
	 	< crypto/rand
	 	< crypto/internal/randutil
	 	< crypto/ed25519
	@@ -443,7 +450,8 @@ var depsRules = `
	 	< golang.org/x/crypto/hkdf
	 	< crypto/x509/internal/macos
	 	< crypto/x509/pkix
	-	< crypto/x509
	+	< crypto/x509;
	+	crypto/internal/boring/fipstls, crypto/x509
	 	< crypto/tls;

	 	# crypto-aware packages
	@@ -653,6 +661,9 @@ func findImports(pkg string) ([]string, error) {
	 	}
	 	var imports []string
	 	var haveImport = map[string]bool{}
	+	if pkg == "crypto/internal/boring" {
	+		haveImport["C"] = true // kludge: prevent C from appearing in crypto/internal/boring imports
	+	}
	 	fset := token.NewFileSet()
	 	for _, file := range files {
	 		name := file.Name()

For #51940.

Change-Id: I26fc752484310d77d22adb06495120a361568d04
Reviewed-on: https://go-review.googlesource.com/c/go/+/395877
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Roland Shoemaker <roland@golang.org>
Run-TryBot: Russ Cox <rsc@golang.org>
gopherbot pushed a commit that referenced this issue Apr 29, 2022
This API was added only for BoringCrypto, never shipped in standard
Go. This API is also not compatible with the expected future evolution
of crypto/x509, as we move closer to host verifiers on macOS and Windows.

If we want to merge BoringCrypto into the main tree, it is best not to
have differing API. So instead of a hook set by crypto/tls, move the
actual check directly into crypto/x509, eliminating the need for
exposed API.

For #51940.

Change-Id: Ia2ae98c745de818d39501777014ea8166cab0b03
Reviewed-on: https://go-review.googlesource.com/c/go/+/395878
TryBot-Result: Gopher Robot <gobot@golang.org>
Run-TryBot: Russ Cox <rsc@golang.org>
Reviewed-by: Roland Shoemaker <roland@golang.org>
gopherbot pushed a commit that referenced this issue Apr 29, 2022
In the original BoringCrypto port, ecdsa and rsa's public and private
keys added a 'boring unsafe.Pointer' field to cache the BoringCrypto
form of the key. This led to problems with code that “knew” the layout
of those structs and in particular that they had no unexported fields.

In response, as an awful kludge, I changed the compiler to pretend
that field did not exist when laying out reflect data. Because we want
to merge BoringCrypto in the main tree, we need a different solution.

The different solution is this CL's boring.Cache, which is a
concurrent, GC-aware map from unsafe.Pointer to unsafe.Pointer (if
generics were farther along we could use them nicely here, but I am
afraid of breaking tools that aren't ready to see generics in the
standard library yet).

More complex approaches are possible, but a simple, fixed-size hash
table is easy to make concurrent and should be fine.

For #51940.

Change-Id: I44062a8defbd87b705a787cffc64c6a9d0132785
Reviewed-on: https://go-review.googlesource.com/c/go/+/395882
Run-TryBot: Russ Cox <rsc@golang.org>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Cherry Mui <cherryyz@google.com>
gopherbot pushed a commit that referenced this issue Apr 29, 2022
In the original BoringCrypto port, ecdsa and rsa's public and private
keys added a 'boring unsafe.Pointer' field to cache the BoringCrypto
form of the key. This led to problems with code that “knew” the layout
of those structs and in particular that they had no unexported fields.

In response, as an awful kludge, I changed the compiler to pretend
that field did not exist when laying out reflect data. Because we want
to merge BoringCrypto in the main tree, we need a different solution.
Using boring.Cache is that solution.

For #51940.

Change-Id: Ideb2b40b599a1dc223082eda35a5ea9abcc01e30
Reviewed-on: https://go-review.googlesource.com/c/go/+/395883
Run-TryBot: Russ Cox <rsc@golang.org>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Roland Shoemaker <roland@golang.org>
gopherbot pushed a commit that referenced this issue Apr 29, 2022
CL 60271 introduced this “AwfulBoringCryptoKludge.”
iant approved that CL saying “As long as it stays out of master...”

Now that the rsa and ecdsa code uses boring.Cache, the
“boring unsafe.Pointer” fields are gone from the key structs, and this
code is no longer needed. So delete it.

With the kludge deleted, we are one step closer to being able to merge
dev.boringcrypto into master.

For #51940.

Change-Id: Ie549db14b0b699c306dded2a2163f18f31d45530
Reviewed-on: https://go-review.googlesource.com/c/go/+/395884
Reviewed-by: Ian Lance Taylor <iant@golang.org>
Reviewed-by: Ian Lance Taylor <iant@google.com>
@gopherbot
Copy link

@gopherbot gopherbot commented May 3, 2022

Change https://go.dev/cl/403615 mentions this issue: [dev.boringcrypto] README.boringcrypto.md: add note about the demise of the branch

gopherbot pushed a commit that referenced this issue May 3, 2022
…of the branch

CL 403154 merged the branch back into the standard Go branch.

Fixes #51940.

Change-Id: Ibda89792c01630adce7ee3fbd56edbd780fcf79e
Reviewed-on: https://go-review.googlesource.com/c/go/+/403615
Run-TryBot: Russ Cox <rsc@golang.org>
TryBot-Result: Gopher Robot <gobot@golang.org>
Auto-Submit: Russ Cox <rsc@golang.org>
Reviewed-by: Roland Shoemaker <roland@golang.org>
@gopherbot
Copy link

@gopherbot gopherbot commented May 4, 2022

Change https://go.dev/cl/403976 mentions this issue: all: boringcrypto post-merge cleanup

@srikanthps
Copy link

@srikanthps srikanthps commented May 6, 2022

How will the go distribution with boring crypto be made available? Will they still be available from - https://go-boringcrypto.storage.googleapis.com/ ?

Can the link to pre-built go-boringcrypto be provided in https://go.dev/dl/? This will help us to easily find the package to download.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
NeedsFix
Projects
None yet
Development

No branches or pull requests

4 participants