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: Export TLS errors / alerts #56691

Closed
maxmoehl opened this issue Nov 10, 2022 · 1 comment
Closed

proposal: crypto/tls: Export TLS errors / alerts #56691

maxmoehl opened this issue Nov 10, 2022 · 1 comment
Labels
Milestone

Comments

@maxmoehl
Copy link

TL;DR: export everything in crypto/tls/alert.go (except alertText?) and wrap the alerts where appropriate.

Possibly related to: #48151 #51495

Issue

In the cloudfoundry/gorouter we are silently retrying failed requests which are safe to retry, either because they are idempotent or never reached the upstream server.

This requires detailed understanding of the error that caused the request to fail. The crypto/tls package is currently rather secretive about its errors (in terms of comparability). All TLS alerts are mapped to go constants in crypto/tls/alert.go but not exported, therefore go applications cannot easily distinguish the error that occurred, neither between TLS and other errors, nor between different kinds of TLS errors.

There are probably more use-cases but this is the one that triggered discussions around this topic in our team.

Proposal

Export the alert type, export all defined alerts, wrap those alerts where appropriate.

This allows go code to easily check for the specific TLS error / alert and act accordingly:

_, err := client.Delete("https://foo.bar/item")
if err != nil {
    if errors.Is(err, tls.AlertHandshakeFailure) {
        // do retry
    }
    return err
}

The TLS Implementation distinguishes between two kinds of errors / alerts: those caused locally and those received from remote.

Remote Alerts

are covered by this piece of code, which ultimately wraps the alert and returns it:

return c.in.setErrorLocked(&net.OpError{Op: "remote error", Err: alert(data[1])})

Those can be checked easily once the alerts are exposed since they are already wrapped properly.

Local Alerts

are not at all returned. Most of the return statements currently look like this:

c.vers, ok = c.config.mutualVersion(roleServer, clientVersions)
if !ok {
    c.sendAlert(alertProtocolVersion)
    return nil, fmt.Errorf("tls: client offered only unsupported versions: %x", clientVersions)
}

To properly support unwrapping of errors all of these statements would need to be changed to something that wraps the alert properly (fmt.Errorf, custom error type, etc.). This could be considered an incompatible change since the error message would also change, I'm not sure if such a change is in line with the Go 1 Compatibility Promise, but I would guess yes.

@gopherbot gopherbot added this to the Proposal milestone Nov 10, 2022
@seankhliao
Copy link
Member

Duplicate of #35234

@seankhliao seankhliao marked this as a duplicate of #35234 Nov 10, 2022
@seankhliao seankhliao closed this as not planned Won't fix, can't repro, duplicate, stale Nov 10, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants