Skip to content

crypto/tls: wrong error returned in ECHConfig parsing #71706

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

Closed
mholt opened this issue Feb 13, 2025 · 5 comments
Closed

crypto/tls: wrong error returned in ECHConfig parsing #71706

mholt opened this issue Feb 13, 2025 · 5 comments
Labels
BugReport Issues describing a possible bug in the Go implementation. NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.

Comments

@mholt
Copy link

mholt commented Feb 13, 2025

Go version

go version go1.24.0 linux/amd64

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/matt/.cache/go-build'
GOCACHEPROG=''
GODEBUG=''
GOENV='/home/matt/.config/go/env'
GOEXE=''
GOEXPERIMENT=''
GOFIPS140='off'
GOFLAGS=''
GOGCCFLAGS='-fPIC -m64 -pthread -Wl,--no-gc-sections -fmessage-length=0 -ffile-prefix-map=/tmp/go-build3610820285=/tmp/go-build -gno-record-gcc-switches'
GOHOSTARCH='amd64'
GOHOSTOS='linux'
GOINSECURE=''
GOMOD='/home/matt/Dev/caddyserver/caddy/go.mod'
GOMODCACHE='/home/matt/go/pkg/mod'
GONOPROXY=''
GONOSUMDB=''
GOOS='linux'
GOPATH='/home/matt/go'
GOPRIVATE=''
GOPROXY='https://proxy.golang.org,direct'
GOROOT='/usr/lib/go'
GOSUMDB='sum.golang.org'
GOTELEMETRY='on'
GOTELEMETRYDIR='/home/matt/.config/go/telemetry'
GOTMPDIR=''
GOTOOLCHAIN='auto'
GOTOOLDIR='/usr/lib/go/pkg/tool/linux_amd64'
GOVCS=''
GOVERSION='go1.24.0'
GOWORK=''
PKG_CONFIG='pkg-config'

What did you do?

Implementing Encrypted ClientHello (server-side).

What did you see happen?

errMalformedECHConfig ("tls: malformed ECHConfigList") is returned in at least 17 places, although most are errors parsing ECHConfig, not ECHConfigList.

Additionally, this error is very vague. Since we have to serialize the ECHConfig ourselves, and crypto/tls adds extra requirements, any chance we could get a little more info as to what is wrong with it?

Also, the following godoc comment seems to have a mistake:

type EncryptedClientHelloKey struct {
	// Config should be a marshalled ECHConfig associated with PrivateKey. This
	// must match the config provided to clients byte-for-byte. The config
	// should only specify the DHKEM(X25519, HKDF-SHA256) KEM ID (0x0020), the
	// HKDF-SHA256 KDF ID (0x0001), and a subset of the following AEAD IDs:
	// AES-128-GCM (0x0000), AES-256-GCM (0x0001), ChaCha20Poly1305 (0x0002).
	Config [][byte](https://pkg.go.dev/builtin#byte)

This part:

// AES-128-GCM (0x0000), AES-256-GCM (0x0001), ChaCha20Poly1305 (0x0002)

doesn't seem to match RFC 9180, and caused me some head scratching while troubleshooting this.

I believe the comment should be:

AES-128-GCM (0x0001), AES-256-GCM (0x0002), ChaCha20Poly1305 (0x0003)

As evidenced by the RFC and the actual code: https://cs.opensource.google/go/go/+/refs/tags/go1.24.0:src/crypto/internal/hpke/hpke.go;l=157-159

type AEADID uint16

const (
	AEAD_AES_128_GCM      = 0x0001
	AEAD_AES_256_GCM      = 0x0002
	AEAD_ChaCha20Poly1305 = 0x0003
)

What did you expect to see?

  • Less misleading error message
  • A more descriptive error message to help pinpoint serialization problems. We are required to serialize the ECHConfig ourselves, but the Go standard library currently adds stringent requirements on some parts of the ECHConfig, so more detailed errors would be appreciated. Otherwise, we can write spec-compliant serialization code, but still have it fail with the crypto/tls package for unknown reasons.
  • Godoc comment to have the correct AEAD IDs.

Thanks for adding ECH to Go 1.24! Caddy will soon ship with it.

I did get ECH working in Caddy tonight!

@gabyhelp gabyhelp added the BugReport Issues describing a possible bug in the Go implementation. label Feb 13, 2025
@seankhliao seankhliao changed the title crypto/tls: Wrong error returned in ECHConfig parsing; confusing typo in godoc comment crypto/tls: wrong error returned in ECHConfig parsing Feb 13, 2025
@dmitshur dmitshur added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Feb 13, 2025
@dmitshur
Copy link
Contributor

CC @golang/security.

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/650720 mentions this issue: crypto/tls: improve ech parsing errors

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/650719 mentions this issue: crypto/tls: fix incorrect EncryptedClientHelloKey comment

gopherbot pushed a commit that referenced this issue Feb 20, 2025
Updates #71706

Change-Id: Id689ec476eb3f76500dbf59d716a4376749de700
Reviewed-on: https://go-review.googlesource.com/c/go/+/650719
Reviewed-by: Daniel McCarney <daniel@binaryparadox.net>
Auto-Submit: Roland Shoemaker <roland@golang.org>
Reviewed-by: Michael Knyszek <mknyszek@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
@mholt
Copy link
Author

mholt commented Feb 20, 2025

Thank you ^_^ Looks great!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
BugReport Issues describing a possible bug in the Go implementation. NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Development

No branches or pull requests

4 participants