auth/upstream: keep only printable ASCII, and name the cause's class

auth/exchange/exchange_test.go

@@ -1400,3 +1400,114 @@ func TestDroppedRuneCannotRejoinASecret(t *testing.T) {
 		})
 	}
 }
+
+
+// TestErrorDescriptionRedactsReplacementSplitCredentials: a credential split
+// by U+FFFD, or by bytes that are not valid UTF-8 (which the JSON decoder
+// turns into U+FFFD), must not dodge exact redaction and then rejoin when the
+// rendering drops the non-ASCII between its halves. The secrets are shorter
+// than the token-run backstop, so only the exact match can catch them, and
+// both the terminal description and an infrastructure failure's rendered
+// text are checked.
+func TestErrorDescriptionRedactsReplacementSplitCredentials(t *testing.T) {
+	const (
+		secret  = "abcdef123456789" // #nosec G101 -- test fixture
+		subject = "subj0123456789"
+	)
+	splitters := map[string]string{
+		"escaped U+FFFD":       `�`,
+		"literal U+FFFD":       "�",
+		"lone surrogate":       `\ud800`,
+		"invalid byte 0xff":    "\xff",
+		"overlong NUL":         "\xc0\x80",
+		"truncated multi-byte": "\xe2\x82",
+		"double U+FFFD":        "��",
+	}
+	split := func(s, sep string) string { return s[:6] + sep + s[6:] }
+	for name, sep := range splitters {
+		for _, code := range []string{"invalid_grant", "invalid_client"} {
+			t.Run(name+"/"+code, func(t *testing.T) {
+				srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+					w.Header().Set("Content-Type", "application/json")
+					w.WriteHeader(http.StatusBadRequest)
+					_, _ = fmt.Fprintf(w, `{"error":%q,"error_description":"secret=%s subject=%s"}`,
+						code, split(secret, sep), split(subject, sep))
+				}))
+				defer srv.Close()
+
+				ex, err := New(Config{TokenURL: srv.URL, Auth: ClientSecretPost("id", secret)})
+				if err != nil {
+					t.Fatal(err)
+				}
+				_, err = ex.Exchange(context.Background(), subject)
+				if err == nil {
+					t.Fatal("want an error")
+				}
+				texts := []string{err.Error()}
+				var te *TerminalError
+				if errors.As(err, &te) {
+					texts = append(texts, te.Description, upstream.ScrubDescription(te.Description))
+				}
+				var ie *InfraError
+				if errors.As(err, &ie) {
+					texts = append(texts, ie.Description)
+				}
+				if te == nil && ie == nil {
+					t.Fatalf("want TerminalError or InfraError, got %T: %v", err, err)
+				}
+				for _, text := range texts {
+					// Also drop everything but printable ASCII, as a viewer
+					// or normalizer downstream might.
+					for _, s := range []string{text, upstream.PrintableASCII(text)} {
+						if strings.Contains(s, secret) || strings.Contains(s, subject) {
+							t.Fatalf("split credential rejoined: %q", s)
+						}
+					}
+				}
+			})
+		}
+	}
+}
+
+// TestInvisibleMarkCannotSplitASecret: a combining mark (variation selector
+// 16, combining grapheme joiner) is invisible but neither a control nor a
+// format character. Threaded through an echoed credential every few bytes,
+// it splits the value into pieces shorter than both the fragment and the
+// token-run redactions, and a viewer that drops it shows the credential
+// whole. Only printable ASCII survives the strip, so the pieces rejoin before
+// redaction sees them.
+func TestInvisibleMarkCannotSplitASecret(t *testing.T) {
+	const secret = "abcdef123456789" // #nosec G101 -- test fixture
+	for _, mark := range []string{"͏", "️", "⃝"} {
+		t.Run(fmt.Sprintf("%U", []rune(mark)[0]), func(t *testing.T) {
+			var threaded strings.Builder
+			for i := 0; i < len(secret); i += 3 {
+				threaded.WriteString(secret[i:min(i+3, len(secret))] + mark)
+			}
+			srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+				w.Header().Set("Content-Type", "application/json")
+				w.WriteHeader(http.StatusUnauthorized)
+				_, _ = fmt.Fprintf(w, `{"error":"invalid_client","error_description":"was %s"}`, threaded.String())
+			}))
+			defer srv.Close()
+			ex, err := New(Config{TokenURL: srv.URL, Auth: ClientSecretPost("id", secret)})
+			if err != nil {
+				t.Fatal(err)
+			}
+			_, err = ex.Exchange(context.Background(), "subject-tok")
+			if !IsInfra(err) {
+				t.Fatalf("want InfraError, got %v", err)
+			}
+			// As a viewer that drops what it can't show would render it.
+			shown := strings.Map(func(r rune) rune {
+				if r < 0x20 || r >= 0x7f {
+					return -1
+				}
+				return r
+			}, err.Error())
+			if strings.Contains(shown, secret) {
+				t.Fatalf("secret rejoined in the error text: %q", err.Error())
+			}
+		})
+	}
+}

