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

Jump to bottom

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

Closed
rsc opened this issue Mar 25, 2022 · 40 comments
Closed

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

rsc opened this issue Mar 25, 2022 · 40 comments
Assignees
@rsc
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Milestone
Go1.19

Comments

@rsc
Copy link
Contributor

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 The path to resolution is known, but the work has not been done. 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

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

@gopherbot
Copy link

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

@gopherbot
Copy link

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

@gopherbot
Copy link

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

@gopherbot
Copy link

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

@gopherbot
Copy link

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

@gopherbot
Copy link

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

@gopherbot
Copy link

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

@gopherbot
Copy link

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

@gopherbot
Copy link

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 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.

@dagood dagood mentioned this issue Mar 28, 2022
Closed
gopherbot pushed a commit that referenced this issue Mar 30, 2022
@rsc
[dev.boringcrypto] make.bash: disable GOEXPERIMENT when using bootstr…
9d6ab82 
…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

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

@gopherbot
Copy link

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
@rsc
make.bash: disable GOEXPERIMENT when using bootstrap toolchain
ef55053 
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
@rsc
internal/goexperiment: add GOEXPERIMENT=boringcrypto
5c4ed73 
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
@rsc
dashboard: add linux-amd64-boringcrypto builder
d176b7f 
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

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

@gopherbot
Copy link

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

@gopherbot
Copy link

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

@gopherbot
Copy link

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

@gopherbot
Copy link

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

@gopherbot
Copy link

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

@gopherbot
Copy link

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

@gopherbot
Copy link

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

@gopherbot
Copy link

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

@seankhliao seankhliao mentioned this issue Jun 5, 2022
Closed
gopherbot pushed a commit that referenced this issue Jun 6, 2022
@rsc
all: boringcrypto post-merge cleanup
ea5d7cb 
This CL addresses the comments on CL 403154.

For #51940.

Change-Id: I99bb3530916d469077bfbd53095bfcd1d2aa82ef
Reviewed-on: https://go-review.googlesource.com/c/go/+/403976
Reviewed-by: Roland Shoemaker <roland@golang.org>
TryBot-Result: Gopher Robot <gobot@golang.org>
Run-TryBot: Russ Cox <rsc@golang.org>
Reviewed-by: Ian Lance Taylor <iant@google.com>
@seankhliao seankhliao mentioned this issue Jun 7, 2022
Closed
@qmuntal qmuntal mentioned this issue Jun 22, 2022
Closed
@gopherbot
Copy link

Change https://go.dev/cl/413754 mentions this issue: crypto/sha1: remove boring indirection

@qmuntal
Copy link
Contributor

qmuntal commented Jun 23, 2022

@rsc I've submitted https://go.dev/cl/413754 which cleans a longstanding crypto/sha1 leftover related to boring. Hope it can make it into go1.19 as this issue is still open.

@ianlancetaylor
Copy link
Contributor

@rsc Is there anything left to do for this issue?

@rsc
Copy link
Contributor Author

rsc commented Aug 2, 2022

This is all done.

@rsc rsc closed this as completed Aug 2, 2022
@liggitt
Copy link
Contributor

liggitt commented Aug 5, 2022 •
edited

from #51940 (comment):

@srikanthps They will not. You will be able to use go build -tags boringcrypto in a standard Go 1.19 release. Note that all things boringcrypto are unsupported, and this is not guaranteed to continue working in future releases.

maybe I misunderstood, but I was not able to get this to work:

go build -tags boringcrypto ...

This did work (and auto-set a boringcrypto build tag internally that was usable in .go files):

GOEXPERIMENT=boringcrypto go build ...

@ianlancetaylor
Copy link
Contributor

@rsc Was the new approach of GOEXPERIMENT=boringcrypto ever documented anywhere?

@rsc
Copy link
Contributor Author

rsc commented Sep 26, 2022

We don't officially support boringcrypto. To the extent that there was documentation before, it was at https://go.googlesource.com/go/+/refs/heads/dev.boringcrypto/README.boringcrypto.md, and that has been updated.

@rsc
Copy link
Contributor Author

rsc commented Sep 26, 2022

@liggitt, missed your Aug 5 comment. You found the right answer: GOEXPERIMENT works, -tags does not. I've updated my comment above.

@wadey wadey mentioned this issue Oct 19, 2022
Open
@mjlshen mjlshen mentioned this issue Nov 11, 2022
Merged
@moolen moolen mentioned this issue Nov 17, 2022
Closed
@buger buger mentioned this issue Dec 19, 2022
Open
@lmarchione-r7 lmarchione-r7 mentioned this issue Jan 11, 2023
Closed
@tommyknows tommyknows mentioned this issue Feb 27, 2023
Merged
maxlazio pushed a commit to gitlabhq/gitlab-shell that referenced this issue Mar 3, 2023
@stanhu
Prepare for Go 1.19 FIPS support
3950652 
https://gitlab.com/gitlab-org/gitlab-shell/-/merge_requests/718 will
make Go 1.19 the default for gitlab-shell. Per
golang/go#51940, the dev.boringcrypto branch
no longer exists, and to support FIPS we need to pass along
`GOEXPERIMENT=boringcrypto`.

To do this, we just see if this `GOEXPERIMENT` is available with `go
version` rather than do some more complicated version-specific
comparison.
maxlazio pushed a commit to gitlabhq/gitlab-shell that referenced this issue Mar 3, 2023
@stanhu
Prepare for Go 1.19 FIPS support
0e5e33e 
https://gitlab.com/gitlab-org/gitlab-shell/-/merge_requests/718 will
make Go 1.19 the default for gitlab-shell. Per
golang/go#51940, the dev.boringcrypto branch
no longer exists, and to support FIPS we need to pass along
`GOEXPERIMENT=boringcrypto`.

To do this, we just see if this `GOEXPERIMENT` is available with `go
version` rather than do some more complicated version-specific
comparison.
gitlab-runner-bot pushed a commit to gitlabhq/gitlab-runner that referenced this issue Mar 3, 2023
@stanhu
Use Labkit for FIPS check
6b96eb7 
We can reduce code duplication used for the FIPS check by using
LabKit's implementation.

LabKit uses the `fips` tag instead of the `boringcrypto` tag, which is
deprecated in any case and replaced with `GOEXPERIMENT=boringcrypto`
due to golang/go#51940.

This commit changes the message on a FIPS system from:

```
FIPS mode enabled. Using BoringSSL.
```

to:

```
FIPS mode is enabled. Using an external SSL library.
```

On a non-FIPS system, this commit changes the message from:

```
GitLab Runner was compiled with FIPS mode but BoringSSL is not enabled.
```

to:

```
Binary was compiled with FIPS mode, but an external SSL library was not enabled.
```
maxlazio pushed a commit to gitlabhq/gitlab-shell that referenced this issue Mar 3, 2023
@stanhu
Prepare for Go 1.19 FIPS support
f56f1fc 
https://gitlab.com/gitlab-org/gitlab-shell/-/merge_requests/718 will
make Go 1.19 the default for gitlab-shell. Per
golang/go#51940, the dev.boringcrypto branch
no longer exists, and to support FIPS we need to pass along
`GOEXPERIMENT=boringcrypto`.

To do this, we just see if this `GOEXPERIMENT` is available with `go
version` rather than do some more complicated version-specific
comparison.
@linouk23
Copy link

linouk23 commented Mar 4, 2023

👋 everyone, could someone take a look at https://stackoverflow.com/questions/75638176/how-can-i-check-whether-my-golang-app-is-fips-compliant, thanks!

gitlab-runner-bot pushed a commit to gitlabhq/gitlab-runner that referenced this issue Mar 6, 2023
Fix go-fips build image
ba4c482 
* Install go "manually" from tarball in ci go-fips container. We need go
  1.19 to install go-fips 1.19, and the image only has 1.18.
* Specify `GOEXPERIMENT=boringcrypto` when building the fips version of
  runner. As @stanhu mentions, this is necessary because of
  https://github.com/golang/go/blob/dev.boringcrypto/README.boringcrypto.md

See:
* golang/go#51940
* golang-fips/go#59
maxlazio pushed a commit to gitlabhq/gitlab-shell that referenced this issue Mar 7, 2023
@stanhu
Prepare for Go 1.19 FIPS support
4f5f998 
https://gitlab.com/gitlab-org/gitlab-shell/-/merge_requests/718 will
make Go 1.19 the default for gitlab-shell. Per
golang/go#51940, the dev.boringcrypto branch
no longer exists, and to support FIPS we need to pass along
`GOEXPERIMENT=boringcrypto`.

To do this, we just see if this `GOEXPERIMENT` is available with `go
version` rather than do some more complicated version-specific
comparison.
maxlazio pushed a commit to gitlabhq/gitlab-shell that referenced this issue Mar 15, 2023
@stanhu
Prepare for Go 1.19 FIPS support
b6f01ec 
https://gitlab.com/gitlab-org/gitlab-shell/-/merge_requests/718 will
make Go 1.19 the default for gitlab-shell. Per
golang/go#51940, the dev.boringcrypto branch
no longer exists, and to support FIPS we need to pass along
`GOEXPERIMENT=boringcrypto`.

To do this, we just see if this `GOEXPERIMENT` is available with `go
version` rather than do some more complicated version-specific
comparison.
@muhlemmer muhlemmer mentioned this issue Mar 23, 2023
Open
5 tasks
@golang golang locked and limited conversation to collaborators Mar 3, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees

@rsc rsc

Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Projects
Release Status & Release Blockers
Status: Done
Milestone
Go1.19
Development

No branches or pull requests
8 participants
@srikanthps @rsc @liggitt @ianlancetaylor @qmuntal @linouk23 @gopherbot @pfox1969