amadan

oficina / meet Public

relay: fix round 1 — stop echoing fields, redact Region, pin edges

- Parse's per-element errors name the index only; e.Region no longer
  reaches an error message (HIGH).
- Region.String() redacts Key, so a %v/%+v of a parsed region can
  never print one by accident.
- TestParseRefusesWithoutEchoingTheValue: each case now carries an
  unwanted []string naming a secret it plants; new cases plant the
  vector key in region, key and username_suffix, plus a JSON
  syntax-error case with a NEEDLE token.
- Parse of [] or null now answers nil, nil, same as "".
- An element with an empty url is refused, naming the index and
  position, never the url.
- Mint documents its expiry precondition and skips minting on a
  negative Unix time; Mint documents (and a snapshot test pins) that
  it never mutates regions and is safe for concurrent callers.
- New: a golden-JSON test of the ICEServer wire shape, one negative
  case per suffix field plus a five-field and trailing-newline case,
  a Credential vector that exercises '+'/'/' to pin the standard
  alphabet, and a TURN: uppercase-scheme case.

Verified against the pre-fix relay.go: the region-is-the-vector-key,
empty-url and negative-expiry tests fail there and pass here.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01CzvnuKhLzudvq7jZ7SvVt2
Paul Campbell pushed by paul@keymail.dev 991352e4e2c38917d56c99bb65b953bcde3d7e28 parent be5a4ce
2 files changed, +267 −20
  • internal/meet/relay/relay.go +39 −4
  • internal/meet/relay/relay_test.go +228 −16
diff --git a/internal/meet/relay/relay.go b/internal/meet/relay/relay.go
index 75b528d..0ad3665 100644
--- a/internal/meet/relay/relay.go
+++ b/internal/meet/relay/relay.go
@@ -28,6 +28,13 @@ type Region struct {
UsernameSuffix string
}
+// String redacts Key, so a %v or %+v of a Region — a test failure
+// message, a debugger, anything that did not mean to log a key — never
+// prints one.
+func (r Region) String() string {
+ return fmt.Sprintf("{Region:%s URLs:%v Key:[%d bytes redacted] UsernameSuffix:%s}", r.Region, r.URLs, len(r.Key), r.UsernameSuffix)
+}
+
// ICEServer is one RTCIceServer as the browser wants it. A stun entry
// carries no username; a turn entry carries the minted pair.
type ICEServer struct {
@@ -72,18 +79,31 @@ func Parse(raw string) ([]Region, error) {
return nil, fmt.Errorf("relay: element %d has no region", i)
}
if len(e.URLs) == 0 {
- return nil, fmt.Errorf("relay: element %d (%s) has no urls", i, e.Region)
+ return nil, fmt.Errorf("relay: element %d has no urls", i)
+ }
+ for pos, u := range e.URLs {
+ if u == "" {
+ return nil, fmt.Errorf("relay: element %d has an empty url at position %d", i, pos)
+ }
}
key, err := base64.StdEncoding.DecodeString(e.Key)
if err != nil || len(key) != 32 {
- return nil, fmt.Errorf("relay: element %d (%s) key is not 32 base64 bytes", i, e.Region)
+ return nil, fmt.Errorf("relay: element %d key is not 32 base64 bytes", i)
}
fields := strings.Split(e.UsernameSuffix, ":")
if len(fields) != 4 || !accountRe.MatchString(fields[0]) || !appRe.MatchString(fields[1]) || !incarnationRe.MatchString(fields[2]) || !kidRe.MatchString(fields[3]) {
- return nil, fmt.Errorf("relay: element %d (%s) username_suffix is not account:app:incarnation:kid", i, e.Region)
+ return nil, fmt.Errorf("relay: element %d username_suffix is not account:app:incarnation:kid", i)
}
out = append(out, Region{Region: e.Region, URLs: append([]string(nil), e.URLs...), Key: key, UsernameSuffix: e.UsernameSuffix})
}
+ // An empty array, a JSON null, or an all-whitespace value: all three
+ // decode without error to zero regions, and all three mean the same
+ // thing as an absent variable — no relay — so they answer the same
+ // nil, not a non-nil empty slice one caller happens to get and
+ // another doesn't.
+ if len(out) == 0 {
+ return nil, nil
+ }
return out, nil
}
@@ -99,9 +119,24 @@ func Credential(key []byte, username string) string {
// contract's order: one bare entry for every URL whose scheme is not
// turn or turns (stun, stuns, or a scheme the contract adds later),
// then one credentialed entry for the turn and turns URLs. Either may
-// be absent. Expiry is the caller's; §5.2 uses three hours ahead.
+// be absent. Expiry is the caller's; §5.2 uses three hours ahead, and
+// §5.3's maximum is four; Mint does not enforce either bound — the
+// caller owns keeping expiry inside the contract, exactly as the
+// caller already owns picking it. The one thing Mint refuses on its
+// own is a negative Unix time: that is not a real expiry (TURN's REST
+// scheme embeds it as a bare decimal, and a leading "-" would name a
+// username no server was issued a credential for), so Mint mints
+// nothing rather than a credential built on a nonsense clock.
+//
+// Mint only reads regions — it never writes to a Region or its slices
+// — so many goroutines may call Mint with the same regions
+// concurrently; that is exactly the shape a.relayRegions() hands
+// concurrent /ice requests.
func Mint(regions []Region, expiry time.Time) []ICEServer {
out := []ICEServer{}
+ if expiry.Unix() < 0 {
+ return out
+ }
username := strconv.FormatInt(expiry.Unix(), 10)
for _, r := range regions {
var bare, turn []string
diff --git a/internal/meet/relay/relay_test.go b/internal/meet/relay/relay_test.go
index ce60a34..d18a306 100644
--- a/internal/meet/relay/relay_test.go
+++ b/internal/meet/relay/relay_test.go
@@ -3,6 +3,7 @@ package relay
import (
"encoding/base64"
"encoding/json"
+ "reflect"
"strings"
"testing"
"time"
@@ -36,13 +37,35 @@ func TestThePlatformVectorMints(t *testing.T) {
}
}
+// TestCredentialUsesTheStandardAlphabet pins base64.StdEncoding rather
+// than base64.URLEncoding: the two agree except on '+'/'-' and '/'/'_',
+// so a vector whose digest never needs either byte would pass under
+// either encoding. This username's digest does need one.
+func TestCredentialUsesTheStandardAlphabet(t *testing.T) {
+ key, _ := base64.StdEncoding.DecodeString(vectorKey)
+ const username = "1900000001:abc:meet:0123456789abcdef:k1"
+ got := Credential(key, username)
+ if !strings.ContainsAny(got, "+/") {
+ t.Fatalf("test vector %q does not exercise the standard alphabet, got %q", username, got)
+ }
+ if got != "XcTS/L4xj87J1gqfQHjq1Z3F5iL4iEXlOAqxAmI8Cow=" {
+ t.Fatalf("credential %q does not match the platform vector", got)
+ }
+}
+
func TestParseAcceptsTheContractAndDecodesTheKey(t *testing.T) {
regions, err := Parse(encode(t, oneRegion()))
if err != nil {
t.Fatal(err)
}
- if len(regions) != 1 || regions[0].Region != "eu-west-1" || len(regions[0].Key) != 32 || regions[0].UsernameSuffix != "abc:meet:0123456789abcdef:k1" || len(regions[0].URLs) != 3 {
- t.Fatalf("parsed %+v", regions)
+ if len(regions) != 1 {
+ t.Fatalf("parsed %d regions, want 1", len(regions))
+ }
+ r := regions[0]
+ if r.Region != "eu-west-1" || len(r.Key) != 32 || r.UsernameSuffix != "abc:meet:0123456789abcdef:k1" || len(r.URLs) != 3 {
+ // Region, UsernameSuffix and the URL/key lengths are not
+ // secrets; the key bytes themselves are never printed here.
+ t.Fatalf("parsed region=%q keylen=%d suffix=%q urls=%d", r.Region, len(r.Key), r.UsernameSuffix, len(r.URLs))
}
}
@@ -53,26 +76,121 @@ func TestParseOfNothingIsNoRelay(t *testing.T) {
}
}
+// TestParseOfAnEmptyArrayOrNullIsNoRelay: an empty array and a JSON
+// null both decode without error to zero regions, and both must mean
+// the same thing an absent variable does — nil, not a non-nil empty
+// slice one caller happens to get and another doesn't.
+func TestParseOfAnEmptyArrayOrNullIsNoRelay(t *testing.T) {
+ for name, body := range map[string]string{"empty array": "[]", "json null": "null"} {
+ t.Run(name, func(t *testing.T) {
+ regions, err := Parse(base64.StdEncoding.EncodeToString([]byte(body)))
+ if err != nil || regions != nil {
+ t.Fatalf("%s must mean no relay: %v %v", name, regions, err)
+ }
+ })
+ }
+}
+
+// TestParseRefusesWithoutEchoingTheValue proves every refusal names the
+// element index (and, for a bad url, its position) and never the value
+// that made it bad. Most cases assert the general "the raw input never
+// appears" property; the cases with a non-nil unwanted also plant a
+// specific secret — the platform vector key — into the one field whose
+// check used to echo it (region, before the fix) or that a careless
+// change to any other field's check could start echoing (key,
+// username_suffix), and into a JSON syntax error, so a leak in any of
+// those paths fails loudly here rather than only in a log line no test
+// is watching.
func TestParseRefusesWithoutEchoingTheValue(t *testing.T) {
- cases := map[string]string{
- "not base64": "%%%",
- "not json": base64.StdEncoding.EncodeToString([]byte("nope")),
- "not an array": base64.StdEncoding.EncodeToString([]byte(`{"region":"x"}`)),
- "short key": encode(t, []map[string]any{{"region": "eu-west-1", "urls": []string{"stun:a:1"}, "key": base64.StdEncoding.EncodeToString([]byte("short")), "username_suffix": "abc:meet:0123456789abcdef:k1"}}),
- "suffix wrong fields": encode(t, []map[string]any{{"region": "eu-west-1", "urls": []string{"stun:a:1"}, "key": vectorKey, "username_suffix": "abc:meet:k1"}}),
- "suffix bad app": encode(t, []map[string]any{{"region": "eu-west-1", "urls": []string{"stun:a:1"}, "key": vectorKey, "username_suffix": "abc:Meet!:0123456789abcdef:k1"}}),
- "no urls": encode(t, []map[string]any{{"region": "eu-west-1", "urls": []string{}, "key": vectorKey, "username_suffix": "abc:meet:0123456789abcdef:k1"}}),
- "empty region": encode(t, []map[string]any{{"region": "", "urls": []string{"stun:a:1"}, "key": vectorKey, "username_suffix": "abc:meet:0123456789abcdef:k1"}}),
- }
- for name, raw := range cases {
+ cases := map[string]struct {
+ raw string
+ unwanted []string
+ }{
+ "not base64": {"%%%", nil},
+ "not json": {base64.StdEncoding.EncodeToString([]byte("nope")), nil},
+ "not an array": {base64.StdEncoding.EncodeToString([]byte(`{"region":"x"}`)), nil},
+ "json syntax error": {
+ base64.StdEncoding.EncodeToString([]byte(`[{"region":"NEEDLE-SHOULD-NOT-LEAK",}]`)),
+ []string{"NEEDLE-SHOULD-NOT-LEAK"},
+ },
+ "short key": {
+ encode(t, []map[string]any{{"region": "eu-west-1", "urls": []string{"stun:a:1"}, "key": base64.StdEncoding.EncodeToString([]byte("short")), "username_suffix": "abc:meet:0123456789abcdef:k1"}}),
+ nil,
+ },
+ "suffix wrong fields": {
+ encode(t, []map[string]any{{"region": "eu-west-1", "urls": []string{"stun:a:1"}, "key": vectorKey, "username_suffix": "abc:meet:k1"}}),
+ nil,
+ },
+ "suffix bad account": {
+ encode(t, []map[string]any{{"region": "eu-west-1", "urls": []string{"stun:a:1"}, "key": vectorKey, "username_suffix": "ABC:meet:0123456789abcdef:k1"}}),
+ nil,
+ },
+ "suffix bad app": {
+ encode(t, []map[string]any{{"region": "eu-west-1", "urls": []string{"stun:a:1"}, "key": vectorKey, "username_suffix": "abc:Meet!:0123456789abcdef:k1"}}),
+ nil,
+ },
+ "suffix bad incarnation": {
+ encode(t, []map[string]any{{"region": "eu-west-1", "urls": []string{"stun:a:1"}, "key": vectorKey, "username_suffix": "abc:meet:XYZ:k1"}}),
+ nil,
+ },
+ "suffix bad kid": {
+ encode(t, []map[string]any{{"region": "eu-west-1", "urls": []string{"stun:a:1"}, "key": vectorKey, "username_suffix": "abc:meet:0123456789abcdef:toolongkid"}}),
+ nil,
+ },
+ "suffix five fields": {
+ encode(t, []map[string]any{{"region": "eu-west-1", "urls": []string{"stun:a:1"}, "key": vectorKey, "username_suffix": "abc:meet:0123456789abcdef:k1:extra"}}),
+ nil,
+ },
+ "suffix trailing newline": {
+ encode(t, []map[string]any{{"region": "eu-west-1", "urls": []string{"stun:a:1"}, "key": vectorKey, "username_suffix": "abc:meet:0123456789abcdef:k1\n"}}),
+ nil,
+ },
+ "no urls": {
+ encode(t, []map[string]any{{"region": "eu-west-1", "urls": []string{}, "key": vectorKey, "username_suffix": "abc:meet:0123456789abcdef:k1"}}),
+ nil,
+ },
+ "empty url": {
+ encode(t, []map[string]any{{"region": "eu-west-1", "urls": []string{"stun:a:1", ""}, "key": vectorKey, "username_suffix": "abc:meet:0123456789abcdef:k1"}}),
+ nil,
+ },
+ "empty region": {
+ encode(t, []map[string]any{{"region": "", "urls": []string{"stun:a:1"}, "key": vectorKey, "username_suffix": "abc:meet:0123456789abcdef:k1"}}),
+ nil,
+ },
+ "region is the vector key": {
+ // Before the fix, the "no urls" and "key" and
+ // "username_suffix" errors all echoed e.Region; this plants
+ // the key there so a regression trips this test, not only a
+ // log line nothing was watching.
+ encode(t, []map[string]any{{"region": vectorKey, "urls": []string{}, "key": vectorKey, "username_suffix": "abc:meet:0123456789abcdef:k1"}}),
+ []string{vectorKey},
+ },
+ "key value carries the vector key": {
+ // Not valid base64 (the trailing junk breaks decoding), but
+ // the vector key is a substring, so a key-check error that
+ // ever echoed e.Key would leak it too.
+ encode(t, []map[string]any{{"region": "eu-west-1", "urls": []string{"stun:a:1"}, "key": vectorKey + "!!!", "username_suffix": "abc:meet:0123456789abcdef:k1"}}),
+ []string{vectorKey},
+ },
+ "username_suffix is the vector key": {
+ encode(t, []map[string]any{{"region": "eu-west-1", "urls": []string{"stun:a:1"}, "key": vectorKey, "username_suffix": vectorKey}}),
+ []string{vectorKey},
+ },
+ }
+ for name, tc := range cases {
t.Run(name, func(t *testing.T) {
- _, err := Parse(raw)
+ _, err := Parse(tc.raw)
if err == nil {
t.Fatal("accepted")
}
- if strings.Contains(err.Error(), vectorKey) || strings.Contains(err.Error(), "0123456789abcdef") || strings.Contains(err.Error(), raw) {
+ if strings.Contains(err.Error(), vectorKey) || strings.Contains(err.Error(), "0123456789abcdef") || strings.Contains(err.Error(), tc.raw) {
t.Fatalf("the error echoes the value: %v", err)
}
+ for _, secret := range tc.unwanted {
+ if strings.Contains(err.Error(), secret) {
+ t.Fatalf("the error echoes the planted secret %q: %v", secret, err)
+ }
+ }
})
}
}
@@ -85,7 +203,7 @@ func TestMintCarriesCredentialsOnTurnOnlyAndPassesURLsThrough(t *testing.T) {
expiry := time.Unix(1900000000, 0)
servers := Mint(regions, expiry)
if len(servers) != 2 {
- t.Fatalf("want a bare stun entry and a credentialed turn entry, got %+v", servers)
+ t.Fatalf("want a bare stun entry and a credentialed turn entry, got %d entries", len(servers))
}
stun, turn := servers[0], servers[1]
if len(stun.URLs) != 1 || stun.URLs[0] != "stun:relay.eu-west-1.oncarlos.com:3478" || stun.Username != "" || stun.Credential != "" {
@@ -112,8 +230,102 @@ func TestMintKeepsConsoleOrderAndUnknownSchemesBare(t *testing.T) {
}
}
+// TestSchemeMatchingIsCaseInsensitive pins scheme()'s case-folding: an
+// uppercase "TURN:" must still be credentialed, not passed through
+// bare, since the contract does not promise the platform lowercases
+// its own URLs.
+func TestSchemeMatchingIsCaseInsensitive(t *testing.T) {
+ region := []map[string]any{{
+ "region": "eu-west-1",
+ "urls": []string{"STUN:relay.eu-west-1.oncarlos.com:3478", "TURN:relay.eu-west-1.oncarlos.com:3478?transport=udp"},
+ "key": vectorKey,
+ "username_suffix": "abc:meet:0123456789abcdef:k1",
+ }}
+ regions, err := Parse(encode(t, region))
+ if err != nil {
+ t.Fatal(err)
+ }
+ servers := Mint(regions, time.Unix(1900000000, 0))
+ if len(servers) != 2 || servers[0].Username != "" || servers[1].Username == "" {
+ t.Fatalf("an uppercase TURN: scheme was not credentialed: %+v", servers)
+ }
+}
+
func TestMintOfNoRegionsIsAnEmptyListNotNil(t *testing.T) {
if got := Mint(nil, time.Now()); got == nil || len(got) != 0 {
t.Fatalf("want [] so the JSON is [] not null, got %#v", got)
}
}
+
+// TestMintSkipsANegativeExpiry: a Unix time before the epoch is not a
+// real expiry, and Mint refuses to build a credential on one rather
+// than mint a username with a "-" a TURN server never issued a
+// credential for.
+func TestMintSkipsANegativeExpiry(t *testing.T) {
+ regions, err := Parse(encode(t, oneRegion()))
+ if err != nil {
+ t.Fatal(err)
+ }
+ got := Mint(regions, time.Unix(-1, 0))
+ if got == nil || len(got) != 0 {
+ t.Fatalf("want [], got %#v", got)
+ }
+}
+
+// TestMintDoesNotMutateRegions: Mint only reads regions, so the same
+// slice a.relayRegions() hands out can be read by many concurrent /ice
+// requests without a copy. A deep snapshot taken before Mint, compared
+// after, is the guard against a future edit that starts writing
+// through it.
+func TestMintDoesNotMutateRegions(t *testing.T) {
+ regions, err := Parse(encode(t, oneRegion()))
+ if err != nil {
+ t.Fatal(err)
+ }
+ want := make([]Region, len(regions))
+ for i, r := range regions {
+ want[i] = Region{
+ Region: r.Region,
+ URLs: append([]string(nil), r.URLs...),
+ Key: append([]byte(nil), r.Key...),
+ UsernameSuffix: r.UsernameSuffix,
+ }
+ }
+ _ = Mint(regions, time.Unix(1900000000, 0))
+ if !reflect.DeepEqual(regions, want) {
+ t.Fatal("Mint mutated regions")
+ }
+}
+
+// TestICEServerJSONShapeIsExactlyTheWireContract is the golden shape a
+// native client relies on: a bare entry carries only "urls"; a
+// credentialed entry carries "urls", "username" and "credential" and
+// nothing else — omitempty must not leave a stray key on either side.
+func TestICEServerJSONShapeIsExactlyTheWireContract(t *testing.T) {
+ regions, err := Parse(encode(t, oneRegion()))
+ if err != nil {
+ t.Fatal(err)
+ }
+ servers := Mint(regions, time.Unix(1900000000, 0))
+ b, err := json.Marshal(servers)
+ if err != nil {
+ t.Fatal(err)
+ }
+ var raw []map[string]json.RawMessage
+ if err := json.Unmarshal(b, &raw); err != nil {
+ t.Fatal(err)
+ }
+ if len(raw) != 2 {
+ t.Fatalf("want a bare entry and a turn entry, got %d", len(raw))
+ }
+ for k := range raw[0] {
+ if k != "urls" {
+ t.Fatalf("bare entry carries an extra key %q", k)
+ }
+ }
+ for k := range raw[1] {
+ if k != "urls" && k != "username" && k != "credential" {
+ t.Fatalf("turn entry carries an extra key %q", k)
+ }
+ }
+}