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: permanently broken tls.Conn should not return temporary net.Error #29971

Closed
jackc opened this issue Jan 29, 2019 · 4 comments
Closed
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Milestone

Comments

@jackc
Copy link

jackc commented Jan 29, 2019

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

$ go version
go version go1.11.5 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
GOARCH="amd64"
GOBIN=""
GOCACHE="/home/jack/.cache/go-build"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOOS="linux"
GOPATH="/home/jack/go"
GOPROXY=""
GORACE=""
GOROOT="/usr/local/go"
GOTMPDIR=""
GOTOOLDIR="/usr/local/go/pkg/tool/linux_amd64"
GCCGO="gccgo"
CC="gcc"
CXX="g++"
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 -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build787904603=/tmp/go-build -gno-record-gcc-switches"

What did you do?

  1. Set a deadline on a TLS connection.
  2. Do a failing write due to that deadline. As documented the connection is permanently broken.
  3. Clear deadline (doesn't really matter because connection is permanently broken - but just to mimic what would work for regular TCP connection).
  4. Do another write.

Demo code: https://github.com/jackc/go-tls-deadline-temporary-error

What did you expect to see?

A net.Error where Temporary() => false or a non-net.Error.

What did you see instead?

An net.Error where Temporary() => true.

This is technically documented behavior After a Write has timed out, the TLS state is corrupt and all future writes will return the same error. But it means that code that works generically with the net.Conn interface cannot rely on temporary errors being (potentially) temporary. It must treat them as permanent.

Perhaps the original failed tls.Conn.Write could return a non-temporary error.

jackc added a commit to jackc/pgx that referenced this issue Jan 29, 2019
With TLS connections a Write timeout caused by a SetDeadline permanently
breaks the connection. However, the errors are reported as temporary. So
there is no way to determine if it really is recoverable. As these were
the only kind of Write error that was recovered all Write errors are now
fatal to the connection.

#494
#506
golang/go#29971
@FiloSottile FiloSottile changed the title Permanently broken tls.Conn should not return temporary net.Error crypto/tls: permanently broken tls.Conn should not return temporary net.Error Jan 29, 2019
@FiloSottile FiloSottile added the NeedsFix The path to resolution is known, but the work has not been done. label Jan 29, 2019
@FiloSottile FiloSottile added this to the Go1.13 milestone Jan 29, 2019
@FiloSottile
Copy link
Contributor

Agreed, we should wrap the net.Error to make Temporary() false.

@odeke-em
Copy link
Member

odeke-em commented Jun 6, 2019

Thanks for reporting this issue @jackc and @FiloSottile for the triage and direction!

Here is a standalone test adapted from @jackc's report and source code that can be used as a regression test

package main

import (
	"crypto/tls"
	"fmt"
	"net"
	"net/http"
	"net/http/httptest"
	"net/url"
	"testing"
	"time"
)

func TestPermanentlyBrokenConnection(t *testing.T) {
	cst := httptest.NewTLSServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
		w.Write([]byte("Hello, World!"))
	}))
	defer cst.Close()

	cstU, _ := url.Parse(cst.URL)
	conn, err := tls.Dial("tcp", cstU.Host, &tls.Config{InsecureSkipVerify: true})
	if err != nil {
		t.Fatalf("Dial err: %v", err)
	}

	if err = conn.SetDeadline(time.Now()); err != nil {
		t.Fatalf("conn.SetDeadline(time.Now()): %v", err)
	}

	if _, err = conn.Write([]byte("should fail")); err == nil {
		t.Fatal("conn.Write succeeded when it should have failed")
	}

	// Clear deadline
	err = conn.SetDeadline(time.Time{})
	if err != nil {
		t.Fatalf("final conn.SetDeadline(time.Time{}): %v", err)
	}

	_, err = conn.Write([]byte("This connection is permanently broken"))
	if err != nil {
		fmt.Println("Write err", err)
	}

	ne := err.(net.Error)
	if ne.Temporary() {
		t.Fatal("Got a net.Error that apparently is temporary")
	}
}

@andybons andybons modified the milestones: Go1.13, Go1.14 Jul 8, 2019
@rsc rsc modified the milestones: Go1.14, Backlog Oct 9, 2019
@FiloSottile FiloSottile modified the milestones: Backlog, Go1.14 Oct 23, 2019
@ianlancetaylor ianlancetaylor modified the milestones: Go1.14, Go1.15 Dec 5, 2019
@gopherbot
Copy link

Change https://golang.org/cl/227774 mentions this issue: crypto/tls: failed tls.Conn.Write returns a permanent error

@gopherbot
Copy link

Change https://golang.org/cl/227840 mentions this issue: crypto/tls: failed tls.Conn.Write returns a permanent error

@golang golang locked and limited conversation to collaborators Apr 13, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Development

No branches or pull requests

8 participants