From 39fc8915a700e3a563559507ffedc81cab695382 Mon Sep 17 00:00:00 2001 From: Oscar Sanderson Date: Tue, 20 Oct 2026 07:07:35 +0800 Subject: [PATCH 1/2] fix!: refuse a sealing key made entirely of printable text Pairwise keys refused a key made entirely of printable text (a passphrase, or a hex or base64 key passed without decoding it), but the other sealers, token sets, CIBA sessions and the session and interaction cookies, accepted one and sealed everything under a key with far less entropy than 32 random bytes. sealkeys.Check, which every sealer uses, now refuses such a key, and pairwise's own copy of the check is gone. A random key is all printable with probability about 4e-14. Test fixtures that used text keys now end in a non-printable byte. UPGRADING.md has a section. BREAKING CHANGE: client.NewTokenSetSealer, client.NewBackchannelSessionSealer, client/sessioncookie and server/interactioncookie refuse a key made entirely of printable text. Generate keys with crypto/rand and decode an encoded key before passing it. Co-Authored-By: Claude Opus 5.5 --- UPGRADING.md | 19 +++++++++++++++ client/essential_acr_test.go | 2 +- client/require_id_token_test.go | 2 +- client/token_set_seal_internal_test.go | 2 +- internal/sealkeys/sealkeys.go | 23 ++++++++++++++++--- internal/sealkeys/sealkeys_test.go | 2 ++ pairwise/hmac.go | 17 -------------- pairwise/reversible.go | 5 ---- .../interactioncookie_fuzz_test.go | 2 +- 9 files changed, 45 insertions(+), 29 deletions(-) diff --git a/UPGRADING.md b/UPGRADING.md index 4e297478..38bfd5ae 100644 --- a/UPGRADING.md +++ b/UPGRADING.md @@ -159,6 +159,25 @@ entity key is also a protocol key. **What to change:** give `keys.FederationEntitySigning` its own key in your key manager. +### Sealing keys must not be text (client, server) + +**Affects:** a key passed to `client.NewTokenSetSealer`, +`client.NewBackchannelSessionSealer`, `client/sessioncookie` or +`server/interactioncookie` that is made entirely of printable text: a +passphrase, or a hex or base64 key passed without decoding it. Pairwise +keys already refused these. + +**Why:** such a key has far less entropy than 32 random bytes, so +everything sealed under it (token sets, CIBA sessions, session and +interaction cookies) is open to anyone who guesses it. + +**What fails:** the constructor returns an error saying the key is all +printable text. + +**What to change:** generate the key with `crypto/rand`, and decode an +encoded key before passing it. A random key is refused with probability +about 4×10⁻¹⁴. + ### Attestation-based client authentication follows draft-12 (server, client) Not a breaking change: clients built to draft-07 keep working, and so diff --git a/client/essential_acr_test.go b/client/essential_acr_test.go index 12b0191d..6ec3f7d6 100644 --- a/client/essential_acr_test.go +++ b/client/essential_acr_test.go @@ -190,7 +190,7 @@ func assertRequestObjectAsksEssentialGoldOrSilver(t *testing.T, request string) // caller storing it between polls would. func resealBackchannelSession(t *testing.T, c *client.Client, session client.BackchannelAuthenticationSession) client.BackchannelAuthenticationSession { t.Helper() - sealer, err := client.NewBackchannelSessionSealer(c, [][]byte{[]byte("essential-acr-test-seal-key-0001")}) + sealer, err := client.NewBackchannelSessionSealer(c, [][]byte{[]byte("essential-acr-test-seal-key-000\x01")}) if err != nil { t.Fatal(err) } diff --git a/client/require_id_token_test.go b/client/require_id_token_test.go index 75fe76c0..bd40382a 100644 --- a/client/require_id_token_test.go +++ b/client/require_id_token_test.go @@ -74,7 +74,7 @@ func TestPollBackchannelAuthenticationRequiresIDTokenForOpenID(t *testing.T) { t.Fatalf("BeginBackchannelAuthentication: %v", err) } // The session survives a restart with its requirement intact. - sealer, err := client.NewBackchannelSessionSealer(c, [][]byte{[]byte("require-id-token-test-seal-key-1")}) + sealer, err := client.NewBackchannelSessionSealer(c, [][]byte{[]byte("require-id-token-test-seal-key-\x01")}) if err != nil { t.Fatal(err) } diff --git a/client/token_set_seal_internal_test.go b/client/token_set_seal_internal_test.go index 1b9506e8..8c8d7fc3 100644 --- a/client/token_set_seal_internal_test.go +++ b/client/token_set_seal_internal_test.go @@ -9,7 +9,7 @@ import ( // but isn't a sealed TokenSet's JSON — only something holding the key // could make one: Open still refuses it as unreadable. func TestTokenSetSealerOpenRefusesNonJSON(t *testing.T) { - sealer, err := NewTokenSetSealer(&Client{}, [][]byte{[]byte("token-set-internal-test-seal-key")}) + sealer, err := NewTokenSetSealer(&Client{}, [][]byte{[]byte("token-set-internal-test-seal-ke\x01")}) if err != nil { t.Fatal(err) } diff --git a/internal/sealkeys/sealkeys.go b/internal/sealkeys/sealkeys.go index 271447bd..6511b00c 100644 --- a/internal/sealkeys/sealkeys.go +++ b/internal/sealkeys/sealkeys.go @@ -15,9 +15,12 @@ const Size = 32 // Check refuses keys unless it holds at least one key, each Size bytes, // none of them a single byte repeated (all zeros, say: a placeholder, -// not a random key), and no key twice. what prefixes the errors. It -// can't tell a random key from any other: it only catches the keys -// that are certainly not random. +// not a random key) or made entirely of printable text (a passphrase, +// or a hex or base64 key not yet decoded), and no key twice. what +// prefixes the errors. It can't tell a random key from any other: it +// only catches the keys that are certainly not random. 32 random bytes +// are all printable with probability (98/256)^32, about 4e-14, so a +// real key is never refused. func Check(what string, keys [][]byte) error { if len(keys) == 0 { return fmt.Errorf("%s: at least one key is required", what) @@ -29,6 +32,9 @@ func Check(what string, keys [][]byte) error { if bytes.Count(k, k[:1]) == Size { return fmt.Errorf("%s: key %d is one byte repeated, not a random key", what, i) } + if isText(k) { + return fmt.Errorf("%s: key %d is all printable text (a passphrase, or a hex or base64 key not yet decoded), not %d random bytes: generate it with crypto/rand, and decode an encoded key before passing it", what, i, Size) + } for j := range i { if bytes.Equal(keys[j], k) { return fmt.Errorf("%s: key %d repeats key %d", what, i, j) @@ -37,3 +43,14 @@ func Check(what string, keys [][]byte) error { } return nil } + +// isText reports whether k is made entirely of printable ASCII and +// whitespace. +func isText(k []byte) bool { + for _, c := range k { + if (c < 0x20 || c > 0x7e) && c != '\t' && c != '\n' && c != '\r' { + return false + } + } + return true +} diff --git a/internal/sealkeys/sealkeys_test.go b/internal/sealkeys/sealkeys_test.go index 5f1d92f7..0c10151d 100644 --- a/internal/sealkeys/sealkeys_test.go +++ b/internal/sealkeys/sealkeys_test.go @@ -30,6 +30,8 @@ func TestCheck(t *testing.T) { "all zero": {[][]byte{make([]byte, Size)}, "key 0 is one byte repeated"}, "repeated byte later": {[][]byte{key(1), bytes.Repeat([]byte{0xab}, Size)}, "key 1 is one byte repeated"}, "duplicate": {[][]byte{key(1), key(2), key(1)}, "key 2 repeats key 0"}, + "passphrase": {[][]byte{[]byte("correct horse battery staple!!!!")}, "key 0 is all printable text"}, + "undecoded hex": {[][]byte{key(1), []byte("00112233445566778899aabbccddeeff")}, "key 1 is all printable text"}, } { t.Run(name, func(t *testing.T) { err := Check("test", tc.keys) diff --git a/pairwise/hmac.go b/pairwise/hmac.go index cc4fc49d..d3917929 100644 --- a/pairwise/hmac.go +++ b/pairwise/hmac.go @@ -46,9 +46,6 @@ func NewHMAC(key []byte, opts ...keys.CustodyOption) (*HMACDeriver, error) { if err := sealkeys.Check("pairwise: HMAC", [][]byte{key}); err != nil { return nil, err } - if err := checkNotText("pairwise: HMAC", key); err != nil { - return nil, err - } return NewHMACWith(memoryHMACOps{key: append([]byte(nil), key...)}, opts...) } @@ -159,17 +156,3 @@ func writeLengthPrefixed(w io.Writer, field string) { _, _ = w.Write(length[:]) _, _ = w.Write([]byte(field)) } - -// checkNotText refuses a key made entirely of printable ASCII (and -// whitespace): a passphrase, or a hex or base64 encoding passed without -// decoding, rather than random bytes. 32 random bytes are all in that -// range with probability (98/256)^32, about 4e-14, so a real key is -// never refused. -func checkNotText(what string, key []byte) error { - for _, c := range key { - if (c < 0x20 || c > 0x7e) && c != '\t' && c != '\n' && c != '\r' { - return nil - } - } - return fmt.Errorf("%s: key is all printable text (a passphrase, or a hex or base64 key not yet decoded), not 32 random bytes: generate it with crypto/rand, and decode an encoded key before passing it", what) -} diff --git a/pairwise/reversible.go b/pairwise/reversible.go index ee717e1d..892fb9bd 100644 --- a/pairwise/reversible.go +++ b/pairwise/reversible.go @@ -151,11 +151,6 @@ func NewReversible(macKey, encKey []byte, opts ...keys.CustodyOption) (*Reversib if err := sealkeys.Check("pairwise: reversible", [][]byte{macKey, encKey}); err != nil { return nil, err } - for _, key := range [...][]byte{macKey, encKey} { - if err := checkNotText("pairwise: reversible", key); err != nil { - return nil, err - } - } return newReversible(macKey, encKey, opts...) } diff --git a/server/interactioncookie/interactioncookie_fuzz_test.go b/server/interactioncookie/interactioncookie_fuzz_test.go index 0b1f2ef6..8f2e596f 100644 --- a/server/interactioncookie/interactioncookie_fuzz_test.go +++ b/server/interactioncookie/interactioncookie_fuzz_test.go @@ -14,7 +14,7 @@ import ( // the browser: it never panics, and never fails other than with // ErrNoInteraction. func FuzzRead(f *testing.F) { - key := []byte("interactioncookie-fuzz-test-key!") + key := []byte("interactioncookie-fuzz-test-key\x01") c, err := interactioncookie.New([][]byte{key}, interactioncookie.Options{}) if err != nil { f.Fatal(err) From 8660a92e47ffc128f5762b48ff5bf7debc60b7fa Mon Sep 17 00:00:00 2001 From: Oscar Sanderson Date: Tue, 20 Oct 2026 07:08:40 +0800 Subject: [PATCH 2/2] fix(fapihttp)!: offer only the ECDHE AES-GCM cipher suites under TLS 1.2 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit FAPI-CIBA §7.9 (as FAPI 1.0 Part 2 §8.5) permits only the ECDHE AES-GCM cipher suites under TLS 1.2. NewClient's transport offered Go's defaults, CBC suites included, and HardeningOf refused only the suites Go itself calls insecure. The transport now offers exactly the four ECDHE AES-GCM suites under TLS 1.2 (Go has no DHE suites, the other two the specs list); TLS 1.3 is unaffected. HardeningOf counts a transport as hardened only while its TLS 1.2 suites are some of those four, so adding a CBC suite, or clearing the list back to Go's defaults, makes it unhardened. UPGRADING.md has a section. BREAKING CHANGE: a fapihttp.NewClient client no longer connects to a server that offers, under TLS 1.2, only cipher suites other than the four ECDHE AES-GCM ones. Co-Authored-By: Claude Opus 5.5 --- UPGRADING.md | 17 +++++++++ fapihttp/hardening.go | 27 +++++++------ fapihttp/hardening_review_internal_test.go | 44 ++++++++++++++++++++++ fapihttp/transport.go | 13 +++++++ 4 files changed, 90 insertions(+), 11 deletions(-) diff --git a/UPGRADING.md b/UPGRADING.md index 38bfd5ae..48f31430 100644 --- a/UPGRADING.md +++ b/UPGRADING.md @@ -178,6 +178,23 @@ printable text. encoded key before passing it. A random key is refused with probability about 4×10⁻¹⁴. +### Outbound TLS 1.2 uses only ECDHE AES-GCM suites (fapihttp) + +**Affects:** a `fapihttp.NewClient` client connecting to a server that +offers, under TLS 1.2, only cipher suites outside the four ECDHE AES-GCM +ones (CBC suites, say). TLS 1.3 servers are unaffected. So is a client +built some other way. + +**Why:** FAPI-CIBA §7.9 (as FAPI 1.0 Part 2 §8.5) permits only those +suites under TLS 1.2. + +**What fails:** the TLS handshake with such a server. `HardeningOf` also +no longer counts a transport as hardened if its TLS 1.2 suites were +changed to include any other suite, or cleared back to Go's defaults. + +**What to change:** nothing on your side; the server needs a TLS 1.2 +ECDHE AES-GCM suite, or TLS 1.3. + ### Attestation-based client authentication follows draft-12 (server, client) Not a breaking change: clients built to draft-07 keep working, and so diff --git a/fapihttp/hardening.go b/fapihttp/hardening.go index 70444398..98aa386f 100644 --- a/fapihttp/hardening.go +++ b/fapihttp/hardening.go @@ -128,9 +128,10 @@ func registerHardened(t *http.Transport, cfg TransportConfig, dialer, verifier * // - TLSClientConfig still requires TLS 1.2 or later, verifies the // server against the RootCAs it was built with // (TransportConfig.RootCAs, or the system's), with the -// VerifyConnection it was built with (or none), and has no -// VerifyPeerCertificate, ServerName, Time, KeyLogWriter or insecure -// cipher suite set. +// VerifyConnection it was built with (or none), has no +// VerifyPeerCertificate, ServerName, Time or KeyLogWriter set, and +// offers under TLS 1.2 only the ECDHE AES-GCM cipher suites +// NewClient set (FAPI-CIBA §7.9), or some of them. // // Adding a client certificate (TransportConfig.Certificates, or setting // TLSClientConfig.Certificates afterwards) changes none of that. Nor can @@ -206,7 +207,7 @@ func tlsIntact(cfg *tls.Config, record hardenedRecord) bool { if cfg.VerifyPeerCertificate != nil || cfg.ServerName != "" || cfg.Time != nil || cfg.KeyLogWriter != nil { return false } - if cfg.RootCAs != record.rootCAs || hasInsecureCipherSuite(cfg.CipherSuites) { + if cfg.RootCAs != record.rootCAs || !onlyPermittedCipherSuites(cfg.CipherSuites) { return false } return isOwnVerifier(cfg.VerifyConnection, record) @@ -227,13 +228,17 @@ func isOwnVerifier(verify func(tls.ConnectionState) error, record hardenedRecord return errors.As(verify(tls.ConnectionState{ServerName: identityProbeServerName}), &probe) && probe.marker == record.verifier } -// hasInsecureCipherSuite reports whether suites names any of -// tls.InsecureCipherSuites. -func hasInsecureCipherSuite(suites []uint16) bool { - for _, insecure := range tls.InsecureCipherSuites() { - if slices.Contains(suites, insecure.ID) { - return true +// onlyPermittedCipherSuites reports whether suites names at least one +// TLS 1.2 cipher suite and only ones in tls12CipherSuites. An empty list +// is Go's default, which offers CBC suites too. +func onlyPermittedCipherSuites(suites []uint16) bool { + if len(suites) == 0 { + return false + } + for _, suite := range suites { + if !slices.Contains(tls12CipherSuites, suite) { + return false } } - return false + return true } diff --git a/fapihttp/hardening_review_internal_test.go b/fapihttp/hardening_review_internal_test.go index dede4e77..57158fbc 100644 --- a/fapihttp/hardening_review_internal_test.go +++ b/fapihttp/hardening_review_internal_test.go @@ -109,6 +109,10 @@ func TestHardeningOfRefusesTLSWeakenings(t *testing.T) { "fixed clock": func(c *tls.Config) { c.Time = func() time.Time { return time.Unix(0, 0) } }, "fixed server name": func(c *tls.Config) { c.ServerName = "example.com" }, "insecure cipher suite": func(c *tls.Config) { c.CipherSuites = []uint16{tls.InsecureCipherSuites()[0].ID} }, + "CBC cipher suite added": func(c *tls.Config) { + c.CipherSuites = append(c.CipherSuites, tls.TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA) + }, + "default cipher suites": func(c *tls.Config) { c.CipherSuites = nil }, "verify peer certificate": func(c *tls.Config) { c.VerifyPeerCertificate = func([][]byte, [][]*x509.Certificate) error { return nil } }, @@ -315,3 +319,43 @@ func TestNewClientWithoutHTTP2Child(t *testing.T) { } assertSchemesReserved(t, c) } + +// TestNewClientOffersOnlyECDHEGCMUnderTLS12: FAPI-CIBA §7.9 permits only +// the ECDHE AES-GCM cipher suites under TLS 1.2, so a server that +// offers nothing else is reached and one offering only a CBC suite +// isn't. A subset of the permitted suites still counts as hardened. +func TestNewClientOffersOnlyECDHEGCMUnderTLS12(t *testing.T) { + for suite, wantOK := range map[uint16]bool{ + tls.TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256: true, + tls.TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA: false, + } { + ts := httptest.NewUnstartedServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) {})) + ts.TLS = &tls.Config{MaxVersion: tls.VersionTLS12, CipherSuites: []uint16{suite}} + ts.StartTLS() + cfg := RecommendedTransportConfig() + cfg.AllowLoopbackHosts = true + cfg.RootCAs = x509.NewCertPool() + cfg.RootCAs.AddCert(ts.Certificate()) + c, err := NewClient(cfg) + if err != nil { + t.Fatal(err) + } + res, err := c.Get(ts.URL) + if err == nil { + _ = res.Body.Close() + } + if (err == nil) != wantOK { + t.Errorf("%s: Get = %v, want reached %v", tls.CipherSuiteName(suite), err, wantOK) + } + ts.Close() + } + + c, err := NewClient(RecommendedTransportConfig()) + if err != nil { + t.Fatal(err) + } + c.Transport.(*http.Transport).TLSClientConfig.CipherSuites = []uint16{tls.TLS_ECDHE_ECDSA_WITH_AES_256_GCM_SHA384} + if _, ok := HardeningOf(c); !ok { + t.Error("HardeningOf(a subset of the permitted suites) = false, want true") + } +} diff --git a/fapihttp/transport.go b/fapihttp/transport.go index decc22df..8238ec3d 100644 --- a/fapihttp/transport.go +++ b/fapihttp/transport.go @@ -135,6 +135,7 @@ func NewClient(cfg TransportConfig) (*http.Client, error) { TLSHandshakeTimeout: cfg.TLSHandshakeTimeout, TLSClientConfig: &tls.Config{ MinVersion: tls.VersionTLS12, + CipherSuites: slices.Clone(tls12CipherSuites), VerifyConnection: verifyConnection, RootCAs: cfg.RootCAs, Certificates: cfg.Certificates, @@ -404,3 +405,15 @@ func isZero(b []byte) bool { } return true } + +// tls12CipherSuites are the only TLS 1.2 cipher suites NewClient's +// transport offers: the ECDHE AES-GCM ones FAPI-CIBA §7.9 (as FAPI 1.0 +// Part 2 §8.5) permits under TLS 1.2. Go has no DHE suites, the other +// two that list names. TLS 1.3, whose suites Go doesn't let a +// configuration choose, is unaffected. +var tls12CipherSuites = []uint16{ + tls.TLS_ECDHE_ECDSA_WITH_AES_128_GCM_SHA256, + tls.TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256, + tls.TLS_ECDHE_ECDSA_WITH_AES_256_GCM_SHA384, + tls.TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384, +}