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

proposal: crypto/tls: expose TLS alert type for more precise error checks #35234

Open
TheHackerDev opened this issue Oct 29, 2019 · 5 comments
Open
Labels
FeatureRequest NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. Proposal Proposal-Crypto Proposal related to crypto packages or other security issues
Milestone

Comments

@TheHackerDev
Copy link

TheHackerDev commented Oct 29, 2019

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

$ go version
go version go1.13.3 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="auto"
GOARCH="amd64"
GOBIN=""
GOCACHE="/Users/me/Library/Caches/go-build"
GOENV="/Users/me/Library/Application Support/go/env"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="darwin"
GONOPROXY=""
GONOSUMDB=""
GOOS="darwin"
GOPATH="/Users/me/Code/Golang"
GOPRIVATE=""
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/usr/local/go"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/usr/local/go/pkg/tool/darwin_amd64"
GCCGO="gccgo"
AR="ar"
CC="clang"
CXX="clang++"
CGO_ENABLED="1"
GOMOD=""
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 -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=/var/folders/16/7jt3pcs15570rmz95wf9vlrc0000gn/T/go-build533656385=/tmp/go-build -gno-record-gcc-switches -fno-common"

What did you do?

I've hooked http.Server.ConnState using a function that checks for handshake errors on the http.StateClosed state, and logs these. For example:

func ConnStateHook(c net.Conn, cstate http.ConnState) {
	if cstate == http.StateClosed {
		if cc, ok := c.(*tls.Conn); ok {
			tlsState := cc.ConnectionState()
			if err := cc.Handshake(); err != nil {

				if strings.Contains(err.Error(), "remote error: tls: unknown certificate authority") || strings.Contains(err.Error(), "remote error: tls: unknown certificate") {
					// Do something
				}
			}
		}
	}
}

What did you expect to see?

This works, but I really don't like using string comparison on error messages. What would be helpful to ensure accurate comparison (especially if these strings change in the future) is to be able to compare the error with the tls.alert types. The error above (err) is actually a tls.alert wrapped in a *net.OpError type, but because the tls.alert type is not exported, I only get to see the public Error() method that outputs a string.

The result of this would be similar to the http status codes found in net/http/status.go.

@bcmills bcmills added FeatureRequest NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. labels Nov 5, 2019
@bcmills bcmills added this to the Unplanned milestone Nov 5, 2019
@bcmills
Copy link
Member

bcmills commented Nov 5, 2019

CC @FiloSottile @agl @kevinburke

@riraccuia
Copy link

riraccuia commented Mar 10, 2020

Hi guys, are there any plans to implement this in a next release? @FiloSottile

@process0
Copy link

process0 commented Mar 16, 2022

Suprised this isnt already exposed. String matching here is cumbersome

@seankhliao seankhliao changed the title crypto/tls: expose TLS alert type for more precise error checks proposal: crypto/tls: expose TLS alert type for more precise error checks Nov 10, 2022
@seankhliao seankhliao added the Proposal-Crypto Proposal related to crypto packages or other security issues label Nov 10, 2022
@seankhliao
Copy link
Member

seankhliao commented Nov 10, 2022

cc @golang/security

@domdom82
Copy link

domdom82 commented Nov 11, 2022

bump. this would be super helpful 👍 !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
FeatureRequest NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. Proposal Proposal-Crypto Proposal related to crypto packages or other security issues
Projects
Status: Incoming
Development

No branches or pull requests

7 participants