Skip to content

net/http/httptest: add fipsonly compliant certificate in for NewTLSServer(), for dev.boringcrypto branch #48674

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
Closed
saurabhsuniljain opened this issue Sep 28, 2021 · 6 comments
Closed

net/http/httptest: add fipsonly compliant certificate in for NewTLSServer(), for dev.boringcrypto branch #48674

saurabhsuniljain opened this issue Sep 28, 2021 · 6 comments
Labels
FeatureRequest Issues asking for a new feature that does not need a proposal. FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done. Testing An issue that has been verified to require only test changes, not just a test failure.
Milestone
Go1.18

Comments

@saurabhsuniljain
Copy link

What version of Go are you using (go version)?

$ go version
dev.boringcrypto.go1.17 
go version devel +6d5f0ff Tue Mar 23 16:43:45 2021 +0100 linux/amd64

Does this issue reproduce with the latest release?

Yes

What operating system and processor architecture are you using (go env)?

go env Output
$ go env
GO111MODULE=""
GOARCH="amd64"
GOBIN=""
GOCACHE="/root/.cache/go-build"
GOENV="/root/.config/go/env"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOINSECURE=""
GOMODCACHE="/bld/pkg/mod"
GONOPROXY=""
GONOSUMDB=""
GOOS="linux"
GOPATH="/bld"
GOPRIVATE=""
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/usr/local/go"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/usr/local/go/pkg/tool/linux_amd64"
GOVCS=""
GOVERSION="devel +6d5f0ff Tue Mar 23 16:43:45 2021 +0100"
GCCGO="gccgo"
AR="ar"
CC="gcc"
CXX="g++"
CGO_ENABLED="1"
GOMOD="/bld/src/daintree/go.mod"
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -m64 -pthread -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build2061024702=/tmp/go-build -gno-record-gcc-switches"

What did you do?

I am using dev.boringcrypto branch for fips compliance in my code. I am facing issues while writing tests using httptest.NewTLSServer()

Code:

In my code I have an http client which I makes https GET calls.

Testing:

While writing tests I am using NewTLSServer function of httptest package for mocking the server. Then I am using Client() method of mock server created. Then I am calling my function which makes the https GET calls (that function will use the client that I got from mock server).

As my code has import _ "crypto/tls/fipsonly" the test fails. Client fails to verify the server certificate with below error.

http: TLS handshake error from 127.0.0.1:57328: remote error: tls: bad certificate
x509: certificate specifies an incompatible key usage

The issue is the default cert that is used by NewTLSServer method of httptest package is not fipsonly compliant.
The cert could be found in this file.

Also the test ExampleConfig_keyLogWriter here seems to enable InsecureSkipVerify and has the comment // test server certificate is not trusted..

Playground link - https://play.golang.org/p/ahA8mKd7sRr (import _ "crypto/tls/fipsonly" not present as playground doesn't work on boring crypto branches.)

You can add missing import and repro the issue locally.

Note: The code also is bit flaky as it is dependent on multiple goroutine execution. One in 2-3 execution the http GET request would succeed, in other cases select loop will timeout.

What did you expect to see?

Expected the client returned by httptest Server to be compatible and usable in testing, instead we being required to create our own client and set InsecureSkipVerify to true for testing.

What did you see instead?

Client returned by httptest Server (NewTLSServer()) is not compatible in boring crypto branches.
@saurabhsuniljain saurabhsuniljain changed the title Add fipsonly compliant certificate in httptest package in dev.boringcrypto.go1.17 Add fipsonly compliant certificate in for httptest package's NewTLSServer() in dev.boringcrypto.go1.17 Sep 28, 2021
@saurabhsuniljain saurabhsuniljain changed the title Add fipsonly compliant certificate in for httptest package's NewTLSServer() in dev.boringcrypto.go1.17 [dev.boringcrypto.go1.17] Add fipsonly compliant certificate in for httptest package's NewTLSServer() Sep 28, 2021
@mknyszek mknyszek changed the title [dev.boringcrypto.go1.17] Add fipsonly compliant certificate in for httptest package's NewTLSServer() [dev.boringcrypto.go1.17] net/http/httptest: add fipsonly compliant certificate in for NewTLSServer() Oct 4, 2021
@mknyszek mknyszek added FeatureRequest Issues asking for a new feature that does not need a proposal. NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. labels Oct 4, 2021
@mknyszek mknyszek added this to the Backlog milestone Oct 4, 2021
@mknyszek
Copy link
Contributor

mknyszek commented Oct 4, 2021

CC @neild and maybe @FiloSottile ?

@gopherbot
Copy link
Contributor

Change https://golang.org/cl/353869 mentions this issue: net/http/internal/testcert: use FIPS-compliant certificate

@qmuntal qmuntal mentioned this issue Jan 12, 2022
Closed
@neild
Copy link
Contributor

neild commented Jan 12, 2022

@gopherbot please open backport issues

@gopherbot gopherbot mentioned this issue Jan 12, 2022
Closed
@gopherbot
Copy link
Contributor

Backport issue(s) opened: #50585 (for 1.16), #50586 (for 1.17).

Remember to create the cherry-pick CL(s) as soon as the patch is submitted to master, according to https://golang.org/wiki/MinorReleases.

@gopherbot gopherbot mentioned this issue Jan 12, 2022
Closed
@dmitshur dmitshur added NeedsFix The path to resolution is known, but the work has not been done. Testing An issue that has been verified to require only test changes, not just a test failure. and removed NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. labels Jan 13, 2022
@dmitshur dmitshur modified the milestones: Backlog, Go1.18 Jan 13, 2022
@dmitshur dmitshur changed the title [dev.boringcrypto.go1.17] net/http/httptest: add fipsonly compliant certificate in for NewTLSServer() net/http/httptest: add fipsonly compliant certificate in for NewTLSServer(), for dev.boringcrypto branch Jan 13, 2022
@gopherbot
Copy link
Contributor

Change https://golang.org/cl/380995 mentions this issue: [release-branch.go1.17] net/http/internal/testcert: use FIPS-compliant certificate

@gopherbot
Copy link
Contributor

Change https://golang.org/cl/380997 mentions this issue: [release-branch.go1.16] net/http/internal/testcert: use FIPS-compliant certificate

gopherbot pushed a commit that referenced this issue Jan 27, 2022
@neild @dmitshur
[release-branch.go1.16] net/http/internal: use FIPS-compliant certifi…
57d5d8f 
…cate

Upgrade the test certificate from RSA 1024 (not FIPS-approved)
to RSA 2048 (FIPS-approved), allowing tests to pass when
the dev.boringcrypto branch FIPS-only mode is enabled.

For #48674.
Fixes #50585.

Change-Id: I613d2f8d0207bf3683fd0df256bf0167604996c5
Reviewed-on: https://go-review.googlesource.com/c/go/+/353869
Trust: Damien Neil <dneil@google.com>
Run-TryBot: Damien Neil <dneil@google.com>
Reviewed-by: Filippo Valsorda <filippo@golang.org>
(cherry picked from commit 90860e0)
Reviewed-on: https://go-review.googlesource.com/c/go/+/380997
Trust: Dmitri Shuralyov <dmitshur@golang.org>
Run-TryBot: Dmitri Shuralyov <dmitshur@golang.org>
TryBot-Result: Gopher Robot <gobot@golang.org>
gopherbot pushed a commit that referenced this issue Jan 27, 2022
@neild @dmitshur
[release-branch.go1.17] net/http/internal/testcert: use FIPS-complian…

Partially verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
We cannot verify signatures from co-authors, and some of the co-authors attributed to this commit require their commits to be signed.
3a34273 
…t certificate

Upgrade the test certificate from RSA 1024 (not FIPS-approved)
to RSA 2048 (FIPS-approved), allowing tests to pass when
the dev.boringcrypto branch FIPS-only mode is enabled.

For #48674.
Fixes #50586.

Change-Id: I613d2f8d0207bf3683fd0df256bf0167604996c5
Reviewed-on: https://go-review.googlesource.com/c/go/+/353869
Trust: Damien Neil <dneil@google.com>
Run-TryBot: Damien Neil <dneil@google.com>
Reviewed-by: Filippo Valsorda <filippo@golang.org>
(cherry picked from commit 90860e0)
Reviewed-on: https://go-review.googlesource.com/c/go/+/380995
Trust: Dmitri Shuralyov <dmitshur@golang.org>
Run-TryBot: Dmitri Shuralyov <dmitshur@golang.org>
TryBot-Result: Gopher Robot <gobot@golang.org>
jproberts pushed a commit to jproberts/go that referenced this issue Jun 21, 2022
@neild @jproberts
net/http/internal/testcert: use FIPS-compliant certificate
6915717 
Upgrade the test certificate from RSA 1024 (not FIPS-approved)
to RSA 2048 (FIPS-approved), allowing tests to pass when
the dev.boringcrypto branch FIPS-only mode is enabled.

Fixes golang#48674.

Change-Id: I613d2f8d0207bf3683fd0df256bf0167604996c5
Reviewed-on: https://go-review.googlesource.com/c/go/+/353869
Trust: Damien Neil <dneil@google.com>
Run-TryBot: Damien Neil <dneil@google.com>
Reviewed-by: Filippo Valsorda <filippo@golang.org>
@golang golang locked and limited conversation to collaborators Jan 26, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FeatureRequest Issues asking for a new feature that does not need a proposal. FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done. Testing An issue that has been verified to require only test changes, not just a test failure.
Projects
None yet
Milestone
Go1.18
Development

No branches or pull requests
5 participants
@neild @mknyszek @dmitshur @saurabhsuniljain @gopherbot