auth/exchange/oautherror.go

@@ -47,23 +47,13 @@ func ParseOAuthError(body []byte, form url.Values) (code, description string) {
 	return code, description
 }
 
-// stripControls removes the runes upstream.LogSafeRune rejects (controls,
-// line and paragraph separators, format characters, U+FFFD). Applied to both the
-// endpoint text and each redaction candidate so a secret carrying a tab
-// arrives control-stripped in the echoed text and only an identically
-// normalized candidate can match it.
-func stripControls(v string) string {
-	var b strings.Builder
-	for _, r := range v {
-		// The same runes upstream.ScrubDescription drops when rendering:
-		// one left in here would split a credential past redaction and
-		// rejoin it in the rendered text.
-		if upstream.LogSafeRune(r) {
-			b.WriteRune(r)
-		}
-	}
-	return b.String()
-}
+// stripControls keeps only printable ASCII (upstream.PrintableASCII), the
+// same strip upstream.ScrubDescription applies when rendering: a rune left in
+// here would split a credential past redaction and rejoin it in the rendered
+// text. Applied to both the endpoint text and each redaction candidate so a
+// secret carrying a tab arrives stripped in the echoed text and only an
+// identically normalized candidate can match it.
+func stripControls(v string) string { return upstream.PrintableASCII(v) }
 
 // credentialFields are the form parameters whose values are secrets at ANY
 // length, spanning both the token-exchange form (subject_token,

auth/upstream/failure.go

@@ -22,10 +22,17 @@
 package upstream
 
 import (
+	"context"
+	"crypto/tls"
+	"crypto/x509"
+	"encoding/json"
 	"errors"
 	"fmt"
+	"io"
+	"net"
+	"net/http"
 	"strings"
-	"unicode"
+	"syscall"
 	"unicode/utf8"
 )
 
@@ -37,7 +44,11 @@ const MaxDescriptionBytes = 200
 // minTokenRun is the shortest run of token-alphabet characters a Failure's
 // description redacts. bc3's tokens, JWT client assertions and generated
 // client secrets are all longer; English words in an OAuth error_description
-// are all shorter.
+// are all shorter. A shorter run is kept, so a fragment of a credential under
+// this length can render if an endpoint truncates its own echo: the guarantee
+// is that no usable credential renders, not that no byte of one does. Every
+// credential the request submitted is also removed whole, at any length, by
+// exchange.ParseOAuthError before the description gets here.
 const minTokenRun = 20
 
 // Failure is a failed call to a credentialed endpoint. Its Error text is safe
@@ -89,6 +100,10 @@ func (f *Failure) Error() string {
 		b.WriteString(": ")
 		b.WriteString(f.Reason)
 	}
+	if c := causeClass(f.cause); c != "" {
+		b.WriteString(": ")
+		b.WriteString(c)
+	}
 	if d := ScrubDescription(f.Description); d != "" {
 		fmt.Fprintf(&b, " (%s)", d)
 	}
@@ -107,31 +122,75 @@ func (f *Failure) As(target any) bool {
 	return f.cause != nil && errors.As(f.cause, target)
 }
 
