Repository navigation
test(lk): property test for credential and project resolution - #1072
Conversation
81c5844 to
7d625e4
Compare
2442d50 to
15707ee
Compare
7d625e4 to
e07a424
Compare
15707ee to
0b83af7
Compare
With mint set, the fake answers a Public API token request it has no canned response for as CreateToken does: an access token carrying the requested grants, signed with a key for that project, so lk's grant check passes and it goes on to the project's server. It mints only for a bearer in sessions; any other gets the Public API's 401. For keys whose secret it knows, it logs a token signed with the wrong secret as badsig. Both use livekit/protocol/auth.
TestCredentialResolution draws every input lk uses to pick a credential and a project (auth-mode flags, --project, --subdomain, --dev, inline credentials from flags, env or both, configured projects, the default project, livekit.toml, signed-in users and their projects, the default user, --user, and the Public API URL) and runs room list, egress list or project key list, one for each way lk loads a project. It checks rules that hold whatever lk's precedence picks: no request names a project the user didn't name, no token from a known key is signed with the wrong secret, each key and each minted token only reaches its own project's server, each auth mode keeps to its own credentials, and lk fails without naming a project when no user can act. Which project or user lk picks among the allowed ones is left to the scripts, so the test doesn't restate the resolver. Failures shrink to a minimal case and print a seed to replay; cmd/lk/testdata/rapid/ is gitignored.
0b83af7 to
4bd70d2
Compare
| cloud := newFakeCloud() | ||
| defer cloud.Close() |
There was a problem hiding this comment.
When a case has --dev, or an inline key and secret but no URL, lk uses the --url default http://localhost:7880. Go's http.ProxyFromEnvironment never proxies localhost, so the request skips HTTP_PROXY and goes straight to port 7880:
$ HTTP_PROXY=http://127.0.0.1:9 lk --api-key APIflag --api-secret secretflag room list
twirp error internal: failed to do request: Post "http://localhost:7880/twirp/livekit.RoomService/ListRooms": dial tcp [::1]:7880: connect: connection refused
So the fake never logs these requests, and none of the rules run on them: a regression that sends a configured project's key to the default URL passes. On a machine running livekit-server --dev, the test also calls ListRooms and ListEgress on that server. Having the fake also serve localhost:7880, as localhost.test, brings them under the rules. If something else holds the port, the case fails instead of talking to it. This needs net and net/http/httptest imports, plus the follow-ups on lines 98, 163, 430 and 473:
| cloud := newFakeCloud() | |
| defer cloud.Close() | |
| cloud := newFakeCloud() | |
| defer cloud.Close() | |
| // lk's --url default, http://localhost:7880, never goes through | |
| // HTTP_PROXY, so the fake also serves it, as localhost.test. | |
| direct := httptest.NewUnstartedServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { | |
| r.URL.Scheme, r.URL.Host = "http", "localhost.test" | |
| cloud.serve(w, r) | |
| })) | |
| direct.Listener.Close() | |
| if direct.Listener, err = net.Listen("tcp", "localhost:7880"); err != nil { | |
| t.Fatalf("lk's default URL must reach the fake, not a real server: %v", err) | |
| } | |
| direct.Start() | |
| defer direct.Close() |
| t.Fatal(err) | ||
| } | ||
| } | ||
| for _, name := range []string{"one", "two", "flag", "env"} { |
There was a problem hiding this comment.
With the fake serving lk's default URL as localhost.test (see line 86), a case that resolves to the default URL needs canned room and egress responses there too:
| for _, name := range []string{"one", "two", "flag", "env"} { | |
| for _, name := range []string{"one", "two", "flag", "env", "localhost"} { |
| } | ||
| case "APIflag", "APIenv", "devkey": | ||
| // Inline and --dev keys only go to the inline URL's host. | ||
| if url, _, _ := c.inlineCredentials(); url == "" || hostOf(url) != project { |
There was a problem hiding this comment.
With inlineCredentials returning ws://localhost.test when no URL is given (see line 430), an inline or --dev key must go to localhost.test in that case. Today, url == "" fails any such request, though sending it to the default URL is correct:
| if url, _, _ := c.inlineCredentials(); url == "" || hostOf(url) != project { | |
| if url, _, _ := c.inlineCredentials(); hostOf(url) != project { |
| } | ||
| return "" | ||
| } | ||
| return pick(c.urlFrom, "ws://flag.test", "ws://env.test"), |
There was a problem hiding this comment.
lk's --url has a default, so a case with no inline URL still has one, http://localhost:7880, which line 86 makes the fake serve as localhost.test. Returning that stand-in keeps the inline-host checks (line 163, namedProjects) honest for --dev and inline keys without a URL:
| return pick(c.urlFrom, "ws://flag.test", "ws://env.test"), | |
| return cmp.Or(pick(c.urlFrom, "ws://flag.test", "ws://env.test"), "ws://localhost.test"), |
| // inlineCredentials are the URL, API key and API secret lk reads from flags, | ||
| // each beating its env var. |
There was a problem hiding this comment.
With the stand-in on line 442:
| // inlineCredentials are the URL, API key and API secret lk reads from flags, | |
| // each beating its env var. | |
| // inlineCredentials are the URL, API key and API secret lk reads from flags, | |
| // each beating its env var. The URL defaults to lk's http://localhost:7880, | |
| // which the fake serves as localhost.test. |
| // Inline keys and secrets can come from different sources, so only | ||
| // configured projects' pairs are checked. | ||
| for _, name := range []string{"one", "two"} { | ||
| cloud.secrets["API"+name] = "secret" + name | ||
| } |
There was a problem hiding this comment.
The fake only checks a token's signature when it knows the secret for that token's key, and only APIone and APItwo are registered. So a token signed with devkey (--dev), APIflag or APIenv is never checked. If lk signed --dev tokens with secretone instead of secret, this test would still pass. I planted that bug, and it did.
The case already knows which secret should go with each inline key, even when they come from different sources. inlineCredentials() picks the key and the secret the way lk does, each flag beating its own env var. For example, --api-key APIflag with LIVEKIT_API_SECRET=secretenv means lk must sign APIflag tokens with secretenv. --dev always means devkey with secret. Registering those pairs catches the planted bug (apikey=devkey badsig) and passes against the current code:
| // Inline keys and secrets can come from different sources, so only | |
| // configured projects' pairs are checked. | |
| for _, name := range []string{"one", "two"} { | |
| cloud.secrets["API"+name] = "secret" + name | |
| } | |
| for _, name := range []string{"one", "two"} { | |
| cloud.secrets["API"+name] = "secret" + name | |
| } | |
| cloud.secrets["devkey"] = "secret" | |
| if _, key, secret := c.inlineCredentials(); key != "" { | |
| cloud.secrets[key] = secret | |
| } |
Separately, --dev is rarely exercised. lk only uses the dev credentials when --dev is drawn and nothing ahead of it in resolveProject wins (--project, --subdomain, or an inline key or secret, which make --dev an error). Over 3,000 cases, only one sent a devkey request. So at rapid's default 100 checks, a --dev regression would almost never be caught, even with this fix.
| cmd.Env = append(c.env(), "HOME="+home, "USERPROFILE="+home, "HTTP_PROXY="+cloud.URL, "HTTPS_PROXY="+cloud.URL, | ||
| "GORACE=atexit_sleep_ms=0") // under -race, lk would otherwise sleep a second before each clean exit |
There was a problem hiding this comment.
testscript passes the caller's GORACE through, but here cmd.Env starts from c.env(), so the caller's value is dropped. With GORACE="halt_on_error=1 log_path=/tmp/race" go test -race ./cmd/lk, each lk run gets only atexit_sleep_ms=0, and a race in lk shows up as a confusing case failure instead of a report. Appending keeps both:
| cmd.Env = append(c.env(), "HOME="+home, "USERPROFILE="+home, "HTTP_PROXY="+cloud.URL, "HTTPS_PROXY="+cloud.URL, | |
| "GORACE=atexit_sleep_ms=0") // under -race, lk would otherwise sleep a second before each clean exit | |
| cmd.Env = append(c.env(), "HOME="+home, "USERPROFILE="+home, "HTTP_PROXY="+cloud.URL, "HTTPS_PROXY="+cloud.URL, | |
| "GORACE="+strings.TrimSpace(os.Getenv("GORACE")+" atexit_sleep_ms=0")) // under -race, lk would otherwise sleep a second before each clean exit |
| case c.userAuth && strings.Contains(line, " apikey=") && !strings.Contains(line, " apikey="+mintKey("")): | ||
| t.Fatalf("under --user-auth, a project server got a token not minted by the Public API: %s", line) | ||
| case c.userAuth && strings.Contains(line, " bearer=") && !strings.HasPrefix(target, publicAPI+"/"): | ||
| t.Fatalf("a session token went to a server it wasn't signed in to: %s", line) |
There was a problem hiding this comment.
Under --user-auth, these cases only fire on lines that contain apikey= or bearer=. A project-server request with no Authorization header passes every rule, for example POST http://one.test/twirp/livekit.RoomService/ListRooms with nothing after it, if lk skipped the mint. Requiring a minted token on everything outside the Public API covers both that and a token that wasn't minted. The bearer case goes first so a session sent to a project server keeps its own message:
| case c.userAuth && strings.Contains(line, " apikey=") && !strings.Contains(line, " apikey="+mintKey("")): | |
| t.Fatalf("under --user-auth, a project server got a token not minted by the Public API: %s", line) | |
| case c.userAuth && strings.Contains(line, " bearer=") && !strings.HasPrefix(target, publicAPI+"/"): | |
| t.Fatalf("a session token went to a server it wasn't signed in to: %s", line) | |
| case c.userAuth && strings.Contains(line, " bearer=") && !strings.HasPrefix(target, publicAPI+"/"): | |
| t.Fatalf("a session token went to a server it wasn't signed in to: %s", line) | |
| case c.userAuth && !strings.HasPrefix(target, publicAPI+"/") && !strings.Contains(line, " apikey="+mintKey("")): | |
| t.Fatalf("under --user-auth, a project server got a request without a token minted by the Public API: %s", line) |
| return "", false | ||
| } | ||
| host, _, _ := strings.Cut(strings.TrimPrefix(fields[1], "http://"), "/") | ||
| if host == "publicapi.test" { |
There was a problem hiding this comment.
http://other.test is the Public API URL nobody is signed in to, but only publicapi.test is excluded here. So under --user-auth --public-api-url http://other.test, a call like GET http://other.test/v0/projects is read as naming a project called other. The case then fails with request names project "other", but only [...] were named, which hides the real problem. It also sets named, which skews the mustFail check:
| if host == "publicapi.test" { | |
| if host == "publicapi.test" || host == "other.test" { |
| return c.command.publicAPI | ||
| case !slices.ContainsFunc(c.users, func(u sessionUser) bool { return !u.expired }): | ||
| return true | ||
| case c.user != "" && !slices.ContainsFunc(c.users, func(u sessionUser) bool { return u.id == c.user || u.email == c.user }): |
There was a problem hiding this comment.
This checks only that the --user exists, not that their session is valid, and the case above passes as long as any user is unexpired. So with u_1 expired and u_3 valid, --user-auth --user u_1 room list (or --user dev@example.com) isn't required to fail, and a regression where lk quietly falls back to u_3's session passes:
| case c.user != "" && !slices.ContainsFunc(c.users, func(u sessionUser) bool { return u.id == c.user || u.email == c.user }): | |
| case c.user != "" && !slices.ContainsFunc(c.users, func(u sessionUser) bool { return (u.id == c.user || u.email == c.user) && !u.expired }): |
…s and missing tokens Go never proxies localhost, so a case whose credentials point at lk's --url default, http://localhost:7880 (--dev, or an inline key without a URL), sent its requests past the fake, and no rule saw them. The test now also serves that port, as localhost.test, and fails if something else holds it. The fake now knows every key's secret, not just the configured projects': --dev's, and the inline key paired with the inline secret. A --dev token signed with another project's secret used to pass. Under --user-auth, every request to a project server must carry a minted token, so one with no token fails instead of matching no rule. A --user whose session has expired must fail, instead of passing as long as some other user's session is valid. A request to http://other.test, the Public API nobody is signed in to, no longer reads as naming a project called "other". The caller's GORACE is kept, with atexit_sleep_ms=0 appended. --dev only takes effect when nothing ahead of it in resolveProject wins, which the draws rarely lined up, so a third of cases now clear the way for it.
|
thanks, these were all real. took all of them in 18bebfc, and made --dev cases much more common (a third of cases now clear the way for it). planted your two --dev bugs plus a skipped mint and an expired --user falling back: 0/5 before, 5/5 after for the first three, and the expired --user one is caught at 1000 cases (it's a rare draw at the default 100). one side effect of serving localhost:7880: anything else talking to that port during a run lands in the fake's log, and a second run can't start while one holds it |
Why
The scripts in #1069 pin specific scenarios. This sweeps the combinations: any mix of flags, env vars, config and sessions should only ever act on a project the user named, with that project's credential.
What changed
cmd/lk/credential_property_test.go: a pgregory.net/rapid property.--project,--subdomainand--dev;livekit.toml;--user, and the Public API URL.room list,egress listorproject key list, one for each way lk loads a project.--devkey only the inline URL's host, and a token minted for one project never reaches another's;--user-auth, project servers only see minted tokens and session tokens only go to the Public API; in API-key mode, neither is used;--user, or a Public API nobody is signed in to), when both mode flags are passed, or whenproject key listruns without--user-auth.CreateTokenResponse. It flags a token signed with the wrong secret. Both uselivekit/protocol/auth.cmd/lk/testdata/rapid/is gitignored.Testing
go build ./...,go vet ./...,go test -count=1 ./...,go test -race ./cmd/lkand golangci-lint all pass, and each commit builds and passes on its own.-racetime forcmd/lkgoes from 32.0s (test(lk): run lk from scripts against a fake LiveKit Cloud #1069) to 41.7s.--api-secret;--projectunder user auth using the first project;egress listfalling back to the only project;project key listignoring--project(4 of 5);project key listrunning without--user-auth.