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.

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

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

Closed
anitgandhi opened this issue Apr 1, 2022 · 15 comments
Closed

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

anitgandhi opened this issue Apr 1, 2022 · 15 comments
Labels
NeedsFix The path to resolution is known, but the work has not been done.
Milestone
Go1.21

Comments

@anitgandhi
Copy link
Contributor

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 •
edited

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

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

/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

cc @rolandshoemaker @kevinburke

@anitgandhi
Copy link
Contributor Author

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

CC @golang/security

@anitgandhi anitgandhi mentioned this issue Jun 6, 2022
Closed
@gopherbot
Copy link

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

@rolandshoemaker
Copy link
Member

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

@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

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

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

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

@anitgandhi
Copy link
Contributor Author

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

@anitgandhi
Copy link
Contributor Author

anitgandhi commented Jan 4, 2023 •
edited

Hi @rolandshoemaker , Happy New Year!
Gentle ping on the above now that 1.20 RC2 is already out - is it still possible for this to make it in 1.20 (since it was reviewed and approved a couple months back)?

It would be a huge help for our organization when debugging mTLS errors

@cherrymui cherrymui mentioned this issue Jan 20, 2023
Closed
@dmitshur dmitshur added NeedsFix The path to resolution is known, but the work has not been done. and removed NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. labels Jan 20, 2023
@dmitshur dmitshur modified the milestones: Backlog, Go1.21 Jan 20, 2023
anitgandhi added a commit to anitgandhi/go that referenced this issue Jun 23, 2023
@anitgandhi
doc/go1.21: document changes in crypto/tls related to client authenti…
5a70544 
…cation alerts

For golang#52113
For golang#58645
@anitgandhi anitgandhi mentioned this issue Jun 23, 2023
Closed
@gopherbot
Copy link

Change https://go.dev/cl/505436 mentions this issue: doc/go1.21: document changes in crypto/tls related to client authentication alerts

anitgandhi added a commit to anitgandhi/go that referenced this issue Jun 27, 2023
@anitgandhi
doc/go1.21: document changes in crypto/tls related to client authenti…
94eabfe 
…cation alerts

For golang#52113
For golang#58645
gopherbot pushed a commit that referenced this issue Aug 3, 2023
@anitgandhi @gopherbot
doc/go1.21: document changes in crypto/tls related to client authenti…
3ca90ed 
…cation alerts

For #52113
For #58645

Change-Id: Id7dff2570132588da95fb4216a86faf34fa2cbdc
GitHub-Last-Rev: 94eabfe
GitHub-Pull-Request: #60972
Reviewed-on: https://go-review.googlesource.com/c/go/+/505436
Run-TryBot: Roland Shoemaker <roland@golang.org>
Auto-Submit: Roland Shoemaker <roland@golang.org>
Reviewed-by: Dmitri Shuralyov <dmitshur@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Roland Shoemaker <roland@golang.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Milestone
Go1.21
Development

Successfully merging a pull request may close this issue.

crypto/tls: improve client auth failure alerts anitgandhi/go
6 participants
@dmitshur @ianlancetaylor @anitgandhi @rolandshoemaker @gopherbot @cherrymui