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: encryptedExtensions.echRetryConfigs not returned to client #70915

Closed
rthellend opened this issue Dec 19, 2024 · 3 comments
Closed
Labels
NeedsFix The path to resolution is known, but the work has not been done. release-blocker
Milestone

Comments

@rthellend
Copy link

rthellend commented Dec 19, 2024

Go version

go version 1.24rc1

Output of go env in your module/workspace:

AR='ar'
CC='gcc'
CGO_CFLAGS='-O2 -g'
CGO_CPPFLAGS=''
CGO_CXXFLAGS='-O2 -g'
CGO_ENABLED='1'
CGO_FFLAGS='-O2 -g'
CGO_LDFLAGS='-O2 -g'
CXX='g++'
GCCGO='gccgo'
GO111MODULE=''
GOAMD64='v1'
GOARCH='amd64'
GOAUTH='netrc'
GOBIN=''
GOCACHE='/home/robin/.cache/go-build'
GODEBUG=''
GOENV='/home/robin/.config/go/env'
GOEXE=''
GOEXPERIMENT=''
GOFIPS140='off'
GOFLAGS=''
GOGCCFLAGS='-fPIC -m64 -pthread -Wl,--no-gc-sections -fmessage-length=0 -ffile-prefix-map=/tmp/go-build1150069972=/tmp/go-build -gno-record-gcc-switches'
GOHOSTARCH='amd64'
GOHOSTOS='linux'
GOINSECURE=''
GOMOD='/home/robin/src/bug/go.mod'
GOMODCACHE='/home/robin/go/pkg/mod'
GONOPROXY=''
GONOSUMDB=''
GOOS='linux'
GOPATH='/home/robin/go'
GOPRIVATE=''
GOPROXY='https://proxy.golang.org,direct'
GOROOT='/home/robin/src/goroot'
GOSUMDB='sum.golang.org'
GOTELEMETRY='local'
GOTELEMETRYDIR='/home/robin/.config/go/telemetry'
GOTMPDIR=''
GOTOOLCHAIN='auto'
GOTOOLDIR='/home/robin/src/goroot/pkg/tool/linux_amd64'
GOVCS=''
GOVERSION='devel go1.24-95b433eed4 Tue Dec 17 13:28:29 2024 -0800'
GOWORK=''
PKG_CONFIG='pkg-config'

What did you do?

I've been testing ECH in go1.24rc1. I noticed that when the client uses and incorrect / outdated echConfigList, the server correctly returns echRetryConfigs in EncryptedExtensions, but the client doesn't use it.

https://go.dev/play/p/BTzFrLzaxjV?v=gotip

This code outputs:

devel go1.24-95b433eed4 Tue Dec 17 13:28:29 2024 -0800
RetryConfigListRetry: []

With this change:

diff --git a/src/crypto/tls/handshake_client_tls13.go b/src/crypto/tls/handshake_client_tls13.go
index 38c6025db7..6601fcf9d7 100644
--- a/src/crypto/tls/handshake_client_tls13.go
+++ b/src/crypto/tls/handshake_client_tls13.go
@@ -39,7 +39,8 @@ type clientHandshakeStateTLS13 struct {
        masterSecret  *tls13.MasterSecret
        trafficSecret []byte // client_application_traffic_secret_0
 
-       echContext *echClientContext
+       echContext         *echClientContext
+       echRetryConfigList []byte
 }
 
 // handshake requires hs.c, hs.hello, hs.serverHello, hs.keyShareKeys, and,
@@ -117,6 +118,7 @@ func (hs *clientHandshakeStateTLS13) handshake() error {
                        // If the server sent us retry configs, we'll return these to
                        // the user so they can update their Config.
                        echRetryConfigList = hs.serverHello.encryptedClientHello
+                       _ = echRetryConfigList
                }
        }
 
@@ -155,7 +157,7 @@ func (hs *clientHandshakeStateTLS13) handshake() error {
 
        if hs.echContext != nil && hs.echContext.echRejected {
                c.sendAlert(alertECHRequired)
-               return &ECHRejectionError{echRetryConfigList}
+               return &ECHRejectionError{hs.echRetryConfigList}
        }
 
        c.isHandshakeComplete.Store(true)
@@ -605,6 +607,7 @@ func (hs *clientHandshakeStateTLS13) readServerParameters() error {
                c.sendAlert(alertUnsupportedExtension)
                return errors.New("tls: server sent encrypted client hello retry configs after accepting encrypted client hello")
        }
+       hs.echRetryConfigList = encryptedExtensions.echRetryConfigs
 
        return nil
 }

I get:

$ go run .
devel go1.24-95b433eed4 Tue Dec 17 13:28:29 2024 -0800
RetryConfigListRetry: [0 62 254 13 0 58 1 0 32 0 32 223 67 164 145 141 206 235 159 159 106 211 183 218 14 188 83 72 66 133 229 14 63 128 130 46 169 103 246 116 108 254 88 0 4 0 1 0 3 0 11 101 120 97 109 112 108 101 46 99 111 109 0 0]

What did you see happen?

RetryConfigList is empty:

RetryConfigList: []

What did you expect to see?

RetryConfigList: [0 62 254 13 0 58 1 0 32 0 32 223 67 164 145 141 206 235 159 159 106 211 183 218 14 188 83 72 66 133 229 14 63 128 130 46 169 103 246 116 108 254 88 0 4 0 1 0 3 0 11 101 120 97 109 112 108 101 46 99 111 109 0 0]
@dr2chase
Copy link
Contributor

@FiloSottile FiloSottile modified the milestones: Go1.25, Go1.24 Dec 22, 2024
@cherrymui cherrymui added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Dec 23, 2024
rthellend added a commit to c2FmZQ/ech that referenced this issue Dec 25, 2024
rthellend added a commit to c2FmZQ/ech that referenced this issue Dec 25, 2024
@gopherbot
Copy link
Contributor

Change https://go.dev/cl/638583 mentions this issue: crypto/tls: properly return ECH retry configs

@dmitshur dmitshur added NeedsFix The path to resolution is known, but the work has not been done. and removed NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. labels Jan 4, 2025
wyf9661 pushed a commit to wyf9661/go that referenced this issue Jan 21, 2025
When ECH is rejected, properly take retry configs from the encrypted
extensions message. Also fix the bogo shim to properly test for this
behavior.

We should properly map the full BoringSSL -> Go errors so that we don't
run into a similar failure in the future, but this is left for a follow
up CL.

Fixes golang#70915

Change-Id: Icc1878ff6f87df059e7b83e0a431f50f1fea833c
Reviewed-on: https://go-review.googlesource.com/c/go/+/638583
Reviewed-by: Damien Neil <dneil@google.com>
Reviewed-by: Filippo Valsorda <filippo@golang.org>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
NeedsFix The path to resolution is known, but the work has not been done. release-blocker
Projects
None yet
Development

No branches or pull requests

7 participants