-// LogSafeRune reports whether r may stay in a log line rendered from
-// endpoint text. It drops every control character (C0, DEL and C1, which
-// includes NEL), the Unicode line and paragraph separators, format
-// characters (bidi overrides, zero-width joiners) and U+FFFD: anything a
-// sink might render as a line break or use to disguise text. ScrubDescription
-// and the exchange's redaction both strip with it, so a credential split by
-// a dropped rune is rejoined before redaction rather than after.
-func LogSafeRune(r rune) bool {
-	return r != utf8.RuneError && !unicode.IsControl(r) && !unicode.In(r, unicode.Zl, unicode.Zp, unicode.Cf)
+// causeClass names what kind of failure cause is, in fixed text, so an
+// operator reading a log line can still tell a timeout from a refused
+// connection from a bad certificate without the cause's own text. Every
+// phrase is chosen here; nothing is read out of the cause but its type and
+// the identity of sentinel errors. The one exception is a JSON type error's
+// Field, which is the path of the Go struct field being decoded: this
+// server's own names, never response bytes.
+func causeClass(err error) string {
+	if err == nil {
+		return ""
+	}
+	var (
+		netErr     net.Error
+		dnsErr     *net.DNSError
+		opErr      *net.OpError
+		unknownCA  x509.UnknownAuthorityError
+		certErr    x509.CertificateInvalidError
+		hostErr    x509.HostnameError
+		recordErr  tls.RecordHeaderError
+		syntaxErr  *json.SyntaxError
+		typeErr    *json.UnmarshalTypeError
+		maxBytes   *http.MaxBytesError
+		protoError *http.ProtocolError
+	)
+	switch {
+	case errors.Is(err, context.DeadlineExceeded):
+		return "deadline exceeded"
+	case errors.Is(err, context.Canceled):
+		return "canceled"
+	case errors.As(err, &netErr) && netErr.Timeout():
+		return "timeout"
+	case errors.As(err, &dnsErr):
+		return "DNS lookup failed"
+	case errors.Is(err, syscall.ECONNREFUSED):
+		return "connection refused"
+	case errors.Is(err, syscall.ECONNRESET):
+		return "connection reset"
+	case errors.As(err, &unknownCA), errors.As(err, &certErr), errors.As(err, &hostErr):
+		return "TLS certificate verification failed"
+	case errors.As(err, &recordErr):
+		return "TLS handshake failed"
+	case errors.Is(err, io.ErrUnexpectedEOF), errors.Is(err, io.EOF):
+		return "response ended early"
+	case errors.As(err, &syntaxErr):
+		return "response is not valid JSON"
+	case errors.As(err, &typeErr):
+		if typeErr.Field != "" {
+			return "unexpected JSON value for " + typeErr.Field
+		}
+		return "unexpected JSON value"
+	case errors.As(err, &maxBytes):
+		return "response too large"
+	case errors.As(err, &protoError):
+		return "malformed HTTP response"
+	case errors.As(err, &opErr):
+		return "network error during " + opErr.Op
+	default:
+		return ""
+	}
 }
 
-// ScrubDescription renders endpoint-supplied text for a log line: runes
-// LogSafeRune rejects stripped, every run of at least minTokenRun token-alphabet
+// ScrubDescription renders endpoint-supplied text for a log line: control
+// and everything but printable ASCII stripped (PrintableASCII), every run of at least minTokenRun token-alphabet
 // characters replaced with [redacted], and the result bounded to
 // MaxDescriptionBytes. Redaction runs over the whole text before the bound,
 // so the bound can never cut a token down to a prefix that escapes the run
 // length.
 func ScrubDescription(s string) string {
-	var stripped strings.Builder
-	for _, r := range s {
-		if LogSafeRune(r) {
-			stripped.WriteRune(r)
-		}
-	}
-	text := stripped.String()
+	text := PrintableASCII(s)
 
 	var b strings.Builder
 	run := 0
@@ -172,6 +231,25 @@ func ScrubDescription(s string) string {
 	return out[:cut] + "…"
 }
 
+// PrintableASCII keeps only printable ASCII (space through tilde) from s.
+// Redaction runs on the result, so nothing invisible can sit inside an
+// echoed credential and split it into pieces that exact matching and the
+// token-run length both miss, for a viewer or a normalizer that drops
+// invisible characters to put back together. An allowlist, because the
+// invisible set is not: C0 and DEL, then C1 and format characters, then
+// variation selectors were each found in turn on review, and Unicode has
+// more. OAuth error descriptions are ASCII by specification (RFC 6749
+// §5.2), so nothing a conformant endpoint says is lost.
+func PrintableASCII(s string) string {
+	var b strings.Builder
+	for i := 0; i < len(s); i++ {
+		if c := s[i]; c >= 0x20 && c < 0x7f {
+			b.WriteByte(c)
+		}
+	}
+	return b.String()
+}
+
 // isTokenByte reports whether c can appear in a bearer token, a JWT, a
 // base64 or base64url secret, or a form-encoded one.
 func isTokenByte(c byte) bool {