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

crypto/tls: client auth failure alert codes can be improved #52113

Open
anitgandhi opened this issue Apr 1, 2022 · 13 comments · May be fixed by #53251
Open

crypto/tls: client auth failure alert codes can be improved #52113

anitgandhi opened this issue Apr 1, 2022 · 13 comments · May be fixed by #53251
Labels
NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made.
Milestone

Comments

@anitgandhi
Copy link
Contributor

anitgandhi commented Apr 1, 2022

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

$ go version
go version go1.18 darwin/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="/Users/agandhi/Library/Caches/go-build"
GOENV="/Users/agandhi/Library/Application Support/go/env"
GOEXE=""
GOEXPERIMENT=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="darwin"
GOINSECURE=""
GOMODCACHE="/Users/agandhi/go/pkg/mod"
GONOPROXY=""
GONOSUMDB=""
GOOS="darwin"
GOPATH="/Users/agandhi/go"
GOPRIVATE=""
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/Users/agandhi/.gimme/versions/go1.18.darwin.amd64"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/Users/agandhi/.gimme/versions/go1.18.darwin.amd64/pkg/tool/darwin_amd64"
GOVCS=""
GOVERSION="go1.18"
GCCGO="gccgo"
GOAMD64="v1"
AR="ar"
CC="clang"
CXX="clang++"
CGO_ENABLED="1"
GOMOD="/dev/null"
GOWORK=""
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -arch x86_64 -m64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=/var/folders/8b/jm61yqyn0g126b4p23hrfz7c0000gp/T/go-build4159564691=/tmp/go-build -gno-record-gcc-switches -fno-common"

What did you do?

  1. Run a Go TLS server with ClientAuthType configuration higher than RequestClientCert. This could be an HTTPS server, gRPC, etc.
  2. Connect to it using curl, openssl s_client, etc and provide an invalid client certificate.

What did you expect to see?

I'd like to see one of the more meaningful TLS alerts that RFC 5246 (TLSv1.2) or RFC 8446 (TLSv1.3) define. Namely:

  • certificate_required when the server is configured with tls.RequireAnyClientCert or tls.RequireAndVerifyClientCert, and the client doesn't provide any client cert
  • certificate_expired when the client provides a client cert but it's expired (or not yet valid)
  • unknown_ca when the client provides a client cert but it's not signed by an authority that the Go TLS server is configured to require.

What did you see instead?

TLS alert bad_certificate is always returned, which is often confusing to end users because it doesn't surface enough information to quickly find out if/how they can fix their client certificate.

@anitgandhi
Copy link
Contributor Author

anitgandhi commented Apr 1, 2022

I actually have this fixed already in a fork of mine, you can view it here: master...anitgandhi:improve-client-auth-alerts
I've tested this locally and it's working well.

But, I wanted to just quickly have a discussion to here to ensure the maintainers are open to changing this behavior.
I tried to track down any older issues or git history to see if it was an explicit choice to just always use bad_certificate, or if it was mostly just an oversight.

If folks are open to fixing this, I'll be happy to open a PR/CL from my branch linked above.

Thanks in advance!

@anitgandhi
Copy link
Contributor Author

anitgandhi commented Apr 1, 2022

Perhaps this requires a separate issue or discussion, but I would have also liked to take advantage of the certificate_revoked alert code.

I know crypto/tls nor crypto/x509.Verify does not directly do any CRL checking; however, I was considering a special case such that if VerifyPeerCertificate is not nil, and it returns a known/sentinel error indicating revocation, then crypto/tls would use the certificate_revoked alert code.

I imagine the sentinel error part could be done by extending the existing crypto/x509.CertificateInvalidError by adding a new InvalidReason value like Revoked. Then, crypto/tls could just check the VerifyPeerCertificate returned err like so:

if c.config.VerifyPeerCertificate != nil {
	if err := c.config.VerifyPeerCertificate(certificates, c.verifiedChains); err != nil {
		var errCertificateInvalid x509.CertificateInvalidError
		var alert alert
		if errors.As(err, &errCertificateInvalid) && errCertificateInvalid.Reason == x509.Revoked {
			alert = alertCertificateRevoked
		} else {
			alert = alertBadCertificate
		}
		c.sendAlert(alert)
		return err
	}
}

@anitgandhi
Copy link
Contributor Author

anitgandhi commented Apr 5, 2022

/cc @FiloSottile

@cherrymui cherrymui added the NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. label Apr 5, 2022
@cherrymui cherrymui added this to the Backlog milestone Apr 5, 2022
@cherrymui
Copy link
Member

cherrymui commented Apr 5, 2022

cc @rolandshoemaker @kevinburke

@anitgandhi
Copy link
Contributor Author

anitgandhi commented Apr 21, 2022

sorry for the ping on this folks, any thoughts? i wanted to try and get this in before the 1.19 dev freeze, if possible.

@ianlancetaylor
Copy link
Contributor

ianlancetaylor commented Apr 21, 2022

CC @golang/security

@anitgandhi anitgandhi linked a pull request Jun 6, 2022 that will close this issue
@gopherbot
Copy link

gopherbot commented Jun 6, 2022

Change https://go.dev/cl/410496 mentions this issue: crypto/tls: improve client auth failure alerts

@rolandshoemaker
Copy link
Member

rolandshoemaker commented Jun 6, 2022

Hey, sorry for the lag. The main portion of this (returning the more specific alerts) seems reasonable. We're currently in the 1.19 freeze, so we'll have to wait for 1.20 to get this in (the tree should re-open in August.)

@anitgandhi
Copy link
Contributor Author

anitgandhi commented Jun 6, 2022

@rolandshoemaker no problem, and sounds good. I went ahead and opened the CL; is it ok to just leave it as-is until August, or should I close it out?

@rolandshoemaker
Copy link
Member

rolandshoemaker commented Jun 6, 2022

Yup leave it open and I'll give it a review, and once the tree opens we can submit it.

@anitgandhi
Copy link
Contributor Author

anitgandhi commented Aug 10, 2022

Hi @rolandshoemaker , just a friendly ping on this/the CL now that the tree has been opened for 1.20. Thanks in advance!

@rolandshoemaker
Copy link
Member

rolandshoemaker commented Oct 18, 2022

Sorry for the long wait, left a comment on the CL.

@anitgandhi
Copy link
Contributor Author

anitgandhi commented Nov 8, 2022

@rolandshoemaker 👋 I've resolved the comment on the CL; any chance this could still make it in for 1.20?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants